Re: [net-next PATCH v5 3/6] virtio_net: Add XDP support

[Michael S. Tsirkin]

On Wed, Dec 07, 2016 at 09:14:48PM -0800, John Fastabend wrote:
> On 16-12-07 08:48 PM, Michael S. Tsirkin wrote:
> > On Wed, Dec 07, 2016 at 12:11:57PM -0800, John Fastabend wrote:
> >> From: John Fastabend 
> >>
> >> This adds XDP support to virtio_net. Some requirements must be
> >> met for XDP to be enabled depending on the mode. First it will
> >> only be supported with LRO disabled so that data is not pushed
> >> across multiple buffers. Second the MTU must be less than a page
> >> size to avoid having to handle XDP across multiple pages.
> >>
> >> If mergeable receive is enabled this patch only supports the case
> >> where header and data are in the same buf which we can check when
> >> a packet is received by looking at num_buf. If the num_buf is
> >> greater than 1 and a XDP program is loaded the packet is dropped
> >> and a warning is thrown. When any_header_sg is set this does not
> >> happen and both header and data is put in a single buffer as expected
> >> so we check this when XDP programs are loaded.  Subsequent patches
> >> will process the packet in a degraded mode to ensure connectivity
> >> and correctness is not lost even if backend pushes packets into
> >> multiple buffers.
> >>
> >> If big packets mode is enabled and MTU/LRO conditions above are
> >> met then XDP is allowed.
> >>
> >> This patch was tested with qemu with vhost=on and vhost=off where
> >> mergeable and big_packet modes were forced via hard coding feature
> >> negotiation. Multiple buffers per packet was forced via a small
> >> test patch to vhost.c in the vhost=on qemu mode.
> >>
> >> Suggested-by: Shrijeet Mukherjee 
> >> Signed-off-by: John Fastabend 
> > 
> > I'd like to note that I don't think disabling LRO is a good
> > plan long-term. It's really important for virtio performance,
> > so IMHO we need a fix for that.
> > I'm guessing that a subset of XDP programs would be quite
> > happy with just looking at headers, and that is there in the 1st buffer.
> > So how about teaching XDP that there could be a truncated packet?
> > 
> > Then we won't have to disable LRO.
> > 
> 
> Agreed long-term we can drop this requirement this type of improvement
> would also allow working with jumbo frames on nics.
> 
> I don't think it should block this patch series though.
> 
> .John

Right.

Re: [net-next PATCH v5 3/6] virtio_net: Add XDP support

[John Fastabend]

On 16-12-07 08:48 PM, Michael S. Tsirkin wrote:
> On Wed, Dec 07, 2016 at 12:11:57PM -0800, John Fastabend wrote:
>> From: John Fastabend 
>>
>> This adds XDP support to virtio_net. Some requirements must be
>> met for XDP to be enabled depending on the mode. First it will
>> only be supported with LRO disabled so that data is not pushed
>> across multiple buffers. Second the MTU must be less than a page
>> size to avoid having to handle XDP across multiple pages.
>>
>> If mergeable receive is enabled this patch only supports the case
>> where header and data are in the same buf which we can check when
>> a packet is received by looking at num_buf. If the num_buf is
>> greater than 1 and a XDP program is loaded the packet is dropped
>> and a warning is thrown. When any_header_sg is set this does not
>> happen and both header and data is put in a single buffer as expected
>> so we check this when XDP programs are loaded.  Subsequent patches
>> will process the packet in a degraded mode to ensure connectivity
>> and correctness is not lost even if backend pushes packets into
>> multiple buffers.
>>
>> If big packets mode is enabled and MTU/LRO conditions above are
>> met then XDP is allowed.
>>
>> This patch was tested with qemu with vhost=on and vhost=off where
>> mergeable and big_packet modes were forced via hard coding feature
>> negotiation. Multiple buffers per packet was forced via a small
>> test patch to vhost.c in the vhost=on qemu mode.
>>
>> Suggested-by: Shrijeet Mukherjee 
>> Signed-off-by: John Fastabend 
> 
> I'd like to note that I don't think disabling LRO is a good
> plan long-term. It's really important for virtio performance,
> so IMHO we need a fix for that.
> I'm guessing that a subset of XDP programs would be quite
> happy with just looking at headers, and that is there in the 1st buffer.
> So how about teaching XDP that there could be a truncated packet?
> 
> Then we won't have to disable LRO.
> 

Agreed long-term we can drop this requirement this type of improvement
would also allow working with jumbo frames on nics.

I don't think it should block this patch series though.

.John

Re: [net-next PATCH v5 3/6] virtio_net: Add XDP support

[Michael S. Tsirkin]

On Wed, Dec 07, 2016 at 12:11:57PM -0800, John Fastabend wrote:
> From: John Fastabend 
> 
> This adds XDP support to virtio_net. Some requirements must be
> met for XDP to be enabled depending on the mode. First it will
> only be supported with LRO disabled so that data is not pushed
> across multiple buffers. Second the MTU must be less than a page
> size to avoid having to handle XDP across multiple pages.
> 
> If mergeable receive is enabled this patch only supports the case
> where header and data are in the same buf which we can check when
> a packet is received by looking at num_buf. If the num_buf is
> greater than 1 and a XDP program is loaded the packet is dropped
> and a warning is thrown. When any_header_sg is set this does not
> happen and both header and data is put in a single buffer as expected
> so we check this when XDP programs are loaded.  Subsequent patches
> will process the packet in a degraded mode to ensure connectivity
> and correctness is not lost even if backend pushes packets into
> multiple buffers.
> 
> If big packets mode is enabled and MTU/LRO conditions above are
> met then XDP is allowed.
> 
> This patch was tested with qemu with vhost=on and vhost=off where
> mergeable and big_packet modes were forced via hard coding feature
> negotiation. Multiple buffers per packet was forced via a small
> test patch to vhost.c in the vhost=on qemu mode.
> 
> Suggested-by: Shrijeet Mukherjee 
> Signed-off-by: John Fastabend 

I'd like to note that I don't think disabling LRO is a good
plan long-term. It's really important for virtio performance,
so IMHO we need a fix for that.
I'm guessing that a subset of XDP programs would be quite
happy with just looking at headers, and that is there in the 1st buffer.
So how about teaching XDP that there could be a truncated packet?

Then we won't have to disable LRO.


> ---
>  drivers/net/virtio_net.c |  175 
> +-
>  1 file changed, 170 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index a5c47b1..a009299 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -81,6 +82,8 @@ struct receive_queue {
>  
>   struct napi_struct napi;
>  
> + struct bpf_prog __rcu *xdp_prog;
> +
>   /* Chain pages by the private ptr. */
>   struct page *pages;
>  
> @@ -324,6 +327,38 @@ static struct sk_buff *page_to_skb(struct virtnet_info 
> *vi,
>   return skb;
>  }
>  
> +static u32 do_xdp_prog(struct virtnet_info *vi,
> +struct bpf_prog *xdp_prog,
> +struct page *page, int offset, int len)
> +{
> + int hdr_padded_len;
> + struct xdp_buff xdp;
> + u32 act;
> + u8 *buf;
> +
> + buf = page_address(page) + offset;
> +
> + if (vi->mergeable_rx_bufs)
> + hdr_padded_len = sizeof(struct virtio_net_hdr_mrg_rxbuf);
> + else
> + hdr_padded_len = sizeof(struct padded_vnet_hdr);
> +
> + xdp.data = buf + hdr_padded_len;
> + xdp.data_end = xdp.data + (len - vi->hdr_len);
> +
> + act = bpf_prog_run_xdp(xdp_prog, );
> + switch (act) {
> + case XDP_PASS:
> + return XDP_PASS;
> + default:
> + bpf_warn_invalid_xdp_action(act);
> + case XDP_TX:
> + case XDP_ABORTED:
> + case XDP_DROP:
> + return XDP_DROP;
> + }
> +}
> +
>  static struct sk_buff *receive_small(struct virtnet_info *vi, void *buf, 
> unsigned int len)
>  {
>   struct sk_buff * skb = buf;
> @@ -340,14 +375,32 @@ static struct sk_buff *receive_big(struct net_device 
> *dev,
>  void *buf,
>  unsigned int len)
>  {
> + struct bpf_prog *xdp_prog;
>   struct page *page = buf;
> - struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len, PAGE_SIZE);
> + struct sk_buff *skb;
>  
> + rcu_read_lock();
> + xdp_prog = rcu_dereference(rq->xdp_prog);
> + if (xdp_prog) {
> + struct virtio_net_hdr_mrg_rxbuf *hdr = buf;
> + u32 act;
> +
> + if (unlikely(hdr->hdr.gso_type || hdr->hdr.flags))
> + goto err_xdp;
> + act = do_xdp_prog(vi, xdp_prog, page, 0, len);
> + if (act == XDP_DROP)
> + goto err_xdp;
> + }
> + rcu_read_unlock();
> +
> + skb = page_to_skb(vi, rq, page, 0, len, PAGE_SIZE);
>   if (unlikely(!skb))
>   goto err;
>  
>   return skb;
>  
> +err_xdp:
> + rcu_read_unlock();
>  err:
>   dev->stats.rx_dropped++;
>   give_pages(rq, page);
> @@ -365,11 +418,42 @@ static struct sk_buff *receive_mergeable(struct 
> net_device *dev,
>   u16 num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
>   struct 

[net-next PATCH v5 3/6] virtio_net: Add XDP support

[John Fastabend]

From: John Fastabend 

This adds XDP support to virtio_net. Some requirements must be
met for XDP to be enabled depending on the mode. First it will
only be supported with LRO disabled so that data is not pushed
across multiple buffers. Second the MTU must be less than a page
size to avoid having to handle XDP across multiple pages.

If mergeable receive is enabled this patch only supports the case
where header and data are in the same buf which we can check when
a packet is received by looking at num_buf. If the num_buf is
greater than 1 and a XDP program is loaded the packet is dropped
and a warning is thrown. When any_header_sg is set this does not
happen and both header and data is put in a single buffer as expected
so we check this when XDP programs are loaded.  Subsequent patches
will process the packet in a degraded mode to ensure connectivity
and correctness is not lost even if backend pushes packets into
multiple buffers.

If big packets mode is enabled and MTU/LRO conditions above are
met then XDP is allowed.

This patch was tested with qemu with vhost=on and vhost=off where
mergeable and big_packet modes were forced via hard coding feature
negotiation. Multiple buffers per packet was forced via a small
test patch to vhost.c in the vhost=on qemu mode.

Suggested-by: Shrijeet Mukherjee 
Signed-off-by: John Fastabend 
---
 drivers/net/virtio_net.c |  175 +-
 1 file changed, 170 insertions(+), 5 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index a5c47b1..a009299 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -81,6 +82,8 @@ struct receive_queue {
 
struct napi_struct napi;
 
+   struct bpf_prog __rcu *xdp_prog;
+
/* Chain pages by the private ptr. */
struct page *pages;
 
@@ -324,6 +327,38 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
return skb;
 }
 
+static u32 do_xdp_prog(struct virtnet_info *vi,
+  struct bpf_prog *xdp_prog,
+  struct page *page, int offset, int len)
+{
+   int hdr_padded_len;
+   struct xdp_buff xdp;
+   u32 act;
+   u8 *buf;
+
+   buf = page_address(page) + offset;
+
+   if (vi->mergeable_rx_bufs)
+   hdr_padded_len = sizeof(struct virtio_net_hdr_mrg_rxbuf);
+   else
+   hdr_padded_len = sizeof(struct padded_vnet_hdr);
+
+   xdp.data = buf + hdr_padded_len;
+   xdp.data_end = xdp.data + (len - vi->hdr_len);
+
+   act = bpf_prog_run_xdp(xdp_prog, );
+   switch (act) {
+   case XDP_PASS:
+   return XDP_PASS;
+   default:
+   bpf_warn_invalid_xdp_action(act);
+   case XDP_TX:
+   case XDP_ABORTED:
+   case XDP_DROP:
+   return XDP_DROP;
+   }
+}
+
 static struct sk_buff *receive_small(struct virtnet_info *vi, void *buf, 
unsigned int len)
 {
struct sk_buff * skb = buf;
@@ -340,14 +375,32 @@ static struct sk_buff *receive_big(struct net_device *dev,
   void *buf,
   unsigned int len)
 {
+   struct bpf_prog *xdp_prog;
struct page *page = buf;
-   struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len, PAGE_SIZE);
+   struct sk_buff *skb;
 
+   rcu_read_lock();
+   xdp_prog = rcu_dereference(rq->xdp_prog);
+   if (xdp_prog) {
+   struct virtio_net_hdr_mrg_rxbuf *hdr = buf;
+   u32 act;
+
+   if (unlikely(hdr->hdr.gso_type || hdr->hdr.flags))
+   goto err_xdp;
+   act = do_xdp_prog(vi, xdp_prog, page, 0, len);
+   if (act == XDP_DROP)
+   goto err_xdp;
+   }
+   rcu_read_unlock();
+
+   skb = page_to_skb(vi, rq, page, 0, len, PAGE_SIZE);
if (unlikely(!skb))
goto err;
 
return skb;
 
+err_xdp:
+   rcu_read_unlock();
 err:
dev->stats.rx_dropped++;
give_pages(rq, page);
@@ -365,11 +418,42 @@ static struct sk_buff *receive_mergeable(struct 
net_device *dev,
u16 num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
struct page *page = virt_to_head_page(buf);
int offset = buf - page_address(page);
-   unsigned int truesize = max(len, mergeable_ctx_to_buf_truesize(ctx));
+   struct sk_buff *head_skb, *curr_skb;
+   struct bpf_prog *xdp_prog;
+   unsigned int truesize;
+
+   rcu_read_lock();
+   xdp_prog = rcu_dereference(rq->xdp_prog);
+   if (xdp_prog) {
+   u32 act;
+
+   /* No known backend devices should send packets with
+* more than a single buffer when XDP conditions are
+* met. However it is not strictly illegal so the case
+