From owner-freebsd-current Wed May 29 11:41:22 2002 Delivered-To: freebsd-current@freebsd.org Received: from rwcrmhc51.attbi.com (rwcrmhc51.attbi.com [204.127.198.38]) by hub.freebsd.org (Postfix) with ESMTP id 6E29837B428; Wed, 29 May 2002 11:40:12 -0700 (PDT) Received: from InterJet.elischer.org ([12.232.206.8]) by rwcrmhc51.attbi.com (InterMail vM.4.01.03.27 201-229-121-127-20010626) with ESMTP id <20020529184011.MTIY11426.rwcrmhc51.attbi.com@InterJet.elischer.org>; Wed, 29 May 2002 18:40:11 +0000 Received: from localhost (localhost.elischer.org [127.0.0.1]) by InterJet.elischer.org (8.9.1a/8.9.1) with ESMTP id LAA18253; Wed, 29 May 2002 11:26:28 -0700 (PDT) Date: Wed, 29 May 2002 11:26:26 -0700 (PDT) From: Julian Elischer To: John Baldwin Cc: FreeBSD current users Subject: RE: Seeking OK to commit KSE MIII In-Reply-To: Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG 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 <>< 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