On Mon, Dec 2, 2019 at 10:30 AM Maxim Mikityanskiy [off-list ref] wrote:

On 2019-11-29 11:51, Magnus Karlsson wrote:

quoted

The rings in AF_XDP between user space and kernel space have the
following semantics:

producer                         consumer

if (LOAD ->consumer) {           LOAD ->producer
                    (A)           smp_rmb()       (C)
    STORE $data                   LOAD $data
    smp_wmb()       (B)           smp_mb()        (D)
    STORE ->producer              STORE ->consumer
}

The consumer function xskq_has_addrs() below loads the producer
pointer and updates the locally cached copy of it. However, it does
not issue the smp_rmb() operation required by the lockless ring. This
would have been ok had the function not updated the locally cached
copy, as that could not have resulted in new data being read from the
ring. But as it updates the local producer pointer, a subsequent peek
operation, such as xskq_peek_addr(), might load data from the ring
without issuing the required smp_rmb() memory barrier.

Thanks for paying attention to it, but I don't think it can really
happen. xskq_has_addrs only updates prod_tail, but xskq_peek_addr
doesn't use prod_tail, it reads from cons_tail to cons_head, and every
cons_head update has the necessary smp_rmb.

You are correct, it cannot happen. I am working on a 10 part patch set
that simplifies the rings and was staring blindly at that. In that
patch set it can happen since I only have two cached pointers instead
of four so there is a dependency, but not in the current code. I will
include this barrier in my patch set at the appropriate place. Thanks
for looking into this Maxim.

Please drop this patch.

/Magnus