RE: PATCH: typo in socreate() or i'm missing something
Dear Hackers, < said: > > Interestingly, socreate() in Lite2 always does a can-wait malloc() so > > our current soalloc(M_NOWAIT) does the same thing as Lite2 and is only > > wrong if the FreeBSD change from can-wait to "can-wait-if p != 0" > > change was needed and is still needed. > > When I initially revamped that code, I waited unconditionally and was > rewarded with an appropriate panic for sleeping in interrupt context. > I cannot speak as to whether it is still needed. well, what is the best way to proceed here? as far as i can see there are three options here: 1) leave it as it is for now 2) change it to so = soalloc(0); (i.e. never sleep) 3) revert it back to rotted so = soalloc(td != 0); in this case people like me will call socreate() with td == 0, and other will call socreate() with real td pointer or curthread. i personally do not like option 1) at all. are there any other options? suggestions? thanks, max To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: PATCH: typo in socreate() or i'm missing something
< said: > Interestingly, socreate() in Lite2 always does a can-wait malloc() so > our current soalloc(M_NOWAIT) does the same thing as Lite2 and is only > wrong if the FreeBSD change from can-wait to "can-wait-if p != 0" > change was needed and is still needed. When I initially revamped that code, I waited unconditionally and was rewarded with an appropriate panic for sleeping in interrupt context. I cannot speak as to whether it is still needed. -GAWollman To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: PATCH: typo in socreate() or i'm missing something
Bruce Evans wrote: > On Fri, 28 Feb 2003, Maksim Yevmenkin wrote: > > sonewconn() calls soalloc(0). > > tcp_input() is a critical caller of sonewconn() and it obviously shouldn't > sleep. But sonewconn() is called from process context elsewhere. > sonewconn() has used a no-wait malloc() since at least 4.4Lite2 (in Lite2 > there is no soalloc() and the malloc() is done directly in sonewconn()). > I think the call from tcp_input() is the usual case and/or no one cared > to distinguish the other cases. The interesting case in the tcp_input() path is an outstanding "accept" call, in which a new connection is created at interrupt (in LRP) o NETISR (in traditional, livelockable FreeBSD). In general, the blocking is done in order to get a process (or now thread, I guess) context from which to obtain a cred for the newly created socket. I've discussed my own approach to this problem with both Jeffrey Hsu and Jonathan Lemon, and I think it's pretty clever: don't get the credential from the process (thread). So how do you get the same/right credential? Easy. You cannot accept except on an outstanding listen socket; so copy the cred from the listening socket, which *is* in scope, even if p == 0 (or td == 0). I believe that addresses the problem case... 8-). -- Terry To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: PATCH: typo in socreate() or i'm missing something
On Fri, 28 Feb 2003, Maksim Yevmenkin wrote: > > [About creeping bitrot:] > > so = soalloc(p != 0); --> > > so = soalloc(td != 0); --> > > so = soalloc(M_NOWAIT); > > [and proposed fixes:] --> > > so = soalloc(0); or --> > > so = soalloc(1); > I see, however I find it very confusing to use M_NOWAIT just because > it is defined as 1. I'm sure there are plenty other defines that can > be used :) Why M_NOWAIT? Why just not write soalloc(1)? When I was > looking at the code I got the impression that soalloc() should not > sleep. I think the translation to M_NOWAIT was just a mistake. > > Changing it to your version: > > > > so = soalloc(M_NOWAIT); > huh? I hope you meant 0 here :) That > is what I did in my patch. Otherwise I'm deeply confused :) Oops. I meant 0. > > is safer since it assumes the worst case (that socreate() is called in > > non-process context and/or with locks held), but it leaves soalloc()'s > > interface bogus (it's waitfor arg is always 0). > > Right, but isn't soalloc() interface bogus already? It's "waitok" > arguments is always set to 1. As far as I can tell there only two > functions that call soalloc(): socreate() and sonewconn(). BTW Actually, the arg is currently always 0 in sonewconn() and always 1 (M_NOWAIT) in socreate(). > sonewconn() calls soalloc(0). tcp_input() is a critical caller of sonewconn() and it obviously shouldn't sleep. But sonewconn() is called from process context elsewhere. sonewconn() has used a no-wait malloc() since at least 4.4Lite2 (in Lite2 there is no soalloc() and the malloc() is done directly in sonewconn()). I think the call from tcp_input() is the usual case and/or no one cared to distinguish the other cases. Interestingly, socreate() in Lite2 always does a can-wait malloc() so our current soalloc(M_NOWAIT) does the same thing as Lite2 and is only wrong if the FreeBSD change from can-wait to "can-wait-if p != 0" change was needed and is still needed. I hope that the author of this change (wollman?) will respond. ISTR some discussion about loss of this change when the change to soalloc(M_NOWAIT) was made, but it didn't affect the result. > In my code I open socket from inside kernel, i.e. I call socreate() > directly with mutex held. This gives me "could sleep with" WITNESS > warning. I could make the mutex sleepable, but, frankly, there is > no reason to do so. It is not the end of the world for my code if > I can't create a socket. That's a better reason for making things simple by not permitting sleeping. We already do it in sonewconn(). > Perhaps socreate() could accept another flag that tell it where > it can sleep or not. Is there any other/better way? A flag seems to be needed if we actually care. I think we don't want to go that way. Many functions would have to be changed to pass the flag for the flag to actually work in all cases. Locks make the problem even more cases. It is difficult for functions to know which locks all of their callers may or do hold without passing around even more flags. Bruce To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: PATCH: typo in socreate() or i'm missing something
Bruce, Please find the attached patch for socreate() in uipc_socket.c. I think the code was supposed to call soalloc(0) rather then soalloc(M_NOWAIT). Note M_NOWAIT defined as 1. Is that a real typo or i'm missing something here? soalloc(1) was intended (and was obtained by misspelling 1 as M_NOWAIT), but that was probably wrong. Things have regressed from RELENG_4 where the code was: so = soalloc(p != 0); socreate()'s `p' arg was abused as a flag to say that the call is made in process context. In -current, the corresponding `td' is never NULL so it is harder to tell if we are in process context or if this distinction actually matters. The code first rotted to: so = soalloc(td != 0); Then it rotted to: so = soalloc(M_NOWAIT); which is equivalent to the previously rotted state since td is never NULL and M_NOWAIT happens to be 1 (boolean true). I see, however I find it very confusing to use M_NOWAIT just because it is defined as 1. I'm sure there are plenty other defines that can be used :) Why M_NOWAIT? Why just not write soalloc(1)? When I was looking at the code I got the impression that soalloc() should not sleep. Changing it to your version: so = soalloc(M_NOWAIT); huh? I hope you meant 0 here :) That is what I did in my patch. Otherwise I'm deeply confused :) is safer since it assumes the worst case (that socreate() is called in non-process context and/or with locks held), but it leaves soalloc()'s interface bogus (it's waitfor arg is always 0). Right, but isn't soalloc() interface bogus already? It's "waitok" arguments is always set to 1. As far as I can tell there only two functions that call soalloc(): socreate() and sonewconn(). BTW sonewconn() calls soalloc(0). In my code I open socket from inside kernel, i.e. I call socreate() directly with mutex held. This gives me "could sleep with" WITNESS warning. I could make the mutex sleepable, but, frankly, there is no reason to do so. It is not the end of the world for my code if I can't create a socket. Perhaps socreate() could accept another flag that tell it where it can sleep or not. Is there any other/better way? I don't trust the locking for this even in RELENG_4. Networking code liked to do things like: s = splnet(); /* "Lock" something or other. */ lotsa_stuff(); splx(s); where lotsa_stuff() calls malloc(..., M_WAITOK) from a deeply nested routine. This may or may not be a race, depending on whether the code depends on splnet() to lock _everything_ against softnet activity until the splx(). I'm not sure if this happens for socreate(). Eventually, locking bugs in this area will be caught by locking assertions. I think they mostly aren't now, since the interrupt context check in malloc() has rotted and other checks aren't reached unless malloc() actually sleeps (which rarely happens). thanks, max To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: PATCH: typo in socreate() or i'm missing something
On Thu, 27 Feb 2003, Maksim Yevmenkin wrote: > Please find the attached patch for socreate() in uipc_socket.c. > I think the code was supposed to call soalloc(0) rather then > soalloc(M_NOWAIT). Note M_NOWAIT defined as 1. > > Is that a real typo or i'm missing something here? soalloc(1) was intended (and was obtained by misspelling 1 as M_NOWAIT), but that was probably wrong. Things have regressed from RELENG_4 where the code was: so = soalloc(p != 0); socreate()'s `p' arg was abused as a flag to say that the call is made in process context. In -current, the corresponding `td' is never NULL so it is harder to tell if we are in process context or if this distinction actually matters. The code first rotted to: so = soalloc(td != 0); Then it rotted to: so = soalloc(M_NOWAIT); which is equivalent to the previously rotted state since td is never NULL and M_NOWAIT happens to be 1 (boolean true). Changing it to your version: so = soalloc(M_NOWAIT); is safer since it assumes the worst case (that socreate() is called in non-process context and/or with locks held), but it leaves soalloc()'s interface bogus (it's waitfor arg is always 0). I don't trust the locking for this even in RELENG_4. Networking code liked to do things like: s = splnet(); /* "Lock" something or other. */ lotsa_stuff(); splx(s); where lotsa_stuff() calls malloc(..., M_WAITOK) from a deeply nested routine. This may or may not be a race, depending on whether the code depends on splnet() to lock _everything_ against softnet activity until the splx(). I'm not sure if this happens for socreate(). Eventually, locking bugs in this area will be caught by locking assertions. I think they mostly aren't now, since the interrupt context check in malloc() has rotted and other checks aren't reached unless malloc() actually sleeps (which rarely happens). Bruce To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
PATCH: typo in socreate() or i'm missing something
Dear Hackets, Please find the attached patch for socreate() in uipc_socket.c. I think the code was supposed to call soalloc(0) rather then soalloc(M_NOWAIT). Note M_NOWAIT defined as 1. Is that a real typo or i'm missing something here? thanks, max --- uipc_socket.c.orig Thu Feb 27 15:24:52 2003 +++ uipc_socket.c Thu Feb 27 15:25:08 2003 @@ -194,7 +194,7 @@ if (prp->pr_type != type) return (EPROTOTYPE); - so = soalloc(M_NOWAIT); + so = soalloc(0); if (so == NULL) return (ENOBUFS);
