Message-ID: <20151130173619.GA9444@lerouge>
Date:	Mon, 30 Nov 2015 18:36:22 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Chris Metcalf <cmetcalf@...hip.com>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Luiz Capitulino <lcapitulino@...hat.com>,
	Christoph Lameter <cl@...ux.com>,
	Ingo Molnar <mingo@...nel.org>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	Rik van Riel <riel@...hat.com>
Subject: Re: [PATCH 1/7] atomic: Export fetch_or()

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 <stdatomic.h>
> 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 :-)
--
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