From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964790AbbDMVtT (ORCPT ); Mon, 13 Apr 2015 17:49:19 -0400 Received: from www.linutronix.de ([62.245.132.108]:33845 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751620AbbDMVtP (ORCPT ); Mon, 13 Apr 2015 17:49:15 -0400 Date: Mon, 13 Apr 2015 23:49:34 +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: <1428952229-3577-1-git-send-email-mathieu.desnoyers@efficios.com> Message-ID: References: <1428952229-3577-1-git-send-email-mathieu.desnoyers@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 Mon, 13 Apr 2015, Mathieu Desnoyers wrote: > [ Andrew, can you take this for the 4.1 merge window ? ] You probably mean 4.2, right? This fails the basic test for exposure in linux-next, adds syscalls without the explicit ack of any x86 maintainer and exposes a user space ABI with a magic undocumented flags argument. > This patch only adds the system call to x86. So the changes to > include/uapi/asm-generic/unistd.h | 4 +- are just cosmetic, right? > +/* System call membarrier "flags" argument. */ > +enum { > + /* > + * Query whether the rest of the specified flags are supported, > + * without performing synchronization. > + */ Docbook has support for enums. > + MEMBARRIER_QUERY = (1 << 31), > +}; Why is this an anonymous enum? > +#ifdef CONFIG_SMP So documentation is SMP only, right? > +/* Docbook comments start with "/**" > + * sys_membarrier - issue memory barrier on all running threads > + * @flags: MEMBARRIER_QUERY: > + * Query whether the rest of the specified flags are supported, > + * without performing synchronization. @flags: Explain what this is for, not what a particular implemented value is used for. The values should be proper documented in the enum Why is this an int and not a named enum? 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. 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? > + * 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? > + */ > +SYSCALL_DEFINE1(membarrier, int, flags) > +{ > + int retval; > + > + retval = membarrier_validate_flags(flags); > + if (retval) > + goto end; > + if (unlikely(flags & MEMBARRIER_QUERY) || num_online_cpus() == 1) > + goto end; So why not doing the obvious? enum modes { QUERY = 0, FULL_SYNC = 1 << 0, }; #define IMPLEMENTED_MODES (FULL_SYNC) switch (mode) { case FULL_SYNC: synchronize_sched_on_smp(); return 0; case QUERY: return IMPLEMENTED_MODES; } return -EINVAL; And later on you do enum modes { QUERY = 0, FULL_SYNC = 1 << 0, + MAGIC_SYNC = 1 << 1, }; -#define IMPLEMENTED_MODES (FULL_SYNC) +#define IMPLEMENTED_MODES (FULL_SYNC | MAGIC_SYNC) All you need to make sure is that any mode is a power of 2. Hmm? Thanks, tglx