sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common

sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common

[Guenter Roeck]

Hi all,

I see the following build failure when compiling sparc64:allmodconfig
in the upstream kernel (v4.0-7820-g04b7fe6a4a23).

arch/sparc/kernel/pci_sun4v.o:(.discard+0x1): multiple definition of `__pcpu_unique_iommu_pool_hash'
arch/sparc/kernel/iommu.o:(.discard+0x0): first defined here
make[2]: *** [arch/sparc/kernel/built-in.o] Error 1
make[1]: *** [arch/sparc/kernel] Error 2

The problem is caused by commit f1600e549b94 ("sparc: Make sparc64
use scalable lib/iommu-common.c functions"), which introduces 

static DEFINE_PER_CPU(unsigned int, iommu_pool_hash);

in both files.

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, which explains the problem (and makes me wonder what the
'static' keyword in front of DEFINE_PER_CPU is supposed to accomplish).

I thought about fixing the problem by renaming one of the variables, but
I am not sure if that is what is intended. Specifically, I am not sure if
the variables are supposed to be different, as it looks like, or if they
are supposed to be the same.

In case it is relevant, I use gcc version 4.6.3 for my build test.

Guenter

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-co

[Sowmini Varadhan]

On (04/17/15 22:47), Guenter Roeck wrote:
> #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, which explains the problem (and makes me wonder what the
> 'static' keyword in front of DEFINE_PER_CPU is supposed to accomplish).
> 
> I thought about fixing the problem by renaming one of the variables, but
> I am not sure if that is what is intended. Specifically, I am not sure if
> the variables are supposed to be different, as it looks like, or if they
> are supposed to be the same.

In this particular case, the intention is to have this variable only
be static to lib/iommu-common.c which tries to generalize the code
in arch/powerpc/kernel/iommu.c. So it would be fine to rename one
of those variables, but I'm not sure you can safely do that by
changing the macro above, as that would impact other uses of this.

> In case it is relevant, I use gcc version 4.6.3 for my build test.

Let me reproduce your error on my test machine and try to put
a fix together..

--Sowmini

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-co

[Sowmini Varadhan]

On (04/17/15 22:47), Guenter Roeck wrote:
> The problem is caused by commit f1600e549b94 ("sparc: Make sparc64
> use scalable lib/iommu-common.c functions"), which introduces 
> 
> static DEFINE_PER_CPU(unsigned int, iommu_pool_hash);

I have to confess that I'm a little confused about what happened here..

The specific patch (2/3) above should have come from this submission
  http://www.spinics.net/lists/sparclinux/msg13786.html
aka 
  http://patchwork.ozlabs.org/patch/459803/

This does not add any additional defines for iommu_pool_hash.

It also does not have any references to fields like page_table_map_base:
instead, these were switched over to things like:
-	*dma_addrp = (iommu->page_table_map_base +
+	*dma_addrp = (iommu->tbl.table_map_base +
 		      ((iopte - iommu->page_table) << IO_PAGE_SHIFT));


But when I clone 
  git://git.kernel.org/pub/scm/linux/kernel/git/davem/sparc.git
and do a git show of the commit-id above, I see deltas that
dont make sense (they seem to be from a patchset from somewhere
in the middle of the review chain from the thread). 

What am I missing? 

--Sowmini

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-co

[Guenter Roeck]

On 04/18/2015 05:05 AM, Sowmini Varadhan wrote:
> On (04/17/15 22:47), Guenter Roeck wrote:
>> The problem is caused by commit f1600e549b94 ("sparc: Make sparc64
>> use scalable lib/iommu-common.c functions"), which introduces
>>
>> static DEFINE_PER_CPU(unsigned int, iommu_pool_hash);
>
> I have to confess that I'm a little confused about what happened here..
>
> The specific patch (2/3) above should have come from this submission
>    http://www.spinics.net/lists/sparclinux/msg13786.html
> aka
>    http://patchwork.ozlabs.org/patch/459803/
>
> This does not add any additional defines for iommu_pool_hash.
>
> It also does not have any references to fields like page_table_map_base:
> instead, these were switched over to things like:
> -	*dma_addrp = (iommu->page_table_map_base +
> +	*dma_addrp = (iommu->tbl.table_map_base +
>   		      ((iopte - iommu->page_table) << IO_PAGE_SHIFT));
>
>
> But when I clone
>    git://git.kernel.org/pub/scm/linux/kernel/git/davem/sparc.git
> and do a git show of the commit-id above, I see deltas that
> dont make sense (they seem to be from a patchset from somewhere
> in the middle of the review chain from the thread).
>
> What am I missing?
>

Some merge gone wrong, maybe ? I tried to revert f1600e549b94
and apply http://patchwork.ozlabs.org/patch/459803/ instead.
The result doesn't compile, so there may have been some other
changes in the same area.

Guenter

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-co

[Sowmini Varadhan]

On (04/18/15 11:28), Guenter Roeck wrote:
> 
> Some merge gone wrong, maybe ? I tried to revert f1600e549b94
> and apply http://patchwork.ozlabs.org/patch/459803/ instead.

That patch is a part-2 of a 3-part patch set. In order, this should
have been v10: applied as:

   http://patchwork.ozlabs.org/patch/459804/
   http://patchwork.ozlabs.org/patch/459803/
   http://patchwork.ozlabs.org/patch/459802/

The mail thread for the 3-part patch set was here
   http://www.spinics.net/lists/sparclinux/msg13785.html

I noticed that the existing dates on the iommu commits were also odd:

  commit 671d773297969bebb1732e1cdc1ec03aa53c6be2
  Author: Sowmini Varadhan <sowmini.varadhan@oracle.com>
  Date:   Thu Mar 12 20:02:37 2015 -0400


  commit f1600e549b948a32ad7672e069b2915314637ae3
  Author: Sowmini Varadhan <sowmini.varadhan@oracle.com>
  Date:   Thu Mar 12 20:02:36 2015 -0400
  
  commit 10b88a4b17d31a7409494b179dcb76e7ab2fcaea
  Author: Sowmini Varadhan <sowmini.varadhan@oracle.com>
  Date:   Thu Mar 12 20:02:35 2015 -0400

We have had many discussions of this patch set since March 12,
and the correct version should have been v10, dated something
after Apr 9, and acked by Benjamin Herrenschmidt.

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-co

[David Miller]

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Date: Sat, 18 Apr 2015 08:05:10 -0400

> But when I clone 
>   git://git.kernel.org/pub/scm/linux/kernel/git/davem/sparc.git
> and do a git show of the commit-id above, I see deltas that
> dont make sense (they seem to be from a patchset from somewhere
> in the middle of the review chain from the thread). 
> 
> What am I missing? 

I am pretty sure I applied V10 of your IOMMU submission.

http://patchwork.ozlabs.org/patch/459804/
http://patchwork.ozlabs.org/patch/459803/
http://patchwork.ozlabs.org/patch/459802/

and follow-on parisc build fix:

http://patchwork.ozlabs.org/patch/461905/

if I somehow botched this up, wow that's impressive :-)

If you can sort out what I did wrong and send me a relative
fixup patch, I'd really appreciate it.

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-co

[David Miller]

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Date: Sat, 18 Apr 2015 14:38:44 -0400

> On (04/18/15 11:28), Guenter Roeck wrote:
>> 
>> Some merge gone wrong, maybe ? I tried to revert f1600e549b94
>> and apply http://patchwork.ozlabs.org/patch/459803/ instead.
> 
> That patch is a part-2 of a 3-part patch set. In order, this should
> have been v10: applied as:
> 
>    http://patchwork.ozlabs.org/patch/459804/
>    http://patchwork.ozlabs.org/patch/459803/
>    http://patchwork.ozlabs.org/patch/459802/
> 
> The mail thread for the 3-part patch set was here
>    http://www.spinics.net/lists/sparclinux/msg13785.html
> 
> I noticed that the existing dates on the iommu commits were also odd:
> 
>   commit 671d773297969bebb1732e1cdc1ec03aa53c6be2
>   Author: Sowmini Varadhan <sowmini.varadhan@oracle.com>
>   Date:   Thu Mar 12 20:02:37 2015 -0400
> 
> 
>   commit f1600e549b948a32ad7672e069b2915314637ae3
>   Author: Sowmini Varadhan <sowmini.varadhan@oracle.com>
>   Date:   Thu Mar 12 20:02:36 2015 -0400
>   
>   commit 10b88a4b17d31a7409494b179dcb76e7ab2fcaea
>   Author: Sowmini Varadhan <sowmini.varadhan@oracle.com>
>   Date:   Thu Mar 12 20:02:35 2015 -0400
> 
> We have had many discussions of this patch set since March 12,
> and the correct version should have been v10, dated something
> after Apr 9, and acked by Benjamin Herrenschmidt.

Dammit, somehow I applied V4 :-/

Sorry about that.  I'll try to sort this out.

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-co

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Sat, 18 Apr 2015 15:27:53 -0400 (EDT)

> Dammit, somehow I applied V4 :-/
> 
> Sorry about that.  I'll try to sort this out.

Sowmini, I think I sorted this out in the 'sparc' GIT tree.

Can you take a look?

Thanks!

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-co

[Sowmini Varadhan]

On (04/18/15 15:40), David Miller wrote:
> 
> Sowmini, I think I sorted this out in the 'sparc' GIT tree.
> 
> Can you take a look?
> 

checking it right now.. give me a few minutes..

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-co

[Sowmini Varadhan]

> 
> Sowmini, I think I sorted this out in the 'sparc' GIT tree.
> 
> Can you take a look?

The patches look right now. These are the commit-ids I checked

  ff7d37a502022149655c18035b99a53391be0383
  bb620c3d3925aec0ed4f21010c86df08ec18a8c7
  0ae53ed15d9b87b883b593a9884957cfa4fc2480

--Sowmini

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-co

[David Miller]

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Date: Sat, 18 Apr 2015 15:55:14 -0400

>> 
>> Sowmini, I think I sorted this out in the 'sparc' GIT tree.
>> 
>> Can you take a look?
> 
> The patches look right now. These are the commit-ids I checked
> 
>   ff7d37a502022149655c18035b99a53391be0383
>   bb620c3d3925aec0ed4f21010c86df08ec18a8c7
>   0ae53ed15d9b87b883b593a9884957cfa4fc2480

Thanks for checking, I'll get this to Linus ASAP.

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-co

[Guenter Roeck]

On 04/18/2015 02:41 PM, David Miller wrote:
> From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> Date: Sat, 18 Apr 2015 15:55:14 -0400
>
>>>
>>> Sowmini, I think I sorted this out in the 'sparc' GIT tree.
>>>
>>> Can you take a look?
>>
>> The patches look right now. These are the commit-ids I checked
>>
>>    ff7d37a502022149655c18035b99a53391be0383
>>    bb620c3d3925aec0ed4f21010c86df08ec18a8c7
>>    0ae53ed15d9b87b883b593a9884957cfa4fc2480
>
> Thanks for checking, I'll get this to Linus ASAP.
>
Latest upstream (v4.0-8110-g64fb1d0) passes my tests.

Guenter

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-co

[Guenter Roeck]

On 04/18/2015 09:13 PM, Guenter Roeck wrote:
> On 04/18/2015 02:41 PM, David Miller wrote:
>> From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
>> Date: Sat, 18 Apr 2015 15:55:14 -0400
>>
>>>>
>>>> Sowmini, I think I sorted this out in the 'sparc' GIT tree.
>>>>
>>>> Can you take a look?
>>>
>>> The patches look right now. These are the commit-ids I checked
>>>
>>>    ff7d37a502022149655c18035b99a53391be0383
>>>    bb620c3d3925aec0ed4f21010c86df08ec18a8c7
>>>    0ae53ed15d9b87b883b593a9884957cfa4fc2480
>>
>> Thanks for checking, I'll get this to Linus ASAP.
>>
> Latest upstream (v4.0-8110-g64fb1d0) passes my tests.
>

I spoke too early. Now I get a similar failure in the powerpc:allmodconfig build
(which previously failed for a different reason).

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

And, yes, there are still two instances of iommu_pool_hash.

arch/powerpc/kernel/iommu.c:static DEFINE_PER_CPU(unsigned int, iommu_pool_hash);
lib/iommu-common.c:static       DEFINE_PER_CPU(unsigned int, iommu_pool_hash);

Guenter

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-co

[Sowmini Varadhan]

On (04/18/15 21:23), Guenter Roeck wrote:
> 
> I spoke too early. Now I get a similar failure in the
> powerpc:allmodconfig build
> (which previously failed for a different reason).

I think this duplicate symbol is genuine.. there's a definition
in arch/powerpc/kernel/iommu.c. To avoid conflicting with that,
the one in lib/iommu-common.c can be renamed.  I guess we missed
this during review, and this went under the radar when I did
all my testing on sparc.

I'll put a patch together for this today..

--Sowmini

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-co

[David Miller]

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Date: Sun, 19 Apr 2015 06:51:57 -0400

> On (04/18/15 21:23), Guenter Roeck wrote:
>> 
>> I spoke too early. Now I get a similar failure in the
>> powerpc:allmodconfig build
>> (which previously failed for a different reason).
> 
> I think this duplicate symbol is genuine.. there's a definition
> in arch/powerpc/kernel/iommu.c. To avoid conflicting with that,
> the one in lib/iommu-common.c can be renamed.  I guess we missed
> this during review, and this went under the radar when I did
> all my testing on sparc.
> 
> I'll put a patch together for this today..

Maybe ping the powerpc folks becuase if they can do a quick
conversion, this change isn't necessary.