On Sat, Jan 2, 2021 at 3:07 PM Jiri Olsa [off-list ref] wrote:

On Sat, Jan 02, 2021 at 02:25:34PM -0800, Andrii Nakryiko wrote:

SNIP

quoted

quoted

so your .config has
  CONFIG_CRYPTO_DEV_BCM_SPU=y

and that defines 'struct device_private' which
clashes with the same struct defined in drivers/base/base.h

so several networking structs will be doubled, like net_device:

        $ bpftool btf dump file ../vmlinux.config | grep net_device\' | grep STRUCT
        [2731] STRUCT 'net_device' size=2240 vlen=133
        [113981] STRUCT 'net_device' size=2240 vlen=133

each is using different 'struct device_private' when it's unwinded

and that will confuse BTFIDS logic, becase we have multiple structs
with the same name, and we can't be sure which one to pick

perhaps we should check on this in pahole and warn earlier with
better error message.. I'll check, but I'm not sure if pahole can
survive another hastab ;-)

Andrii, any ideas on this? ;-)

It's both unavoidable and correct from the C type system's
perspective, so there is nothing for pahole to warn about. We used to
have (for a long time) a similar clash with two completely different
ring_buffer structs. Eventually they just got renamed to avoid
duplication of related structs (task_struct and tons of other). But
both BTF dedup and CO-RE relocation algorithms are designed to handle
this correctly, ...

AFAIU it's all correctly dedulicated, but still all structs that
contain (at some point) 'struct device_private' will appear twice
in BTF data.. each with different 'struct device_private'

it's correct from the type system perspective, right. Those two
duplicates of struct device_private are parts of two different
hierarchies of types. However inconvenient it is, C allows it,
unfortunately :(

quoted

... so perhaps BTFIDS should be able to handle this as
well?

hm, BTFIDS sees BTF data with two same struct names and has no
way to tell which one to use

unless we have some annotation data for BTF types I don't
see a way to handle this correctly.. but I think we can
detect this directly in BTFIDS and print more accurate error
message

as long as we dont see this on daily basis, I think that better
error message + following struct rename is good solution

Perhaps warning and handling this gracefully is a bit better way to
handle this. Renaming is definitely good, but shouldn't block the
kernel build process. I don't remember the exact details for why
duplicate struct would cause troubles for resolve_btfids, but maybe
just picking the struct with minimal ID (out of 2+ duplicates) would
be ok in practice most of the time. In any case, that's what most
users (including libbpf) will do, when searching for the type by name.

quoted

quoted

easy fix is the patch below that renames the bcm's structs,
it makes the kernel to compile.. but of course the new name
is probably wrong and we should push this through that code
authors

In this case, I think renaming generic device_private name is a good
thing regardless.

ok, I'll send the change

great, thanks

jirka