Message-ID: <alpine.DEB.2.11.1504142107470.3845@nanos>
Date:	Tue, 14 Apr 2015 21:31:53 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org,
	Josh Triplett <josh@...htriplett.org>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Nicholas Miell <nmiell@...cast.net>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Ingo Molnar <mingo@...hat.com>,
	Alan Cox <gnomes@...rguk.ukuu.org.uk>,
	Lai Jiangshan <laijs@...fujitsu.com>,
	Stephen Hemminger <stephen@...workplumber.org>,
	Peter Zijlstra <peterz@...radead.org>,
	David Howells <dhowells@...hat.com>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists