All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alan Maguire <alan.maguire@oracle.com>
To: Tony Ambardar <tony.ambardar@gmail.com>,
	Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: martin.lau@linux.dev, ast@kernel.org, andrii@kernel.org,
	alexis.lothore@bootlin.com, eddyz87@gmail.com, song@kernel.org,
	yonghong.song@linux.dev, john.fastabend@gmail.com,
	kpsingh@kernel.org, sdf@fomichev.me, haoluo@google.com,
	jolsa@kernel.org, mykolal@fb.com, bpf@vger.kernel.org,
	dwarves@vger.kernel.org
Subject: Re: [RFC bpf-next 0/3] bpf: handle 0-sized structs properly
Date: Wed, 14 May 2025 11:30:45 +0100	[thread overview]
Message-ID: <4bc9b6c3-4e02-48d3-9b07-c7b1069bfd35@oracle.com> (raw)
In-Reply-To: <aCG8kz1eZjjw+sSU@kodidev-ubuntu>

On 12/05/2025 10:17, Tony Ambardar wrote:
> On Fri, May 09, 2025 at 11:40:47AM -0700, Andrii Nakryiko wrote:
>> On Thu, May 8, 2025 at 6:22 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>>>
>>> When testing v1 of [1] we noticed that functions with 0-sized structs
>>> as parameters were not part of BTF encoding; this was fixed in v2.
>>> However we need to make sure we handle such zero-sized structs
>>> correctly since they confound the calling convention expectations -
>>> no registers are used for the empty struct so this has knock-on effects
>>> for subsequent register-parameter matching.
>>
>> Do you have a list (or at least an example) of the function we are
>> talking about, just curious to see what's that.
>>
> 
> BTW, Alan shared an example in the other pahole patch thread:
> https://lore.kernel.org/dwarves/07d92da1-36f3-44d2-a0a4-cf7dabf278c6@oracle.com/
>

Yep, the one I came across on x86_64 was

static int __io_run_local_work(struct io_ring_ctx *ctx, io_tw_token_t
tw, int min_events, int max_events);

(the io_tw_token_t parameter is a typedef for

struct io_tw_state {
};


>> The question I have is whether it's safe to assume that regardless of
>> architecture we can assume that zero-sized struct has no effect on
>> register allocation (which would seem logical, but is that true for
>> all ABIs).
>>
>> BTW, while looking at patch #2, I noticed that
>> btf_distill_func_proto() disallows functions returning
>> struct-by-value, which seems overly aggressive, at least for structs
>> of up to 8 bytes. So maybe if we can validate that both cases are not
>> introducing any new quirks across all supported architectures, we can
>> solve both limitations?
>>
>

Good idea. I'll try and address this and add a return value test.

> Given pahole (and my related patch) assume pass-by-value for well-sized
> structs, I'd like to see this too. But while the pahole patch works on
> 64/32-bit archs, I noticed from patch #1 that e.g. ___bpf_treg_cnt()
> seems to hard-code a 64-bit register size. Perhaps we can fix that too? 
>

So I think your concern is the assumptions


        __builtin_choose_expr(sizeof(t) == 8, 1,        \
        __builtin_choose_expr(sizeof(t) == 16, 2,        \

? We may need arch-specific macros that specify register size that we
can use here, or is there a better way?

>> P.S., oh, and s390x selftest (test_struct_args) isn't happy, please check.

Yep, working to repro this locally now, thanks.

Alan

  reply	other threads:[~2025-05-14 10:31 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-05-08 13:22 [RFC bpf-next 0/3] bpf: handle 0-sized structs properly Alan Maguire
2025-05-08 13:22 ` [RFC bpf-next 1/3] libbpf: update BPF_PROG2() to handle empty structs Alan Maguire
2025-05-08 13:45   ` Alan Maguire
2025-05-08 13:22 ` [RFC bpf-next 2/3] bpf: allow 0-sized structs as function parameters Alan Maguire
2025-05-08 13:22 ` [RFC bpf-next 3/3] selftests/bpf: add 0-length struct testing to tracing_struct tests Alan Maguire
2025-05-09 18:40 ` [RFC bpf-next 0/3] bpf: handle 0-sized structs properly Andrii Nakryiko
2025-05-12  9:17   ` Tony Ambardar
2025-05-14 10:30     ` Alan Maguire [this message]
2025-05-14 16:22       ` Andrii Nakryiko
2025-05-15  8:02         ` Tony Ambardar
2025-05-15 16:23           ` Andrii Nakryiko
2025-05-15 10:56   ` Alan Maguire
2025-05-20  8:59     ` Alan Maguire
2025-05-21  0:58       ` Tony Ambardar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4bc9b6c3-4e02-48d3-9b07-c7b1069bfd35@oracle.com \
    --to=alan.maguire@oracle.com \
    --cc=alexis.lothore@bootlin.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=dwarves@vger.kernel.org \
    --cc=eddyz87@gmail.com \
    --cc=haoluo@google.com \
    --cc=john.fastabend@gmail.com \
    --cc=jolsa@kernel.org \
    --cc=kpsingh@kernel.org \
    --cc=martin.lau@linux.dev \
    --cc=mykolal@fb.com \
    --cc=sdf@fomichev.me \
    --cc=song@kernel.org \
    --cc=tony.ambardar@gmail.com \
    --cc=yonghong.song@linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.