Messages in this thread | | | From | Marco Elver <> | Date | Thu, 6 Feb 2020 20:29:06 +0100 | Subject | Re: [PATCH v3] skbuff: fix a data race in skb_queue_len() |
| |
On Thu, 6 Feb 2020 at 19:43, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > On Thu, Feb 06, 2020 at 10:22:02AM -0800, Eric Dumazet wrote: > > On 2/6/20 10:12 AM, Jason A. Donenfeld wrote: > > > On Thu, Feb 6, 2020 at 6:10 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > >> Unfortunately we do not have ADD_ONCE() or something like that. > > > > > > I guess normally this is called "atomic_add", unless you're thinking > > > instead about something like this, which generates the same > > > inefficient code as WRITE_ONCE: > > > > > > #define ADD_ONCE(d, s) *(volatile typeof(d) *)&(d) += (s) > > > > > > > Dmitry Vyukov had a nice suggestion few months back how to implement this. > > > > https://lkml.org/lkml/2019/10/5/6 > > That trick appears to work well in clang but not gcc: > > #define ADD_ONCE(d, i) ({ \ > typeof(d) *__p = &(d); \ > __atomic_store_n(__p, (i) + __atomic_load_n(__p, __ATOMIC_RELAXED), __ATOMIC_RELAXED); \ > }) > > gcc 9.2 gives: > > 0: 8b 47 10 mov 0x10(%rdi),%eax > 3: 83 e8 01 sub $0x1,%eax > 6: 89 47 10 mov %eax,0x10(%rdi) > > clang 9.0.1 gives: > > 0: 81 47 10 ff ff ff ff addl $0xffffffff,0x10(%rdi) > > But actually, clang does equally as well with: > > #define ADD_ONCE(d, i) *(volatile typeof(d) *)&(d) += (i)
I feel that ADD_ONCE conveys that it adds actually once (atomically), that is, if there are concurrent ADD_ONCE, all of them will succeed. This is not the case with the above variants and the 'ONCE' can turn into a 'MAYBE', and since we probably want to avoid making this more expensive on e.g. x86 that would need a LOCK-prefix.
In the case here, what we actually want is something that safely increments/decrements if there are only concurrent readers (concurrent writers disallowed). So 'add_exclusive(var, val)' (all-caps or not) might be more appropriate. [As an aside, recent changes to KCSAN would also allow us to assert for something like 'add_exclusive()' that there are in fact no other writers but only concurrent readers, even if all accesses are marked.]
If the single-writer constraint isn't wanted, but should still not be atomic, maybe 'add_lossy()'?
Thanks, -- Marco
> And testing further back, it generates the same code with your original > WRITE_ONCE. > > If clang's optimization here is technically correct, maybe we should go > talk to the gcc people about catching this case?
| |