Re: [PATCH] tiny af_packet.c cleanup

Mitchell Blank Jr [off-list ref] :
[...]

I thought that without the comment someone might think that the second
"if()" wasn't needed (since we had just checked the same value against
NULL a few lines up)

Ok, I completely missed the intent.

Actually packet_rcv() is run in a BH context and doesn't race with
SO_{ATTACH/DETACH}_FILTER from sock_setsockopt() which does the
appropriate BH locking (spin_lock_bh(&sk->sk_lock.slock) in
net/core/sock.c::sock_setsockopt and in net/core/filter.c::sk_attach_filter).
packet_rcv() doesn't race with BH either due to the bh_lock_sock (a spin_lock
in disguise) you quoted.

That being said, I don't see how such an explanation could fit in a short,
inlined comment. :o)

--
Ueimor

Francois Romieu wrote:

Actually packet_rcv() is run in a BH context and doesn't race with
SO_{ATTACH/DETACH}_FILTER from sock_setsockopt() which does the
appropriate BH locking (spin_lock_bh(&sk->sk_lock.slock) in
net/core/sock.c::sock_setsockopt and in net/core/filter.c::sk_attach_filter).
packet_rcv() doesn't race with BH either due to the bh_lock_sock (a spin_lock
in disguise) you quoted.

I don't understand what you're saying - are you saying that the locking
isn't needed?  Or just the recheck isn't needed?  It sounds like if we're
only avoiding the race because of the bh_lock_sock() then we do need to
recheck, right?

Could you do a patch for what you think it should look like?  You obviously
understand the locking issues here better than I.

-Mitch