Date: Wed, 29 May 2002 15:45:50 -0700 (PDT) From: Julian Elischer <julian@elischer.org> To: John Baldwin <jhb@FreeBSD.org> Cc: FreeBSD current users <current@freebsd.org> Subject: RE: Seeking OK to commit KSE MIII Message-ID: <Pine.BSF.4.21.0205291403090.12315-100000@InterJet.elischer.org> In-Reply-To: <Pine.BSF.4.21.0205291057020.12315-100000@InterJet.elischer.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 29 May 2002, Julian Elischer wrote:
>
>
> On Wed, 29 May 2002, John Baldwin wrote:
>
> >
> > 4) It would be nice if you didn't mix in gratuitous style changes with
> > actual content changes such as extra braces in else clause of
> > PROCFS_CTRL_WAIT case in procfs_ctl.c
I don't consider that to be a "gratuitous style change".
I'm rewriting the clause and consider that as a 3 line clasue with 2
levels of indent it makes it more "error resistant" to have the braces..
The 'then' clause has braces so teh else one should too.
if (foo) {
fumble();
futz();
} else
bah;
always really annoys me.
If I'm looking for the end of the if statement, I'm looing for a '}'..
>
> >
> > 5) The comment by thread_unsuspend() in procfs_ctl.c seems gratuitous,
> > that is what one would expect from such a function.
>
> I'll go take a look in a minute..
+ thread_unsuspend(p); /* If it can run, let it do so. */
ok, the reason is that "thread_unsuspend may NOT result in the thread
running as it may still be blocked by other "suspend types".
It is possible that the correct answer is to rename thread_unsuspend()
to proc_check_unsuspend(). There are three reasons a process may be
suspended and we've only released one of them here.
It was called thread_unsuspend() because it is one of a suite of functions
that are in the thread control code, but it's function really is
process-wide so it should probably start with proc_..
thoughts?
>
>
> >
> > 6) In cpu_set_retval() you have:
> >
> > + frame->tf_edx = aux; /* I dunno */
[..]
> >
I'll make is a bit more correct
>
> >
> > 7) Please follow the established convention for names of members in
> > pcb_ext and call 'refcount' 'ext_refccount'.
>
removed
>
> >
> > 8) It would be nice if the CURSIG() -> cursig() change were committed
> > separately to avoid obfuscating the other diffs.
>
> yeah that is a "mini" thing :-)
I'll see what I can do.
> >
> > 10) In kern_sig.c, prototypes are declared on one line and near the
> > beginning of the file, not in the middle of code (tdsignal).
>
Fixed
> >
> > 11) Why is there a whole chunk of code #if 0'd out in kern_sig.c?
It's contriversial..
matt and I feel you shouldn't be checking this stop state in issignal
but rather at kernel exit. I have other code that does this
(thread_suspend_check()) but there is some chance I may need to re-enable
this code if people notice a change in behaviour.
> >
> > 12) You lock p_pptr twice (can't do that) before psignal(). Looks
> > like you added an extra one along with gratuitous braces and a
> > whitespace change.
>
fixed
>
> >
> > 13) Could you call readjustrunqueue() runq_readjust() to follow the
> > other namespaces already in kern_switch.c? Also, I find it easier
> > to read personally.
done
>
> >
> > 14) Hmm, I'm not sure why you need TDF_INMSLEEP if a KSE always has
> > a spare thread that is replenished first thing when the spare
> > thread prepares to do an upcall.
>
> It was a leftover that mini didn't remove.. I'll investigate if it's still
> needed.
There may still be a race, however as we call
thread_alloc() from thread_schedule_upcall() which is in turn called
from within msleep().. personally this just protects us from trying to
recurse if we end up trying to sleep because we are trying to allocate a
new one.
I'm not convinced that this recursion cannot happen, even with the new
allocation code.
>
> >
> > 15) The 'um... dunno' comment in abortsleep() is a bit unsettling, do
> > you think you could clarify / run some tests to figure out exactly
> > what is going on there?
I still dunno :-)
I'll look into it more over the next couple of days..
> >
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?Pine.BSF.4.21.0205291403090.12315-100000>