Messages in this thread | | | Date | Mon, 3 Jul 2017 15:57:03 -0400 (EDT) | From | Alan Stern <> | Subject | Re: [PATCH RFC 01/26] netfilter: Replace spin_unlock_wait() with lock/unlock pair |
| |
On Mon, 3 Jul 2017, Manfred Spraul wrote:
> >>> + /* 2) read nf_conntrack_locks_all, with ACQUIRE semantics */ > >>> + if (likely(smp_load_acquire(&nf_conntrack_locks_all) == false)) > >>> + return; > >> As far as I can tell, this read does not need to have ACQUIRE > >> semantics. > >> > >> You need to guarantee that two things can never happen: > >> > >> (1) We read nf_conntrack_locks_all == false, and this routine's > >> critical section for nf_conntrack_locks[i] runs after the > >> (empty) critical section for that lock in > >> nf_conntrack_all_lock(). > >> > >> (2) We read nf_conntrack_locks_all == true, and this routine's > >> critical section for nf_conntrack_locks_all_lock runs before > >> the critical section in nf_conntrack_all_lock(). > I was looking at nf_conntrack_all_unlock: > There is a smp_store_release() - which memory barrier does this pair with? > > nf_conntrack_all_unlock() > <arbitrary writes> > smp_store_release(a, false) > spin_unlock(b); > > nf_conntrack_lock() > spin_lock(c); > xx=read_once(a) > if (xx==false) > return > <arbitrary read>
Ah, I see your point. Yes, I did wonder about what would happen when nf_conntrack_locks_all was set back to false. But I didn't think about it any further, because the relevant code wasn't in your patch.
> I tried to pair the memory barriers: > nf_conntrack_all_unlock() contains a smp_store_release(). > What does that pair with?
You are right, this does need to be smp_load_acquire() after all. Perhaps the preceding comment should mention that it pairs with the smp_store_release() from an earlier invocation of nf_conntrack_all_unlock().
(Alternatively, you could make nf_conntrack_all_unlock() do a lock+unlock on all the locks in the array, just like nf_conntrack_all_lock(). But of course, that would be a lot less efficient.)
Alan Stern
| |