From owner-freebsd-arch@FreeBSD.ORG Mon Jul 2 23:25:18 2007 Return-Path: X-Original-To: arch@freebsd.org Delivered-To: freebsd-arch@FreeBSD.ORG Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 8EAD016A41F for ; Mon, 2 Jul 2007 23:25:18 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from ug-out-1314.google.com (ug-out-1314.google.com [66.249.92.170]) by mx1.freebsd.org (Postfix) with ESMTP id 18E2013C44B for ; Mon, 2 Jul 2007 23:25:17 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: by ug-out-1314.google.com with SMTP id o4so1010901uge for ; Mon, 02 Jul 2007 16:25:17 -0700 (PDT) DKIM-Signature: a=rsa-sha1; c=relaxed/relaxed; d=gmail.com; s=beta; h=domainkey-signature:received:received:message-id:date:from:sender:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references:x-google-sender-auth; b=adkLGzqBdbi4iwKcgVLbrXMvQW1hRn8cnZB+5o14GidoAq+TOe7ks+mRx1PGbtWinL6r0wSaoMa97KvCraRYJtKIq32aq5lOxzKzC5WNEXAwE7K4tkbzNpk3OFuTXIT6vAxiX56zKJ9pJu05oXgXsl/M5wU8Lg7qZSYVBiTRSN4= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=beta; h=received:message-id:date:from:sender:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references:x-google-sender-auth; b=Qg4j+kDFEurW9+824PKNHqsZynXYG8tlOE77DY3bHiYka6UpA0L67/+VhYzpdXbpCoKK2gQoj793BRlqpXufE4IVC8PCdxCWlqKOJE4LmOI1rdUtFIkhTzTk4c5A05HwiALBH3WDFQloS0Apv1AktyXpyxpzC5wTN6OglzkOHa4= Received: by 10.78.147.6 with SMTP id u6mr3215887hud.1183417088012; Mon, 02 Jul 2007 15:58:08 -0700 (PDT) Received: by 10.78.97.18 with HTTP; Mon, 2 Jul 2007 15:58:07 -0700 (PDT) Message-ID: <3bbf2fe10707021558i413eed3cv5c5f38a820e9d249@mail.gmail.com> Date: Tue, 3 Jul 2007 00:58:07 +0200 From: "Attilio Rao" Sender: asmrookie@gmail.com To: "Jeff Roberson" In-Reply-To: <20070701160540.Y552@10.0.0.1> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <20070701160540.Y552@10.0.0.1> X-Google-Sender-Auth: 549b63d37d858390 Cc: arch@freebsd.org Subject: Re: wakeup_flags patch. X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 02 Jul 2007 23:25:18 -0000 2007/7/2, Jeff Roberson : > http://people.freebsd.org/~jeff/wakeupflags.diff > > It didn't workout very cleanly since the flags have to go through three > layers. I could define wakeup and sleepq flags to be the same and skip a > bunch of conditionals. However, we'd then have to know which flags were > free to use in each case. Are there any further opinions on the style? Honestly, IMHO we should not depend by precise values of flags passed between layers. Your code is clean enough to not require those magics too. Specific to the patch: > +void > +wakeup_flags(void *ident, int flags) > +{ > + int sqflag; > + > + sqflag = SLEEPQ_SLEEP; > + if (flags & WAKEUP_LOCAL) > + sqflag |= SLEEPQ_LOCAL; > + if (flags & WAKEUP_TAIL) > + sqflag |= SLEEPQ_TAIL; > + sleepq_lock(ident); > + if (flags & WAKEUP_ONE) { > + sleepq_signal(ident, sqflag, -1, 0); > + sleepq_release(ident); > + } else if (flags & WAKEUP_ALL) > + sleepq_broadcast(ident, sqflag, -1, 0); > + else > + panic("wakeup_flags: Bad flags %d\n", flags); > +} > + I would arrange this a little bit differently in order to make it homogeneous with other locking primitives: void wakeup_flags(void *ident, int flags) { int sqflag; MPASS(flags & (WAKEUP_ONE | WAKEUP_ALL)); ... if (flags & WAKEUP_ONE) { sleepq_signal(ident, sqflag, -1, 0); sleepq_release(ident); } else sleepq_broadcast(ident, sqflag, -1, 0); } > @@ -943,7 +943,7 @@ > resetpriority(td); > } > td->td_slptime = 0; > - sched_add(td, SRQ_BORING); > + sched_add(td, SRQ_BORING|flags); > } This doesn't imply a style violation? (There are several of this). It should be: sched_add(td, SRQ_BORING | flags); > @@ -716,13 +718,17 @@ > * the tail of sleep queues. > */ > besttd = NULL; > - TAILQ_FOREACH(td, &sq->sq_blocked[queue], td_slpq) { > - if (besttd == NULL || td->td_priority < besttd->td_priority) > - besttd = td; > - } > + if ((flags & SLEEPQ_TAIL) == 0) { > + TAILQ_FOREACH(td, &sq->sq_blocked[queue], td_slpq) { > + if (besttd == NULL || > + td->td_priority < besttd->td_priority) > + besttd = td; > + } > + } else All those brackets here are not really necessary, IMHO. > -void setrunnable(struct thread *); > +void setrunnable(struct thread *, int); Wouldn't be better to have style(9) matching prototypes (with named arguments)? > @@ -88,6 +88,8 @@ > #define SLEEPQ_PAUSE 0x02 /* Used by pause. */ > #define SLEEPQ_SX 0x03 /* Used by an sx lock. */ > #define SLEEPQ_INTERRUPTIBLE 0x100 /* Sleep is interruptible. */ > +#define SLEEPQ_LOCAL 0x200 /* Wake-up on the local cpu. */ > +#define SLEEPQ_TAIL 0x400 /* Wake-up from the tail. */ This is not related to your patch, but this let me think that we should have an effective mask of 0xFF for filtering between consumers and flags (in particular when consumers are checked). However, actually sleepqueues consumers identifier isn't basically used. However, I'd like to see this ported for condvar since I strongly hope one day msleep/wakeup will be deprecated in favor of condvar. Thanks, Attilio -- Peace can only be achieved by understanding - A. Einstein