On Fri, May 29, 2020 at 01:01:51PM -0700, Andrii Nakryiko wrote: > > question though; why are you using xchg() for the commit? Isn't that > > more expensive than it should be? > > > > That is, why isn't that: > > > > smp_store_release(&hdr->len, new_len); > > > > ? Or are you needing the smp_mb() for the store->load ordering for the > > ->consumer_pos load? That really needs a comment. > > Yeah, smp_store_release() is not strong enough, this memory barrier is > necessary. And yeah, I'll follow up with some more comments, that's > been what Joel requested as well. Ok, great. > > I think you can get rid of the smp_load_acquire() there, you're ordering > > a load->store and could rely on the branch to do that: > > > > cons_pos = READ_ONCE(&rb->consumer_pos) & rb->mask; > > if ((flags & BPF_RB_FORCE_WAKEUP) || (cons_pos == rec_pos && !(flags &BPF_RB_NO_WAKEUP)) > > irq_work_queue(&rq->work); > > > > should be a control dependency. > > Could be. I tried to keep consistent > smp_load_acquire/smp_store_release usage to keep it simpler. It might > not be the absolutely minimal amount of ordering that would still be > correct. We might be able to tweak and tune this without changing > correctness. We can even rely on the irq_work_queue() being an atomic, but sure, get it all working and correct first before you wreck it ;-)
