On 7/1/26 5:02 AM, Toke Høiland-Jørgensen wrote:
> David Ahern <[email protected]> writes:
>> Seems to me the fib_lookup for xdp needs to return the bottom device,
>> not the vlan device, for forwarding to work. That's why I added the
>> fields to the struct. That allows the program to push the vlan header if
>> required. My preference (dream?) was that Tx path had support to tell
>> the redirect the vlan and h/w added it on send.
>
> Sure, returning the bottom device index with the VLAN tag makes sense,
> and that's basically what this series does (but bails out on stacked
> VLANs). However, that's not what the helper does today, which is why the
> flag is there, to opt-in to the new behaviour. I don't think we can just
> change the ifindex without breaking existing applications (as noted
> up-thread).
I do not see it as breaking existing programs which is why I chimed in
on the thread.
>
>> But really, once stacked devices come into play, I just wanted to make
>> sure thought is given to different use cases. As you know the lookup
>> struct if hard bound to 64B and it is trying to cover a lot of use cases.
>
> Agreed, I don't think we can handle stacked devices in this helper. But
> we could split it out into a new one. Something like:
>
> struct lower_device_info {
> enum device_type type;
> struct {
> __be16 h_vlan_proto;
> __be16 h_vlan_TCI;
> } vlan;
> /* add other types here */
> };
>
> int xdp_get_lower_device(int ifindex, struct lower_device_info *info);
>
> called like:
>
> int xdp_program(struct xdp_md *ctx)
> {
> struct lower_device_info dev_info = {};
> int ifindex, ret;
>
> ifindex = find_destination(ctx); /* does fib lookup, or something
> else */
>
> while ((ret = xdp_get_lower_device_info(ifindex, &dev_info)) > 0) {
> if (dev_info.type == VLAN) {
> push_vlan_tag(ctx, &dev_info.vlan);
> ifindex = ret;
> } else {
> return XDP_PASS; /* we only handle VLAN devices */
> }
> }
>
> return bpf_redirect(ifindex, 0);
> }
>
>
> With a helper like this, we obviously don't strictly speaking need to
> change the fib lookup helper at all. However, for the single-tagged VLAN
> case, I think supporting it directly in the fib lookup could still have
> value, as an optimisation: it saves an extra call for resolving the
> ifindex, and the fields are already there. So I think my preference
> would be to merge this series as-is, and then follow up with a new kfunc
> to handle the stacked case. But we could also just drop this series and
> go straight to the new kfunc.
>
> WDYT?
no preference. I only chimed in because of the added flag to the uapi
which I do not see as needed. If the consensus is that it is in fact
needed, all good then.
