diff mbox series

[v2,6/9] virtio_net: transmit the multi-buffer xdp

Message ID 20221220141449.115918-7-hengqi@linux.alibaba.com (mailing list archive)
State Deferred
Delegated to: Netdev Maintainers
Headers show
Series virtio_net: support multi buffer xdp | expand

Checks

Context Check Description
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 2 maintainers not CCed: virtualization@lists.linux-foundation.org hawk@kernel.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 44 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Heng Qi Dec. 20, 2022, 2:14 p.m. UTC 
This serves as the basis for XDP_TX and XDP_REDIRECT
to send a multi-buffer xdp_frame.

Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

Comments

Jason Wang Dec. 27, 2022, 7:12 a.m. UTC | #1 
在 2022/12/20 22:14, Heng Qi 写道:
> This serves as the basis for XDP_TX and XDP_REDIRECT
> to send a multi-buffer xdp_frame.
>
> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   drivers/net/virtio_net.c | 27 ++++++++++++++++++++++-----
>   1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 40bc58fa57f5..9f31bfa7f9a6 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -563,22 +563,39 @@ static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
>   				   struct xdp_frame *xdpf)
>   {
>   	struct virtio_net_hdr_mrg_rxbuf *hdr;
> -	int err;
> +	struct skb_shared_info *shinfo;
> +	u8 nr_frags = 0;
> +	int err, i;
>   
>   	if (unlikely(xdpf->headroom < vi->hdr_len))
>   		return -EOVERFLOW;
>   
> -	/* Make room for virtqueue hdr (also change xdpf->headroom?) */
> +	if (unlikely(xdp_frame_has_frags(xdpf))) {
> +		shinfo = xdp_get_shared_info_from_frame(xdpf);
> +		nr_frags = shinfo->nr_frags;
> +	}
> +
> +	/* Need to adjust this to calculate the correct postion
> +	 * for shinfo of the xdpf.
> +	 */
> +	xdpf->headroom -= vi->hdr_len;


Any reason we need to do this here? (Or if it is, is it only needed for 
multibuffer XDP?)

Other looks good.

Thanks


>   	xdpf->data -= vi->hdr_len;
>   	/* Zero header and leave csum up to XDP layers */
>   	hdr = xdpf->data;
>   	memset(hdr, 0, vi->hdr_len);
>   	xdpf->len   += vi->hdr_len;
>   
> -	sg_init_one(sq->sg, xdpf->data, xdpf->len);
> +	sg_init_table(sq->sg, nr_frags + 1);
> +	sg_set_buf(sq->sg, xdpf->data, xdpf->len);
> +	for (i = 0; i < nr_frags; i++) {
> +		skb_frag_t *frag = &shinfo->frags[i];
> +
> +		sg_set_page(&sq->sg[i + 1], skb_frag_page(frag),
> +			    skb_frag_size(frag), skb_frag_off(frag));
> +	}
>   
> -	err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdp_to_ptr(xdpf),
> -				   GFP_ATOMIC);
> +	err = virtqueue_add_outbuf(sq->vq, sq->sg, nr_frags + 1,
> +				   xdp_to_ptr(xdpf), GFP_ATOMIC);
>   	if (unlikely(err))
>   		return -ENOSPC; /* Caller handle free/refcnt */
>
Heng Qi Dec. 27, 2022, 8:26 a.m. UTC | #2 
在 2022/12/27 下午3:12, Jason Wang 写道:
>
> 在 2022/12/20 22:14, Heng Qi 写道:
>> This serves as the basis for XDP_TX and XDP_REDIRECT
>> to send a multi-buffer xdp_frame.
>>
>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>> ---
>>   drivers/net/virtio_net.c | 27 ++++++++++++++++++++++-----
>>   1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 40bc58fa57f5..9f31bfa7f9a6 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -563,22 +563,39 @@ static int __virtnet_xdp_xmit_one(struct 
>> virtnet_info *vi,
>>                      struct xdp_frame *xdpf)
>>   {
>>       struct virtio_net_hdr_mrg_rxbuf *hdr;
>> -    int err;
>> +    struct skb_shared_info *shinfo;
>> +    u8 nr_frags = 0;
>> +    int err, i;
>>         if (unlikely(xdpf->headroom < vi->hdr_len))
>>           return -EOVERFLOW;
>>   -    /* Make room for virtqueue hdr (also change xdpf->headroom?) */
>> +    if (unlikely(xdp_frame_has_frags(xdpf))) {
>> +        shinfo = xdp_get_shared_info_from_frame(xdpf);
>> +        nr_frags = shinfo->nr_frags;
>> +    }
>> +
>> +    /* Need to adjust this to calculate the correct postion
>> +     * for shinfo of the xdpf.
>> +     */
>> +    xdpf->headroom -= vi->hdr_len;
>
>
> Any reason we need to do this here? (Or if it is, is it only needed 
> for multibuffer XDP?)

Going back to its wrapping function virtnet_xdp_xmit(), we need to free 
up the pending old buffers.
If the "is_xdp_frame(ptr)" condition is met, then we need to calculate 
the position of skb_shared_info
in xdp_get_frame_len() and xdp_return_frame(), which will involve to 
xdpf->data and xdpf->headroom.
Therefore, we need to update the value of headroom synchronously here.

Also, it's not necessary for single-buffer xdp, but we need to keep it 
because it's harmless and as it should be.

Thanks.

>
> Other looks good.
>
> Thanks
>
>
>>       xdpf->data -= vi->hdr_len;
>>       /* Zero header and leave csum up to XDP layers */
>>       hdr = xdpf->data;
>>       memset(hdr, 0, vi->hdr_len);
>>       xdpf->len   += vi->hdr_len;
>>   -    sg_init_one(sq->sg, xdpf->data, xdpf->len);
>> +    sg_init_table(sq->sg, nr_frags + 1);
>> +    sg_set_buf(sq->sg, xdpf->data, xdpf->len);
>> +    for (i = 0; i < nr_frags; i++) {
>> +        skb_frag_t *frag = &shinfo->frags[i];
>> +
>> +        sg_set_page(&sq->sg[i + 1], skb_frag_page(frag),
>> +                skb_frag_size(frag), skb_frag_off(frag));
>> +    }
>>   -    err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdp_to_ptr(xdpf),
>> -                   GFP_ATOMIC);
>> +    err = virtqueue_add_outbuf(sq->vq, sq->sg, nr_frags + 1,
>> +                   xdp_to_ptr(xdpf), GFP_ATOMIC);
>>       if (unlikely(err))
>>           return -ENOSPC; /* Caller handle free/refcnt */
Jason Wang Dec. 28, 2022, 6:30 a.m. UTC | #3 
在 2022/12/27 16:26, Heng Qi 写道:
>
>
> 在 2022/12/27 下午3:12, Jason Wang 写道:
>>
>> 在 2022/12/20 22:14, Heng Qi 写道:
>>> This serves as the basis for XDP_TX and XDP_REDIRECT
>>> to send a multi-buffer xdp_frame.
>>>
>>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
>>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>> ---
>>>   drivers/net/virtio_net.c | 27 ++++++++++++++++++++++-----
>>>   1 file changed, 22 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 40bc58fa57f5..9f31bfa7f9a6 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -563,22 +563,39 @@ static int __virtnet_xdp_xmit_one(struct 
>>> virtnet_info *vi,
>>>                      struct xdp_frame *xdpf)
>>>   {
>>>       struct virtio_net_hdr_mrg_rxbuf *hdr;
>>> -    int err;
>>> +    struct skb_shared_info *shinfo;
>>> +    u8 nr_frags = 0;
>>> +    int err, i;
>>>         if (unlikely(xdpf->headroom < vi->hdr_len))
>>>           return -EOVERFLOW;
>>>   -    /* Make room for virtqueue hdr (also change xdpf->headroom?) */
>>> +    if (unlikely(xdp_frame_has_frags(xdpf))) {
>>> +        shinfo = xdp_get_shared_info_from_frame(xdpf);
>>> +        nr_frags = shinfo->nr_frags;
>>> +    }
>>> +
>>> +    /* Need to adjust this to calculate the correct postion
>>> +     * for shinfo of the xdpf.
>>> +     */
>>> +    xdpf->headroom -= vi->hdr_len;
>>
>>
>> Any reason we need to do this here? (Or if it is, is it only needed 
>> for multibuffer XDP?)
>
> Going back to its wrapping function virtnet_xdp_xmit(), we need to 
> free up the pending old buffers.
> If the "is_xdp_frame(ptr)" condition is met, then we need to calculate 
> the position of skb_shared_info
> in xdp_get_frame_len() and xdp_return_frame(), which will involve to 
> xdpf->data and xdpf->headroom.
> Therefore, we need to update the value of headroom synchronously here.


Let's tweak the comment above to something like this.

With that fixed,

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks


>
> Also, it's not necessary for single-buffer xdp, but we need to keep it 
> because it's harmless and as it should be.
>
> Thanks.
>
>>
>> Other looks good.
>>
>> Thanks
>>
>>
>>>       xdpf->data -= vi->hdr_len;
>>>       /* Zero header and leave csum up to XDP layers */
>>>       hdr = xdpf->data;
>>>       memset(hdr, 0, vi->hdr_len);
>>>       xdpf->len   += vi->hdr_len;
>>>   -    sg_init_one(sq->sg, xdpf->data, xdpf->len);
>>> +    sg_init_table(sq->sg, nr_frags + 1);
>>> +    sg_set_buf(sq->sg, xdpf->data, xdpf->len);
>>> +    for (i = 0; i < nr_frags; i++) {
>>> +        skb_frag_t *frag = &shinfo->frags[i];
>>> +
>>> +        sg_set_page(&sq->sg[i + 1], skb_frag_page(frag),
>>> +                skb_frag_size(frag), skb_frag_off(frag));
>>> +    }
>>>   -    err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdp_to_ptr(xdpf),
>>> -                   GFP_ATOMIC);
>>> +    err = virtqueue_add_outbuf(sq->vq, sq->sg, nr_frags + 1,
>>> +                   xdp_to_ptr(xdpf), GFP_ATOMIC);
>>>       if (unlikely(err))
>>>           return -ENOSPC; /* Caller handle free/refcnt */
>
Heng Qi Dec. 28, 2022, 8:25 a.m. UTC | #4 
在 2022/12/28 下午2:30, Jason Wang 写道:
>
> 在 2022/12/27 16:26, Heng Qi 写道:
>>
>>
>> 在 2022/12/27 下午3:12, Jason Wang 写道:
>>>
>>> 在 2022/12/20 22:14, Heng Qi 写道:
>>>> This serves as the basis for XDP_TX and XDP_REDIRECT
>>>> to send a multi-buffer xdp_frame.
>>>>
>>>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
>>>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>> ---
>>>>   drivers/net/virtio_net.c | 27 ++++++++++++++++++++++-----
>>>>   1 file changed, 22 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>> index 40bc58fa57f5..9f31bfa7f9a6 100644
>>>> --- a/drivers/net/virtio_net.c
>>>> +++ b/drivers/net/virtio_net.c
>>>> @@ -563,22 +563,39 @@ static int __virtnet_xdp_xmit_one(struct 
>>>> virtnet_info *vi,
>>>>                      struct xdp_frame *xdpf)
>>>>   {
>>>>       struct virtio_net_hdr_mrg_rxbuf *hdr;
>>>> -    int err;
>>>> +    struct skb_shared_info *shinfo;
>>>> +    u8 nr_frags = 0;
>>>> +    int err, i;
>>>>         if (unlikely(xdpf->headroom < vi->hdr_len))
>>>>           return -EOVERFLOW;
>>>>   -    /* Make room for virtqueue hdr (also change xdpf->headroom?) */
>>>> +    if (unlikely(xdp_frame_has_frags(xdpf))) {
>>>> +        shinfo = xdp_get_shared_info_from_frame(xdpf);
>>>> +        nr_frags = shinfo->nr_frags;
>>>> +    }
>>>> +
>>>> +    /* Need to adjust this to calculate the correct postion
>>>> +     * for shinfo of the xdpf.
>>>> +     */
>>>> +    xdpf->headroom -= vi->hdr_len;
>>>
>>>
>>> Any reason we need to do this here? (Or if it is, is it only needed 
>>> for multibuffer XDP?)
>>
>> Going back to its wrapping function virtnet_xdp_xmit(), we need to 
>> free up the pending old buffers.
>> If the "is_xdp_frame(ptr)" condition is met, then we need to 
>> calculate the position of skb_shared_info
>> in xdp_get_frame_len() and xdp_return_frame(), which will involve to 
>> xdpf->data and xdpf->headroom.
>> Therefore, we need to update the value of headroom synchronously here.
>
>
> Let's tweak the comment above to something like this.

Ok, thanks for your energy.

>
> With that fixed,
>
> Acked-by: Jason Wang <jasowang@redhat.com>
>
> Thanks
>
>
>>
>> Also, it's not necessary for single-buffer xdp, but we need to keep 
>> it because it's harmless and as it should be.
>>
>> Thanks.
>>
>>>
>>> Other looks good.
>>>
>>> Thanks
>>>
>>>
>>>>       xdpf->data -= vi->hdr_len;
>>>>       /* Zero header and leave csum up to XDP layers */
>>>>       hdr = xdpf->data;
>>>>       memset(hdr, 0, vi->hdr_len);
>>>>       xdpf->len   += vi->hdr_len;
>>>>   -    sg_init_one(sq->sg, xdpf->data, xdpf->len);
>>>> +    sg_init_table(sq->sg, nr_frags + 1);
>>>> +    sg_set_buf(sq->sg, xdpf->data, xdpf->len);
>>>> +    for (i = 0; i < nr_frags; i++) {
>>>> +        skb_frag_t *frag = &shinfo->frags[i];
>>>> +
>>>> +        sg_set_page(&sq->sg[i + 1], skb_frag_page(frag),
>>>> +                skb_frag_size(frag), skb_frag_off(frag));
>>>> +    }
>>>>   -    err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdp_to_ptr(xdpf),
>>>> -                   GFP_ATOMIC);
>>>> +    err = virtqueue_add_outbuf(sq->vq, sq->sg, nr_frags + 1,
>>>> +                   xdp_to_ptr(xdpf), GFP_ATOMIC);
>>>>       if (unlikely(err))
>>>>           return -ENOSPC; /* Caller handle free/refcnt */
>>
diff mbox series

Patch

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 40bc58fa57f5..9f31bfa7f9a6 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -563,22 +563,39 @@  static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
 				   struct xdp_frame *xdpf)
 {
 	struct virtio_net_hdr_mrg_rxbuf *hdr;
-	int err;
+	struct skb_shared_info *shinfo;
+	u8 nr_frags = 0;
+	int err, i;
 
 	if (unlikely(xdpf->headroom < vi->hdr_len))
 		return -EOVERFLOW;
 
-	/* Make room for virtqueue hdr (also change xdpf->headroom?) */
+	if (unlikely(xdp_frame_has_frags(xdpf))) {
+		shinfo = xdp_get_shared_info_from_frame(xdpf);
+		nr_frags = shinfo->nr_frags;
+	}
+
+	/* Need to adjust this to calculate the correct postion
+	 * for shinfo of the xdpf.
+	 */
+	xdpf->headroom -= vi->hdr_len;
 	xdpf->data -= vi->hdr_len;
 	/* Zero header and leave csum up to XDP layers */
 	hdr = xdpf->data;
 	memset(hdr, 0, vi->hdr_len);
 	xdpf->len   += vi->hdr_len;
 
-	sg_init_one(sq->sg, xdpf->data, xdpf->len);
+	sg_init_table(sq->sg, nr_frags + 1);
+	sg_set_buf(sq->sg, xdpf->data, xdpf->len);
+	for (i = 0; i < nr_frags; i++) {
+		skb_frag_t *frag = &shinfo->frags[i];
+
+		sg_set_page(&sq->sg[i + 1], skb_frag_page(frag),
+			    skb_frag_size(frag), skb_frag_off(frag));
+	}
 
-	err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdp_to_ptr(xdpf),
-				   GFP_ATOMIC);
+	err = virtqueue_add_outbuf(sq->vq, sq->sg, nr_frags + 1,
+				   xdp_to_ptr(xdpf), GFP_ATOMIC);
 	if (unlikely(err))
 		return -ENOSPC; /* Caller handle free/refcnt */