[PATCH] lib/atomic64_test: do not build on non-atomic64 systems

[PATCH] lib/atomic64_test: do not build on non-atomic64 systems

[Mike Frysinger]

If the arch doesn't provide atomic64 functionality (there are quite a
few), then don't bother trying to build this test.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 lib/atomic64_test.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/lib/atomic64_test.c b/lib/atomic64_test.c
index 250ed11..0ac1a66 100644
--- a/lib/atomic64_test.c
+++ b/lib/atomic64_test.c
@@ -12,6 +12,8 @@
 #include <linux/kernel.h>
 #include <asm/atomic.h>
 
+#ifdef ATOMIC64_INIT
+
 #define INIT(c) do { atomic64_set(&v, c); r = c; } while (0)
 static __init int test_atomic64(void)
 {
@@ -164,3 +166,5 @@ static __init int test_atomic64(void)
 }
 
 core_initcall(test_atomic64);
+
+#endif
-- 
1.7.3.1

Re: [PATCH] lib/atomic64_test: do not build on non-atomic64 systems

[Andrew Morton]

On Sat, 16 Oct 2010 13:27:15 -0400
Mike Frysinger <vapier@gentoo.org> wrote:

> If the arch doesn't provide atomic64 functionality (there are quite a
> few), then don't bother trying to build this test.
> 

I don't get it.  If the arch doesn't implement atomic64 then this file
will get zillions of build errors, won't it?

> diff --git a/lib/atomic64_test.c b/lib/atomic64_test.c
> index 250ed11..0ac1a66 100644
> --- a/lib/atomic64_test.c
> +++ b/lib/atomic64_test.c
> @@ -12,6 +12,8 @@
>  #include <linux/kernel.h>
>  #include <asm/atomic.h>
>  
> +#ifdef ATOMIC64_INIT

hm, that's a bit lazy.  It should really use a CONFIG_HAVE_ thing.  What
a pita.

>  #define INIT(c) do { atomic64_set(&v, c); r = c; } while (0)
>  static __init int test_atomic64(void)
>  {
> @@ -164,3 +166,5 @@ static __init int test_atomic64(void)
>  }
>  
>  core_initcall(test_atomic64);
> +
> +#endif
> -- 
> 1.7.3.1

Re: [PATCH] lib/atomic64_test: do not build on non-atomic64 systems

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 924 bytes --]

On Thursday, October 21, 2010 18:02:50 Andrew Morton wrote:
> On Sat, 16 Oct 2010 13:27:15 -0400 Mike Frysinger wrote:
> > If the arch doesn't provide atomic64 functionality (there are quite a
> > few), then don't bother trying to build this test.
> 
> I don't get it.  If the arch doesn't implement atomic64 then this file
> will get zillions of build errors, won't it?

... which is why i added the ifdef protection

> > diff --git a/lib/atomic64_test.c b/lib/atomic64_test.c
> > index 250ed11..0ac1a66 100644
> > --- a/lib/atomic64_test.c
> > +++ b/lib/atomic64_test.c
> > @@ -12,6 +12,8 @@
> > 
> >  #include <linux/kernel.h>
> >  #include <asm/atomic.h>
> > 
> > +#ifdef ATOMIC64_INIT
> 
> hm, that's a bit lazy.  It should really use a CONFIG_HAVE_ thing.  What
> a pita.

ATOMIC64_INIT is required for atomic64 headers to provide, and having a 
Kconfig knob doesnt gain anything else
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] lib/atomic64_test: do not build on non-atomic64 systems

[Andrew Morton]

On Thu, 21 Oct 2010 18:23:37 -0400
Mike Frysinger <vapier@gentoo.org> wrote:

> On Thursday, October 21, 2010 18:02:50 Andrew Morton wrote:
> > On Sat, 16 Oct 2010 13:27:15 -0400 Mike Frysinger wrote:
> > > If the arch doesn't provide atomic64 functionality (there are quite a
> > > few), then don't bother trying to build this test.
> > 
> > I don't get it.  If the arch doesn't implement atomic64 then this file
> > will get zillions of build errors, won't it?
> 
> ... which is why i added the ifdef protection

So the changelog was poor.  Please write complete changelogs so I need
to have this sort of conversation less often?

Why doesn't this cause lots of you-broke-my-build complaints?  Nobody's
running allmodonfig?

> > > diff --git a/lib/atomic64_test.c b/lib/atomic64_test.c
> > > index 250ed11..0ac1a66 100644
> > > --- a/lib/atomic64_test.c
> > > +++ b/lib/atomic64_test.c
> > > @@ -12,6 +12,8 @@
> > > 
> > >  #include <linux/kernel.h>
> > >  #include <asm/atomic.h>
> > > 
> > > +#ifdef ATOMIC64_INIT
> > 
> > hm, that's a bit lazy.  It should really use a CONFIG_HAVE_ thing.  What
> > a pita.
> 
> ATOMIC64_INIT is required for atomic64 headers to provide, and having a 
> Kconfig knob doesnt gain anything else

I know that.  But the standard way for an architecture to indicate to
the core that it impements a feature is for it to define CONFIG_HAVE_*.
Picking some related #define which architectures happen to implement
is atypical and unexpected.

Will it cause problems?  Probably not, unless the arch goes and defines
ATOMIC64_INIT without actually implementing atomic64.  But it's
atypical and unexpected and, yes, lazy!

Re: [PATCH] lib/atomic64_test: do not build on non-atomic64 systems

[Mike Frysinger]

On Thu, Oct 21, 2010 at 18:55, Andrew Morton wrote:
> On Thu, 21 Oct 2010 18:23:37 -0400 Mike Frysinger wrote:
>> On Thursday, October 21, 2010 18:02:50 Andrew Morton wrote:
>> > On Sat, 16 Oct 2010 13:27:15 -0400 Mike Frysinger wrote:
>> > > If the arch doesn't provide atomic64 functionality (there are quite a
>> > > few), then don't bother trying to build this test.
>> >
>> > I don't get it.  If the arch doesn't implement atomic64 then this file
>> > will get zillions of build errors, won't it?
>>
>> ... which is why i added the ifdef protection
>
> So the changelog was poor.  Please write complete changelogs so I need
> to have this sort of conversation less often?

the changelog seems pretty clear to me.  arch doesnt provide atomic64,
so dont build code that uses atomic64.

> Why doesn't this cause lots of you-broke-my-build complaints?  Nobody's
> running allmodonfig?

that's how i noticed.  perhaps the other arches that dont provide
atomic64 arent doing as many build tests as i am.

>> > > diff --git a/lib/atomic64_test.c b/lib/atomic64_test.c
>> > > index 250ed11..0ac1a66 100644
>> > > --- a/lib/atomic64_test.c
>> > > +++ b/lib/atomic64_test.c
>> > > @@ -12,6 +12,8 @@
>> > >
>> > >  #include <linux/kernel.h>
>> > >  #include <asm/atomic.h>
>> > >
>> > > +#ifdef ATOMIC64_INIT
>> >
>> > hm, that's a bit lazy.  It should really use a CONFIG_HAVE_ thing.  What
>> > a pita.
>>
>> ATOMIC64_INIT is required for atomic64 headers to provide, and having a
>> Kconfig knob doesnt gain anything else
>
> I know that.  But the standard way for an architecture to indicate to
> the core that it impements a feature is for it to define CONFIG_HAVE_*.
> Picking some related #define which architectures happen to implement
> is atypical and unexpected.
>
> Will it cause problems?  Probably not, unless the arch goes and defines
> ATOMIC64_INIT without actually implementing atomic64.  But it's
> atypical and unexpected and, yes, lazy!

you can say "lazy" all you like.  i dont see the point in going that route.
-mike

Re: [PATCH] lib/atomic64_test: do not build on non-atomic64 systems

[Andrew Morton]

On Thu, 21 Oct 2010 19:04:36 -0400
Mike Frysinger <vapier@gentoo.org> wrote:

> On Thu, Oct 21, 2010 at 18:55, Andrew Morton wrote:
> > On Thu, 21 Oct 2010 18:23:37 -0400 Mike Frysinger wrote:
> >> On Thursday, October 21, 2010 18:02:50 Andrew Morton wrote:
> >> > On Sat, 16 Oct 2010 13:27:15 -0400 Mike Frysinger wrote:
> >> > > If the arch doesn't provide atomic64 functionality (there are quite a
> >> > > few), then don't bother trying to build this test.
> >> >
> >> > I don't get it. __If the arch doesn't implement atomic64 then this file
> >> > will get zillions of build errors, won't it?
> >>
> >> ... which is why i added the ifdef protection
> >
> > So the changelog was poor. __Please write complete changelogs so I need
> > to have this sort of conversation less often?
> 
> the changelog seems pretty clear to me.  arch doesnt provide atomic64,
> so dont build code that uses atomic64.

That the patch fixes build errors is rather important information.

> > I know that. __But the standard way for an architecture to indicate to
> > the core that it impements a feature is for it to define CONFIG_HAVE_*.
> > Picking some related #define which architectures happen to implement
> > is atypical and unexpected.
> >
> > Will it cause problems? __Probably not, unless the arch goes and defines
> > ATOMIC64_INIT without actually implementing atomic64. __But it's
> > atypical and unexpected and, yes, lazy!
> 
> you can say "lazy" all you like.  i dont see the point in going that route.

Try

	grep HAVE arch/x86/Kconfig

If all of those were instead to use some random #define which the
particular feature happened to define in some header file then we would
have a mess on our hands.

There's your point.

Re: [PATCH] lib/atomic64_test: do not build on non-atomic64 systems

[Mike Frysinger]

On Thu, Oct 21, 2010 at 19:24, Andrew Morton wrote:
> On Thu, 21 Oct 2010 19:04:36 -0400 Mike Frysinger wrote:
>> you can say "lazy" all you like.  i dont see the point in going that route.
>
> Try
>
>        grep HAVE arch/x86/Kconfig
>
> If all of those were instead to use some random #define which the
> particular feature happened to define in some header file then we would
> have a mess on our hands.

fun times.  new tact.

Luca: your new atomic64_t test build fails on all arches that lack
atomic64_t.  please fix.
-mike

Re: [PATCH] lib/atomic64_test: do not build on non-atomic64 systems

[Andrew Morton]

On Fri, 22 Oct 2010 16:14:49 -0400
Mike Frysinger <vapier@gentoo.org> wrote:

> On Thu, Oct 21, 2010 at 19:24, Andrew Morton wrote:
> > On Thu, 21 Oct 2010 19:04:36 -0400 Mike Frysinger wrote:
> >> you can say "lazy" all you like. __i dont see the point in going that route.
> >
> > Try
> >
> > __ __ __ __grep HAVE arch/x86/Kconfig
> >
> > If all of those were instead to use some random #define which the
> > particular feature happened to define in some header file then we would
> > have a mess on our hands.
> 
> fun times.  new tact.
> 
> Luca: your new atomic64_t test build fails on all arches that lack
> atomic64_t.  please fix.

That's only part of the problem.  The following won't build also:

net/rds
kernel/perf_event.c
drivers/staging/octeon
drivers/infiniband/hw

with more to come.  These things should be made dependent upon
CONFIG_HAVE_ATOMIC64 in Kconfig.  (Can't use #ifdef ATOMIC64_INIT for this!)

Or, much better, we implement atomic64 on the offending architectures.

Re: [PATCH] lib/atomic64_test: do not build on non-atomic64 systems

[Mike Frysinger]

On Fri, Oct 22, 2010 at 16:31, Andrew Morton wrote:
> On Fri, 22 Oct 2010 16:14:49 -0400 Mike Frysinger wrote:
>> On Thu, Oct 21, 2010 at 19:24, Andrew Morton wrote:
>> > On Thu, 21 Oct 2010 19:04:36 -0400 Mike Frysinger wrote:
>> >> you can say "lazy" all you like. __i dont see the point in going that route.
>> >
>> > Try
>> >
>> > __ __ __ __grep HAVE arch/x86/Kconfig
>> >
>> > If all of those were instead to use some random #define which the
>> > particular feature happened to define in some header file then we would
>> > have a mess on our hands.
>>
>> fun times.  new tact.
>>
>> Luca: your new atomic64_t test build fails on all arches that lack
>> atomic64_t.  please fix.
>
> That's only part of the problem.  The following won't build also:
>
> net/rds

not true.  that code base is already using my suggestion:
net/rds/rds.h:
#ifdef ATOMIC64_INIT
#define KERNEL_HAS_ATOMIC64
#endif

but this isnt a matter of "use atomic64_t or atomic_t" ... this code
manually takes care of doing a spinlock around a u64 member.  i
imagine if you'd notice this before it was merged you'd have made them
fix this cleanly.

> kernel/perf_event.c

also not true -- this requires arches to opt in to HAVE_PERF_EVENTS
and only arches which have validated it works (i.e. they have
atomic64) have done that

> drivers/staging/octeon

not an issue -- this depends on CPU_CAVIUM_OCTEON which is only
provided by mips which provides atomic64

> drivers/infiniband/hw

the only code usage of atomic64 is in code that already depends on the
Kconfig symbol 64BIT

as for why it depends on this, i dont know ... maybe it's because of atomic64_t

> Or, much better, we implement atomic64 on the offending architectures.

i dont want to give people the impression that 64 atomics are free to
use if in reality they're pretty expensive.  on a Blackfin system, i'd
need to implement every access with basically a spinlock.

> with more to come.  These things should be made dependent upon
> CONFIG_HAVE_ATOMIC64 in Kconfig.  (Can't use #ifdef ATOMIC64_INIT for this!)

these are actually compelling arguments unlike the original one.  even
if all these examples ended up not really being true (only the new
atomic64_t test code is available to build on arches that lack
atomic64).
-mike

Re: [PATCH] lib/atomic64_test: do not build on non-atomic64 systems

[Andrew Morton]

On Fri, 22 Oct 2010 16:47:36 -0400
Mike Frysinger <vapier@gentoo.org> wrote:

> On Fri, Oct 22, 2010 at 16:31, Andrew Morton wrote:
> > On Fri, 22 Oct 2010 16:14:49 -0400 Mike Frysinger wrote:
> >> On Thu, Oct 21, 2010 at 19:24, Andrew Morton wrote:
> >> > On Thu, 21 Oct 2010 19:04:36 -0400 Mike Frysinger wrote:
> >> >> you can say "lazy" all you like. __i dont see the point in going that route.
> >> >
> >> > Try
> >> >
> >> > __ __ __ __grep HAVE arch/x86/Kconfig
> >> >
> >> > If all of those were instead to use some random #define which the
> >> > particular feature happened to define in some header file then we would
> >> > have a mess on our hands.
> >>
> >> fun times. __new tact.
> >>
> >> Luca: your new atomic64_t test build fails on all arches that lack
> >> atomic64_t. __please fix.
> >
> > That's only part of the problem. __The following won't build also:
> >
> > net/rds
> 
> not true.  that code base is already using my suggestion:
> net/rds/rds.h:
> #ifdef ATOMIC64_INIT
> #define KERNEL_HAS_ATOMIC64
> #endif

IOW, your suggestion led to a nasty local hack.  One which would be
unneeded had we implemented this properly via Kconfig.

> but this isnt a matter of "use atomic64_t or atomic_t" ... this code
> manually takes care of doing a spinlock around a u64 member.  i
> imagine if you'd notice this before it was merged you'd have made them
> fix this cleanly.

I'd have suggested that they use Kconfig.

I'd also have suggested that they implement a generic spinlocked
atomic64 library rather than open-coding stuff down in net/rds/.

People do tend to prefer to do localised expedient things rather than
sticking their necks out and implementing proper, generic kernel-wide
functions.  If I see it happen, I'll tell them.  Usually I don't see
it until months after it's merged :(

> > kernel/perf_event.c
> 
> also not true -- this requires arches to opt in to HAVE_PERF_EVENTS
> and only arches which have validated it works (i.e. they have
> atomic64) have done that

kernel/perf_event.c has a dependency on the missing CONFIG_HAVE_ATOMIC64.

> > drivers/staging/octeon
> 
> not an issue -- this depends on CPU_CAVIUM_OCTEON which is only
> provided by mips which provides atomic64
> 
> > drivers/infiniband/hw
> 
> the only code usage of atomic64 is in code that already depends on the
> Kconfig symbol 64BIT

Again, that's just wrong and it's a fluke.  Will break if a 64-bit arch
doesn't implement atomic64.

> as for why it depends on this, i dont know ... maybe it's because of atomic64_t
> 
> > Or, much better, we implement atomic64 on the offending architectures.
> 
> i dont want to give people the impression that 64 atomics are free to
> use if in reality they're pretty expensive.  on a Blackfin system, i'd
> need to implement every access with basically a spinlock.

spin_lock_irqsave(), really.  Becomes local_irq_save() on UP.

But what's the alternative?  Either entire features become unavailable
on those architectures or we grow local hacks like rds_ib_get_ack()
which, yup, uses spin_lock_irqsave().

> > with more to come.  These things should be made dependent upon
> > CONFIG_HAVE_ATOMIC64 in Kconfig.  (Can't use #ifdef ATOMIC64_INIT for this!)
> 
> these are actually compelling arguments unlike the original one.  even
> if all these examples ended up not really being true (only the new
> atomic64_t test code is available to build on arches that lack
> atomic64).

Really, the atomic64_t implementation was imcomplete.  It should have
provided the generic arch-neutral fallback code.

Re: [PATCH] lib/atomic64_test: do not build on non-atomic64 systems

[Mike Frysinger]

On Fri, Oct 22, 2010 at 17:00, Andrew Morton wrote:
> On Fri, 22 Oct 2010 16:47:36 -0400 Mike Frysinger wrote:
>> On Fri, Oct 22, 2010 at 16:31, Andrew Morton wrote:
>> > On Fri, 22 Oct 2010 16:14:49 -0400 Mike Frysinger wrote:
>> >> On Thu, Oct 21, 2010 at 19:24, Andrew Morton wrote:
>> >> > On Thu, 21 Oct 2010 19:04:36 -0400 Mike Frysinger wrote:
>> >> >> you can say "lazy" all you like. __i dont see the point in going that route.
>> >> >
>> >> > Try
>> >> >
>> >> > __ __ __ __grep HAVE arch/x86/Kconfig
>> >> >
>> >> > If all of those were instead to use some random #define which the
>> >> > particular feature happened to define in some header file then we would
>> >> > have a mess on our hands.
>> >>
>> >> fun times. __new tact.
>> >>
>> >> Luca: your new atomic64_t test build fails on all arches that lack
>> >> atomic64_t. __please fix.
>> >
>> > That's only part of the problem. __The following won't build also:
>> >
>> > net/rds
>>
>> not true.  that code base is already using my suggestion:
>> net/rds/rds.h:
>> #ifdef ATOMIC64_INIT
>> #define KERNEL_HAS_ATOMIC64
>> #endif
>
> IOW, your suggestion led to a nasty local hack.  One which would be
> unneeded had we implemented this properly via Kconfig.

no.  mine was specific to the atomic64 api and not random consumers.
the test code is tightly coupled to the atomic64 api by nature, so my
proposal as constrained by the single instance makes perfect sense.
as soon as you start talking about more things needing to know "is
atomic64 available" and doing so at the config dependency level, then
a Kconfig option makes sense.  thus i didnt and still dont buy your
original argument against the original patch and constraint.  but with
real tree data, your suggestion makes sense.  perhaps i'll implement
it if i find time.
-mike

Re: [PATCH] lib/atomic64_test: do not build on non-atomic64 systems

[Roland Dreier]

 > That's only part of the problem.  The following won't build also:

 > drivers/infiniband/hw

Interesting... I hadn't looked at that usage before.  Both drivers that
seem to use atomic64 already depend on 64BIT in Kconfig, so I suspect
the intersection of 64BIT and !ATOMIC64 is empty?

But eg drivers/infiniband/hw/qib/qib_rc.c does essentially:

	atomic64_t *maddr;
	u64 sdata = <something>
	...
	maddr = (atomic64_t *) qp->r_sge.sge.vaddr;
	atomic64_add_return(sdata, maddr)

is it legit to cast some random address to atomic64_t and expect it to
work across archs that implement atomic64?  (I expect x86_64 is probably
OK but I wonder if other architectures might not be so understanding)

 - R.

Re: [PATCH] lib/atomic64_test: do not build on non-atomic64 systems

[Andrew Morton]

On Sun, 24 Oct 2010 09:20:34 -0700 Roland Dreier <rdreier@cisco.com> wrote:

>  > That's only part of the problem.  The following won't build also:
> 
>  > drivers/infiniband/hw
> 
> Interesting... I hadn't looked at that usage before.  Both drivers that
> seem to use atomic64 already depend on 64BIT in Kconfig, so I suspect
> the intersection of 64BIT and !ATOMIC64 is empty?
> 
> But eg drivers/infiniband/hw/qib/qib_rc.c does essentially:
> 
> 	atomic64_t *maddr;
> 	u64 sdata = <something>
> 	...
> 	maddr = (atomic64_t *) qp->r_sge.sge.vaddr;
> 	atomic64_add_return(sdata, maddr)
> 
> is it legit to cast some random address to atomic64_t and expect it to
> work across archs that implement atomic64?

Not really.  If someone implements atomic64 on 32-bit they may 
do it by putting a spinlock in the atomic64_t.  Or they might use
hashed spinlocks, in which case that'll work.

Probably hashed spinlocks, given the (realtively new) convention that
the all-zeroes pattern is a legit way of initialising an atomic_t.