linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: mark.rutland@arm.com (Mark Rutland)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2] arm: Added support for getcpu() vDSO using TPIDRURW
Date: Wed, 5 Oct 2016 21:12:54 +0100	[thread overview]
Message-ID: <20161005201254.GA26721@remoulade> (raw)
In-Reply-To: <CAKdL+dS4_My6hyMEGNc65mzDapia_tMiVzZ9DMw=ddZM+XiwAw@mail.gmail.com>

On Wed, Oct 05, 2016 at 02:25:22PM +0200, Fredrik Markstr?m wrote:
> On Tue, Oct 4, 2016 at 7:08 PM Mark Rutland <mark.rutland@arm.com> wrote:
> > On Tue, Oct 04, 2016 at 05:35:33PM +0200, Fredrik Markstrom wrote:
> The way I was trying to defend the breakage was by reasoning that that if it
> was an ABI we broke it both with a4780ad and with 6a1c531, and since we don't
> break ABI:s, it can't be one.

Prior to commit 6a1c531, other programs and/or cpuidle could have corrupted
TPIDRURW to arbitrary values at any point in time, so it couldn't have been
relied upon.

Arguably, someone could have (ab)used TPIDRURW between commits 6a1c531 and
a4780ad to detect context switches, but in practice they don't appear to have,
and we know of an established user relying on the current behaviour.

For better or worse, the current behaviour is ABI.

> > I was under the impression that other mechanisms were being considered
> > for fast userspace access to per-cpu data structures, e.g. restartable
> > sequences. What is the state of those? Why is this better?
> >
> > If getcpu() specifically is necessary, is there no other way to
> > implement it?
> 
> If you are referring to the user space stuff can probably be implemented
> other ways, it's just convenient since the interface is there and it will
> speed up stuff like lttng without modifications (well, except glibc). It's
> also already implemented as a vDSO on other major architectures (like x86,
> x86_64, ppc32 and ppc64).
> 
> If you are referring to the implementation of the vdso call, there are other
> possibilities, but I haven't found any that doesn't introduce overhead in
> context switching.
>
> But if TPIDRURW is definitely a no go, I can work on a patch that does this
> with a thread notifier and the vdso data page. Would that be a viable option?

As pointed out, that won't work for SMP, but perhaps we can come up with
something that does.

> > > +notrace int __vdso_getcpu(unsigned int *cpup, unsigned int *nodep,
> > > +                       struct getcpu_cache *tcache)
> > > +{
> > > +     unsigned long node_and_cpu;
> > > +
> > > +     asm("mrc p15, 0, %0, c13, c0, 2\n" : "=r"(node_and_cpu));
> > > +
> > > +     if (nodep)
> > > +             *nodep = cpu_to_node(node_and_cpu >> 16);
> > > +     if (cpup)
> > > +             *cpup  = node_and_cpu & 0xffffUL;
> >
> > Given this is directly user-accessible, this format is a de-facto ABI,
> > even if it's not documented as such. Is this definitely the format you
> > want long-term?
> 
> Yes, this (the interface) is indeed the important part and therefore I tried
> not to invent anything on my own.  This is the interface used by ppc32,
> ppc64, x86, x86_64. It's also this is how the getcpu(2) system call is
> documented.

I was referring to the value in TPIDRURW specifically. If an application
started reading that directly (rather than going through the vDSO), we wouldn't
be able to change the register value in future.

That's all moot, given we can't repurpose TPIDRURW.

Thanks,
Mark.

      parent reply	other threads:[~2016-10-05 20:12 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-10-04 13:49 [PATCH] arm: Added support for getcpu() vDSO using TPIDRURW Fredrik Markstrom
2016-10-04 15:35 ` [PATCH v2] " Fredrik Markstrom
2016-10-04 17:07   ` Mark Rutland
2016-10-05 12:25     ` Fredrik Markström
2016-10-05 16:39       ` Fredrik Markström
2016-10-05 17:48         ` Robin Murphy
2016-10-05 19:53           ` Russell King - ARM Linux
     [not found]             ` <CAKdL+dSt+cBCpwW5q+VCQh+7XeKrnyJgfTsEsuo2nKoUr9ytxw@mail.gmail.com>
2016-10-10 15:29               ` Will Deacon
2016-10-10 16:15                 ` Restartable Sequences benchmarks (was: Re: [PATCH v2] arm: Added support for getcpu() vDSO using TPIDRURW) Mathieu Desnoyers
     [not found]           ` <CAKdL+dQH=9C2aGf7ys5-vXM7pkdPYUQ8xYWLipwVbABOz09f1g@mail.gmail.com>
2016-10-05 20:44             ` [PATCH v2] arm: Added support for getcpu() vDSO using TPIDRURW Mark Rutland
2016-10-05 21:01               ` Russell King - ARM Linux
2016-10-05 21:47                 ` Mark Rutland
2016-10-05 21:37               ` Fredrik Markström
2016-10-05 20:12       ` Mark Rutland [this message]

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=20161005201254.GA26721@remoulade \
    --to=mark.rutland@arm.com \
    --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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).