From owner-freebsd-current Wed May 29 10:12:29 2002 Delivered-To: freebsd-current@freebsd.org Received: from mail.speakeasy.net (mail16.speakeasy.net [216.254.0.216]) by hub.freebsd.org (Postfix) with ESMTP id 07D5D37B8A0 for ; Wed, 29 May 2002 09:49:40 -0700 (PDT) Received: (qmail 28807 invoked from network); 29 May 2002 16:47:47 -0000 Received: from unknown (HELO server.baldwin.cx) ([216.27.160.63]) (envelope-sender ) by mail16.speakeasy.net (qmail-ldap-1.03) with DES-CBC3-SHA encrypted SMTP for ; 29 May 2002 16:47:47 -0000 Received: from laptop.baldwin.cx (gw1.twc.weather.com [216.133.140.1]) by server.baldwin.cx (8.11.6/8.11.6) with ESMTP id g4TGlkF28301; Wed, 29 May 2002 12:47:47 -0400 (EDT) (envelope-from jhb@FreeBSD.org) Message-ID: X-Mailer: XFMail 1.5.2 on FreeBSD X-Priority: 3 (Normal) Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 8bit MIME-Version: 1.0 In-Reply-To: Date: Wed, 29 May 2002 12:47:10 -0400 (EDT) From: John Baldwin To: Julian Elischer Subject: RE: Seeking OK to commit KSE MIII Cc: FreeBSD current users 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 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 <>< 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