Re: [PATCH bpf-next v2 5/8] selftests/bpf: Verify xdp_metadata xdp->af_xdp path

From: Stanislav Fomichev <sdf@google.com>
To: Anton Protopopov <aspsk@isovalent.com>
Cc: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
	andrii@kernel.org, martin.lau@linux.dev, song@kernel.org,
	yhs@fb.com, john.fastabend@gmail.com, kpsingh@kernel.org,
	haoluo@google.com, jolsa@kernel.org,
	David Ahern <dsahern@gmail.com>, Jakub Kicinski <kuba@kernel.org>,
	Willem de Bruijn <willemb@google.com>,
	Jesper Dangaard Brouer <brouer@redhat.com>,
	Anatoly Burakov <anatoly.burakov@intel.com>,
	Alexander Lobakin <alexandr.lobakin@intel.com>,
	Magnus Karlsson <magnus.karlsson@gmail.com>,
	Maryam Tahhan <mtahhan@redhat.com>,
	xdp-hints@xdp-project.net, netdev@vger.kernel.org
Subject: Re: [PATCH bpf-next v2 5/8] selftests/bpf: Verify xdp_metadata xdp->af_xdp path
Date: Tue, 29 Nov 2022 10:52:00 -0800	[thread overview]
Message-ID: <CAKH8qBtZf1rHFH-JG1yJOBrH+on7g6WwOOu_vwbAaoD+vniCWQ@mail.gmail.com> (raw)
In-Reply-To: <Y4XZkZJHVvLgTIk9@lavr>

On Tue, Nov 29, 2022 at 2:06 AM Anton Protopopov <aspsk@isovalent.com> wrote:
>
> On 22/11/21 10:25, Stanislav Fomichev wrote:
> >
> > [...]
> >
> > +
> > +     if (bpf_xdp_metadata_rx_timestamp_supported(ctx))
> > +             meta->rx_timestamp = bpf_xdp_metadata_rx_timestamp(ctx);
> > +
> > +     if (bpf_xdp_metadata_rx_hash_supported(ctx))
> > +             meta->rx_hash = bpf_xdp_metadata_rx_hash(ctx);
>
> Is there a case when F_supported and F are not called in a sequence? If not,
> then you can join them:
>
>         bool (*ndo_xdp_rx_timestamp)(const struct xdp_md *ctx, u64 *timestamp);
>
> so that a calling XDP program does one indirect call instead of two for one
> field
>
>         if (bpf_xdp_metadata_rx_timestamp(ctx, &meta->rx_timestamp)) {
>                 /* ... couldn't get the timestamp */
>         }

The purpose of the original bpf_xdp_metadata_rx_hash_supported was to
allow unrolling and support dropping some dead branches by the
verifier.
Since there is still a chance we might eventually unroll some of
these, maybe it makes sense to keep as is?