| Message ID | 158757174266.1370371.14475202001364271065.stgit@firesoul |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [net-next,01/33] xdp: add frame size to xdp_buff | expand |
On 2020/4/23 上午12:09, Jesper Dangaard Brouer wrote: > In vhost_net_build_xdp() the 'buf' that gets queued via an xdp_buff > have embedded a struct tun_xdp_hdr (located at xdp->data_hard_start) > which contains the buffer length 'buflen' (with tailroom for > skb_shared_info). Also storing this buflen in xdp->frame_sz, does not > obsolete struct tun_xdp_hdr, as it also contains a struct > virtio_net_hdr with other information. > > Cc: Jason Wang <jasowang@redhat.com> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > --- > drivers/vhost/net.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c > index 87469d67ede8..69af007e22f4 100644 > --- a/drivers/vhost/net.c > +++ b/drivers/vhost/net.c > @@ -745,6 +745,7 @@ static int vhost_net_build_xdp(struct vhost_net_virtqueue *nvq, > xdp->data = buf + pad; > xdp->data_end = xdp->data + len; > hdr->buflen = buflen; > + xdp->frame_sz = buflen; > > --net->refcnt_bias; > alloc_frag->offset += buflen; Tun_xdp_one() will use hdr->buflen as the frame_sz (patch 19), so it looks to me there's no need to do this? Thanks > >
On Mon, 27 Apr 2020 13:50:15 +0800 Jason Wang <jasowang@redhat.com> wrote: > On 2020/4/23 上午12:09, Jesper Dangaard Brouer wrote: > > In vhost_net_build_xdp() the 'buf' that gets queued via an xdp_buff > > have embedded a struct tun_xdp_hdr (located at xdp->data_hard_start) > > which contains the buffer length 'buflen' (with tailroom for > > skb_shared_info). Also storing this buflen in xdp->frame_sz, does not > > obsolete struct tun_xdp_hdr, as it also contains a struct > > virtio_net_hdr with other information. > > > > Cc: Jason Wang <jasowang@redhat.com> > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > > --- > > drivers/vhost/net.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c > > index 87469d67ede8..69af007e22f4 100644 > > --- a/drivers/vhost/net.c > > +++ b/drivers/vhost/net.c > > @@ -745,6 +745,7 @@ static int vhost_net_build_xdp(struct vhost_net_virtqueue *nvq, > > xdp->data = buf + pad; > > xdp->data_end = xdp->data + len; > > hdr->buflen = buflen; > > + xdp->frame_sz = buflen; > > > > --net->refcnt_bias; > > alloc_frag->offset += buflen; > > > Tun_xdp_one() will use hdr->buflen as the frame_sz (patch 19), so it > looks to me there's no need to do this? I was thinking to go the "other way", meaning let tun_xdp_one() use xdp->frame_sz, which gets set here. This would allow us to refactor the code, and drop struct tun_xdp_hdr, as (see pahole below) it only carries 'buflen' and the remaining part comes from struct virtio_net_hdr, which could be used directly instead. As this will be a code refactor, I would prefer we do it after this patchseries is agreed upon. $ pahole -C tun_xdp_hdr drivers/net/tap.o struct tun_xdp_hdr { int buflen; /* 0 4 */ struct virtio_net_hdr gso; /* 4 10 */ /* size: 16, cachelines: 1, members: 2 */ /* padding: 2 */ /* last cacheline: 16 bytes */ };
On 2020/4/30 下午5:54, Jesper Dangaard Brouer wrote: > On Mon, 27 Apr 2020 13:50:15 +0800 > Jason Wang <jasowang@redhat.com> wrote: > >> On 2020/4/23 上午12:09, Jesper Dangaard Brouer wrote: >>> In vhost_net_build_xdp() the 'buf' that gets queued via an xdp_buff >>> have embedded a struct tun_xdp_hdr (located at xdp->data_hard_start) >>> which contains the buffer length 'buflen' (with tailroom for >>> skb_shared_info). Also storing this buflen in xdp->frame_sz, does not >>> obsolete struct tun_xdp_hdr, as it also contains a struct >>> virtio_net_hdr with other information. >>> >>> Cc: Jason Wang <jasowang@redhat.com> >>> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> >>> --- >>> drivers/vhost/net.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c >>> index 87469d67ede8..69af007e22f4 100644 >>> --- a/drivers/vhost/net.c >>> +++ b/drivers/vhost/net.c >>> @@ -745,6 +745,7 @@ static int vhost_net_build_xdp(struct vhost_net_virtqueue *nvq, >>> xdp->data = buf + pad; >>> xdp->data_end = xdp->data + len; >>> hdr->buflen = buflen; >>> + xdp->frame_sz = buflen; >>> >>> --net->refcnt_bias; >>> alloc_frag->offset += buflen; >> >> Tun_xdp_one() will use hdr->buflen as the frame_sz (patch 19), so it >> looks to me there's no need to do this? > I was thinking to go the "other way", meaning let tun_xdp_one() use > xdp->frame_sz, which gets set here. This would allow us to refactor > the code, and drop struct tun_xdp_hdr, as (see pahole below) it only > carries 'buflen' and the remaining part comes from struct > virtio_net_hdr, which could be used directly instead. > > As this will be a code refactor, I would prefer we do it after this > patchseries is agreed upon. > > $ pahole -C tun_xdp_hdr drivers/net/tap.o > struct tun_xdp_hdr { > int buflen; /* 0 4 */ > struct virtio_net_hdr gso; /* 4 10 */ > > /* size: 16, cachelines: 1, members: 2 */ > /* padding: 2 */ > /* last cacheline: 16 bytes */ > }; Ok I get this. Thanks
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c index 87469d67ede8..69af007e22f4 100644 --- a/drivers/vhost/net.c +++ b/drivers/vhost/net.c @@ -745,6 +745,7 @@ static int vhost_net_build_xdp(struct vhost_net_virtqueue *nvq, xdp->data = buf + pad; xdp->data_end = xdp->data + len; hdr->buflen = buflen; + xdp->frame_sz = buflen; --net->refcnt_bias; alloc_frag->offset += buflen;
In vhost_net_build_xdp() the 'buf' that gets queued via an xdp_buff have embedded a struct tun_xdp_hdr (located at xdp->data_hard_start) which contains the buffer length 'buflen' (with tailroom for skb_shared_info). Also storing this buflen in xdp->frame_sz, does not obsolete struct tun_xdp_hdr, as it also contains a struct virtio_net_hdr with other information. Cc: Jason Wang <jasowang@redhat.com> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> --- drivers/vhost/net.c | 1 + 1 file changed, 1 insertion(+)