All of lore.kernel.org
 help / color / mirror / Atom feed
From: steve.capper@linaro.org (Steve Capper)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH V4] arm64: percpu: Implement this_cpu operations
Date: Wed, 19 Nov 2014 15:49:21 +0000	[thread overview]
Message-ID: <20141119154920.GA25504@linaro.org> (raw)
In-Reply-To: <20141117104010.GB18061@arm.com>

On Mon, Nov 17, 2014 at 10:40:11AM +0000, Will Deacon wrote:
> Hi Steve,

Hey Will,

[sorry for the late reply, was recovering from plague]

> 
> On Fri, Nov 14, 2014 at 03:03:33PM +0000, Steve Capper wrote:
> > The generic this_cpu operations disable interrupts to ensure that the
> > requested operation is protected from pre-emption. For arm64, this is
> > overkill and can hurt throughput and latency.
> > 
> > This patch provides arm64 specific implementations for the this_cpu
> > operations. Rather than disable interrupts, we use the exclusive
> > monitor or atomic operations as appropriate.
> > 
> > The following operations are implemented: add, add_return, and, or,
> > read, write, xchg. We also wire up a cmpxchg implementation from
> > cmpxchg.h.
> > 
> > Testing was performed using the percpu_test module and hackbench on a
> > Juno board running 3.18-rc4.
> 
> Looks good. I notice that this change drops the compiler barriers too,
> which we used to get via local_irq_{enable/disable}. I *think* that's fine
> (at least, I couldn't find a place that breaks due to that) but it would be
> nice to know that it was deliberate :)

Yes it was deliberate. :-)

My understanding of the this_cpu... set of accessors is that they are
there to solely protect the access with respect to pre-emption, not other
CPUs, thus we shouldn't need barriers?

> 
> > +static inline void __percpu_write(void *ptr, unsigned long val, int size)
> > +{
> > +	switch (size) {
> > +	case 1:
> > +		ACCESS_ONCE(*((u8 *)ptr)) = (u8) val;
> > +		break;
> > +	case 2:
> > +		ACCESS_ONCE(*((u16 *)ptr)) = (u16) val;
> > +		break;
> > +	case 4:
> > +		ACCESS_ONCE(*((u32 *)ptr)) = (u32) val;
> > +		break;
> > +	case 8:
> > +		ACCESS_ONCE(*((u64 *)ptr)) = (u64) val;
> > +		break;
> 
> I think you've gone a bit overboard with brackets and spacing here. Can't
> you just do something like:
> 
>   ACCESS_ONCE(*(u64 *)ptr) = (u64)val;

Yeah, I went a little paranthesis crazy there, I'll correct this lest
it gets confused with LISP.

> 
> Anyway:
> 
>   Reviewed-by: Will Deacon <will.deacon@arm.com>

Ta Will,
I'll send out a V5 with the parantheses sanetised.

Cheers,
-- 
Steve

  reply	other threads:[~2014-11-19 15:49 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-11-06 11:12 [PATCH] arm64: percpu: Implement this_cpu operations Steve Capper
2014-11-06 11:26 ` Ard Biesheuvel
2014-11-06 11:52   ` Steve Capper
2014-11-06 12:23     ` [PATCH V2] " Steve Capper
2014-11-06 12:27 ` [PATCH] " Will Deacon
2014-11-07 13:52   ` Steve Capper
2014-11-14 11:37     ` [PATCH V3] " Steve Capper
2014-11-14 13:46       ` Will Deacon
2014-11-14 15:03         ` [PATCH V4] " Steve Capper
2014-11-17 10:40           ` Will Deacon
2014-11-19 15:49             ` Steve Capper [this message]
2014-11-19 16:53               ` [PATCH V5] " Steve Capper

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20141119154920.GA25504@linaro.org \
    --to=steve.capper@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.