Re: [PATCH RFC 01/26] netfilter: Replace spin_unlock_wait() with lock/unlock pair

From: Alan Stern
Date: Mon Jul 03 2017 - 15:57:09 EST


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