Date: Fri, 28 Feb 2003 10:25:52 -0800 From: Maksim Yevmenkin <myevmenk@exodus.net> To: Bruce Evans <bde@zeta.org.au>, current@FreeBSD.ORG Subject: Re: PATCH: typo in socreate() or i'm missing something Message-ID: <3E5FA9B0.90306@exodus.net> References: <20030228212340.Q22122-100000@gamplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3E5FA9B0.90306>