Marcus Wichelmann wrote:
Am 18.02.25 um 02:47 schrieb Willem de Bruijn:
Marcus Wichelmann wrote:
[...]
- metasize = max(xdp->data - xdp->data_meta, 0);
Can xdp->data_meta ever be greater than xdp->data?
When an xdp_buff has no metadata support, then this is marked by setting xdp->data_meta to xdp->data + 1. See xdp_prepare_buff or xdp_set_data_meta_invalid.
In the case of tun_xdp_one, the xdp_buff is externally created by another driver and passed to the tun driver using sendmsg and TUN_MSG_PTR. For now, the vhost_net driver is the only driver doing that, and xdp->data_meta is set to xdp->data there, marking support for metadata.
So knowing that vhost_net is currently the only driver passing xdp_buffs to tun_sendmsg, the check is not strictly necessary. But other drivers may use this API as well in the future. That's why I'd like to not make the assumption that other drivers always create the xdp_buffs with metadata support, when they pass them to tun_sendmsg.
Or am I just to careful about this? What do you think?
I agree.
This is pointer comparison, which is tricky wrt type. It likely is ptrdiff_t and thus signed. But may want to use max_t(long int, ..) to make this explicit.
Ah, I see, good point.
So like that?
metasize = max_t(long int, xdp->data - xdp->data_meta, 0); if (metasize) skb_metadata_set(skb, metasize);
Or just this? Also ensures the test uses signed int.
int metasize;
...
metasize = xdp->data - xdp->data_meta; if (metasize > 0) skb_metadata_set(skb, metasize);
Alternatively, there is also xdp_data_meta_unsupported(xdp_buff) which could be used to make this check very explicit, but I don't see it being used in network drivers elsewhere. Not sure why.
- if (metasize)
skb_metadata_set(skb, metasize);Not strictly needed. As skb_metadata_clear is just skb_metadata_set(skb, 0). But also not wrong, so fine to keep.
Oh, haven't seen that. I'm following a common pattern here that I've seen in many other network drivers (grep for "skb_metadata_set"):
unsigned int metasize = xdp->data - xdp->data_meta; [...] if (metasize) skb_metadata_set(skb, metasize);
Thanks for that context. Sounds good.