On 01/02/2023 17:01, Arnaldo Carvalho de Melo wrote:
> Em Wed, Feb 01, 2023 at 08:49:07AM -0800, Alexei Starovoitov escreveu:
>> On Wed, Feb 1, 2023 at 7:19 AM David Vernet <void@xxxxxxxxxxxxx> wrote:
>>>
>>> On Wed, Feb 01, 2023 at 12:02:07PM -0300, Arnaldo Carvalho de Melo wrote:
>>>> Em Wed, Feb 01, 2023 at 01:59:30PM +0000, Alan Maguire escreveu:
>>>>> On 01/02/2023 03:02, David Vernet wrote:
>>>>>> On Tue, Jan 31, 2023 at 04:14:13PM -0800, Alexei Starovoitov wrote:
>>>>>>> On Tue, Jan 31, 2023 at 3:59 PM David Vernet <void@xxxxxxxxxxxxx> wrote:
>>>>>>>>
>>>>>>>> On Tue, Jan 31, 2023 at 11:45:29PM +0000, Alan Maguire wrote:
>>>>>>>>> On 31/01/2023 18:16, Alexei Starovoitov wrote:
>>>>>>>>>> On Tue, Jan 31, 2023 at 9:43 AM Alexei Starovoitov
>>>>>>>>>> <alexei.starovoitov@xxxxxxxxx> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Tue, Jan 31, 2023 at 4:14 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On 31/01/2023 01:04, Arnaldo Carvalho de Melo wrote:
>>>>>>>>>>>>> Em Mon, Jan 30, 2023 at 09:25:17PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>>>>>>>>>>> Em Mon, Jan 30, 2023 at 10:37:56PM +0000, Alan Maguire escreveu:
>>>>>>>>>>>>>>> On 30/01/2023 20:23, Arnaldo Carvalho de Melo wrote:
>>>>>>>>>>>>>>>> Em Mon, Jan 30, 2023 at 05:10:51PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>>>>>>>>>>>>>> +++ b/dwarves.h
>>>>>>>>>>>>>>>>> @@ -262,6 +262,7 @@ struct cu {
>>>>>>>>>>>>>>>>> uint8_t has_addr_info:1;
>>>>>>>>>>>>>>>>> uint8_t uses_global_strings:1;
>>>>>>>>>>>>>>>>> uint8_t little_endian:1;
>>>>>>>>>>>>>>>>> + uint8_t nr_register_params;
>>>>>>>>>>>>>>>>> uint16_t language;
>>>>>>>>>>>>>>>>> unsigned long nr_inline_expansions;
>>>>>>>>>>>>>>>>> size_t size_inline_expansions;
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks for this, never thought of cross-builds to be honest!
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Tested just now on x86_64 and aarch64 at my end, just ran
>>>>>>>>>>>>>>> into one small thing on one system; turns out EM_RISCV isn't
>>>>>>>>>>>>>>> defined if using a very old elf.h; below works around this
>>>>>>>>>>>>>>> (dwarves otherwise builds fine on this system).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Ok, will add it and will test with containers for older distros too.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Its on the 'next' branch, so that it gets tested in the libbpf github
>>>>>>>>>>>>> repo at:
>>>>>>>>>>>>>
>>>>>>>>>>>>> https://github.com/libbpf/libbpf/actions/workflows/pahole.yml
>>>>>>>>>>>>>
>>>>>>>>>>>>> It failed yesterday and today due to problems with the installation of
>>>>>>>>>>>>> llvm, probably tomorrow it'll be back working as I saw some
>>>>>>>>>>>>> notifications floating by.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I added the conditional EM_RISCV definition as well as removed the dup
>>>>>>>>>>>>> iterator that Jiri noticed.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks again Arnaldo! I've hit an issue with this series in
>>>>>>>>>>>> BTF encoding of kfuncs; specifically we see some kfuncs missing
>>>>>>>>>>>> from the BTF representation, and as a result:
>>>>>>>>>>>>
>>>>>>>>>>>> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
>>>>>>>>>>>> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
>>>>>>>>>>>> WARN: resolve_btfids: unresolved symbol bpf_ct_change_status
>>>>>>>>>>>>
>>>>>>>>>>>> Not sure why I didn't notice this previously.
>>>>>>>>>>>>
>>>>>>>>>>>> The problem is the DWARF - and therefore BTF - generated for a function like
>>>>>>>>>>>>
>>>>>>>>>>>> int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
>>>>>>>>>>>> {
>>>>>>>>>>>> return -EOPNOTSUPP;
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> looks like this:
>>>>>>>>>>>>
>>>>>>>>>>>> <8af83a2> DW_AT_external : 1
>>>>>>>>>>>> <8af83a2> DW_AT_name : (indirect string, offset: 0x358bdc): bpf_xdp_metadata_rx_hash
>>>>>>>>>>>> <8af83a6> DW_AT_decl_file : 5
>>>>>>>>>>>> <8af83a7> DW_AT_decl_line : 737
>>>>>>>>>>>> <8af83a9> DW_AT_decl_column : 5
>>>>>>>>>>>> <8af83aa> DW_AT_prototyped : 1
>>>>>>>>>>>> <8af83aa> DW_AT_type : <0x8ad8547>
>>>>>>>>>>>> <8af83ae> DW_AT_sibling : <0x8af83cd>
>>>>>>>>>>>> <2><8af83b2>: Abbrev Number: 38 (DW_TAG_formal_parameter)
>>>>>>>>>>>> <8af83b3> DW_AT_name : ctx
>>>>>>>>>>>> <8af83b7> DW_AT_decl_file : 5
>>>>>>>>>>>> <8af83b8> DW_AT_decl_line : 737
>>>>>>>>>>>> <8af83ba> DW_AT_decl_column : 51
>>>>>>>>>>>> <8af83bb> DW_AT_type : <0x8af421d>
>>>>>>>>>>>> <2><8af83bf>: Abbrev Number: 35 (DW_TAG_formal_parameter)
>>>>>>>>>>>> <8af83c0> DW_AT_name : (indirect string, offset: 0x27f6a2): hash
>>>>>>>>>>>> <8af83c4> DW_AT_decl_file : 5
>>>>>>>>>>>> <8af83c5> DW_AT_decl_line : 737
>>>>>>>>>>>> <8af83c7> DW_AT_decl_column : 61
>>>>>>>>>>>> <8af83c8> DW_AT_type : <0x8adc424>
>>>>>>>>>>>>
>>>>>>>>>>>> ...and because there are no further abstract origin references
>>>>>>>>>>>> with location information either, we classify it as lacking
>>>>>>>>>>>> locations for (some of) the parameters, and as a result
>>>>>>>>>>>> we skip BTF encoding. We can work around that by doing this:
>>>>>>>>>>>>
>>>>>>>>>>>> __attribute__ ((optimize("O0"))) int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash)
>>>>>>>>>>>
>>>>>>>>>>> replied in the other thread. This attr is broken and discouraged by gcc.
>>>>>>>>>>>
>>>>>>>>>>> For kfuncs where aregs are unused, please try __used and __may_unused
>>>>>>>>>>> applied to arguments.
>>>>>>>>>>> If that won't work, please add barrier_var(arg) to the body of kfunc
>>>>>>>>>>> the way we do in selftests.
>>>>>>>>>>
>>>>>>>>>> There is also
>>>>>>>>>> # define __visible __attribute__((__externally_visible__))
>>>>>>>>>> that probably fits the best here.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> testing thus for seems to show that for x86_64, David's series
>>>>>>>>> (using __used noinline in the BPF_KFUNC() wrapper and extended
>>>>>>>>> to cover recently-arrived kfuncs like cpumask) is sufficient
>>>>>>>>> to avoid resolve_btfids warnings.
>>>>>>>>
>>>>>>>> Nice. Alexei -- lmk how you want to proceed. I think using the
>>>>>>>> __bpf_kfunc macro in the short term (with __used and noinline) is
>>>>>>>> probably the least controversial way to unblock this, but am open to
>>>>>>>> other suggestions.
>>>>>>>
>>>>>>> Sounds good to me, but sounds like __used and noinline are not
>>>>>>> enough to address the issues on aarch64?
>>>>>>
>>>>>> Indeed, we'll have to make sure that's also addressed. Alan -- did you
>>>>>> try Alexei's suggestion to use __weak? Does that fix the issue for
>>>>>> aarch64? I'm still confused as to why it's only complaining for a small
>>>>>> subset of kfuncs, which include those that have external linkage.
>>>>>>
>>>>>
>>>>> I finally got to the bottom of the aarch64 issues; there was a 1-line bug
>>>>> in the changes I made to the DWARF handling code which leads to BTF generation;
>>>>> it was excluding a bunch of functions incorrectly, marking them as optimized out.
>>>>> The fix is:
>>>>>
>>>>> diff --git a/dwarf_loader.c b/dwarf_loader.c
>>>>> index dba2d37..8364e17 100644
>>>>> --- a/dwarf_loader.c
>>>>> +++ b/dwarf_loader.c
>>>>> @@ -1074,7 +1074,7 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
>>>>> Dwarf_Op *expr = loc.expr;
>>>>>
>>>>> switch (expr->atom) {
>>>>> - case DW_OP_reg1 ... DW_OP_reg31:
>>>>> + case DW_OP_reg0 ... DW_OP_reg31:
>>>>> case DW_OP_breg0 ... DW_OP_breg31:
>>>>> break;
>>>>> default:
>>>>>
>>>>> ..and because reg0 is the first parameter for aarch64, we were
>>>>> incorrectly landing in the "default:" of the switch statement
>>>>> and marking a bunch of functions as optimized out
>>>>> because we thought the first argument was. Sorry about this,
>>>>> and thanks for all the suggestions!
>>>
>>> Great, so inline and __used with __bpf_kfunc sounds like the way forward
>>> in the short term. Arnaldo / Alexei -- how do you want to resolve the
>>> dependency here? Going through bpf-next is probably a good idea so that
>>> we get proper CI coverage, and any kfuncs added to bpf-next after this
>>> can use the macro. Does that work for you?
>>
>> It feels fixed pahole should be done under some flag
>> otherwise when people update the pahole the existing and older
>> kernels might stop building with warns:
>> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
>> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
>> ...
>>
Good point, something like
--skip_inconsistent_proto Skip functions that have multiple inconsistent
function prototypes sharing the same name, or
have optimized-out parameters.
? Implementation needs a bit of thought though because we're
not really doing the same thing that we were before. Previously we
were adding the first instance of a function in the CU we came across.
Probably safest to resurrect that behaviour for the legacy
non-skip-inconsistent-proto case I think. The final patch handling
inconsistent function prototypes will need to be reworked a bit to
support this, since we tossed this approach and used saving/merging
multiple instances in the tree instead. Once I've built bpf trees I'll
have a go at getting this working.
>> Arnaldo, could you check what warns do you see with this fixed pahole
>> in bpf tree ?
>
> Sure.
>
I can collect this for x86_64/aarch64 too; might take a few hours
before I have the results.
>> If there are only few warns then we can manually add __used noinline
>> to these places, push to bpf tree and push to stable.
>>
>> Then in bpf-next we can clean up everything with __bpf_kfunc.
>
