Re: [PATCH] liburing/barrier.h: Add prefix io_uring to barriers
On Mon, Aug 19, 2019 at 4:46 PM Jens Axboe wrote: > > On 8/12/19 6:39 AM, Julia Suvorova wrote: > > The names of the barriers conflict with the namespaces of other projects > > when trying to directly include liburing.h. Avoid using popular global > > names. > > I have applied this now. I don't think we can avoid having the > barriers in the namespace, as we do need them in various macros. Thanks. I've sent a follow-up patch to cover arm barriers too. Best regards, Julia Suvorova.
Re: [PATCH] liburing/barrier.h: Add prefix io_uring to barriers
On 8/12/19 6:39 AM, Julia Suvorova wrote: > The names of the barriers conflict with the namespaces of other projects > when trying to directly include liburing.h. Avoid using popular global > names. I have applied this now. I don't think we can avoid having the barriers in the namespace, as we do need them in various macros. -- Jens Axboe
Re: [PATCH] liburing/barrier.h: Add prefix io_uring to barriers
On Wed, Aug 14, 2019 at 11:24 AM Julia Suvorova wrote:
>
> On Mon, Aug 12, 2019 at 6:03 PM Jens Axboe wrote:
> >
> > On 8/12/19 7:55 AM, Bart Van Assche wrote:
> > > On 8/12/19 5:39 AM, Julia Suvorova wrote:
> > >> -#define mb()asm volatile("mfence" ::: "memory")
> > >> -#define rmb() asm volatile("lfence" ::: "memory")
> > >> -#define wmb() asm volatile("sfence" ::: "memory")
> > >> -#define smp_rmb() barrier()
> > >> -#define smp_wmb() barrier()
> > >> +#define io_uring_mb() asm volatile("mfence" ::: "memory")
> > >> +#define io_uring_rmb() asm volatile("lfence" ::: "memory")
> > >> +#define io_uring_wmb() asm volatile("sfence" ::: "memory")
> > >> +#define io_uring_smp_rmb() io_uring_barrier()
> > >> +#define io_uring_smp_wmb() io_uring_barrier()
> > >
> > > Do users of liburing need these macros? If not, have you considered to
> > > move these macros to a new header file that is only used inside liburing
> > > and such that these macros are no longer visible to liburing users?
> >
> > The exposed API should not need any explicit barriers from the user,
> > so this suggestion makes a lot of sense to me.
>
> How about moving the definition of io_uring_cqe_seen() with whole
> io_uring_cq_advance() and io_uring_for_each_cqe() from liburing.h to
> queue.c? This way we can cover all barriers, and leave barrier.h local.
>
> Do you need io_uring_cq_advance and io_uring_for_each_cqe in the
> library?
>
This is one of the usage patterns:
io_uring_cqe* cqe;
int head;
int count = 0;
io_uring_for_each_cqe(&m_io_uring, head, cqe)
{
/* ... */
count++;
}
io_uring_cq_advance(&m_io_uring, count);
A little bit more performance is squeezed out this way.
Hrvoje Zeba
Re: [PATCH] liburing/barrier.h: Add prefix io_uring to barriers
On Mon, Aug 12, 2019 at 6:03 PM Jens Axboe wrote:
>
> On 8/12/19 7:55 AM, Bart Van Assche wrote:
> > On 8/12/19 5:39 AM, Julia Suvorova wrote:
> >> -#define mb()asm volatile("mfence" ::: "memory")
> >> -#define rmb() asm volatile("lfence" ::: "memory")
> >> -#define wmb() asm volatile("sfence" ::: "memory")
> >> -#define smp_rmb() barrier()
> >> -#define smp_wmb() barrier()
> >> +#define io_uring_mb() asm volatile("mfence" ::: "memory")
> >> +#define io_uring_rmb() asm volatile("lfence" ::: "memory")
> >> +#define io_uring_wmb() asm volatile("sfence" ::: "memory")
> >> +#define io_uring_smp_rmb() io_uring_barrier()
> >> +#define io_uring_smp_wmb() io_uring_barrier()
> >
> > Do users of liburing need these macros? If not, have you considered to
> > move these macros to a new header file that is only used inside liburing
> > and such that these macros are no longer visible to liburing users?
>
> The exposed API should not need any explicit barriers from the user,
> so this suggestion makes a lot of sense to me.
How about moving the definition of io_uring_cqe_seen() with whole
io_uring_cq_advance() and io_uring_for_each_cqe() from liburing.h to
queue.c? This way we can cover all barriers, and leave barrier.h local.
Do you need io_uring_cq_advance and io_uring_for_each_cqe in the
library?
Best regards, Julia Suvorova.
Re: [PATCH] liburing/barrier.h: Add prefix io_uring to barriers
On 8/12/19 7:55 AM, Bart Van Assche wrote:
> On 8/12/19 5:39 AM, Julia Suvorova wrote:
>> -#define mb()asm volatile("mfence" ::: "memory")
>> -#define rmb() asm volatile("lfence" ::: "memory")
>> -#define wmb() asm volatile("sfence" ::: "memory")
>> -#define smp_rmb() barrier()
>> -#define smp_wmb() barrier()
>> +#define io_uring_mb() asm volatile("mfence" ::: "memory")
>> +#define io_uring_rmb() asm volatile("lfence" ::: "memory")
>> +#define io_uring_wmb() asm volatile("sfence" ::: "memory")
>> +#define io_uring_smp_rmb() io_uring_barrier()
>> +#define io_uring_smp_wmb() io_uring_barrier()
>
> Do users of liburing need these macros? If not, have you considered to
> move these macros to a new header file that is only used inside liburing
> and such that these macros are no longer visible to liburing users?
The exposed API should not need any explicit barriers from the user,
so this suggestion makes a lot of sense to me.
--
Jens Axboe
Re: [PATCH] liburing/barrier.h: Add prefix io_uring to barriers
On 8/12/19 5:39 AM, Julia Suvorova wrote:
-#define mb() asm volatile("mfence" ::: "memory")
-#define rmb() asm volatile("lfence" ::: "memory")
-#define wmb() asm volatile("sfence" ::: "memory")
-#define smp_rmb() barrier()
-#define smp_wmb() barrier()
+#define io_uring_mb() asm volatile("mfence" ::: "memory")
+#define io_uring_rmb() asm volatile("lfence" ::: "memory")
+#define io_uring_wmb() asm volatile("sfence" ::: "memory")
+#define io_uring_smp_rmb() io_uring_barrier()
+#define io_uring_smp_wmb() io_uring_barrier()
Do users of liburing need these macros? If not, have you considered to
move these macros to a new header file that is only used inside liburing
and such that these macros are no longer visible to liburing users?
Thanks,
Bart.
Re: [PATCH] liburing/barrier.h: Add prefix io_uring to barriers
On Mon, Aug 12, 2019 at 1:39 PM Julia Suvorova wrote:
> diff --git a/src/include/liburing/barrier.h b/src/include/liburing/barrier.h
> index 98be9e5..4f3d1d7 100644
> --- a/src/include/liburing/barrier.h
> +++ b/src/include/liburing/barrier.h
> @@ -23,7 +23,7 @@ after the acquire operation executes. This is implemented
> using
> /* From tools/include/linux/compiler.h */
> /* Optimization barrier */
> /* The "volatile" is due to gcc bugs */
> -#define barrier() __asm__ __volatile__("": : :"memory")
> +#define io_uring_barrier() __asm__ __volatile__("": : :"memory")
>
> /* From tools/virtio/linux/compiler.h */
> #define WRITE_ONCE(var, val) \
Please prefix WRITE_ONCE() and READ_ONCE() with IO_URING_ as well.
They are fairly likely to be used in code derived from the Linux
kernel.
Otherwise:
Reviewed-by: Stefan Hajnoczi
