Re: Re: [PATCHv5] atomic: add *_dec_not_zero
On Sunday 04 December 2011 21:33:16 Russell King - ARM Linux wrote: [...] > > +#define atomic64_dec_not_zero(v) atomic64_add_unless((v), -1LL, 0LL) > > I think this is rather silly - all these definitions are very similar to > each other. Is there really no way to put this into include/linux/atomic.h, > maybe as something like: > > #ifndef atomic64_dec_not_zero > #define atomic64_dec_not_zero(v) atomic64_add_unless((v), -1, 0) > #endif > > and avoid having to add essentially the same definition to 12 individual > files? > > Architectures which want to override it can do by the following: > > #define atomic64_dec_not_zero atomic64_dec_not_zero > > which won't have any effect on C nor asm code. * https://lkml.org/lkml/2011/5/8/15 * https://lkml.org/lkml/2011/5/8/16 * https://lkml.org/lkml/2011/5/8/321 Kind regards, Sven signature.asc Description: This is a digitally signed message part. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: Re: [PATCHv5] atomic: add *_dec_not_zero
On Sun, Dec 04, 2011 at 10:49:10PM +0100, Sven Eckelmann wrote:
> On Sunday 04 December 2011 21:33:16 Russell King - ARM Linux wrote:
> [...]
> > > +#define atomic64_dec_not_zero(v) atomic64_add_unless((v), -1LL, 0LL)
> >
> > I think this is rather silly - all these definitions are very similar to
> > each other. Is there really no way to put this into include/linux/atomic.h,
> > maybe as something like:
> >
> > #ifndef atomic64_dec_not_zero
> > #define atomic64_dec_not_zero(v)atomic64_add_unless((v), -1, 0)
> > #endif
> >
> > and avoid having to add essentially the same definition to 12 individual
> > files?
> >
> > Architectures which want to override it can do by the following:
> >
> > #define atomic64_dec_not_zero atomic64_dec_not_zero
> >
> > which won't have any effect on C nor asm code.
>
> * https://lkml.org/lkml/2011/5/8/15
> * https://lkml.org/lkml/2011/5/8/16
> * https://lkml.org/lkml/2011/5/8/321
I don't see any reason in that set of messages _not_ to do what I suggest.
Even on SMP architectures, your:
#define atomic64_dec_not_zero(v)atomic64_add_unless((v), -1, 0)
makes total sense - and with the adjustments I've suggested it means
that architectures (like x86) can still override it if have a more
optimal way to perform this operation.
Not only that, but we already do this kind of thing in
include/linux/atomic.h for the non-64 bit ops, for example:
#ifndef atomic_inc_unless_negative
static inline int atomic_inc_unless_negative(atomic_t *p)
{
int v, v1;
for (v = 0; v >= 0; v = v1) {
v1 = atomic_cmpxchg(p, v, v + 1);
if (likely(v1 == v))
return 1;
}
return 0;
}
#endif
And really, I believe it would be a good cleanup if all the standard
definitions for atomic64 ops (like atomic64_add_negative) were also
defined in include/linux/atomic.h rather than individually in every
atomic*.h header throughout the kernel source, except where an arch
wants to explicitly override it. Yet again, virtually all architectures
define these in exactly the same way.
We have more than enough code in arch/ for any architecture to worry
about, we don't need schemes to add more when there's simple and
practical solutions to avoiding doing so if the right design were
chosen (preferably from the outset.)
So, I'm not going to offer my ack for a change which I don't believe
is the correct approach.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: Re: [PATCHv5] atomic: add *_dec_not_zero
On Sun, 2011-12-04 at 22:18 +, Russell King - ARM Linux wrote: .../... > And really, I believe it would be a good cleanup if all the standard > definitions for atomic64 ops (like atomic64_add_negative) were also > defined in include/linux/atomic.h rather than individually in every > atomic*.h header throughout the kernel source, except where an arch > wants to explicitly override it. Yet again, virtually all architectures > define these in exactly the same way. > > We have more than enough code in arch/ for any architecture to worry > about, we don't need schemes to add more when there's simple and > practical solutions to avoiding doing so if the right design were > chosen (preferably from the outset.) > > So, I'm not going to offer my ack for a change which I don't believe > is the correct approach. I agree with Russell, his approach is a lot easier to maintain long run, we should even consider converting existing definitions. Cheers, Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: Re: [PATCHv5] atomic: add *_dec_not_zero
Looking at this:
> #ifndef atomic_inc_unless_negative
> static inline int atomic_inc_unless_negative(atomic_t *p)
> {
> int v, v1;
> for (v = 0; v >= 0; v = v1) {
> v1 = atomic_cmpxchg(p, v, v + 1);
> if (likely(v1 == v))
> return 1;
> }
> return 0;
> }
> #endif
why is it optimised for '*p' being zero??
I'd have though the initial assignment to 'v' should
be made by reading '*p' without any memory barriers (etc).
David
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: Re: Re: [PATCHv5] atomic: add *_dec_not_zero
On Sunday 04 December 2011 22:18:50 Russell King - ARM Linux wrote:
> On Sun, Dec 04, 2011 at 10:49:10PM +0100, Sven Eckelmann wrote:
> > On Sunday 04 December 2011 21:33:16 Russell King - ARM Linux wrote:
> > [...]
> >
> > > > +#define atomic64_dec_not_zero(v) atomic64_add_unless((v), -1LL,
> > > > 0LL)
> > >
> > > I think this is rather silly - all these definitions are very
> > > similar to each other. Is there really no way to put this into
> > > include/linux/atomic.h, maybe as something like:
> > >
> > > #ifndef atomic64_dec_not_zero
> > > #define atomic64_dec_not_zero(v) atomic64_add_unless((v), -1, 0)
> > > #endif
> > >
> > > and avoid having to add essentially the same definition to 12
> > > individual files?
> > >
> > > Architectures which want to override it can do by the following:
> > >
> > > #define atomic64_dec_not_zero atomic64_dec_not_zero
> > >
> > > which won't have any effect on C nor asm code.
> >
> > * https://lkml.org/lkml/2011/5/8/15
> > * https://lkml.org/lkml/2011/5/8/16
> > * https://lkml.org/lkml/2011/5/8/321
>
> I don't see any reason in that set of messages _not_ to do what I suggest.
> Even on SMP architectures, your:
>
> #define atomic64_dec_not_zero(v) atomic64_add_unless((v), -1, 0)
>
> makes total sense - and with the adjustments I've suggested it means
> that architectures (like x86) can still override it if have a more
> optimal way to perform this operation.
>
> Not only that, but we already do this kind of thing in
> include/linux/atomic.h for the non-64 bit ops, for example:
>
> #ifndef atomic_inc_unless_negative
> static inline int atomic_inc_unless_negative(atomic_t *p)
> {
> int v, v1;
> for (v = 0; v >= 0; v = v1) {
> v1 = atomic_cmpxchg(p, v, v + 1);
> if (likely(v1 == v))
> return 1;
> }
> return 0;
> }
> #endif
>
> And really, I believe it would be a good cleanup if all the standard
> definitions for atomic64 ops (like atomic64_add_negative) were also
> defined in include/linux/atomic.h rather than individually in every
> atomic*.h header throughout the kernel source, except where an arch
> wants to explicitly override it. Yet again, virtually all architectures
> define these in exactly the same way.
>
> We have more than enough code in arch/ for any architecture to worry
> about, we don't need schemes to add more when there's simple and
> practical solutions to avoiding doing so if the right design were
> chosen (preferably from the outset.)
>
> So, I'm not going to offer my ack for a change which I don't believe
> is the correct approach.
Ok, I wanted to say that I just did what is currently done and did not offer a
redesign. There is just a difference between adding something and replacing
everything with something else. But I am fine with not getting the ack because
now somebody at least made a statement.
Kind regards,
Sven
signature.asc
Description: This is a digitally signed message part.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: Re: Re: [PATCHv5] atomic: add *_dec_not_zero
On Monday 05 December 2011 09:41:55 Benjamin Herrenschmidt wrote: > On Sun, 2011-12-04 at 22:18 +, Russell King - ARM Linux wrote: > > .../... > > > And really, I believe it would be a good cleanup if all the standard > > definitions for atomic64 ops (like atomic64_add_negative) were also > > defined in include/linux/atomic.h rather than individually in every > > atomic*.h header throughout the kernel source, except where an arch > > wants to explicitly override it. Yet again, virtually all architectures > > define these in exactly the same way. > > > > We have more than enough code in arch/ for any architecture to worry > > about, we don't need schemes to add more when there's simple and > > practical solutions to avoiding doing so if the right design were > > chosen (preferably from the outset.) > > > > So, I'm not going to offer my ack for a change which I don't believe > > is the correct approach. > > I agree with Russell, his approach is a lot easier to maintain long run, > we should even consider converting existing definitions. I would rather go with "the existing definitions have to converted" and this means "not by this patch". At the moment, the atomic64 stuff exist only as separate generic or arch specific implementation. It is fine that Russell King noticed that people like Arun Sharma did a lot of work to made it true for atomic_t, but atomic64_t is a little bit different right now (at least as I understand it). Kind regards, Sven signature.asc Description: This is a digitally signed message part. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: Re: Re: [PATCHv5] atomic: add *_dec_not_zero
On Mon, 2011-12-05 at 08:57 +0100, Sven Eckelmann wrote: > On Monday 05 December 2011 09:41:55 Benjamin Herrenschmidt wrote: > > On Sun, 2011-12-04 at 22:18 +, Russell King - ARM Linux wrote: > > > > .../... > > > > > And really, I believe it would be a good cleanup if all the standard > > > definitions for atomic64 ops (like atomic64_add_negative) were also > > > defined in include/linux/atomic.h rather than individually in every > > > atomic*.h header throughout the kernel source, except where an arch > > > wants to explicitly override it. Yet again, virtually all architectures > > > define these in exactly the same way. > > > > > > We have more than enough code in arch/ for any architecture to worry > > > about, we don't need schemes to add more when there's simple and > > > practical solutions to avoiding doing so if the right design were > > > chosen (preferably from the outset.) > > > > > > So, I'm not going to offer my ack for a change which I don't believe > > > is the correct approach. > > > > I agree with Russell, his approach is a lot easier to maintain long run, > > we should even consider converting existing definitions. > > I would rather go with "the existing definitions have to converted" and this > means "not by this patch". Right. I didn't suggest -you- had to do it as a pre-req to your patch. > At the moment, the atomic64 stuff exist only as > separate generic or arch specific implementation. It is fine that Russell > King > noticed that people like Arun Sharma did a lot of work to made it true for > atomic_t, but atomic64_t is a little bit different right now (at least as I > understand it). Cheers, Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
