Re: socket locks

On Tue, 21 May 2002 10:39:13 -0400 (EDT),
  John Baldwin <[e-mail]> said:

jhb> On 21-May-2002 Jeffrey Hsu wrote:
>> > tanimura    2002/05/19 22:41:09 PDT
>> > Modified files:
>> >   (too many files)
>> > Log:
>> > Lock down a socket, milestone 1.
>> >
>> > Reviewed by:    alfred
>> 
>> This patch mechanically adds a lot of locks around uses of socket fields.
>> For example, in tcp_output.c:
>> 
>> @@ -889,8 +897,11 @@ send:
>> #ifdef IPSEC
>> ipsec_setsocket(m, so);
>> #endif /*IPSEC*/
>> +       SOCK_LOCK(so);
>> +       soopts = (so->so_options & SO_DONTROUTE);
>> +       SOCK_UNLOCK(so);
>> error = ip_output(m, tp->t_inpcb->inp_options, &tp->t_inpcb->inp_route,
>> -           (so->so_options & SO_DONTROUTE), 0);
>> +           soopts, 0);
>> }
>> 
>> Locking and immediately unlocking accesses to a socket field doesn't
>> accomplish anything.  We want to lock the socket at the beginning of an
>> operation and release it at the end. This leads to way fewer locks inserted
>> into the networking code.

jhb> Agreed, the value you read with the lock held above becomes invalid/stale as
jhb> soon as you drop the lock, so the lock really isn't doing much good.  It is,
jhb> however, obfuscating the code and will probably have to be backed out when
jhb> the code is more fully locked.  Changes like this don't make reading/writing
jhb> so_options safe.  It is not enough just to lock the reads and writes, you
jhb> need to lock the results of actions taken on those values as well to ensure
jhb> that multiple actions taken on an object all perform them on a consistent
jhb> snapshot of that object.

When we call a function the network swi may call, it should be all
right to lock a socket across the function because it does not block.
ip_output(), tcp_output(), and udp_output() should be safe.  Although
I have not investigated completely yet, any functions called via
pru_send of struct pr_usrreqs seem to be OK.


>> Furthermore, these mechanical bottom-up socket locks don't respect
>> lock ordering with respect to the top-down inpcb locks.  Fortunately,
>> cvs update only resulted in a few conflicts with the inpcb locking code
>> and I can just turn off the socket lock macros in my tree.
>> 
>> We should coordiante better on working to lock up the networking code so
>> we're not working at cross-purposes.

If we were to lock a PCB prior to a socket, we have to add a new
method (pru_lock?) to struct struct pr_usrreqs so that the socket APIs
can lock a PCB opaque to a socket.


jhb> Can the various folks working on the network stack possibly "sit down" and
jhb> write up a description of the overall locking strategy for the stack including
jhb> the required lock orders, etc.?  That would be a big help in coordinating
jhb> things better.

jhb> -- 

jhb> John Baldwin <[e-mail]>  <><  http://www.FreeBSD.org/~jhb/
jhb> "Power Users Use the Power to Serve!"  -  http://www.FreeBSD.org/

jhb> To Unsubscribe: send mail to [e-mail]
jhb> with "unsubscribe freebsd-smp" in the body of the message
-- 
Seigo Tanimura <[e-mail]> <[e-mail]>

To Unsubscribe: send mail to [e-mail]
with "unsubscribe freebsd-smp" in the body of the message