From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753554AbbDNTbl (ORCPT ); Tue, 14 Apr 2015 15:31:41 -0400 Received: from www.linutronix.de ([62.245.132.108]:37592 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752431AbbDNTbe (ORCPT ); Tue, 14 Apr 2015 15:31:34 -0400 Date: Tue, 14 Apr 2015 21:31:53 +0200 (CEST) From: Thomas Gleixner To: Mathieu Desnoyers cc: Andrew Morton , linux-kernel@vger.kernel.org, Josh Triplett , KOSAKI Motohiro , Steven Rostedt , Nicholas Miell , Linus Torvalds , Ingo Molnar , Alan Cox , Lai Jiangshan , Stephen Hemminger , Peter Zijlstra , David Howells Subject: Re: [PATCH v14 for 4.1] sys_membarrier(): system-wide memory barrier (x86) In-Reply-To: <76701995.30245.1429038138581.JavaMail.zimbra@efficios.com> Message-ID: References: <1428952229-3577-1-git-send-email-mathieu.desnoyers@efficios.com> <76701995.30245.1429038138581.JavaMail.zimbra@efficios.com> User-Agent: Alpine 2.11 (DEB 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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