From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754262AbbK3Rg1 (ORCPT ); Mon, 30 Nov 2015 12:36:27 -0500 Received: from mail-wm0-f54.google.com ([74.125.82.54]:36132 "EHLO mail-wm0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751909AbbK3Rg0 (ORCPT ); Mon, 30 Nov 2015 12:36:26 -0500 Date: Mon, 30 Nov 2015 18:36:22 +0100 From: Frederic Weisbecker To: Chris Metcalf Cc: LKML , Peter Zijlstra , Thomas Gleixner , Luiz Capitulino , Christoph Lameter , Ingo Molnar , Viresh Kumar , Rik van Riel Subject: Re: [PATCH 1/7] atomic: Export fetch_or() Message-ID: <20151130173619.GA9444@lerouge> References: <1447424529-13671-1-git-send-email-fweisbec@gmail.com> <1447424529-13671-2-git-send-email-fweisbec@gmail.com> <56548908.50509@ezchip.com> <20151124211951.GA16609@lerouge> <5654DB33.1080706@ezchip.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5654DB33.1080706@ezchip.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Nov 24, 2015 at 04:48:35PM -0500, Chris Metcalf wrote: > On 11/24/2015 04:19 PM, Frederic Weisbecker wrote: > >>Also, I wonder about the nomenclature here: other than cmpxchg > >>>and xchg, all the atomic ops are named "atomic_xxx". For something > >>>that returns the old value, I'd expect it to be atomic_or_return() > >>>and be otherwise like the existing atomic_or() routine, and thus you'd > >>>specify "atomic_t tick_dependency". > >I think Peterz needs it to be type-generic, like cmpxchg, such that he can > >use it on tsk->thread_info->flags which type can vary. But if we happen to > >need an atomic_t version, we can also provide an atomic_fetch_or() version. > > Yes, I think my point is that Peter Z's version is what is needed for > the scheduler, but it may not be the thing we want to start providing > to the entire kernel without thinking a little harder about the semantics, > the namespace issues, and whether there is or should be room for > some appropriate family of similar calls. Just tossing in fetch_or() > because it's easy doesn't necessarily seem like what we should be doing. > > >Also note that or_return() means that you first do OR and then return the new value. > > Yeah, actually fair point. I keep forgetting Linux does this "backwards". > > I still think we should use an atomic_xxx() name here rather than just > adding things into the namespace willy-nilly. > > It's tempting to use atomic_fetch_or() but that actually conflicts with the > C11 names, and remarkably, we haven't done that yet in the kernel, > so we may want to avoid doing so for now. I can imagine in the not > too distant future we detect C11 compilers and allow using > if possible. (Obviously some platforms require kernel support or > other tricky things for stdatomic.h, so we couldn't use it everywhere.) > > We could use something like gcc's old __sync_fetch_and_XXX vs > __sync_XXX_and_fetch nomenclature and call it atomic_return_or() > to contrast with atomic_or_return(). That avoids clashing with C11 > for now and is obviously distinct from atomic_or_return(). I suppose > something like atomic_fetch_and_or() isn't terrible either. > > There is boilerplate for building generic atomic functions already in > include/asm-generic/atomic.h and that could be extended. > Unfortunately the atomic64 generic code isn't similarly constructed, > so you can't just provide a default atomic64_return_or() based on that > stuff, or at least, you only can on platforms that use an array of locks > to implement 64-bit atomics. Ugh. > > If we did this and then wanted Peter Z's code to take advantage of it, > in principle we could just have some macrology which would compare > the sizeof(thread_info.flags) to sizeof(int) and sizeof(long) to see which > one to use and then cast to the appropriate atomic_t or atomic64_t. > That's a little painful but not terrible. > > Boy, the whole situation is pretty tangled up, though. > > Unless you want to take a big diversion into atomics, I'd be tempted > to leave Peter's macro alone and just write it off as necessary evil > to handle the fact that thread_info.flags is all kinds of different sizes > and types on different platforms, and definitely never an atomic_t. > Instead just create an inline function atomic_return_or(), or > whatever name you prefer, that operates on an atomic_t, and use > the atomic_t type for your structure field. It's clearly a win to mark > the data types as being atomic to the extent we can do so, I think. I agree that cmpxchg, test_and_set_bit, fetch_or... functions with loose namespaces aren't the best layout. But casting thread_info to atomic_t really worries me, I'm not sure the ending result would be correct at all. I prefer to sacrify correctness over namespace sanity :-)