On Mon, Dec 2, 2019 at 10:30 AM Maxim Mikityanskiy <maximmi@xxxxxxxxxxxx> wrote:
>
> On 2019-11-29 11:51, Magnus Karlsson wrote:
> > 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
