Re: [PATCH net-next 1/2] virtio-net: re enable XDP_REDIRECT for mergeable buffer

2018-03-01 Thread Jason Wang 



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

On Thu, Mar 01, 2018 at 11:19:04AM +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. Other complaints are, the
complex linearize logic and EWMA estimation may increase the
possibility of linearizing.

Signed-off-by: Jason Wang

How about reposting just this patch for net? It's pretty small
and this way we don't have broken redirect there.
Probably keeping the linearize in here to reduce the
amount of changes.



Looks possible, let me post a version for net.

Thanks

Re: [PATCH net-next 1/2] virtio-net: re enable XDP_REDIRECT for mergeable buffer

2018-03-01 Thread Michael S. Tsirkin 
On Thu, Mar 01, 2018 at 11:19:04AM +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. Other complaints are, the
> complex linearize logic and EWMA estimation may increase the
> possibility of linearizing.
> 
> Signed-off-by: Jason Wang 

How about reposting just this patch for net? It's pretty small
and this way we don't have broken redirect there.
Probably keeping the linearize in here to reduce the
amount of changes.

> ---
>  drivers/net/virtio_net.c | 107 
> +--
>  1 file changed, 67 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 9bb9e56..81190ba 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -537,6 +537,26 @@ static struct page *xdp_linearize_page(struct 
> receive_queue *rq,
>   return NULL;
>  }
>  
> +static struct sk_buff *virtnet_skb_xdp(struct receive_queue *rq,
> +struct sk_buff *skb)
> +{
> + struct bpf_prog *xdp_prog;
> + int ret;
> +
> + rcu_read_lock();
> + xdp_prog = rcu_dereference(rq->xdp_prog);
> + if (xdp_prog) {
> + ret = do_xdp_generic(xdp_prog, skb);
> + if (ret != XDP_PASS) {
> + rcu_read_unlock();
> + return NULL;
> + }
> + }
> + rcu_read_unlock();
> +
> + return skb;
> +}
> +
>  static struct sk_buff *receive_small(struct net_device *dev,
>struct virtnet_info *vi,
>struct receive_queue *rq,
> @@ -689,31 +709,30 @@ static struct sk_buff *receive_mergeable(struct 
> net_device *dev,
>   struct bpf_prog *xdp_prog;
>   unsigned int truesize;
>   unsigned int headroom = mergeable_ctx_to_headroom(ctx);
> - bool sent;
> + bool sent, skb_xdp = false;
> + int err;
>  
>   head_skb = NULL;
>  
>   rcu_read_lock();
>   xdp_prog = rcu_dereference(rq->xdp_prog);
>   if (xdp_prog) {
> - struct page *xdp_page;
>   struct xdp_buff xdp;
>   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. In both cases,
> +  * for simplicity, we will offload them to generic
> +  * XDP routine. 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 */
> - xdp_page = xdp_linearize_page(rq, _buf,
> -   page, offset,
> -   VIRTIO_XDP_HEADROOM,
> -   );
> - if (!xdp_page)
> - goto err_xdp;
> - offset = VIRTIO_XDP_HEADROOM;
> - } else {
> - xdp_page = page;
> + skb_xdp = true;
> + goto skb_xdp;
>   }
>  
>   /* Transient failure which in theory could occur if
> @@ -727,7 +746,7 @@ static struct sk_buff *receive_mergeable(struct 
> net_device *dev,
>   /* Allow consuming headroom but reserve enough space to push
>* the descriptor on if we get an XDP_TX return code.
>*/
> - data = page_address(xdp_page) + offset;
> + data = page_address(page) + offset;
>   xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
>   xdp.data = data + vi->hdr_len;
>   xdp_set_data_meta_invalid();
> @@ -736,9 +755,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
> @@ -746,28 +762,22 @@ static struct sk_buff *receive_mergeable(struct 
> net_device *dev,
>* skb and avoid using offset
>*/
>   offset = xdp.data -
> - page_address(xdp_page) - vi->hdr_len;
> -
> - /*

Re: [PATCH net-next 1/2] virtio-net: re enable XDP_REDIRECT for mergeable buffer

2018-03-01 Thread Jason Wang 



On 2018年03月01日 16:41, Jesper Dangaard Brouer wrote:

On Thu,  1 Mar 2018 11:19:04 +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. Other complaints are, the
complex linearize logic and EWMA estimation may increase the
possibility of linearizing.

This patch also have the intermixing issues, I mentioned for patch 2/2.

On Thu, 1 Mar 2018 09:02:06 +0100
Jesper Dangaard Brouer  wrote:


1. XDP generic is not feature complete, e.g. cpumap will drop these
packets. It might not be possible to implement some features, think
of (AF_XDP) zero-copy.

2. This can easily cause out-of-order packets.

3. It makes it harder to troubleshoot, when diagnosing issues
around #1, we have a hard time determining what path an XDP packet
took (the xdp tracepoints doesn't know).


It is slightly better, as it is consistent in calling XDP-generic in
the XDP_REDIRECT action, which an action under heavy development, here
we want the freedom to develop in different code tempi.


Looks not, this patch did:

+   case XDP_REDIRECT:
+   err = xdp_do_redirect(dev, , xdp_prog);
+   if (err)
    goto err_xdp;
+   *xdp_xmit = true;
    rcu_read_unlock();
    goto xdp_xmit;

So native version is used for most cases. XDP-generic will be only used 
for the packets of insufficient headroom which should be rare (just 
first few packets).



   And some
features might never be available in XDP-generic. Thus, when a feature
is missing/broken it will be consistent for the user.


This sounds bad.



The remaining question is how will a user know that XDP "mode" she is
using?


So for this patch, skb mode will only happen for the first several 
packets. So mostly native mode. And it's even not clear to me if user 
need to know about this since the performance should be the same.



   The user clearly loaded an XDP-native program, and expect the
associated performance, but XDP_REDIRECT will be using the slow
XDP-generic code path...


As I replied above, XDP_REDIRECT use native mode except for the first 
few packets.






Signed-off-by: Jason Wang 
---
  drivers/net/virtio_net.c | 107 +--
  1 file changed, 67 insertions(+), 40 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 9bb9e56..81190ba 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

[...]

@@ -689,31 +709,30 @@ static struct sk_buff *receive_mergeable(struct 
net_device *dev,
struct bpf_prog *xdp_prog;
unsigned int truesize;
unsigned int headroom = mergeable_ctx_to_headroom(ctx);
-   bool sent;
+   bool sent, skb_xdp = false;
+   int err;
  
  	head_skb = NULL;
  
  	rcu_read_lock();

xdp_prog = rcu_dereference(rq->xdp_prog);
if (xdp_prog) {
-   struct page *xdp_page;
struct xdp_buff xdp;
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. In both cases,
+* for simplicity, we will offload them to generic
+* XDP routine. 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))) {

I think you also need to check the tailroom here? (AFAIK this is hidden
in the len_to_ctx as the "truesize").


add_recvbuf_mergeable() did this:

    unsigned int headroom = virtnet_get_headroom(vi);
    unsigned int tailroom = headroom ? sizeof(struct skb_shared_info) : 0;

So if headroom is not zero, it implies tailroom here.

Thanks




-   /* linearize data for XDP */
-   xdp_page = xdp_linearize_page(rq, _buf,
- page, offset,
- VIRTIO_XDP_HEADROOM,
- );
-   if (!xdp_page)
-   goto err_xdp;
-   offset = VIRTIO_XDP_HEADROOM;
-   } else {
-   xdp_page = page;
+   skb_xdp = true;
+   goto skb_xdp;
}
  
  		/* Transient failure which in theory could occur if

Re: [PATCH net-next 1/2] virtio-net: re enable XDP_REDIRECT for mergeable buffer

2018-03-01 Thread Jesper Dangaard Brouer 

On Thu,  1 Mar 2018 11:19:04 +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. Other complaints are, the
> complex linearize logic and EWMA estimation may increase the
> possibility of linearizing.

This patch also have the intermixing issues, I mentioned for patch 2/2.

On Thu, 1 Mar 2018 09:02:06 +0100
Jesper Dangaard Brouer  wrote:

> 1. XDP generic is not feature complete, e.g. cpumap will drop these
>packets. It might not be possible to implement some features, think
>of (AF_XDP) zero-copy.
> 
> 2. This can easily cause out-of-order packets.
> 
> 3. It makes it harder to troubleshoot, when diagnosing issues
>around #1, we have a hard time determining what path an XDP packet
>took (the xdp tracepoints doesn't know).


It is slightly better, as it is consistent in calling XDP-generic in
the XDP_REDIRECT action, which an action under heavy development, here
we want the freedom to develop in different code tempi.  And some
features might never be available in XDP-generic. Thus, when a feature
is missing/broken it will be consistent for the user.

The remaining question is how will a user know that XDP "mode" she is
using?  The user clearly loaded an XDP-native program, and expect the
associated performance, but XDP_REDIRECT will be using the slow
XDP-generic code path...



> Signed-off-by: Jason Wang 
> ---
>  drivers/net/virtio_net.c | 107 
> +--
>  1 file changed, 67 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 9bb9e56..81190ba 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
[...]
> @@ -689,31 +709,30 @@ static struct sk_buff *receive_mergeable(struct 
> net_device *dev,
>   struct bpf_prog *xdp_prog;
>   unsigned int truesize;
>   unsigned int headroom = mergeable_ctx_to_headroom(ctx);
> - bool sent;
> + bool sent, skb_xdp = false;
> + int err;
>  
>   head_skb = NULL;
>  
>   rcu_read_lock();
>   xdp_prog = rcu_dereference(rq->xdp_prog);
>   if (xdp_prog) {
> - struct page *xdp_page;
>   struct xdp_buff xdp;
>   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. In both cases,
> +  * for simplicity, we will offload them to generic
> +  * XDP routine. 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))) {

I think you also need to check the tailroom here? (AFAIK this is hidden
in the len_to_ctx as the "truesize").

> - /* linearize data for XDP */
> - xdp_page = xdp_linearize_page(rq, _buf,
> -   page, offset,
> -   VIRTIO_XDP_HEADROOM,
> -   );
> - if (!xdp_page)
> - goto err_xdp;
> - offset = VIRTIO_XDP_HEADROOM;
> - } else {
> - xdp_page = page;
> + skb_xdp = true;
> + goto skb_xdp;
>   }
>  
>   /* Transient failure which in theory could occur if


-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

[PATCH net-next 1/2] virtio-net: re enable XDP_REDIRECT for mergeable buffer

2018-02-28 Thread 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. Other complaints are, the
complex linearize logic and EWMA estimation may increase the
possibility of linearizing.

Signed-off-by: Jason Wang 
---
 drivers/net/virtio_net.c | 107 +--
 1 file changed, 67 insertions(+), 40 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 9bb9e56..81190ba 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -537,6 +537,26 @@ static struct page *xdp_linearize_page(struct 
receive_queue *rq,
return NULL;
 }
 
+static struct sk_buff *virtnet_skb_xdp(struct receive_queue *rq,
+  struct sk_buff *skb)
+{
+   struct bpf_prog *xdp_prog;
+   int ret;
+
+   rcu_read_lock();
+   xdp_prog = rcu_dereference(rq->xdp_prog);
+   if (xdp_prog) {
+   ret = do_xdp_generic(xdp_prog, skb);
+   if (ret != XDP_PASS) {
+   rcu_read_unlock();
+   return NULL;
+   }
+   }
+   rcu_read_unlock();
+
+   return skb;
+}
+
 static struct sk_buff *receive_small(struct net_device *dev,
 struct virtnet_info *vi,
 struct receive_queue *rq,
@@ -689,31 +709,30 @@ static struct sk_buff *receive_mergeable(struct 
net_device *dev,
struct bpf_prog *xdp_prog;
unsigned int truesize;
unsigned int headroom = mergeable_ctx_to_headroom(ctx);
-   bool sent;
+   bool sent, skb_xdp = false;
+   int err;
 
head_skb = NULL;
 
rcu_read_lock();
xdp_prog = rcu_dereference(rq->xdp_prog);
if (xdp_prog) {
-   struct page *xdp_page;
struct xdp_buff xdp;
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. In both cases,
+* for simplicity, we will offload them to generic
+* XDP routine. 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 */
-   xdp_page = xdp_linearize_page(rq, _buf,
- page, offset,
- VIRTIO_XDP_HEADROOM,
- );
-   if (!xdp_page)
-   goto err_xdp;
-   offset = VIRTIO_XDP_HEADROOM;
-   } else {
-   xdp_page = page;
+   skb_xdp = true;
+   goto skb_xdp;
}
 
/* Transient failure which in theory could occur if
@@ -727,7 +746,7 @@ static struct sk_buff *receive_mergeable(struct net_device 
*dev,
/* Allow consuming headroom but reserve enough space to push
 * the descriptor on if we get an XDP_TX return code.
 */
-   data = page_address(xdp_page) + offset;
+   data = page_address(page) + offset;
xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
xdp.data = data + vi->hdr_len;
xdp_set_data_meta_invalid();
@@ -736,9 +755,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
@@ -746,28 +762,22 @@ static struct sk_buff *receive_mergeable(struct 
net_device *dev,
 * skb and avoid using offset
 */
offset = xdp.data -
-   page_address(xdp_page) - vi->hdr_len;
-
-   /* We can only create skb based on xdp_page. */
-   if (unlikely(xdp_page != page)) {
-   rcu_read_unlock();
-   put_page(page);
-   head_skb = page_to_skb(vi, rq, xdp_page,
-  offset,