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
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
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
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
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 <[email protected]>
Date: Thu Mar 12 20:02:37 2015 -0400
commit f1600e549b948a32ad7672e069b2915314637ae3
Author: Sowmini Varadhan <[email protected]>
Date: Thu Mar 12 20:02:36 2015 -0400
commit 10b88a4b17d31a7409494b179dcb76e7ab2fcaea
Author: Sowmini Varadhan <[email protected]>
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.
From: Sowmini Varadhan <[email protected]>
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.
From: Sowmini Varadhan <[email protected]>
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 <[email protected]>
> Date: Thu Mar 12 20:02:37 2015 -0400
>
>
> commit f1600e549b948a32ad7672e069b2915314637ae3
> Author: Sowmini Varadhan <[email protected]>
> Date: Thu Mar 12 20:02:36 2015 -0400
>
> commit 10b88a4b17d31a7409494b179dcb76e7ab2fcaea
> Author: Sowmini Varadhan <[email protected]>
> 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.
From: David Miller <[email protected]>
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!
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..
>
> 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
From: Sowmini Varadhan <[email protected]>
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.
On 04/18/2015 02:41 PM, David Miller wrote:
> From: Sowmini Varadhan <[email protected]>
> 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
On 04/18/2015 09:13 PM, Guenter Roeck wrote:
> On 04/18/2015 02:41 PM, David Miller wrote:
>> From: Sowmini Varadhan <[email protected]>
>> 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
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
From: Sowmini Varadhan <[email protected]>
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.