Message-ID: <5654DB33.1080706@ezchip.com>
Date:	Tue, 24 Nov 2015 16:48:35 -0500
From:	Chris Metcalf <cmetcalf@...hip.com>
To:	Frederic Weisbecker <fweisbec@...il.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 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.

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com

--
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