Re: [PATCH v4 1/5] getcpu_cache system call: cache CPU number of running thread

----- On Feb 26, 2016, at 6:04 PM, H. Peter Anvin hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org wrote:

On February 26, 2016 12:24:15 PM PST, Mathieu Desnoyers
[off-list ref] wrote:

quoted

----- On Feb 26, 2016, at 1:01 PM, Thomas Gleixner tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org
wrote:

quoted

On Fri, 26 Feb 2016, Mathieu Desnoyers wrote:

quoted

----- On Feb 26, 2016, at 11:29 AM, Thomas Gleixner

tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org wrote:

quoted

quoted

quoted

Right. There is no point in having two calls and two update

mechanisms for a

quoted

quoted

quoted

very similar purpose.

So let userspace have one struct where cpu/seq and whatever is

required for

quoted

quoted

quoted

rseq is located and flag at register time which parts of the

struct need to be

quoted

quoted

quoted

updated.

If we put both cpu/seq/other in that structure, why not plan ahead

and make

quoted

quoted

it extensible then ?

That looks very much like the "Thread-local ABI" series I posted

last year.

quoted

quoted

See https://lkml.org/lkml/2015/12/22/464

Here is why I ended up introducing the specialized "getcpu_cache"

system call

quoted

quoted

rather than the "generic" system call (quote from the getcpu_cache

changelog):

quoted

quoted

    Rationale for the getcpu_cache system call rather than the

thread-local

quoted

quoted

    ABI system call proposed earlier:
    
    Rather than doing a "generic" thread-local ABI, specialize this

system

quoted

quoted

    call for a cpu number cache only. Anyway, the thread-local ABI

approach

quoted

quoted

    would have required that we introduce "feature" flags, which

would have

quoted

quoted

    ended up reimplementing multiplexing of features on top of a

system

quoted

quoted

    call. It seems better to introduce one system call per feature

instead.

quoted

quoted

If everyone end up preferring that we introduce a system call that

implements

quoted

quoted

many features at once, that's indeed something we can do, but I

remember

quoted

quoted

being told in the past that this is generally a bad idea.

It's a bad idea if you mix stuff which does not belong together, but

if you

quoted

have stuff which shares a substantial amount of things then it makes

a lot of

quoted

sense. Especially if it adds similar stuff into hotpathes.

quoted

For one thing, it would make the interface more cumbersome to deal

with

quoted

quoted

from user-space in terms of feature detection: if we want to make

this

quoted

quoted

interface extensible, in addition to check -1, errno=ENOSYS,

userspace

quoted

quoted

would have to deal with a field containing the length of the

structure

quoted

quoted

as expected by user-space and kernel, and feature flags to see the

common

quoted

quoted

set of features supported by kernel and user-space.

Having one system call per feature seems simpler to handle in terms

of

quoted

quoted

feature availability detection from a userspace point of view.

That might well be, but that does not justify two fastpath updates,

two

quoted

seperate pointers to handle, etc ....

Keeping two separate pointers in the task_struct rather than a single
one
might indeed be unwelcome, but I'm not sure I fully grasp the fast path
argument in this case: getcpu_cache only sets a notifier thread flag
on thread migration, whereas AFAIU rseq adds code to context switch and
signal
delivery, which are prone to have a higher impact.

Indeed both will have their own code in the resume notifier, but is it
really
a fast path ?

From my point of view, making it easy for userspace to just enable
getcpu_cache
without having the scheduler and signal delivery fast-path overhead of
rseq seems
like a good thing. I'm not all that sure that saving an extra pointer
in
task_struct justifies the added system call interface complexity.

Thanks,

Mathieu

I think it would be a good idea to make this a general pointer for the kernel to
be able to write per thread state to user space, which obviously can't be done
with the vDSO.

This means the libc per thread startup should query the kernel for the size of
this structure and allocate thread local data accordingly.  We can then grow
this structure if needed without making the ABI even more complex.

This is more than a system call: this is an entirely new way for userspace to
interact with the kernel.  Therefore we should make it a general facility.

I'm really glad to see I'm not the only one seeing potential for
genericity here. :-) This is exactly what I had in mind
last year when proposing the thread_local_abi() system call:
a generic way to register an extensible per-thread data structure
so the kernel can communicate with user-space and vice-versa.

Rather than having the libc query the kernel for size of the structure,
I would recommend that libc tells the kernel the size of the thread-local
ABI structure it supports. The idea here is that both the kernel and libc
need to know about the fields in that structure to allow a two-way
interaction. Fields known only by either the kernel or userspace
are useless for a given thread anyway. This way, libc could statically
define the structure.

I would be tempted to also add "features" flags, so both user-space
and the kernel could tell each other what they support: user-space
would announce the set of features it supports, and it could also
query the kernel for the set of supported features. One simple approach
would be to use a uint64_t as type for those feature flags, and
reserve the last bit for extending to future flags if we ever have
more than 64.

Thoughts ?

Thanks,

Mathieu

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