From owner-freebsd-current Sat Mar 1 3:16:45 2003 Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 4EEC337B401; Sat, 1 Mar 2003 03:16:43 -0800 (PST) Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by mx1.FreeBSD.org (Postfix) with ESMTP id B40E743FAF; Sat, 1 Mar 2003 03:16:41 -0800 (PST) (envelope-from bde@zeta.org.au) Received: from gamplex.bde.org (katana.zip.com.au [61.8.7.246]) by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id WAA07250; Sat, 1 Mar 2003 22:16:34 +1100 Date: Sat, 1 Mar 2003 22:18:12 +1100 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: Maksim Yevmenkin Cc: current@FreeBSD.ORG, Subject: Re: PATCH: typo in socreate() or i'm missing something In-Reply-To: <3E5FA9B0.90306@exodus.net> Message-ID: <20030301202521.D25845-100000@gamplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG 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 majordomo@FreeBSD.org with "unsubscribe freebsd-current" in the body of the message