Re: svn commit: r304218 - head/sys/netinet

[Julien Charbon]

 Hi,

On 8/16/16 3:21 PM, Randall Stewart via svn-src-all wrote:
> 
> In theory it *could* be MFC’d to stable-10 and 11 but I am not sure we want 
> to do that. I am
> told by Drew that it does improve performance since in stable-10 you are 
> getting the INFO_WLOCK()
> but I am not sure if folks want it MFC’d…
> 
> R

 A bit late in the game but for information:  Since r309108,in
stable-10 you are getting the INFO_RLOCK instead of the INFO_WLOCK like
in stable-11 and HEAD:

https://svnweb.freebsd.org/base?view=revision=309108

 In summary the same TCP INP_INFO lock logic is used in stable-10, 11
and HEAD which simplify MFC if needed.

 My 2 cents.

--
Julien



signature.asc
Description: OpenPGP digital signature

Re: svn commit: r304218 - head/sys/netinet

[Slawa Olhovchenkov]

On Tue, Aug 16, 2016 at 06:21:14AM -0700, Randall Stewart via svn-src-all wrote:

> 
> In theory it *could* be MFC’d to stable-10 and 11 but I am not sure we want 
> to do that. I am
> told by Drew that it does improve performance since in stable-10 you are 
> getting the INFO_WLOCK()
> but I am not sure if folks want it MFC’d…
> 
> One thing that this code leads us towards is we *in theory* could move the 
> lock acquisition to the
> timer code itself (I think).. we would have to make sure that the callout 
> functions did do the
> unlock since thats part of the lock-dance with reference… but its 
> theoretically possible :-)

What reason to not MFC?
I mean MFCed all don't break API/ABI.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r304218 - head/sys/netinet

[Randall Stewart via svn-src-head]

In theory it *could* be MFC’d to stable-10 and 11 but I am not sure we want to 
do that. I am
told by Drew that it does improve performance since in stable-10 you are 
getting the INFO_WLOCK()
but I am not sure if folks want it MFC’d…

One thing that this code leads us towards is we *in theory* could move the lock 
acquisition to the
timer code itself (I think).. we would have to make sure that the callout 
functions did do the
unlock since thats part of the lock-dance with reference… but its theoretically 
possible :-)

R

> On Aug 16, 2016, at 6:18 AM, Slawa Olhovchenkov  wrote:
> 
> On Tue, Aug 16, 2016 at 12:40:56PM +, Randall Stewart wrote:
> 
>> Author: rrs
>> Date: Tue Aug 16 12:40:56 2016
>> New Revision: 304218
>> URL: https://svnweb.freebsd.org/changeset/base/304218
>> 
>> Log:
>>  This cleans up the timer code in TCP and also makes it so we do not
>>  take the INFO lock *unless* we are really going to delete the TCB.
>> 
>>  Differential Revision:  D7136
> 
> Is this related to stable/10?


Randall Stewart
r...@netflix.com
803-317-4952





___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r304218 - head/sys/netinet

[Slawa Olhovchenkov]

On Tue, Aug 16, 2016 at 12:40:56PM +, Randall Stewart wrote:

> Author: rrs
> Date: Tue Aug 16 12:40:56 2016
> New Revision: 304218
> URL: https://svnweb.freebsd.org/changeset/base/304218
> 
> Log:
>   This cleans up the timer code in TCP and also makes it so we do not
>   take the INFO lock *unless* we are really going to delete the TCB.
>   
>   Differential Revision:  D7136

Is this related to stable/10?
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r304218 - head/sys/netinet

[Randall Stewart via svn-src-head]

Hans:

Take a look at the comments maybe they will help you understand whats going on.

The idea of it is that you *only* need the INFO_RLOCK when the timer function
wants to destroy the tcb (not all timers do this).. and yes usually the timer 
function
is going to call the drop/close path to purge the TCB. So in order to pick-up 
the info
lock you do the refcnt/lock-dance to get both locks in the proper lock order. 
This means
that someone could possibly come in and purge the tcb on you while you are in 
the
process of doing the lock-dance. 

If that occurs (the return code is 1) all the caller has to do is call the 
drop-lock function (the
mate to the add_lock) and then return (since the pcb is in the state the caller 
wants.. i.e. gone).
If the return code is 0, the caller can proceed to purge the tcb.. and then 
call the drop_lock function.

Note that in theory this could be used outside of wanting to kill the tcb.. but 
I am not sure why one
would want to hold the INFO_RLOCK if one did not want to purge the tcb.

R


> On Aug 16, 2016, at 6:14 AM, Hans Petter Selasky  wrote:
> 
> On 08/16/16 15:01, Randall Stewart wrote:
>> Sure
>> 
>> Let me add some comments for you. The idea her is that you pick-up a 
>> reference
>> to the PCB.. so it can’t be removed. Thus when you re-lock the INP you check 
>> the
>> dropped flag (just in case someone did get in).
> 
> And this code is only used before tcp_close() / tcp_drop(), so if others got 
> in it is safe to assume that the inp is dead?
> 
> --HPS


Randall Stewart
r...@netflix.com
803-317-4952





___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r304218 - head/sys/netinet

[Hans Petter Selasky]

On 08/16/16 15:01, Randall Stewart wrote:

Sure

Let me add some comments for you. The idea her is that you pick-up a reference
to the PCB.. so it can’t be removed. Thus when you re-lock the INP you check the
dropped flag (just in case someone did get in).


And this code is only used before tcp_close() / tcp_drop(), so if others 
got in it is safe to assume that the inp is dead?


--HPS
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r304218 - head/sys/netinet

[Randall Stewart via svn-src-head]

Sure

Let me add some comments for you. The idea her is that you pick-up a reference
to the PCB.. so it can’t be removed. Thus when you re-lock the INP you check the
dropped flag (just in case someone did get in).

Let me get that in comments.. (note thats also why when using this function you
have to use its companion function to drop the reference).

> On Aug 16, 2016, at 5:58 AM, Hans Petter Selasky  wrote:
> 
> On 08/16/16 14:40, Randall Stewart wrote:
>> +int
>> +tcp_inpinfo_lock_add(struct inpcb *inp)
>> +{
>> +in_pcbref(inp);
>> +INP_WUNLOCK(inp);
>> +INP_INFO_RLOCK(_tcbinfo);
>> +INP_WLOCK(inp);
>> +if (inp->inp_flags & (INP_TIMEWAIT | INP_DROPPED)) {
>> +return(1);
>> +}
>> +return(0);
>> +
>> +}
> 
> Hi,
> 
> Could you add some comments describing how it is considered safe to drop the 
> INP write-lock and then pick it up again?
> 
> My first impression is that because you are dropping the inp lock, multiple 
> threads can enter the code in question, leaving the window open to races?
> 
> --HPS


Randall Stewart
r...@netflix.com
803-317-4952





___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r304218 - head/sys/netinet

[Hans Petter Selasky]

On 08/16/16 14:40, Randall Stewart wrote:

+int
+tcp_inpinfo_lock_add(struct inpcb *inp)
+{
+   in_pcbref(inp);
+   INP_WUNLOCK(inp);
+   INP_INFO_RLOCK(_tcbinfo);
+   INP_WLOCK(inp);
+   if (inp->inp_flags & (INP_TIMEWAIT | INP_DROPPED)) {
+   return(1);
+   }
+   return(0);
+
+}


Hi,

Could you add some comments describing how it is considered safe to drop 
the INP write-lock and then pick it up again?


My first impression is that because you are dropping the inp lock, 
multiple threads can enter the code in question, leaving the window open 
to races?


--HPS
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"