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.
