Date: Sat, 1 Mar 2003 22:18:12 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: Maksim Yevmenkin <myevmenk@exodus.net> Cc: current@FreeBSD.ORG, <wollman@FreeBSD.ORG> Subject: Re: PATCH: typo in socreate() or i'm missing something Message-ID: <20030301202521.D25845-100000@gamplex.bde.org> In-Reply-To: <3E5FA9B0.90306@exodus.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20030301202521.D25845-100000>