Re: svn commit: r304218 - head/sys/netinet
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&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
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-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r304218 - head/sys/netinet
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-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r304218 - head/sys/netinet
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-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r304218 - head/sys/netinet
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-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r304218 - head/sys/netinet
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-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r304218 - head/sys/netinet
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(&V_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-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r304218 - head/sys/netinet
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(&V_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-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"