On 7/2/26 8:47 AM, Toke Høiland-Jørgensen wrote: > David Ahern <[email protected]> writes: > >> 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. > > Alright, cool - care to provide an ACK, then? :) >
Acked-by: David Ahern <[email protected]>
