Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1276593 > unrolled thread

Re: [PATCH 1/7] atomic: Export fetch_or()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/7] atomic: Export fetch_or() Chris Metcalf <cmetcalf@ezchip.com> - 2015-11-24 17:00 +0100
    Re: [PATCH 1/7] atomic: Export fetch_or() Frederic Weisbecker <fweisbec@gmail.com> - 2015-11-24 22:30 +0100
      Re: [PATCH 1/7] atomic: Export fetch_or() Chris Metcalf <cmetcalf@ezchip.com> - 2015-11-24 22:50 +0100
      Re: [PATCH 1/7] atomic: Export fetch_or() Peter Zijlstra <peterz@infradead.org> - 2015-11-25 10:20 +0100

#1276593 — Re: [PATCH 1/7] atomic: Export fetch_or()

On 11/13/2015 09:22 AM, Frederic Weisbecker wrote:
> Export fetch_or() that's implemented and used internally by the
> scheduler. We are going to use it for NO_HZ so make it generally
> available.
>
> Cc: Christoph Lameter<cl@linux.com>
> Cc: Chris Metcalf<cmetcalf@ezchip.com>
> Cc: Ingo Molnar<mingo@kernel.org>
> Cc: Luiz Capitulino<lcapitulino@redhat.com>
> Cc: Peter Zijlstra<peterz@infradead.org>
> Cc: Rik van Riel<riel@redhat.com>
> Cc: Thomas Gleixner<tglx@linutronix.de>
> Cc: Viresh Kumar<viresh.kumar@linaro.org>
> Signed-off-by: Frederic Weisbecker<fweisbec@gmail.com>
> ---
>   include/linux/atomic.h | 18 ++++++++++++++++++
>   kernel/sched/core.c    | 14 --------------
>   2 files changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/atomic.h b/include/linux/atomic.h
> index 00a5763..c3b99f8 100644
> --- a/include/linux/atomic.h
> +++ b/include/linux/atomic.h
> @@ -451,6 +451,24 @@ static inline int atomic_dec_if_positive(atomic_t *v)
>   }
>   #endif
>   
> +/**
> + * fetch_or - perform *ptr |= mask and return old value of *ptr
> + * @ptr: pointer to value
> + * @mask: mask to OR on the value
> + *
> + * cmpxchg based fetch_or, macro so it works for different integer types
> + */
> +#define fetch_or(ptr, mask)						\
> +({	typeof(*(ptr)) __old, __val = *(ptr);				\
> +	for (;;) {							\
> +		__old = cmpxchg((ptr), __val, __val | (mask));		\
> +		if (__old == __val)					\
> +			break;						\
> +		__val = __old;						\
> +	}								\
> +	__old;								\
> +})
> +
>   #include <asm-generic/atomic-long.h>
>   #ifdef CONFIG_GENERIC_ATOMIC64
>   #include <asm-generic/atomic64.h>

I think this should be guarded by an "#ifndef" like other things in
this file, so architectures can provide their own implementations,
or can use the C11 atomic_fetch_or() for newer compilers.

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

Avoiding all of these issues is probably why fetch_or() is not exported :-)
I made some similar comments last time around:

https://lkml.kernel.org/r/55B2794A.8040707@ezchip.com

-- 
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1276764

On Tue, Nov 24, 2015 at 10:58:00AM -0500, Chris Metcalf wrote:
> On 11/13/2015 09:22 AM, Frederic Weisbecker wrote:
> >Export fetch_or() that's implemented and used internally by the
> >scheduler. We are going to use it for NO_HZ so make it generally
> >available.
> >
> >Cc: Christoph Lameter<cl@linux.com>
> >Cc: Chris Metcalf<cmetcalf@ezchip.com>
> >Cc: Ingo Molnar<mingo@kernel.org>
> >Cc: Luiz Capitulino<lcapitulino@redhat.com>
> >Cc: Peter Zijlstra<peterz@infradead.org>
> >Cc: Rik van Riel<riel@redhat.com>
> >Cc: Thomas Gleixner<tglx@linutronix.de>
> >Cc: Viresh Kumar<viresh.kumar@linaro.org>
> >Signed-off-by: Frederic Weisbecker<fweisbec@gmail.com>
> >---
> >  include/linux/atomic.h | 18 ++++++++++++++++++
> >  kernel/sched/core.c    | 14 --------------
> >  2 files changed, 18 insertions(+), 14 deletions(-)
> >
> >diff --git a/include/linux/atomic.h b/include/linux/atomic.h
> >index 00a5763..c3b99f8 100644
> >--- a/include/linux/atomic.h
> >+++ b/include/linux/atomic.h
> >@@ -451,6 +451,24 @@ static inline int atomic_dec_if_positive(atomic_t *v)
> >  }
> >  #endif
> >+/**
> >+ * fetch_or - perform *ptr |= mask and return old value of *ptr
> >+ * @ptr: pointer to value
> >+ * @mask: mask to OR on the value
> >+ *
> >+ * cmpxchg based fetch_or, macro so it works for different integer types
> >+ */
> >+#define fetch_or(ptr, mask)						\
> >+({	typeof(*(ptr)) __old, __val = *(ptr);				\
> >+	for (;;) {							\
> >+		__old = cmpxchg((ptr), __val, __val | (mask));		\
> >+		if (__old == __val)					\
> >+			break;						\
> >+		__val = __old;						\
> >+	}								\
> >+	__old;								\
> >+})
> >+
> >  #include <asm-generic/atomic-long.h>
> >  #ifdef CONFIG_GENERIC_ATOMIC64
> >  #include <asm-generic/atomic64.h>
> 
> I think this should be guarded by an "#ifndef" like other things in
> this file, so architectures can provide their own implementations,
> or can use the C11 atomic_fetch_or() for newer compilers.

Right.

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

Also note that or_return() means that you first do OR and then return the new value.

I remember debating a bit the name with Peterz and the current one now makes quite
some sense to me too :-)

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1276781

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277121

On Tue, Nov 24, 2015 at 10:19:52PM +0100, Frederic Weisbecker wrote:
> Also note that or_return() means that you first do OR and then return the new value.

Yes, that's useless. OR is an irreversible operator, which means
or_return() looses data. You can never say if a bit included in the
mask was set before.

You really must have fetch_or().
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web