Re: [PATCH bpf-next v5 1/3] bpf: Add BPF_FIB_LOOKUP_VLAN flag to bpf_fib_lookup() helper

[David Ahern]

On 7/2/26 8:47 AM, Toke Høiland-Jørgensen wrote:
> David Ahern <dsahern@xxxxxxxxxx> writes:
> 
>> On 7/1/26 5:02 AM, Toke Høiland-Jørgensen wrote:
>>> David Ahern <dsahern@xxxxxxxxxx> 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.
> 
> Alright, cool - care to provide an ACK, then? :)
> 


Acked-by: David Ahern <dsahern@xxxxxxxxxx>