Fwd: Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

Fwd: Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Sowmini Varadhan]

> On (04/19/15 14:09), David Miller wrote:
>
> > On (04/18/15 21:23), Guenter Roeck wrote:
> >> lib/built-in.o:(.discard+0x1): multiple definition of
> >> `__pcpu_unique_iommu_pool_hash'
> >> arch/powerpc/kernel/built-in.o:(.discard+0x18): first defined here
> >> .. I get a similar failure in the
> >> powerpc:allmodconfig build
    :
> 
> Maybe ping the powerpc folks becuase if they can do a quick
> conversion, this change isn't necessary.

linuxppc-dev,

The disussion above [http://www.spinics.net/lists/sparclinux/msg13835.html]
is in reference to the issue that Guenter Roeck
identified. The problem is that we have a 
  static DEFINE_PER_CPU(unsigned int, iommu_pool_hash);
in both lib/iommu-common.c as well as arch/powerpc/kernel/iommu.c,
and as Guenter correctly pointed out,

" DEFINE_PER_CPU translates to DEFINE_PER_CPU_SECTION, which in turn is
  defined as

  #define DEFINE_PER_CPU_SECTION(type, name, sec)                         \
        __PCPU_DUMMY_ATTRS char __pcpu_scope_##name;                    \
	extern __PCPU_DUMMY_ATTRS char __pcpu_unique_##name;		\
  -->	__PCPU_DUMMY_ATTRS char __pcpu_unique_##name;			\
	extern __PCPU_ATTRS(sec) __typeof__(type) name;			\
	__PCPU_ATTRS(sec) PER_CPU_DEF_ATTRIBUTES __weak                 \
					        __typeof__(type) name

  if CONFIG_DEBUG_FORCE_WEAK_PER_CPU is configured, which is the case here.
  The marked line above shows that __pcpu_unique_iommu_pool_hash is declared as
  global variable"

This results in a build error for allmodconfig on powerpc, due to 
a multiple definition of __pcpu_unique_iommu_pool_hash.

Given that the goal is to use the functions in lib/iommu-common.c
as the Generic IOMMU pooled allocator that will be a superset of
functionality in arch/powerpc/kernel/iommu.c, DaveM suggested that I 
ping you to see if you can do a quick conversion to handle this
duplicate defintion, rather than rename iommu_pool_hash in iommu-common.c.

Would that be possible?

--Sowmini

Re: Fwd: Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Michael Ellerman]

On Sun, 2015-04-19 at 14:36 -0400, Sowmini Varadhan wrote:
> > On (04/19/15 14:09), David Miller wrote:
> >
> > > On (04/18/15 21:23), Guenter Roeck wrote:
> > >> lib/built-in.o:(.discard+0x1): multiple definition of
> > >> `__pcpu_unique_iommu_pool_hash'
> > >> arch/powerpc/kernel/built-in.o:(.discard+0x18): first defined here
> > >> .. I get a similar failure in the
> > >> powerpc:allmodconfig build
>     :
> > 
> > Maybe ping the powerpc folks becuase if they can do a quick
> > conversion, this change isn't necessary.
> 
> linuxppc-dev,
> 
> The disussion above [http://www.spinics.net/lists/sparclinux/msg13835.html]
> is in reference to the issue that Guenter Roeck
> identified. The problem is that we have a 
>   static DEFINE_PER_CPU(unsigned int, iommu_pool_hash);

It's static ..

>   if CONFIG_DEBUG_FORCE_WEAK_PER_CPU is configured, which is the case here.
>   The marked line above shows that __pcpu_unique_iommu_pool_hash is declared as
>   global variable"

OK, so why doesn't CONFIG_DEBUG_FORCE_WEAK_PER_CPU depend on s390 and/or alpha?

Someone needs to be doing s390/alpha builds with that enabled anyway, because
otherwise a clash between generic code and s390/alpha won't be caught.

Or if that's too hard we can rename the powerpc version, but it seems silly to
rename a powerpc variable to deal with a debug option that is only useful for
s390/alpha.

cheers

Re: Fwd: Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Guenter Roeck]

Hi Michael,

On 04/19/2015 08:01 PM, Michael Ellerman wrote:
> On Sun, 2015-04-19 at 14:36 -0400, Sowmini Varadhan wrote:
>>> On (04/19/15 14:09), David Miller wrote:
>>>
>>>> On (04/18/15 21:23), Guenter Roeck wrote:
>>>>> lib/built-in.o:(.discard+0x1): multiple definition of
>>>>> `__pcpu_unique_iommu_pool_hash'
>>>>> arch/powerpc/kernel/built-in.o:(.discard+0x18): first defined here
>>>>> .. I get a similar failure in the
>>>>> powerpc:allmodconfig build
>>      :
>>>
>>> Maybe ping the powerpc folks becuase if they can do a quick
>>> conversion, this change isn't necessary.
>>
>> linuxppc-dev,
>>
>> The disussion above [http://www.spinics.net/lists/sparclinux/msg13835.html]
>> is in reference to the issue that Guenter Roeck
>> identified. The problem is that we have a
>>    static DEFINE_PER_CPU(unsigned int, iommu_pool_hash);
>
> It's static ..
>
Not if CONFIG_DEBUG_FORCE_WEAK_PER_CPU is configured.

>>    if CONFIG_DEBUG_FORCE_WEAK_PER_CPU is configured, which is the case here.
>>    The marked line above shows that __pcpu_unique_iommu_pool_hash is declared as
>>    global variable"
>
> OK, so why doesn't CONFIG_DEBUG_FORCE_WEAK_PER_CPU depend on s390 and/or alpha?
>
The idea is to ensure that per cpu variable names are unique, even if static,
because that is what is needed for s390 and alpha.

> Someone needs to be doing s390/alpha builds with that enabled anyway, because
> otherwise a clash between generic code and s390/alpha won't be caught.
>
> Or if that's too hard we can rename the powerpc version, but it seems silly to
> rename a powerpc variable to deal with a debug option that is only useful for
> s390/alpha.
>

The debug option is intended for all _other_ architectures, to ensure that
changes made for those don't break alpha/s390 builds. alpha/s390 have
ARCH_NEEDS_WEAK_PER_CPU and don't need the debug option.

Sowmini's patch would change the variable name in the lib/ code. But that was
not the question here. The question was if the powerpc code could be changed
to use the generic iommu code instead of using the powerpc specific code.

Thanks,
Guenter

Re: Fwd: Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Michael Ellerman]

On Sun, 2015-04-19 at 22:17 -0700, Guenter Roeck wrote:
> Hi Michael,

Hi Guenter,

> On 04/19/2015 08:01 PM, Michael Ellerman wrote:
> 
> > Someone needs to be doing s390/alpha builds with that enabled anyway, because
> > otherwise a clash between generic code and s390/alpha won't be caught.
> >
> > Or if that's too hard we can rename the powerpc version, but it seems silly to
> > rename a powerpc variable to deal with a debug option that is only useful for
> > s390/alpha.
> 
> The debug option is intended for all _other_ architectures, to ensure that
> changes made for those don't break alpha/s390 builds. alpha/s390 have
> ARCH_NEEDS_WEAK_PER_CPU and don't need the debug option.

But that can't actually work. If you introduce a percpu variable in generic
code that has the same name as a variable in the s390/alpha code, the only way
you can detect that is by building for s390/alpha.

Sure it might catch two variables in generic code that have the same name, but
that will get caught as soon as someone does a s390/alpha build anyway - which
is at least every night for linux-next.

More likely it catches cases like this, which is variables in code that will
never build for s390/alpha. So it just seems like a PITA to me.

> Sowmini's patch would change the variable name in the lib/ code. But that was
> not the question here. The question was if the powerpc code could be changed
> to use the generic iommu code instead of using the powerpc specific code.

Not for this merge window sorry, we need to test it properly.

cheers

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[David Miller]

From: Guenter Roeck <linux@roeck-us.net>
Date: Sun, 19 Apr 2015 22:17:21 -0700

> The debug option is intended for all _other_ architectures, to
> ensure that changes made for those don't break alpha/s390
> builds. alpha/s390 have ARCH_NEEDS_WEAK_PER_CPU and don't need the
> debug option.

Ironically this would not create a build failure for the architectures
where this matters, because only powerpc has the like named percpu
symbol.

So it's not really meeting the stated objective in this case.

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Guenter Roeck]

On Mon, Apr 20, 2015 at 12:25:19PM -0400, David Miller wrote:
> From: Guenter Roeck <linux@roeck-us.net>
> Date: Sun, 19 Apr 2015 22:17:21 -0700
> 
> > The debug option is intended for all _other_ architectures, to
> > ensure that changes made for those don't break alpha/s390
> > builds. alpha/s390 have ARCH_NEEDS_WEAK_PER_CPU and don't need the
> > debug option.
> 
> Ironically this would not create a build failure for the architectures
> where this matters, because only powerpc has the like named percpu
> symbol.
> 
> So it's not really meeting the stated objective in this case.

Yes, that is correct; it can only find problems in non-architecture
code, and on the downside produces false positives and thus build errors
like this one.

Which makes the fix a bit philosophical. Rename iommu_pool_hash in
iommu-common, or drop DEBUG_FORCE_WEAK_PER_CPU. I would rename
iommu_pool_hash, but that is just me. Ultimately, I don't really
care one way or another, as long as the problem gets fixed.

Guenter

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[David Miller]

From: Guenter Roeck <linux@roeck-us.net>
Date: Mon, 20 Apr 2015 09:44:31 -0700

> On Mon, Apr 20, 2015 at 12:25:19PM -0400, David Miller wrote:
>> From: Guenter Roeck <linux@roeck-us.net>
>> Date: Sun, 19 Apr 2015 22:17:21 -0700
>> 
>> > The debug option is intended for all _other_ architectures, to
>> > ensure that changes made for those don't break alpha/s390
>> > builds. alpha/s390 have ARCH_NEEDS_WEAK_PER_CPU and don't need the
>> > debug option.
>> 
>> Ironically this would not create a build failure for the architectures
>> where this matters, because only powerpc has the like named percpu
>> symbol.
>> 
>> So it's not really meeting the stated objective in this case.
> 
> Yes, that is correct; it can only find problems in non-architecture
> code, and on the downside produces false positives and thus build errors
> like this one.
> 
> Which makes the fix a bit philosophical. Rename iommu_pool_hash in
> iommu-common, or drop DEBUG_FORCE_WEAK_PER_CPU. I would rename
> iommu_pool_hash, but that is just me. Ultimately, I don't really
> care one way or another, as long as the problem gets fixed.

If nightly builds of s390 and alpha, the two platforms where this
matters, are being done as reported in this thread, then I really
don't see the value in DEBUG_FORCE_WEAK_PER_CPU.

But I guess that's a more involved longer-term discussion and I guess
I'll apply Sowmini's patches for now.

Thanks.

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Michael Ellerman]

On Mon, 2015-04-20 at 12:50 -0400, David Miller wrote:
> From: Guenter Roeck <linux@roeck-us.net>
> Date: Mon, 20 Apr 2015 09:44:31 -0700
> 
> > On Mon, Apr 20, 2015 at 12:25:19PM -0400, David Miller wrote:
> >> From: Guenter Roeck <linux@roeck-us.net>
> >> Date: Sun, 19 Apr 2015 22:17:21 -0700
> >> 
> >> > The debug option is intended for all _other_ architectures, to
> >> > ensure that changes made for those don't break alpha/s390
> >> > builds. alpha/s390 have ARCH_NEEDS_WEAK_PER_CPU and don't need the
> >> > debug option.
> >> 
> >> Ironically this would not create a build failure for the architectures
> >> where this matters, because only powerpc has the like named percpu
> >> symbol.
> >> 
> >> So it's not really meeting the stated objective in this case.
> > 
> > Yes, that is correct; it can only find problems in non-architecture
> > code, and on the downside produces false positives and thus build errors
> > like this one.
> > 
> > Which makes the fix a bit philosophical. Rename iommu_pool_hash in
> > iommu-common, or drop DEBUG_FORCE_WEAK_PER_CPU. I would rename
> > iommu_pool_hash, but that is just me. Ultimately, I don't really
> > care one way or another, as long as the problem gets fixed.
> 
> If nightly builds of s390 and alpha, the two platforms where this
> matters, are being done as reported in this thread, then I really
> don't see the value in DEBUG_FORCE_WEAK_PER_CPU.

We do an s390 allmodconfig for every linux-next release:

  http://kisskb.ellerman.id.au/kisskb/target/573/

And also for Linus' tree:

  http://kisskb.ellerman.id.au/kisskb/target/568/

We don't have alpha allmodconfig enabled, though we could, but we do build the
defconfig:

  http://kisskb.ellerman.id.au/kisskb/target/2499/
  http://kisskb.ellerman.id.au/kisskb/target/2494/

So I think that should be sufficient to catch any percpus that are introduced
in generic code with the same name as s390/alpha variables.


> But I guess that's a more involved longer-term discussion and I guess
> I'll apply Sowmini's patches for now.

Yeah I guess it is. Thanks for merging the fix.

cheers

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Guenter Roeck]

On 04/20/2015 06:54 PM, Michael Ellerman wrote:
> On Mon, 2015-04-20 at 12:50 -0400, David Miller wrote:
>> From: Guenter Roeck <linux@roeck-us.net>
>> Date: Mon, 20 Apr 2015 09:44:31 -0700
>>
>>> On Mon, Apr 20, 2015 at 12:25:19PM -0400, David Miller wrote:
>>>> From: Guenter Roeck <linux@roeck-us.net>
>>>> Date: Sun, 19 Apr 2015 22:17:21 -0700
>>>>
>>>>> The debug option is intended for all _other_ architectures, to
>>>>> ensure that changes made for those don't break alpha/s390
>>>>> builds. alpha/s390 have ARCH_NEEDS_WEAK_PER_CPU and don't need the
>>>>> debug option.
>>>>
>>>> Ironically this would not create a build failure for the architectures
>>>> where this matters, because only powerpc has the like named percpu
>>>> symbol.
>>>>
>>>> So it's not really meeting the stated objective in this case.
>>>
>>> Yes, that is correct; it can only find problems in non-architecture
>>> code, and on the downside produces false positives and thus build errors
>>> like this one.
>>>
>>> Which makes the fix a bit philosophical. Rename iommu_pool_hash in
>>> iommu-common, or drop DEBUG_FORCE_WEAK_PER_CPU. I would rename
>>> iommu_pool_hash, but that is just me. Ultimately, I don't really
>>> care one way or another, as long as the problem gets fixed.
>>
>> If nightly builds of s390 and alpha, the two platforms where this
>> matters, are being done as reported in this thread, then I really
>> don't see the value in DEBUG_FORCE_WEAK_PER_CPU.

Me not either, but, as you say, that is a different discussion.

>
> We do an s390 allmodconfig for every linux-next release:
>
>    http://kisskb.ellerman.id.au/kisskb/target/573/
>
> And also for Linus' tree:
>
>    http://kisskb.ellerman.id.au/kisskb/target/568/
>
> We don't have alpha allmodconfig enabled, though we could, but we do build the
> defconfig:
>
>    http://kisskb.ellerman.id.au/kisskb/target/2499/
>    http://kisskb.ellerman.id.au/kisskb/target/2494/
>
I cover alpha:allmodconfig in my builds for -next, mainline, as well as all
kernel.org stable releases and release candidates. This discussion is a good
argument for enabling s390:allmodconfig as well.

> So I think that should be sufficient to catch any percpus that are introduced
> in generic code with the same name as s390/alpha variables.
>
Yes, but unfortunately only after the fact, though I don't see a means
to avoid that.

>
>> But I guess that's a more involved longer-term discussion and I guess
>> I'll apply Sowmini's patches for now.
>
Thanks!

Guenter

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Michael Ellerman]

On Mon, 2015-04-20 at 19:32 -0700, Guenter Roeck wrote:
> On 04/20/2015 06:54 PM, Michael Ellerman wrote:
> > On Mon, 2015-04-20 at 12:50 -0400, David Miller wrote:
> >> From: Guenter Roeck <linux@roeck-us.net>
> >> Date: Mon, 20 Apr 2015 09:44:31 -0700
> >>
> >>> On Mon, Apr 20, 2015 at 12:25:19PM -0400, David Miller wrote:
> >>>> From: Guenter Roeck <linux@roeck-us.net>
> >>>> Date: Sun, 19 Apr 2015 22:17:21 -0700
> >>>>
> >>>>> The debug option is intended for all _other_ architectures, to
> >>>>> ensure that changes made for those don't break alpha/s390
> >>>>> builds. alpha/s390 have ARCH_NEEDS_WEAK_PER_CPU and don't need the
> >>>>> debug option.
> >>>>
> >>>> Ironically this would not create a build failure for the architectures
> >>>> where this matters, because only powerpc has the like named percpu
> >>>> symbol.
> >>>>
> >>>> So it's not really meeting the stated objective in this case.
> >>>
> >>> Yes, that is correct; it can only find problems in non-architecture
> >>> code, and on the downside produces false positives and thus build errors
> >>> like this one.
> >>>
> >>> Which makes the fix a bit philosophical. Rename iommu_pool_hash in
> >>> iommu-common, or drop DEBUG_FORCE_WEAK_PER_CPU. I would rename
> >>> iommu_pool_hash, but that is just me. Ultimately, I don't really
> >>> care one way or another, as long as the problem gets fixed.
> >>
> >> If nightly builds of s390 and alpha, the two platforms where this
> >> matters, are being done as reported in this thread, then I really
> >> don't see the value in DEBUG_FORCE_WEAK_PER_CPU.
> 
> Me not either, but, as you say, that is a different discussion.
> 
> >
> > We do an s390 allmodconfig for every linux-next release:
> >
> >    http://kisskb.ellerman.id.au/kisskb/target/573/
> >
> > And also for Linus' tree:
> >
> >    http://kisskb.ellerman.id.au/kisskb/target/568/
> >
> > We don't have alpha allmodconfig enabled, though we could, but we do build the
> > defconfig:
> >
> >    http://kisskb.ellerman.id.au/kisskb/target/2499/
> >    http://kisskb.ellerman.id.au/kisskb/target/2494/
>
> I cover alpha:allmodconfig in my builds for -next, mainline, as well as all
> kernel.org stable releases and release candidates. This discussion is a good
> argument for enabling s390:allmodconfig as well.
> 
> > So I think that should be sufficient to catch any percpus that are introduced
> > in generic code with the same name as s390/alpha variables.
> 
> Yes, but unfortunately only after the fact, though I don't see a means
> to avoid that.

Yeah after the merge into linux-next, which I think is probably good enough for
something like this.

cheers