Date: Wed, 29 May 2002 12:47:10 -0400 (EDT) From: John Baldwin <jhb@FreeBSD.org> To: Julian Elischer <julian@elischer.org> Cc: FreeBSD current users <current@freebsd.org> Subject: RE: Seeking OK to commit KSE MIII Message-ID: <XFMail.20020529124710.jhb@FreeBSD.org> In-Reply-To: <Pine.BSF.4.21.0205281517530.12315-100000@InterJet.elischer.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 28-May-2002 Julian Elischer wrote:
> Now things are moving again.
> Jonathon Mini and I have cleaned up the patches a bit
> and fixed the more obvious problems..
> from teh point of view of non KSE processes
> (that would be all of them at the moment) it shuold act the
> same as now. Hopefully, changes are restricted to instruction flows that
> would only occur in KSE processes.
>
> I would like to commit this in the next couple of days to give it enough
> time to settle BEFORE USENIX.
Ok, here are the random comments I've come up with:
1) Why gratuitous rename of p_stat to p_state?
2) I object to removing cred_free_thread(). It isn't getting in your
way and some of us like to test things.
3) Why did you not make the linux process in linux_machdep.c runnable
before you did a setrunqueue()? You do know we create it
stopped at first and then make it runnable after doing fixups
after fork1(), right?
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
5) The comment by thread_unsuspend() in procfs_ctl.c seems gratuitous,
that is what one would expect from such a function.
6) In cpu_set_retval() you have:
+void
+cpu_set_retval(struct thread *td, int retval, int aux, int success)
+{
+ struct trapframe *frame;
+
+ frame = td->td_frame;
+ frame->tf_eax = retval; /* Child returns zero */
+ frame->tf_edx = aux; /* I dunno */
You could always ask about that instead of having a I dunno comment. :)
I think that we no longer use 2 return values from syscalls for FreeBSD
syscalls (I know we did for fork1() at one point, possibly still do
so that 4.x libc works ok on 5.x kernel). Linux does depend on edx being
preserved across a syscall though IIRC.
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.
9) More gratuitous braces as well as gratuituos ()'s and white space
changes in ithread_schedule() obfuscate the functional diffs.
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?
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.
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?
16) I don't think we need the P_SPARE's.
17) I think SINGLE_WAIT/EXIT is slightly more readable than
SNGLE_WAIT/EXIT. :)
18) I still don't see what's so hard about LIST_FOREACH() with
allproc.
--
John Baldwin <jhb@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve!" - http://www.FreeBSD.org/
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?XFMail.20020529124710.jhb>