lkml.org 
[lkml]   [2015]   [Apr]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v14 for 4.1] sys_membarrier(): system-wide memory barrier (x86)
On Tue, 14 Apr 2015, Mathieu Desnoyers wrote:
> So the question would be: should we introduce this syscall
> in different patches for each architecture, or should
> we add them all in one go ? There is nothing fundamentally
> x86-specific to the implementation of this system call.

We can add that in one go for the generic ones. I was merily pointing
out the inconsistency of your changelog.

> >
> > > +/* System call membarrier "flags" argument. */
> > > +enum {
> > > + /*
> > > + * Query whether the rest of the specified flags are supported,
> > > + * without performing synchronization.
> > > + */
> >
> > Docbook has support for enums.
>
> OK, how about the following ?
>
> /**
> * enum membarrier_flags - membarrier system call "flags" argument bitmask.
> *
> * Bitmask of flags to be passed to the membarrier system call. The "flags"
> * parameter can be either 0 or many of those flags combined with a bitwise
> * "or".

So now add the explanation for the implemented flags.

* MEMBARRIER_QUERY: Insert blurb ....

> Since this type is used as a system call ABI, I'm worried that a compiler
> implementation may compile a user-space program with a enum representation
> of a "char", whereas the kernel would expect an integer, thus causing an
> ABI issue, where userland would fail to clear the high-order bits of the
> register, and a more recent kernel would care about those bits (if we add
> new flags in future kernel releases).

Sigh, no. The compiler CANNOT ever truncate the enum:

"The choice of type is implementation-defined,108) but shall be
capable of representing the values of all the members of the
enumeration."

So if you define a value with 1<<31 the compiler cannot chose to use
a char.

> > Why is this named flags and not given a descriptive name? If I
> > understand your changelog correctly you want to implement other
> > synchronization modes than the current synchronize_sched. So mode
> > might be a proper name.
>
> If we look at other system calls like open(), "flags" describes a
> feature configuration, whereas "mode" is used to specify the
> permissions to use when creating the file. What we want to specify
> here is more a configuration of the syscall, which fits better with
> the existing semantic of open() "flags".

Then name it opmode or opcode, because that's what you want.

flags is usually used for features, but you dont invoke a
sysmembarrier feature. You invoke an operation mode.

flags as the only argument is a clear indicator that we do not know
how that system call should look like in the future, so we have the
opaque flag field to implement random and inconsistent crap.

> > Why is MEMBARRIER_QUERY not a proper operation mode and simply returns
> > the supported modes instead of doing it backwards and asking whether
> > a specific value is supported?
>
> That's an interesting way to do it. We'd have to be careful not to
> conflict with return values like -ENOSYS then. We could define
> MEMBARRIER_QUERY as (1 << 31), which effectively reserves all
> signed negative numbers for the QUERY flag, and then the return value
> of the QUERY flag could be either a negative value (e.g. -ENOSYS)
> or the set of flags supported (bits 0 to 30).

Makes sense.

> >
> > > + * On uniprocessor systems, this system call simply returns 0 after
> > > + * validating the arguments, so user-space knows it is implemented.
> >
> > And the exact point of knowing this is?
>
> I guess the alternative approach would be to return -ENOSYS, and let
> userspace discover that the kernel only support uniprocessor systems
> by other means ?

It's fine to have that, but the explanation sucks.

> I really prefer that the "default" of passing "0" does
> the obviously correct behavior for the system call:
> issuing a memory barrier over the entire system. Otherwise,
> making "0" a no-op could introduce hard-to-find races
> if people misuse the system call.

People should stay away from stuff they do not understand. Whether 0
is a valid opcode or not does not matter. They will screw up anyway.

> Moreover, we need to reserve negative return values for errors like
> -ENOSYS, so we can use the high-order bit for the QUERY flag.

Agreed.

> In summary, how about the following:
>
> - "0" is the default, obviously correct flags parameter,
> which does a memory barrier on all processors.

Then make 0 an explicit value in the enum with a proper explanation
for it. It's a valid argument and wants a proper documentation.

> - 1 << 31 is the QUERY flag, which returns a bitmask of
> all flags supported.
> - We keep negative return values reserved for errors
> (e.g. -ENOSYS), even for the QUERY flag.

Agreed.

Thanks,

tglx


\
 
 \ /
  Last update: 2015-04-14 22:01    [W:0.085 / U:0.328 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site