Date: Wed, 29 May 2002 11:26:26 -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.0205291057020.12315-100000@InterJet.elischer.org> In-Reply-To: <XFMail.20020529124710.jhb@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 29 May 2002, John Baldwin wrote:
>
> 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?
to catch places in 3rd party code that may have the old completely
different one.. it matches td_state and kse_state..
>
> 2) I object to removing cred_free_thread(). It isn't getting in your
> way and some of us like to test things.
that doesn't test anything.. it just complicates the code, and worse,
it changes the way that the code behaves in debug mode from the way it
behaves in normal mode..
We've had this argument so many times before..
I didn't remove it.. I never put it back...It was gone from the KSE code
long before the fight in -current.. One reason I removed it from
current was diff-reduction to KSE :-)..
feel free to add it back to KSE, pointless though it is..
>
> 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?
Because in a threaded world, setrunqueue() also makes it
runnable (see later for reason). that's why I removed ALL the setting of
threads to runnable..
It's part of the atomic operation (under locks) of adding it ot the run
queue. I never have a state in a thread that does not correctly reflect
its queued state.. (at least that is my aim.). If the flags/state say it
is runnable and on the run queue (TDS_RUNQ) then it IS on the run queue,
not "soon to be on the run queue". This allows me to accuratly control
the queues by BELIEVING what the state says. The same queue entry is used
for other queues so by making hte state accuratly reflect what queue it
is on I can know how to handle it without having to know information
that is outside my scope.
If it is actually running it is NOT in state TDS_RUNQ but in
state BTDS_RUNNING.
etc. etc.. i.e. the function that puts it on the run queue (setrunqueue())
alse sets the state saying what queue it is on. Until that moment it is
not on the run queue so the state is not altered.. (It is probably in
state TDS_UNQUEUED). If there were a function to put it on the sleep
queue, then that is where the state would be set to TDS_SLEEP, but that
code is not so clean and it happens in more than one place (sleep +
condvars etc.). My eventual aim is that there be methods to put the thread
into each state, similar to setrunqueue(), and that each method both
queues the thread, AND sets the state. setrunqueue() is just the first.
"Why?"
Remember that in a threaded world, threads state changes are more
complicated as they may or may not result in changes to procs and KSEs
as well, thus it makes more sense to hide thread state changes behind
opaque methods, so that they can be changed and debugged
easier.
>
> 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
hmm ok probably I had changes there that were backed out at some stage.
(like debugging printfs or something..)
>
> 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..
>
> 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.
AH so you dunno either :-)
>
> 7) Please follow the established convention for names of members in
> pcb_ext and call 'refcount' 'ext_refccount'.
ok, actually I think it can be removed.. I'll have to check..
>
> 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 :-)
>
> 9) More gratuitous braces as well as gratuituos ()'s and white space
> changes in ithread_schedule() obfuscate the functional diffs.
I guess so though it made it a hell of a lot more readable to me.
>
> 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).
I'll go take a look
>
> 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.
probably a mis-merge.. will check
>
> 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.
A rose is a rose.. sure..
>
> 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.
>
> 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?
will check
>
> 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. :)
ok
>
> 18) I still don't see what's so hard about LIST_FOREACH() with
> allproc.
I have FOREACH_THREAD_IN_PROC() etc... it just fill sout the set..
and why NOT have it? I think it's a bit more readble and it
fits into the set.
>
> --
>
> John Baldwin <jhb@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/
> "Power Users Use the Power to Serve!" - http://www.FreeBSD.org/
>
Thanks for the comments..
I'll try resolve these today
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.0205291057020.12315-100000>