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) {
} else

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_..


> > 
> > 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'.


> > 
> > 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).


> > 
> > 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.


> > 
> > 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.


> > 
> > 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 [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message

Reply via email to