From owner-svn-src-projects@FreeBSD.ORG Thu Sep 13 14:38:58 2012 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 0678E1065670; Thu, 13 Sep 2012 14:38:58 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-lb0-f182.google.com (mail-lb0-f182.google.com [209.85.217.182]) by mx1.freebsd.org (Postfix) with ESMTP id 7A8F78FC15; Thu, 13 Sep 2012 14:38:56 +0000 (UTC) Received: by lbbgg13 with SMTP id gg13so2446899lbb.13 for ; Thu, 13 Sep 2012 07:38:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:reply-to:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; bh=11Tad2H4YfDJc7Zp4xWH1TbWvkdGF+V+xRh5yN9b2sY=; b=WwSDAcoP+1kziIaIeN4nbiyIJYQOIaBSIrJzOo97kkDRExNYk9+vk85vWLEIEnzJgJ C51euZMU6LvUcDZvp2mA1fmxJpMNnmvzXRd0/lqpGyc5SeU3KC7ywwZOqfWCAtC2E5m8 zDaQK48NQMILtheyfOhCT6cDymMIlZZzK+bzVGPkOvig5G9bcbLAvstI2RvsVTBA2H7m z9m6NpcU2qiA2qTXJ7sAtNuhx1Y0+wfONxJRkfoJOhc9RKdwZdA+vw7olRz4LNgmZU1G WufiWOyWHb89J6gw2xA5c5YHmbaKapEaiLm8yZocgJHqlDOeIm79Q74y/eQ3wwx4NCHh GhaQ== MIME-Version: 1.0 Received: by 10.112.86.41 with SMTP id m9mr996634lbz.108.1347547135147; Thu, 13 Sep 2012 07:38:55 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.112.102.39 with HTTP; Thu, 13 Sep 2012 07:38:54 -0700 (PDT) In-Reply-To: <201209130910.50876.jhb@freebsd.org> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201208021707.22356.jhb@freebsd.org> <201209130910.50876.jhb@freebsd.org> Date: Thu, 13 Sep 2012 15:38:54 +0100 X-Google-Sender-Auth: BAi4PyoHE3Kzhcgl9AuwIj1Q-co Message-ID: From: Attilio Rao To: John Baldwin Content-Type: text/plain; charset=UTF-8 Cc: Davide Italiano , mlaier@freebsd.org, svn-src-projects@freebsd.org, Konstantin Belousov , src-committers@freebsd.org, Stephan Uphoff Subject: Re: svn commit: r238907 - projects/calloutng/sys/kern X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: attilio@FreeBSD.org List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 13 Sep 2012 14:38:58 -0000 On Thu, Sep 13, 2012 at 2:10 PM, John Baldwin wrote: > On Wednesday, September 12, 2012 9:36:58 pm Attilio Rao wrote: >> On Thu, Aug 2, 2012 at 10:07 PM, John Baldwin wrote: >> > On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote: >> >> On 7/30/12, John Baldwin wrote: >> >> > --- //depot/projects/smpng/sys/kern/kern_rmlock.c 2012-03-25 >> >> > 18:45:29.000000000 0000 >> >> > +++ //depot/user/jhb/lock/kern/kern_rmlock.c 2012-06-18 21:20:58.000000000 >> >> > 0000 >> >> > @@ -70,6 +70,9 @@ >> >> > } >> >> > >> >> > static void assert_rm(const struct lock_object *lock, int what); >> >> > +#ifdef DDB >> >> > +static void db_show_rm(const struct lock_object *lock); >> >> > +#endif >> >> > static void lock_rm(struct lock_object *lock, int how); >> >> > #ifdef KDTRACE_HOOKS >> >> > static int owner_rm(const struct lock_object *lock, struct thread >> >> > **owner); >> >> >> >> While here, did you consider also: >> >> - Abstracting compiler_memory_barrier() into a MI, compiler dependent function? >> >> - Fix rm_queue with DCPU possibly >> > >> > Mostly I just wanted to fill in missing functionality and fixup the >> > RM_SLEEPABLE bits a bit. >> >> So what do you think about the following patch? If you agree I will >> send to pho@ for testing in a batch with other patches. > > It's not super clear to me that having it be static vs dynamic is all that > big of a deal. However, your approach in general is better, and it certainly > should have been using PCPU_GET() for the curcpu case all along rather than > inlining pcpu_find(). You mean what is the performance difference between static vs dynamic? Or you mean, why we want such patch at all? In the former question there is a further indirection (pc_dynamic access), for the latter question the patched code avoids namespace pollution at all and makes the code more readable. >> --- a/sys/kern/kern_rmlock.c >> +++ b/sys/kern/kern_rmlock.c >> @@ -167,13 +169,12 @@ rm_tracker_remove(struct pcpu *pc, struct >> rm_priotracker *tracker) >> static void >> rm_cleanIPI(void *arg) >> { >> - struct pcpu *pc; >> struct rmlock *rm = arg; >> struct rm_priotracker *tracker; >> - struct rm_queue *queue; >> - pc = pcpu_find(curcpu); >> + struct rm_queue *queue, *pcpu_rm_queue; >> + pcpu_rm_queue = DPCPU_PTR(rm_queue); > > Can you fix the old style bug of not having a blank line after the > variable declarations? bde@ has sent me a lot of notes on how the files I'm touching are style-broken already. I'm trying to aligning to existing style right now, and will fix all the style bugs pointed out by Bruce (and you) in separate commits. Thus, for the moment, I would leave these chunks like they are now. >> - for (queue = pc->pc_rm_queue.rmq_next; queue != &pc->pc_rm_queue; >> + for (queue = pcpu_rm_queue->rmq_next; queue != NULL; >> queue = queue->rmq_next) { > > It would be nice to use one of the queue macros rather than doing the > list management by hand, but perhaps that isn't possible (and that > should be a separate change even if it possible). I agree on the separate change, not 100% sure if this is feasible but I will check once this goes in. So is the patch approved by you? Attilio -- Peace can only be achieved by understanding - A. Einstein