[RFC PATCH] Fix usage of stdatomic.h for C++ compilers

[RFC PATCH] Fix usage of stdatomic.h for C++ compilers

[Hrvoje Zeba]

Since b9c0bf79aa8, liburing.h doesn't compile with C++ compilers. C++
provides it's own <atomic> interface and <stdatomic.h> can't be used. This
is a minimal change to use <atomic> variants where needed.

Signed-off-by: Hrvoje Zeba <zeba.hrvoje@gmail.com>
---
 src/include/liburing.h         | 18 ++++++++++--------
 src/include/liburing/barrier.h |  8 ++++++++
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/src/include/liburing.h b/src/include/liburing.h
index c9034fc..2bb6efd 100644
--- a/src/include/liburing.h
+++ b/src/include/liburing.h
@@ -2,10 +2,6 @@
 #ifndef LIB_URING_H
 #define LIB_URING_H
 
-#ifdef __cplusplus
-extern "C" {
-#endif
-
 #include <sys/socket.h>
 #include <sys/uio.h>
 #include <sys/stat.h>
@@ -15,9 +11,15 @@ extern "C" {
 #include <inttypes.h>
 #include <time.h>
 #include <linux/swab.h>
+
+#include "liburing/barrier.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
 #include "liburing/compat.h"
 #include "liburing/io_uring.h"
-#include "liburing/barrier.h"
 
 /*
  * Library interface to io_uring
@@ -40,11 +42,11 @@ struct io_uring_sq {
 };
 
 struct io_uring_cq {
-	unsigned *khead;
-	unsigned *ktail;
+	atomic_uint *khead;
+	atomic_uint *ktail;
 	unsigned *kring_mask;
 	unsigned *kring_entries;
-	unsigned *kflags;
+	atomic_uint *kflags;
 	unsigned *koverflow;
 	struct io_uring_cqe *cqes;
 
diff --git a/src/include/liburing/barrier.h b/src/include/liburing/barrier.h
index c8aa421..8f422eb 100644
--- a/src/include/liburing/barrier.h
+++ b/src/include/liburing/barrier.h
@@ -2,7 +2,15 @@
 #ifndef LIBURING_BARRIER_H
 #define LIBURING_BARRIER_H
 
+#ifdef __cplusplus
+#include <atomic>
+using std::atomic_uint;
+using std::memory_order_release;
+using std::memory_order_acquire;
+using std::memory_order_relaxed;
+#else
 #include <stdatomic.h>
+#endif
 
 /*
 From the kernel documentation file refcount-vs-atomic.rst:
-- 
2.27.0

Re: [RFC PATCH] Fix usage of stdatomic.h for C++ compilers

[Hrvoje Zeba]

On Sat, Jun 27, 2020 at 1:55 AM Hrvoje Zeba <zeba.hrvoje@gmail.com> wrote:
>
> Since b9c0bf79aa8, liburing.h doesn't compile with C++ compilers. C++
> provides it's own <atomic> interface and <stdatomic.h> can't be used. This
> is a minimal change to use <atomic> variants where needed.
>

I've tested this with gcc 10.1.0 and clang 10.0.0 and did some limited
testing. Seemed fine but somebody more knowledgeable should also take
a look.

Re: [RFC PATCH] Fix usage of stdatomic.h for C++ compilers

[Bart Van Assche]

On 2020-06-26 22:55, Hrvoje Zeba wrote:
> Since b9c0bf79aa8, liburing.h doesn't compile with C++ compilers. C++
> provides it's own <atomic> interface and <stdatomic.h> can't be used. This
> is a minimal change to use <atomic> variants where needed.

I was not aware that liburing supports C++ compilers?

>  struct io_uring_cq {
> -	unsigned *khead;
> -	unsigned *ktail;
> +	atomic_uint *khead;
> +	atomic_uint *ktail;

I think this is the wrong way to make liburing again compatible with
C++ compilers. Changing these data types causes all dereferences of
these pointers to be translated by the compiler into sequentially
consistent atomic instructions. I expect this patch to have a
negative impact on the performance of liburing.

Bart.

Re: [RFC PATCH] Fix usage of stdatomic.h for C++ compilers

[Hrvoje Zeba]

On Sat, Jun 27, 2020 at 11:27 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 2020-06-26 22:55, Hrvoje Zeba wrote:
> > Since b9c0bf79aa8, liburing.h doesn't compile with C++ compilers. C++
> > provides it's own <atomic> interface and <stdatomic.h> can't be used. This
> > is a minimal change to use <atomic> variants where needed.
>
> I was not aware that liburing supports C++ compilers?
>

Why is this there then?
https://git.kernel.dk/cgit/liburing/tree/src/include/liburing.h#n6

> >  struct io_uring_cq {
> > -     unsigned *khead;
> > -     unsigned *ktail;
> > +     atomic_uint *khead;
> > +     atomic_uint *ktail;
>
> I think this is the wrong way to make liburing again compatible with
> C++ compilers. Changing these data types causes all dereferences of
> these pointers to be translated by the compiler into sequentially
> consistent atomic instructions. I expect this patch to have a
> negative impact on the performance of liburing.
>

Any suggestions?

Re: [RFC PATCH] Fix usage of stdatomic.h for C++ compilers

[Bart Van Assche]

[-- Attachment #1: Type: text/plain, Size: 125 bytes --]

On 2020-06-27 08:39, Hrvoje Zeba wrote:
> Any suggestions?

How about the two attached (untested) patches?

Thanks,

Bart.



[-- Attachment #2: 0001-Make-the-liburing-header-files-again-compatible-with.patch --]
[-- Type: text/x-patch, Size: 2962 bytes --]

From 2d6b8423ece7f00798d73ede08719d4da6abbd46 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bvanassche@acm.org>
Date: Sat, 27 Jun 2020 08:40:23 -0700
Subject: [PATCH liburing 1/2] Make the liburing header files again compatible
 with C++

This patch has been tested by inspecting the output of the following
command:

g++ -I src/include -c -Wall -Wextra src/*.c

Fixes: b9c0bf79aa87 ("src/include/liburing/barrier.h: Use C11 atomics") # .
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 src/include/liburing.h         |  8 ++++----
 src/include/liburing/barrier.h | 37 ++++++++++++++++++++++++++++++++--
 2 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/src/include/liburing.h b/src/include/liburing.h
index c9034fc0df1b..76e2b854f957 100644
--- a/src/include/liburing.h
+++ b/src/include/liburing.h
@@ -2,10 +2,6 @@
 #ifndef LIB_URING_H
 #define LIB_URING_H
 
-#ifdef __cplusplus
-extern "C" {
-#endif
-
 #include <sys/socket.h>
 #include <sys/uio.h>
 #include <sys/stat.h>
@@ -19,6 +15,10 @@ extern "C" {
 #include "liburing/io_uring.h"
 #include "liburing/barrier.h"
 
+#ifdef __cplusplus
+extern "C" {
+#endif
+
 /*
  * Library interface to io_uring
  */
diff --git a/src/include/liburing/barrier.h b/src/include/liburing/barrier.h
index c8aa4210371c..5ad337f33262 100644
--- a/src/include/liburing/barrier.h
+++ b/src/include/liburing/barrier.h
@@ -2,8 +2,6 @@
 #ifndef LIBURING_BARRIER_H
 #define LIBURING_BARRIER_H
 
-#include <stdatomic.h>
-
 /*
 From the kernel documentation file refcount-vs-atomic.rst:
 
@@ -23,6 +21,40 @@ after the acquire operation executes. This is implemented using
 :c:func:`smp_acquire__after_ctrl_dep`.
 */
 
+#ifdef __cplusplus
+#include <atomic>
+
+template <typename T>
+static inline void IO_URING_WRITE_ONCE(T &var, T val)
+{
+	std::atomic_store_explicit(reinterpret_cast<std::atomic<T> *>(&var),
+				   val, std::memory_order_relaxed);
+}
+template <typename T>
+static inline T IO_URING_READ_ONCE(const T &var)
+{
+	return std::atomic_load_explicit(
+		reinterpret_cast<const std::atomic<T> *>(&var),
+		std::memory_order_relaxed);
+}
+
+template <typename T>
+static inline void io_uring_smp_store_release(T *p, T v)
+{
+	std::atomic_store_explicit(reinterpret_cast<std::atomic<T> *>(p), v,
+				   std::memory_order_release);
+}
+
+template <typename T>
+static inline T io_uring_smp_load_acquire(const T *p)
+{
+	return std::atomic_load_explicit(
+		reinterpret_cast<const std::atomic<T> *>(p),
+		std::memory_order_acquire);
+}
+#else
+#include <stdatomic.h>
+
 #define IO_URING_WRITE_ONCE(var, val)				\
 	atomic_store_explicit(&(var), (val), memory_order_relaxed)
 #define IO_URING_READ_ONCE(var)					\
@@ -32,5 +64,6 @@ after the acquire operation executes. This is implemented using
 	atomic_store_explicit((p), (v), memory_order_release)
 #define io_uring_smp_load_acquire(p)				\
 	atomic_load_explicit((p), memory_order_acquire)
+#endif
 
 #endif /* defined(LIBURING_BARRIER_H) */

[-- Attachment #3: 0002-Add-a-C-unit-test.patch --]
[-- Type: text/x-patch, Size: 2740 bytes --]

From 0b896d7bf992caadde8f0c4d57478800b2261507 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bvanassche@acm.org>
Date: Sat, 27 Jun 2020 09:09:59 -0700
Subject: [PATCH liburing 2/2] Add a C++ unit test

Since the liburing header files support C++ compilers, add a C++ unit test.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 test/Makefile       |  9 +++++++--
 test/sq-full-cpp.cc | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 2 deletions(-)
 create mode 100644 test/sq-full-cpp.cc

diff --git a/test/Makefile b/test/Makefile
index e103296fabdd..8b81d2d5f67c 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -10,7 +10,8 @@ override CFLAGS += -Wall -Wextra -Wno-unused-parameter -Wno-sign-compare\
 	-I../src/include/ -include ../config-host.h
 
 all_targets += poll poll-cancel ring-leak fsync io_uring_setup io_uring_register \
-	       io_uring_enter nop sq-full cq-full 35fa71a030ca-test \
+	       io_uring_enter nop sq-full sq-full-cpp cq-full \
+		35fa71a030ca-test \
 		917257daa0fe-test b19062a56726-test eeed8b54e0df-test link \
 		send_recvmsg a4c0b3decb33-test 500f9fbadef8-test timeout \
 		sq-space_left stdout cq-ready cq-peek-batch file-register \
@@ -41,8 +42,12 @@ all: $(all_targets)
 %: %.c
 	$(QUIET_CC)$(CC) $(CFLAGS) -o $@ $< -luring $(XCFLAGS)
 
+%: %.cc
+	$(QUIET_CC)$(CXX) $(CFLAGS) -o $@ $< -luring $(XCFLAGS)
+
 test_srcs := poll.c poll-cancel.c ring-leak.c fsync.c io_uring_setup.c \
-	io_uring_register.c io_uring_enter.c nop.c sq-full.c cq-full.c \
+	io_uring_register.c io_uring_enter.c nop.c sq-full.c sq-full-cpp.cc \
+	cq-full.c \
 	35fa71a030ca-test.c 917257daa0fe-test.c b19062a56726-test.c \
 	eeed8b54e0df-test.c link.c send_recvmsg.c a4c0b3decb33-test.c \
 	500f9fbadef8-test.c timeout.c sq-space_left.c stdout.c cq-ready.c\
diff --git a/test/sq-full-cpp.cc b/test/sq-full-cpp.cc
new file mode 100644
index 000000000000..ba400996e615
--- /dev/null
+++ b/test/sq-full-cpp.cc
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Description: test SQ queue full condition
+ *
+ */
+#include <errno.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <string.h>
+#include <fcntl.h>
+
+#include "liburing.h"
+
+int main(int argc, char *argv[])
+{
+	struct io_uring_sqe *sqe;
+	struct io_uring ring;
+	int ret, i;
+
+	if (argc > 1)
+		return 0;
+
+	ret = io_uring_queue_init(8, &ring, 0);
+	if (ret) {
+		fprintf(stderr, "ring setup failed: %d\n", ret);
+		return 1;
+
+	}
+
+	i = 0;
+	while ((sqe = io_uring_get_sqe(&ring)) != NULL)
+		i++;
+
+	if (i != 8) {
+		fprintf(stderr, "Got %d SQEs, wanted 8\n", i);
+		goto err;
+	}
+
+	io_uring_queue_exit(&ring);
+	return 0;
+err:
+	io_uring_queue_exit(&ring);
+	return 1;
+}

Re: [RFC PATCH] Fix usage of stdatomic.h for C++ compilers

[Hrvoje Zeba]

On Sat, Jun 27, 2020 at 12:23 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 2020-06-27 08:39, Hrvoje Zeba wrote:
> > Any suggestions?
>
> How about the two attached (untested) patches?
>

Tested on my end, looks good.

Thank you!

Re: [RFC PATCH] Fix usage of stdatomic.h for C++ compilers

[Jens Axboe]

On 6/27/20 10:23 AM, Bart Van Assche wrote:
> On 2020-06-27 08:39, Hrvoje Zeba wrote:
>> Any suggestions?
> 
> How about the two attached (untested) patches?

Bart, this looks good to me. I've pushed out a change to detect if we
have C++ on the system, as I don't want to make it a hard requirement.
When you send these out "officially", can you make the addition of the
sq-full-cpp dependent on CONFIG_HAVE_CXX in test/Makefile?

-- 
Jens Axboe