From owner-freebsd-current Fri Feb 28 11:15:58 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 4574A37B43A for ; Fri, 28 Feb 2003 11:15:38 -0800 (PST) Received: from scl8owa02.int.exodus.net (scl8out02.exodus.net [66.35.230.242]) by mx1.FreeBSD.org (Postfix) with ESMTP id 09651441F7 for ; Fri, 28 Feb 2003 10:29:05 -0800 (PST) (envelope-from Maksim.Yevmenkin@cw.com) Received: from scl8owa01.int.exodus.net ([66.35.230.241]) by scl8owa02.int.exodus.net with Microsoft SMTPSVC(5.0.2195.5329); Fri, 28 Feb 2003 10:29:04 -0800 Received: from exodus.net ([165.193.27.35]) by scl8owa01.int.exodus.net over TLS secured channel with Microsoft SMTPSVC(5.0.2195.5329); Fri, 28 Feb 2003 10:29:04 -0800 Message-ID: <3E5FA9B0.90306@exodus.net> Date: Fri, 28 Feb 2003 10:25:52 -0800 From: Maksim Yevmenkin User-Agent: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.1) Gecko/20021126 X-Accept-Language: ru, en-us, en MIME-Version: 1.0 To: Bruce Evans , current@FreeBSD.ORG Subject: Re: PATCH: typo in socreate() or i'm missing something References: <20030228212340.Q22122-100000@gamplex.bde.org> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit X-OriginalArrivalTime: 28 Feb 2003 18:29:04.0333 (UTC) FILETIME=[45A427D0:01C2DF57] 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 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 majordomo@FreeBSD.org with "unsubscribe freebsd-current" in the body of the message