Message-ID: <668290565.8970.1456508825358.JavaMail.zimbra@efficios.com>
Date:	Fri, 26 Feb 2016 17:47:05 +0000 (UTC)
From:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:	Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Russell King <linux@....linux.org.uk>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org,
	linux-api <linux-api@...r.kernel.org>,
	Paul Turner <pjt@...gle.com>, Andrew Hunter <ahh@...gle.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Andy Lutomirski <luto@...capital.net>,
	Andi Kleen <andi@...stfloor.org>,
	Dave Watson <davejwatson@...com>, Chris Lameter <cl@...ux.com>,
	Ben Maurer <bmaurer@...com>, rostedt <rostedt@...dmis.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Josh Triplett <josh@...htriplett.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	Michael Kerrisk <mtk.manpages@...il.com>
Subject: Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of
 running thread

----- On Feb 25, 2016, at 6:32 PM, Rasmus Villemoes linux@...musvillemoes.dk wrote:

> On Wed, Feb 24 2016, Mathieu Desnoyers <mathieu.desnoyers@...icios.com> wrote:
> 
>>
>>        Typically, a library or application will keep the cpu  number
>>        cache  in  a  thread-local  storage variable, or other memory
>>        areas belonging to each thread. It is recommended to  perform
>>        a  volatile  read of the cpu number cache to prevent the com‐
>>        piler from doing load tearing. An alternative approach is  to
>>        read  the  cpu  number cache from inline assembly in a single
>>        instruction.
>>
>>        Each thread is responsible for registering its own cpu number
>>        cache.   Only  one  cpu  cache  address can be registered per
>>        thread.
>>
>>        The symbol  __getcpu_cache_tls  is  recommended  to  be  used
>>        across  libraries  and  applications  wishing  to  register a
>>        thread-local getcpu_cache. The  attribute  "weak"  is  recom‐
>>        mended  when  declaring this variable in libraries.  Applica‐
>>        tions can choose to define their own version of  this  symbol
>>        without the weak attribute as a performance improvement.
>>
>>        In  a  typical usage scenario, the thread registering the cpu
>>        number cache will be performing reads from that cache. It  is
>>        however  also allowed to read the cpu number cache from other
>>        threads. The cpu number cache updates performed by the kernel
>>        provide single-copy atomicity semantics, which guarantee that
>>        other threads performing single-copy atomic reads of the  cpu
>>        number cache will always observe a consistent value.
>>
>>        Memory registered as cpu number cache should never be deallo‐
>>        cated before the thread which registered it  exits:  specifi‐
>>        cally, it should not be freed, and the library containing the
>>        registered thread-local storage should not be dlclose'd.
> 
> Maybe spell out the consequence if this is violated - since the SIGSEGV
> only happens on migration, it may take a while to strike.

Good point.

> 
> Random thoughts: The current implementation ensures that getcpu_cache is
> "idempotent" from within a single thread - once set, it can never get
> unset nor set to some other pointer. I think that can be useful, since
> it means a library can reliably use the TLS variable itself (initialized
> with some negative number) as an indicator of whether
> getcpu_cache(GETCPU_CACHE_SET) has been called. So if a single test on a
> fast path where the library would need to load __getcpu_cache_tls anyway
> is acceptable, it can avoid requiring some library init function to be
> called in each thread - which can sometimes be hard to arrange. Is this
> something we want to guarantee - that is, will we never implement
> GETCPU_CACHE_UNSET or a "force" flag to _SET? Either way, I think we
> should spend a few words on it to avoid the current behaviour becoming
> accidental ABI.

Yes, I would be tempted to state that once set, the address is idempotent
for a thread.

> 
> In another thread:
> 
>> However, there are other use-cases for having a fast mechanism for
>> reading the current CPU number, besides restartable sequences.  For
>> instance, it can be used by glibc to implement a faster sched_getcpu.
> 
> Will glibc do that? It may be a little contentious for glibc to claim a
> unique resource such as task_struct::cpu_cache for itself, even if
> everybody is supposed to use the same symbol. Hm, maybe one could say
> that if an application does define the symbol __getcpu_cache_tls (which
> is techically in the implementation namespace), that gives glibc (and
> any other library) license to do getcpu_cache(SET, &&__getcpu_cache_tls)
> (pseudo-code, of course). If a library initializes its own weak version
> with -2 it can check whether the application defined
> __getcpu_cache_tls. Ok, I'm probably overthinking this...

I've had the exact same thoughts a few days ago then thinking about
how lttng-ust could do a "lazy binding" of the getcpu_cache without
requiring an explicit initialization at thread start. We're reaching
very similar conclusions. We could recommend/require that userspace
does this whenever it defines a __getcpu_cache_tls:

Declare as

__thread __attribute__((weak)) volatile int32_t __getcpu_cache_tls = -1;

Then whenever it loads it, "-1" would mean "uninitialized", and "-2"
could mean "this thread tried to initialize it, but fail, so you
should directly go to a fallback". ">= 0" would mean initialized and
working.

static inline int32_t getcpu_cache_read(void)
{
    int32_t cachev = __getcpu_cache_tls;

    if (likely(cachev >= 0))
        return cachev;

    if (cachev == -1) {
        volatile int32_t *cpu_cache = &__getcpu_cache_tls;

        if (!getcpu_cache(GETCPU_CACHE_SET, &cpu_cache, 0))
            return __getcpu_cache_tls;
        __getcpu_cache_tls = -2;
    }
    /* Fallback on sched_getcpu(). */
    return sched_getcpu();
}

This could be documented in the getcpu_cache system call man page.

Thoughts ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Powered by blists - more mailing lists