Date: Thu, 13 Sep 2012 15:38:54 +0100 From: Attilio Rao <attilio@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: Davide Italiano <davide@freebsd.org>, mlaier@freebsd.org, svn-src-projects@freebsd.org, Konstantin Belousov <kostikbel@gmail.com>, src-committers@freebsd.org, Stephan Uphoff <ups@freebsd.org> Subject: Re: svn commit: r238907 - projects/calloutng/sys/kern Message-ID: <CAJ-FndASH1=i4ozwP=YepF58iC_5%2Bnf4L4MCu3%2B2-xB9FVzyvg@mail.gmail.com> In-Reply-To: <201209130910.50876.jhb@freebsd.org> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201208021707.22356.jhb@freebsd.org> <CAJ-FndA8Yende_=-hgOMjfUkQVhaSdSjAb0W8xthqN1ThwT=Vg@mail.gmail.com> <201209130910.50876.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Sep 13, 2012 at 2:10 PM, John Baldwin <jhb@freebsd.org> wrote:
> On Wednesday, September 12, 2012 9:36:58 pm Attilio Rao wrote:
>> On Thu, Aug 2, 2012 at 10:07 PM, John Baldwin <jhb@freebsd.org> wrote:
>> > On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote:
>> >> On 7/30/12, John Baldwin <jhb@freebsd.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-FndASH1=i4ozwP=YepF58iC_5%2Bnf4L4MCu3%2B2-xB9FVzyvg>