Messages in this thread |  | | Date | Tue, 14 Apr 2015 21:31:53 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [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
|  |