On Thu, Feb 18, 2021 at 3:04 PM Björn Töpel <[email protected]> wrote: > > On Thu, 18 Feb 2021 at 14:53, Jason A. Donenfeld <[email protected]> wrote: > > > > Hey Bjorn, > > > > On Thu, Feb 18, 2021 at 2:50 PM Björn Töpel <[email protected]> wrote: > > > > + > > > > +static void __wg_prev_queue_enqueue(struct prev_queue *queue, struct > > > > sk_buff *skb) > > > > +{ > > > > + WRITE_ONCE(NEXT(skb), NULL); > > > > + smp_wmb(); > > > > + WRITE_ONCE(NEXT(xchg_relaxed(&queue->head, skb)), skb); > > > > +} > > > > + > > > > > > I'll chime in with Toke; This MPSC and Dmitry's links really took me > > > to the "verify with pen/paper"-level! Thanks! > > > > > > I'd replace the smp_wmb()/_relaxed above with a xchg_release(), which > > > might perform better on some platforms. Also, it'll be a nicer pair > > > with the ldacq below. :-P In general, it would be nice with some > > > wording how the fences pair. It would help the readers (like me!) a > > > lot. > > > > Exactly. This is what's been in my dev tree for the last week or so: > > > > Ah, nice! > > > +static void __wg_prev_queue_enqueue(struct prev_queue *queue, struct > > sk_buff *skb) > > +{ > > + WRITE_ONCE(NEXT(skb), NULL); > > + WRITE_ONCE(NEXT(xchg_release(&queue->head, skb)), skb); > > +} > > > > Look good? > > > > Yes, exactly like that!
The downside is that on armv7, this becomes a dmb(ish) instead of a dmb(ishst). But I was unable to measure any actual difference anyway, and the atomic bounded increment is already more expensive, so I think it's okay. Jason
