On 7/2/26 8:47 AM, Toke Høiland-Jørgensen wrote:

David Ahern [off-list ref] writes:

quoted

On 7/1/26 5:02 AM, Toke Høiland-Jørgensen wrote:

quoted

David Ahern [off-list ref] writes:

quoted

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.

quoted

quoted

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.

Alright, cool - care to provide an ACK, then? :)

Acked-by: David Ahern <dsahern@kernel.org>