Re: [PATCH net V2] virtio-net: re enable XDP_REDIRECT for mergeable buffer

[David Miller]

From: Jason Wang 
Date: Mon, 5 Mar 2018 10:43:41 +0800

> 
> 
> On 2018年03月05日 07:38, David Miller wrote:
>> From: Jason Wang 
>> Date: Fri,  2 Mar 2018 17:29:14 +0800
>>
>>> XDP_REDIRECT support for mergeable buffer was removed since commit
>>> 7324f5399b06 ("virtio_net: disable XDP_REDIRECT in receive_mergeable()
>>> case"). This is because we don't reserve enough tailroom for struct
>>> skb_shared_info which breaks XDP assumption. So this patch fixes this
>>> by reserving enough tailroom and using fixed size of rx buffer.
>>>
>>> Signed-off-by: Jason Wang 
>>> ---
>>> Changes from V1:
>>> - do not add duplicated tracepoint when redirection fails
>> Applied to net-next, thanks Jason.
> 
> Hi David,
> 
> Consider the change is not large, any chance to make it for -net to
> keep XDP redirection work?

Ok, I'll apply this to 'net' too.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net V2] virtio-net: re enable XDP_REDIRECT for mergeable buffer

[Jason Wang]

On 2018年03月05日 07:38, David Miller wrote:

From: Jason Wang 
Date: Fri,  2 Mar 2018 17:29:14 +0800


XDP_REDIRECT support for mergeable buffer was removed since commit
7324f5399b06 ("virtio_net: disable XDP_REDIRECT in receive_mergeable()
case"). This is because we don't reserve enough tailroom for struct
skb_shared_info which breaks XDP assumption. So this patch fixes this
by reserving enough tailroom and using fixed size of rx buffer.

Signed-off-by: Jason Wang 
---
Changes from V1:
- do not add duplicated tracepoint when redirection fails

Applied to net-next, thanks Jason.


Hi David,

Consider the change is not large, any chance to make it for -net to keep 
XDP redirection work?


Thanks

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net V2] virtio-net: re enable XDP_REDIRECT for mergeable buffer

[Jason Wang]

On 2018年03月03日 01:36, Michael S. Tsirkin wrote:

On Fri, Mar 02, 2018 at 05:29:14PM +0800, Jason Wang wrote:

XDP_REDIRECT support for mergeable buffer was removed since commit
7324f5399b06 ("virtio_net: disable XDP_REDIRECT in receive_mergeable()
case"). This is because we don't reserve enough tailroom for struct
skb_shared_info which breaks XDP assumption. So this patch fixes this
by reserving enough tailroom and using fixed size of rx buffer.

Signed-off-by: Jason Wang

Acked-by: Michael S. Tsirkin

I think the next incremental step is to look at splitting
out fast path XDP processing to a separate set of functions.



Let me try (probably after 1.1 stuffs).

Thanks
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net V2] virtio-net: re enable XDP_REDIRECT for mergeable buffer

[Jason Wang]

On 2018年03月03日 00:07, Jesper Dangaard Brouer wrote:

On Fri,  2 Mar 2018 17:29:14 +0800
Jason Wang  wrote:


XDP_REDIRECT support for mergeable buffer was removed since commit
7324f5399b06 ("virtio_net: disable XDP_REDIRECT in receive_mergeable()
case"). This is because we don't reserve enough tailroom for struct
skb_shared_info which breaks XDP assumption. So this patch fixes this
by reserving enough tailroom and using fixed size of rx buffer.

Signed-off-by: Jason Wang 
---
Changes from V1:
- do not add duplicated tracepoint when redirection fails

Acked-by: Jesper Dangaard Brouer 

I gave it a quick spin on my testlab, and cpumap seems to
work/not-crash now (if I managed to turn back config to
receive_mergeable() correctly ;-)).




Thanks for the testing and reviewing.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net V2] virtio-net: re enable XDP_REDIRECT for mergeable buffer

[David Miller]

From: Jason Wang 
Date: Fri,  2 Mar 2018 17:29:14 +0800

> XDP_REDIRECT support for mergeable buffer was removed since commit
> 7324f5399b06 ("virtio_net: disable XDP_REDIRECT in receive_mergeable()
> case"). This is because we don't reserve enough tailroom for struct
> skb_shared_info which breaks XDP assumption. So this patch fixes this
> by reserving enough tailroom and using fixed size of rx buffer.
> 
> Signed-off-by: Jason Wang 
> ---
> Changes from V1:
> - do not add duplicated tracepoint when redirection fails

Applied to net-next, thanks Jason.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net V2] virtio-net: re enable XDP_REDIRECT for mergeable buffer

[Michael S. Tsirkin]

On Fri, Mar 02, 2018 at 05:29:14PM +0800, Jason Wang wrote:
> XDP_REDIRECT support for mergeable buffer was removed since commit
> 7324f5399b06 ("virtio_net: disable XDP_REDIRECT in receive_mergeable()
> case"). This is because we don't reserve enough tailroom for struct
> skb_shared_info which breaks XDP assumption. So this patch fixes this
> by reserving enough tailroom and using fixed size of rx buffer.
> 
> Signed-off-by: Jason Wang 

Acked-by: Michael S. Tsirkin 

I think the next incremental step is to look at splitting
out fast path XDP processing to a separate set of functions.

> ---
> Changes from V1:
> - do not add duplicated tracepoint when redirection fails
> ---
>  drivers/net/virtio_net.c | 54 
> +---
>  1 file changed, 42 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 9bb9e56..426dcf7 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -504,6 +504,7 @@ static struct page *xdp_linearize_page(struct 
> receive_queue *rq,
>   page_off += *len;
>  
>   while (--*num_buf) {
> + int tailroom = SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>   unsigned int buflen;
>   void *buf;
>   int off;
> @@ -518,7 +519,7 @@ static struct page *xdp_linearize_page(struct 
> receive_queue *rq,
>   /* guard against a misconfigured or uncooperative backend that
>* is sending packet larger than the MTU.
>*/
> - if ((page_off + buflen) > PAGE_SIZE) {
> + if ((page_off + buflen + tailroom) > PAGE_SIZE) {
>   put_page(p);
>   goto err_buf;
>   }
> @@ -690,6 +691,7 @@ static struct sk_buff *receive_mergeable(struct 
> net_device *dev,
>   unsigned int truesize;
>   unsigned int headroom = mergeable_ctx_to_headroom(ctx);
>   bool sent;
> + int err;
>  
>   head_skb = NULL;
>  
> @@ -701,7 +703,12 @@ static struct sk_buff *receive_mergeable(struct 
> net_device *dev,
>   void *data;
>   u32 act;
>  
> - /* This happens when rx buffer size is underestimated */
> + /* This happens when rx buffer size is underestimated
> +  * or headroom is not enough because of the buffer
> +  * was refilled before XDP is set. This should only
> +  * happen for the first several packets, so we don't
> +  * care much about its performance.
> +  */
>   if (unlikely(num_buf > 1 ||
>headroom < virtnet_get_headroom(vi))) {
>   /* linearize data for XDP */
> @@ -736,9 +743,6 @@ static struct sk_buff *receive_mergeable(struct 
> net_device *dev,
>  
>   act = bpf_prog_run_xdp(xdp_prog, );
>  
> - if (act != XDP_PASS)
> - ewma_pkt_len_add(>mrg_avg_pkt_len, len);
> -
>   switch (act) {
>   case XDP_PASS:
>   /* recalculate offset to account for any header
> @@ -770,6 +774,18 @@ static struct sk_buff *receive_mergeable(struct 
> net_device *dev,
>   goto err_xdp;
>   rcu_read_unlock();
>   goto xdp_xmit;
> + case XDP_REDIRECT:
> + err = xdp_do_redirect(dev, , xdp_prog);
> + if (err) {
> + if (unlikely(xdp_page != page))
> + put_page(xdp_page);
> + goto err_xdp;
> + }
> + *xdp_xmit = true;
> + if (unlikely(xdp_page != page))
> + goto err_xdp;
> + rcu_read_unlock();
> + goto xdp_xmit;
>   default:
>   bpf_warn_invalid_xdp_action(act);
>   case XDP_ABORTED:
> @@ -1013,13 +1029,18 @@ static int add_recvbuf_big(struct virtnet_info *vi, 
> struct receive_queue *rq,
>  }
>  
>  static unsigned int get_mergeable_buf_len(struct receive_queue *rq,
> -   struct ewma_pkt_len *avg_pkt_len)
> +   struct ewma_pkt_len *avg_pkt_len,
> +   unsigned int room)
>  {
>   const size_t hdr_len = sizeof(struct virtio_net_hdr_mrg_rxbuf);
>   unsigned int len;
>  
> - len = hdr_len + clamp_t(unsigned int, ewma_pkt_len_read(avg_pkt_len),
> + if (room)
> + return PAGE_SIZE - room;
> +
> + len = hdr_len + clamp_t(unsigned int, ewma_pkt_len_read(avg_pkt_len),
>   rq->min_buf_len, PAGE_SIZE - hdr_len);
> +
>   return ALIGN(len, L1_CACHE_BYTES);
>  }
>  
> @@ -1028,21 +1049,27 @@ static int add_recvbuf_mergeable(struct 

Re: [PATCH net V2] virtio-net: re enable XDP_REDIRECT for mergeable buffer

[Jesper Dangaard Brouer]

On Fri,  2 Mar 2018 17:29:14 +0800
Jason Wang  wrote:

> XDP_REDIRECT support for mergeable buffer was removed since commit
> 7324f5399b06 ("virtio_net: disable XDP_REDIRECT in receive_mergeable()
> case"). This is because we don't reserve enough tailroom for struct
> skb_shared_info which breaks XDP assumption. So this patch fixes this
> by reserving enough tailroom and using fixed size of rx buffer.
> 
> Signed-off-by: Jason Wang 
> ---
> Changes from V1:
> - do not add duplicated tracepoint when redirection fails

Acked-by: Jesper Dangaard Brouer 

I gave it a quick spin on my testlab, and cpumap seems to
work/not-crash now (if I managed to turn back config to
receive_mergeable() correctly ;-)).

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH net V2] virtio-net: re enable XDP_REDIRECT for mergeable buffer

[Jason Wang]

XDP_REDIRECT support for mergeable buffer was removed since commit
7324f5399b06 ("virtio_net: disable XDP_REDIRECT in receive_mergeable()
case"). This is because we don't reserve enough tailroom for struct
skb_shared_info which breaks XDP assumption. So this patch fixes this
by reserving enough tailroom and using fixed size of rx buffer.

Signed-off-by: Jason Wang 
---
Changes from V1:
- do not add duplicated tracepoint when redirection fails
---
 drivers/net/virtio_net.c | 54 +---
 1 file changed, 42 insertions(+), 12 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 9bb9e56..426dcf7 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -504,6 +504,7 @@ static struct page *xdp_linearize_page(struct receive_queue 
*rq,
page_off += *len;
 
while (--*num_buf) {
+   int tailroom = SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
unsigned int buflen;
void *buf;
int off;
@@ -518,7 +519,7 @@ static struct page *xdp_linearize_page(struct receive_queue 
*rq,
/* guard against a misconfigured or uncooperative backend that
 * is sending packet larger than the MTU.
 */
-   if ((page_off + buflen) > PAGE_SIZE) {
+   if ((page_off + buflen + tailroom) > PAGE_SIZE) {
put_page(p);
goto err_buf;
}
@@ -690,6 +691,7 @@ static struct sk_buff *receive_mergeable(struct net_device 
*dev,
unsigned int truesize;
unsigned int headroom = mergeable_ctx_to_headroom(ctx);
bool sent;
+   int err;
 
head_skb = NULL;
 
@@ -701,7 +703,12 @@ static struct sk_buff *receive_mergeable(struct net_device 
*dev,
void *data;
u32 act;
 
-   /* This happens when rx buffer size is underestimated */
+   /* This happens when rx buffer size is underestimated
+* or headroom is not enough because of the buffer
+* was refilled before XDP is set. This should only
+* happen for the first several packets, so we don't
+* care much about its performance.
+*/
if (unlikely(num_buf > 1 ||
 headroom < virtnet_get_headroom(vi))) {
/* linearize data for XDP */
@@ -736,9 +743,6 @@ static struct sk_buff *receive_mergeable(struct net_device 
*dev,
 
act = bpf_prog_run_xdp(xdp_prog, );
 
-   if (act != XDP_PASS)
-   ewma_pkt_len_add(>mrg_avg_pkt_len, len);
-
switch (act) {
case XDP_PASS:
/* recalculate offset to account for any header
@@ -770,6 +774,18 @@ static struct sk_buff *receive_mergeable(struct net_device 
*dev,
goto err_xdp;
rcu_read_unlock();
goto xdp_xmit;
+   case XDP_REDIRECT:
+   err = xdp_do_redirect(dev, , xdp_prog);
+   if (err) {
+   if (unlikely(xdp_page != page))
+   put_page(xdp_page);
+   goto err_xdp;
+   }
+   *xdp_xmit = true;
+   if (unlikely(xdp_page != page))
+   goto err_xdp;
+   rcu_read_unlock();
+   goto xdp_xmit;
default:
bpf_warn_invalid_xdp_action(act);
case XDP_ABORTED:
@@ -1013,13 +1029,18 @@ static int add_recvbuf_big(struct virtnet_info *vi, 
struct receive_queue *rq,
 }
 
 static unsigned int get_mergeable_buf_len(struct receive_queue *rq,
- struct ewma_pkt_len *avg_pkt_len)
+ struct ewma_pkt_len *avg_pkt_len,
+ unsigned int room)
 {
const size_t hdr_len = sizeof(struct virtio_net_hdr_mrg_rxbuf);
unsigned int len;
 
-   len = hdr_len + clamp_t(unsigned int, ewma_pkt_len_read(avg_pkt_len),
+   if (room)
+   return PAGE_SIZE - room;
+
+   len = hdr_len + clamp_t(unsigned int, ewma_pkt_len_read(avg_pkt_len),
rq->min_buf_len, PAGE_SIZE - hdr_len);
+
return ALIGN(len, L1_CACHE_BYTES);
 }
 
@@ -1028,21 +1049,27 @@ static int add_recvbuf_mergeable(struct virtnet_info 
*vi,
 {
struct page_frag *alloc_frag = >alloc_frag;
unsigned int headroom = virtnet_get_headroom(vi);
+   unsigned int tailroom = headroom ? sizeof(struct skb_shared_info) : 0;
+   unsigned int room = SKB_DATA_ALIGN(headroom + tailroom);
char *buf;
void *ctx;
int err;