Date: Thu, 13 Sep 2012 09:10:50 -0400 From: John Baldwin <jhb@freebsd.org> To: attilio@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: <201209130910.50876.jhb@freebsd.org> In-Reply-To: <CAJ-FndA8Yende_=-hgOMjfUkQVhaSdSjAb0W8xthqN1ThwT=Vg@mail.gmail.com> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201208021707.22356.jhb@freebsd.org> <CAJ-FndA8Yende_=-hgOMjfUkQVhaSdSjAb0W8xthqN1ThwT=Vg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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().
> --- 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?
> - 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).
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201209130910.50876.jhb>