From: John Fastabend <john.fastabend@gmail.com> Date: 2017-01-17 22:26:38
This has a fix to handle small buffer free logic correctly and then
also adds adjust head support.
I pushed adjust head at net (even though its rc3) to avoid having
to push another exception case into virtio_net to catch if the
program uses adjust_head and then block it. If there are any strong
objections to this we can push it at net-next and use a patch from
Jakub to add the exception handling but then user space has to deal
with it either via try/fail logic or via kernel version checks. Granted
we already have some cases that need to be configured to enable XDP
but I don't see any reason to have yet another one when we can fix it
now vs delaying a kernel version.
v2: fix spelling error, convert unsigned -> unsigned int
v3: v2 git crashed during send so retrying sorry for the noise
v4: changed layout of rtnl_lock fixes (Stephen)
moved reset logic into virtio core with new patch (MST)
fixed up linearize and some code cleanup (Jason)
Otherwise did some generic code cleanup so might be a bit
cleaner this time at least that is the hope.
v5: fixed rtnl_lock issue (DaveM)
In order to fix rtnl_lock issue and also to address Jason's
comment questioning the need for a generic virtio_device_reset
routine I exported some virtio core routines and then wrote
virtio_net reset routine. This is the cleanest solution I
came up with today and I do not at this time have any need
for a more generic reset. If folks don't like this I could
revert back to v3 variant but Stephen pointed out that the
pattern used there is also not ideal.
Thanks for the review.
---
John Fastabend (6):
virtio_net: use dev_kfree_skb for small buffer XDP receive
virtio_net: wrap rtnl_lock in test for calling with lock already held
virtio_net: factor out xdp handler for readability
virtio_net: remove duplicate queue pair binding in XDP
virtio_net: refactor freeze/restore logic into virtnet reset logic
virtio_net: XDP support for adjust_head
drivers/net/virtio_net.c | 332 ++++++++++++++++++++++++++++++----------------
drivers/virtio/virtio.c | 42 +++---
include/linux/virtio.h | 4 +
3 files changed, 247 insertions(+), 131 deletions(-)
From: John Fastabend <john.fastabend@gmail.com> Date: 2017-01-17 22:20:58
In the small buffer case during driver unload we currently use
put_page instead of dev_kfree_skb. Resolve this by adding a check
for virtnet mode when checking XDP queue type. Also name the
function so that the code reads correctly to match the additional
check.
Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
Signed-off-by: John Fastabend <redacted>
Acked-by: Jason Wang <redacted>
---
drivers/net/virtio_net.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
From: John Fastabend <john.fastabend@gmail.com> Date: 2017-01-17 22:21:01
For XDP use case and to allow ethtool reset tests it is useful to be
able to use reset paths from contexts where rtnl lock is already
held.
This requries updating virtnet_set_queues and free_receive_bufs the
two places where rtnl_lock is taken in virtio_net. To do this we
use the following pattern,
_foo(...) { do stuff }
foo(...) { rtnl_lock(); _foo(...); rtnl_unlock()};
this allows us to use freeze()/restore() flow from both contexts.
Signed-off-by: John Fastabend <redacted>
---
drivers/net/virtio_net.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
@@ -2317,9 +2332,7 @@ static int virtnet_probe(struct virtio_device *vdev)gotofree_unregister_netdev;}-rtnl_lock();virtnet_set_queues(vi,vi->curr_queue_pairs);-rtnl_unlock();/* Assume link up if device can't report link status,otherwisegetlinkstatusfromconfig.*/
@@ -2428,9 +2441,7 @@ static int virtnet_restore(struct virtio_device *vdev)netif_device_attach(vi->dev);-rtnl_lock();virtnet_set_queues(vi,vi->curr_queue_pairs);-rtnl_unlock();err=virtnet_cpu_notif_add(vi);if(err)
From: John Fastabend <john.fastabend@gmail.com> Date: 2017-01-17 22:21:41
At this point the do_xdp_prog is mostly if/else branches handling
the different modes of virtio_net. So remove it and handle running
the program in the per mode handlers.
Signed-off-by: John Fastabend <redacted>
---
drivers/net/virtio_net.c | 75 +++++++++++++++++-----------------------------
1 file changed, 27 insertions(+), 48 deletions(-)
@@ -576,6 +544,9 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,xdp_prog=rcu_dereference(rq->xdp_prog);if(xdp_prog){structpage*xdp_page;+structxdp_buffxdp;+unsignedintqp;+void*data;u32act;/* This happens when rx buffer size is underestimated */
@@ -598,8 +569,10 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,if(unlikely(hdr->hdr.gso_type))gotoerr_xdp;-act=do_xdp_prog(vi,rq,xdp_prog,-page_address(xdp_page)+offset,len);+data=page_address(xdp_page)+offset;+xdp.data=data+vi->hdr_len;+xdp.data_end=xdp.data+(len-vi->hdr_len);+act=bpf_prog_run_xdp(xdp_prog,&xdp);switch(act){caseXDP_PASS:/* We can only create skb based on xdp_page. */
From: John Fastabend <john.fastabend@gmail.com> Date: 2017-01-17 22:31:08
Add support for XDP adjust head by allocating a 256B header region
that XDP programs can grow into. This is only enabled when a XDP
program is loaded.
In order to ensure that we do not have to unwind queue headroom push
queue setup below bpf_prog_add. It reads better to do a prog ref
unwind vs another queue setup call.
At the moment this code must do a full reset to ensure old buffers
without headroom on program add or with headroom on program removal
are not used incorrectly in the datapath. Ideally we would only
have to disable/enable the RX queues being updated but there is no
API to do this at the moment in virtio so use the big hammer. In
practice it is likely not that big of a problem as this will only
happen when XDP is enabled/disabled changing programs does not
require the reset. There is some risk that the driver may either
have an allocation failure or for some reason fail to correctly
negotiate with the underlying backend in this case the driver will
be left uninitialized. I have not seen this ever happen on my test
systems and for what its worth this same failure case can occur
from probe and other contexts in virtio framework.
Signed-off-by: John Fastabend <redacted>
---
drivers/net/virtio_net.c | 149 +++++++++++++++++++++++++++++++++++++++-------
1 file changed, 125 insertions(+), 24 deletions(-)
@@ -41,6 +41,9 @@#define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)#define GOOD_COPY_LEN 128+/* Amount of XDP headroom to prepend to packets for use by xdp_adjust_head */+#define VIRTIO_XDP_HEADROOM 256+/* RX packet size EWMA. The average packet size is used to determine the packet*buffersizewhenrefillingRXrings.AstheentireRXringmayberefilled*atonce,theweightischosensothattheEWMAwillbeinsensitivetoshort-
@@ -359,6 +362,7 @@ static void virtnet_xdp_xmit(struct virtnet_info *vi,}if(vi->mergeable_rx_bufs){+xdp->data-=sizeof(structvirtio_net_hdr_mrg_rxbuf);/* Zero header and leave csum up to XDP layers */hdr=xdp->data;memset(hdr,0,vi->hdr_len);
@@ -413,11 +418,15 @@ static struct sk_buff *receive_small(struct net_device *dev,if(unlikely(hdr->hdr.gso_type||hdr->hdr.flags))gotoerr_xdp;-xdp.data=skb->data;+xdp.data_hard_start=skb->data;+xdp.data=skb->data+VIRTIO_XDP_HEADROOM;xdp.data_end=xdp.data+len;act=bpf_prog_run_xdp(xdp_prog,&xdp);switch(act){caseXDP_PASS:+/* Recalculate length in case bpf program changed it */+__skb_pull(skb,xdp.data-xdp.data_hard_start);+len=xdp.data_end-xdp.data;break;caseXDP_TX:virtnet_xdp_xmit(vi,rq,&xdp,skb);
@@ -568,18 +579,29 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,if(unlikely(hdr->hdr.gso_type))gotoerr_xdp;+/* Allow consuming headroom but reserve enough space to push+*thedescriptoronifwegetanXDP_TXreturncode.+*/data=page_address(xdp_page)+offset;+xdp.data_hard_start=data-VIRTIO_XDP_HEADROOM+vi->hdr_len;xdp.data=data+vi->hdr_len;xdp.data_end=xdp.data+(len-vi->hdr_len);act=bpf_prog_run_xdp(xdp_prog,&xdp);switch(act){caseXDP_PASS:+/* recalculate offset to account for any header+*adjustments.Noteothercasesdonotbuildan+*skbandavoidusingoffset+*/+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,-0,len,PAGE_SIZE);+offset,len,PAGE_SIZE);ewma_pkt_len_add(&rq->mrg_avg_pkt_len,len);returnhead_skb;}
@@ -828,24 +857,27 @@ static unsigned int get_mergeable_buf_len(struct ewma_pkt_len *avg_pkt_len)returnALIGN(len,MERGEABLE_BUFFER_ALIGN);}-staticintadd_recvbuf_mergeable(structreceive_queue*rq,gfp_tgfp)+staticintadd_recvbuf_mergeable(structvirtnet_info*vi,+structreceive_queue*rq,gfp_tgfp){structpage_frag*alloc_frag=&rq->alloc_frag;+unsignedintheadroom=virtnet_get_headroom(vi);char*buf;unsignedlongctx;interr;unsignedintlen,hole;len=get_mergeable_buf_len(&rq->mrg_avg_pkt_len);-if(unlikely(!skb_page_frag_refill(len,alloc_frag,gfp)))+if(unlikely(!skb_page_frag_refill(len+headroom,alloc_frag,gfp)))return-ENOMEM;buf=(char*)page_address(alloc_frag->page)+alloc_frag->offset;+buf+=headroom;/* advance address leaving hole at front of pkt */ctx=mergeable_buf_to_ctx(buf,len);get_page(alloc_frag->page);-alloc_frag->offset+=len;+alloc_frag->offset+=len+headroom;hole=alloc_frag->size-alloc_frag->offset;-if(hole<len){+if(hole<len+headroom){/* To avoid internal fragmentation, if there is very likely not*enoughspaceforanotherbuffer,addtheremainingspaceto*thecurrentbuffer.Thisextraspaceisnotincludedin
@@ -1727,12 +1760,45 @@ static int virtnet_restore_up(struct virtio_device *vdev)returnerr;}+staticintvirtnet_reset(structvirtnet_info*vi)+{+structvirtio_device*dev=vi->vdev;+intret;++virtio_config_disable(dev);+dev->failed=dev->config->get_status(dev)&VIRTIO_CONFIG_S_FAILED;+virtnet_freeze_down(dev);+_remove_vq_common(vi);++dev->config->reset(dev);+virtio_add_status(dev,VIRTIO_CONFIG_S_ACKNOWLEDGE);+virtio_add_status(dev,VIRTIO_CONFIG_S_DRIVER);++ret=virtio_finalize_features(dev);+if(ret)+gotoerr;++ret=virtnet_restore_up(dev);+if(ret)+gotoerr;+ret=_virtnet_set_queues(vi,vi->curr_queue_pairs);+if(ret)+gotoerr;++virtio_add_status(dev,VIRTIO_CONFIG_S_DRIVER_OK);+virtio_config_enable(dev);+return0;+err:+virtio_add_status(dev,VIRTIO_CONFIG_S_FAILED);+returnret;+}+staticintvirtnet_xdp_set(structnet_device*dev,structbpf_prog*prog){unsignedlongintmax_sz=PAGE_SIZE-sizeof(structpadded_vnet_hdr);structvirtnet_info*vi=netdev_priv(dev);structbpf_prog*old_prog;-u16xdp_qp=0,curr_qp;+u16oxdp_qp,xdp_qp=0,curr_qp;inti,err;if(virtio_has_feature(vi->vdev,VIRTIO_NET_F_GUEST_TSO4)||
@@ -1764,21 +1830,32 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)return-ENOMEM;}+if(prog){+prog=bpf_prog_add(prog,vi->max_queue_pairs-1);+if(IS_ERR(prog))+returnPTR_ERR(prog);+}+err=_virtnet_set_queues(vi,curr_qp+xdp_qp);if(err){dev_warn(&dev->dev,"XDP Device queue allocation failure.\n");-returnerr;+gotovirtio_queue_err;}-if(prog){-prog=bpf_prog_add(prog,vi->max_queue_pairs-1);-if(IS_ERR(prog)){-_virtnet_set_queues(vi,curr_qp);-returnPTR_ERR(prog);-}+oxdp_qp=vi->xdp_queue_pairs;++/* Changing the headroom in buffers is a disruptive operation because+*existingbuffersmustbeflushedandreallocated.Thiswillhappen+*whenaxdpprogramisinitiallyaddedorxdpisdisabledbyremoving+*thexdpprogramresultinginnumberofXDPqueueschanging.+*/+if(vi->xdp_queue_pairs!=xdp_qp){+vi->xdp_queue_pairs=xdp_qp;+err=virtnet_reset(vi);+if(err)+gotovirtio_reset_err;}-vi->xdp_queue_pairs=xdp_qp;netif_set_real_num_rx_queues(dev,curr_qp+xdp_qp);for(i=0;i<vi->max_queue_pairs;i++){
@@ -1789,6 +1866,21 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)}return0;++virtio_reset_err:+/* On reset error do our best to unwind XDP changes inflight and return+*erroruptouserspaceforresolution.Theunderlyingresethungon+*ussonotmuchwecandohere.+*/+dev_warn(&dev->dev,"XDP reset failure and queues unstable\n");+vi->xdp_queue_pairs=oxdp_qp;+virtio_queue_err:+/* On queue set error we can unwind bpf ref count and user space can+*retrythisismostlikelyanallocationfailure.+*/+if(prog)+bpf_prog_sub(prog,vi->max_queue_pairs-1);+returnerr;}staticboolvirtnet_xdp_query(structnet_device*dev)
@@ -2382,6 +2474,15 @@ static int virtnet_probe(struct virtio_device *vdev)returnerr;}+staticvoid_remove_vq_common(structvirtnet_info*vi)+{+vi->vdev->config->reset(vi->vdev);+free_unused_bufs(vi);+_free_receive_bufs(vi);+free_receive_page_frags(vi);+virtnet_del_vqs(vi);+}+staticvoidremove_vq_common(structvirtnet_info*vi){vi->vdev->config->reset(vi->vdev);
@@ -332,15 +332,19 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,staticvoidvirtnet_xdp_xmit(structvirtnet_info*vi,structreceive_queue*rq,-structsend_queue*sq,structxdp_buff*xdp,void*data){structvirtio_net_hdr_mrg_rxbuf*hdr;unsignedintnum_sg,len;+structsend_queue*sq;+unsignedintqp;void*xdp_sent;interr;+qp=vi->curr_queue_pairs-vi->xdp_queue_pairs+smp_processor_id();+sq=&vi->sq[qp];+/* Free up any pending old buffers before queueing new ones. */while((xdp_sent=virtqueue_get_buf(sq->vq,&len))!=NULL){if(vi->mergeable_rx_bufs){
From: John Fastabend <john.fastabend@gmail.com> Date: 2017-01-17 22:57:04
For XDP we will need to reset the queues to allow for buffer headroom
to be configured. In order to do this we need to essentially run the
freeze()/restore() code path. Unfortunately the locking requirements
between the freeze/restore and reset paths are different however so
we can not simply reuse the code.
This patch refactors the code path and adds a reset helper routine.
Signed-off-by: John Fastabend <redacted>
---
drivers/net/virtio_net.c | 75 ++++++++++++++++++++++++++++------------------
drivers/virtio/virtio.c | 42 ++++++++++++++------------
include/linux/virtio.h | 4 ++
3 files changed, 73 insertions(+), 48 deletions(-)
@@ -1684,6 +1684,49 @@ static void virtnet_init_settings(struct net_device *dev).set_settings=virtnet_set_settings,};+staticvoidvirtnet_freeze_down(structvirtio_device*vdev)+{+structvirtnet_info*vi=vdev->priv;+inti;++/* Make sure no work handler is accessing the device */+flush_work(&vi->config_work);++netif_device_detach(vi->dev);+cancel_delayed_work_sync(&vi->refill);++if(netif_running(vi->dev)){+for(i=0;i<vi->max_queue_pairs;i++)+napi_disable(&vi->rq[i].napi);+}+}++staticintinit_vqs(structvirtnet_info*vi);++staticintvirtnet_restore_up(structvirtio_device*vdev)+{+structvirtnet_info*vi=vdev->priv;+interr,i;++err=init_vqs(vi);+if(err)+returnerr;++virtio_device_ready(vdev);++if(netif_running(vi->dev)){+for(i=0;i<vi->curr_queue_pairs;i++)+if(!try_fill_recv(vi,&vi->rq[i],GFP_KERNEL))+schedule_delayed_work(&vi->refill,0);++for(i=0;i<vi->max_queue_pairs;i++)+virtnet_napi_enable(&vi->rq[i]);+}++netif_device_attach(vi->dev);+returnerr;+}+staticintvirtnet_xdp_set(structnet_device*dev,structbpf_prog*prog){unsignedlongintmax_sz=PAGE_SIZE-sizeof(structpadded_vnet_hdr);
@@ -2374,21 +2417,9 @@ static void virtnet_remove(struct virtio_device *vdev)staticintvirtnet_freeze(structvirtio_device*vdev){structvirtnet_info*vi=vdev->priv;-inti;virtnet_cpu_notif_remove(vi);--/* Make sure no work handler is accessing the device */-flush_work(&vi->config_work);--netif_device_detach(vi->dev);-cancel_delayed_work_sync(&vi->refill);--if(netif_running(vi->dev)){-for(i=0;i<vi->max_queue_pairs;i++)-napi_disable(&vi->rq[i].napi);-}-+virtnet_freeze_down(vdev);remove_vq_common(vi);return0;
@@ -2397,25 +2428,11 @@ static int virtnet_freeze(struct virtio_device *vdev)staticintvirtnet_restore(structvirtio_device*vdev){structvirtnet_info*vi=vdev->priv;-interr,i;+interr;-err=init_vqs(vi);+err=virtnet_restore_up(vdev);if(err)returnerr;--virtio_device_ready(vdev);--if(netif_running(vi->dev)){-for(i=0;i<vi->curr_queue_pairs;i++)-if(!try_fill_recv(vi,&vi->rq[i],GFP_KERNEL))-schedule_delayed_work(&vi->refill,0);--for(i=0;i<vi->max_queue_pairs;i++)-virtnet_napi_enable(&vi->rq[i]);-}--netif_device_attach(vi->dev);-virtnet_set_queues(vi,vi->curr_queue_pairs);err=virtnet_cpu_notif_add(vi);
@@ -182,6 +185,7 @@ static int virtio_finalize_features(struct virtio_device *dev)}return0;}+EXPORT_SYMBOL_GPL(virtio_finalize_features);staticintvirtio_dev_probe(structdevice*_d){
@@ -193,7 +197,7 @@ static int virtio_dev_probe(struct device *_d)u64driver_features_legacy;/* We have a driver! */-add_status(dev,VIRTIO_CONFIG_S_DRIVER);+virtio_add_status(dev,VIRTIO_CONFIG_S_DRIVER);/* Figure out what features the device supports. */device_features=dev->config->get_features(dev);
@@ -247,7 +251,7 @@ static int virtio_dev_probe(struct device *_d)return0;err:-add_status(dev,VIRTIO_CONFIG_S_FAILED);+virtio_add_status(dev,VIRTIO_CONFIG_S_FAILED);returnerr;}
@@ -265,7 +269,7 @@ static int virtio_dev_remove(struct device *_d)WARN_ON_ONCE(dev->config->get_status(dev));/* Acknowledge the device's existence again. */-add_status(dev,VIRTIO_CONFIG_S_ACKNOWLEDGE);+virtio_add_status(dev,VIRTIO_CONFIG_S_ACKNOWLEDGE);return0;}
@@ -316,7 +320,7 @@ int register_virtio_device(struct virtio_device *dev)dev->config->reset(dev);/* Acknowledge that we've seen the device. */-add_status(dev,VIRTIO_CONFIG_S_ACKNOWLEDGE);+virtio_add_status(dev,VIRTIO_CONFIG_S_ACKNOWLEDGE);INIT_LIST_HEAD(&dev->vqs);
@@ -325,7 +329,7 @@ int register_virtio_device(struct virtio_device *dev)err=device_register(&dev->dev);out:if(err)-add_status(dev,VIRTIO_CONFIG_S_FAILED);+virtio_add_status(dev,VIRTIO_CONFIG_S_FAILED);returnerr;}EXPORT_SYMBOL_GPL(register_virtio_device);
@@ -365,18 +369,18 @@ int virtio_device_restore(struct virtio_device *dev)dev->config->reset(dev);/* Acknowledge that we've seen the device. */-add_status(dev,VIRTIO_CONFIG_S_ACKNOWLEDGE);+virtio_add_status(dev,VIRTIO_CONFIG_S_ACKNOWLEDGE);/* Maybe driver failed before freeze.*Restorethefailedstatus,fordebugging.*/if(dev->failed)-add_status(dev,VIRTIO_CONFIG_S_FAILED);+virtio_add_status(dev,VIRTIO_CONFIG_S_FAILED);if(!drv)return0;/* We have a driver! */-add_status(dev,VIRTIO_CONFIG_S_DRIVER);+virtio_add_status(dev,VIRTIO_CONFIG_S_DRIVER);ret=virtio_finalize_features(dev);if(ret)
@@ -389,14 +393,14 @@ int virtio_device_restore(struct virtio_device *dev)}/* Finally, tell the device we're all set */-add_status(dev,VIRTIO_CONFIG_S_DRIVER_OK);+virtio_add_status(dev,VIRTIO_CONFIG_S_DRIVER_OK);virtio_config_enable(dev);return0;err:-add_status(dev,VIRTIO_CONFIG_S_FAILED);+virtio_add_status(dev,VIRTIO_CONFIG_S_FAILED);returnret;}EXPORT_SYMBOL_GPL(virtio_device_restore);
Hi John:
I still prefer not open code (part of) virtio_device_freeze() and
virtio_device_restore() here. How about:
1) introduce __virtio_device_freeze/__virtio_device_restore which
accepts a function pointer of free/restore
2) for virtio_device_freeze/virtio_device_restore just pass
drv->freeze/drv->restore (locked version)
3) for virtnet_reset(), we can pass unlocked version of freeze and restore
Just my preference, if both Michael and you stick to this, I'm also fine.
Thanks
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2017-01-18 15:15:55
On Tue, Jan 17, 2017 at 02:22:59PM -0800, John Fastabend wrote:
Add support for XDP adjust head by allocating a 256B header region
that XDP programs can grow into. This is only enabled when a XDP
program is loaded.
In order to ensure that we do not have to unwind queue headroom push
queue setup below bpf_prog_add. It reads better to do a prog ref
unwind vs another queue setup call.
At the moment this code must do a full reset to ensure old buffers
without headroom on program add or with headroom on program removal
are not used incorrectly in the datapath. Ideally we would only
have to disable/enable the RX queues being updated but there is no
API to do this at the moment in virtio so use the big hammer. In
practice it is likely not that big of a problem as this will only
happen when XDP is enabled/disabled changing programs does not
require the reset. There is some risk that the driver may either
have an allocation failure or for some reason fail to correctly
negotiate with the underlying backend in this case the driver will
be left uninitialized. I have not seen this ever happen on my test
systems and for what its worth this same failure case can occur
from probe and other contexts in virtio framework.
Signed-off-by: John Fastabend <redacted>
I've been thinking about it - can't we drop
old buffers without the head room which were posted before
xdp attached?
Avoiding the reset would be much nicer.
Thoughts?
@@ -41,6 +41,9 @@#define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)#define GOOD_COPY_LEN 128+/* Amount of XDP headroom to prepend to packets for use by xdp_adjust_head */+#define VIRTIO_XDP_HEADROOM 256+/* RX packet size EWMA. The average packet size is used to determine the packet*buffersizewhenrefillingRXrings.AstheentireRXringmayberefilled*atonce,theweightischosensothattheEWMAwillbeinsensitivetoshort-
@@ -359,6 +362,7 @@ static void virtnet_xdp_xmit(struct virtnet_info *vi,}if(vi->mergeable_rx_bufs){+xdp->data-=sizeof(structvirtio_net_hdr_mrg_rxbuf);/* Zero header and leave csum up to XDP layers */hdr=xdp->data;memset(hdr,0,vi->hdr_len);
@@ -413,11 +418,15 @@ static struct sk_buff *receive_small(struct net_device *dev,if(unlikely(hdr->hdr.gso_type||hdr->hdr.flags))gotoerr_xdp;-xdp.data=skb->data;+xdp.data_hard_start=skb->data;+xdp.data=skb->data+VIRTIO_XDP_HEADROOM;xdp.data_end=xdp.data+len;act=bpf_prog_run_xdp(xdp_prog,&xdp);switch(act){caseXDP_PASS:+/* Recalculate length in case bpf program changed it */+__skb_pull(skb,xdp.data-xdp.data_hard_start);+len=xdp.data_end-xdp.data;break;caseXDP_TX:virtnet_xdp_xmit(vi,rq,&xdp,skb);
@@ -568,18 +579,29 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,if(unlikely(hdr->hdr.gso_type))gotoerr_xdp;+/* Allow consuming headroom but reserve enough space to push+*thedescriptoronifwegetanXDP_TXreturncode.+*/data=page_address(xdp_page)+offset;+xdp.data_hard_start=data-VIRTIO_XDP_HEADROOM+vi->hdr_len;xdp.data=data+vi->hdr_len;xdp.data_end=xdp.data+(len-vi->hdr_len);act=bpf_prog_run_xdp(xdp_prog,&xdp);switch(act){caseXDP_PASS:+/* recalculate offset to account for any header+*adjustments.Noteothercasesdonotbuildan+*skbandavoidusingoffset+*/+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,-0,len,PAGE_SIZE);+offset,len,PAGE_SIZE);ewma_pkt_len_add(&rq->mrg_avg_pkt_len,len);returnhead_skb;}
@@ -828,24 +857,27 @@ static unsigned int get_mergeable_buf_len(struct ewma_pkt_len *avg_pkt_len)returnALIGN(len,MERGEABLE_BUFFER_ALIGN);}-staticintadd_recvbuf_mergeable(structreceive_queue*rq,gfp_tgfp)+staticintadd_recvbuf_mergeable(structvirtnet_info*vi,+structreceive_queue*rq,gfp_tgfp){structpage_frag*alloc_frag=&rq->alloc_frag;+unsignedintheadroom=virtnet_get_headroom(vi);char*buf;unsignedlongctx;interr;unsignedintlen,hole;len=get_mergeable_buf_len(&rq->mrg_avg_pkt_len);-if(unlikely(!skb_page_frag_refill(len,alloc_frag,gfp)))+if(unlikely(!skb_page_frag_refill(len+headroom,alloc_frag,gfp)))return-ENOMEM;buf=(char*)page_address(alloc_frag->page)+alloc_frag->offset;+buf+=headroom;/* advance address leaving hole at front of pkt */ctx=mergeable_buf_to_ctx(buf,len);get_page(alloc_frag->page);-alloc_frag->offset+=len;+alloc_frag->offset+=len+headroom;hole=alloc_frag->size-alloc_frag->offset;-if(hole<len){+if(hole<len+headroom){/* To avoid internal fragmentation, if there is very likely not*enoughspaceforanotherbuffer,addtheremainingspaceto*thecurrentbuffer.Thisextraspaceisnotincludedin
@@ -1727,12 +1760,45 @@ static int virtnet_restore_up(struct virtio_device *vdev)returnerr;}+staticintvirtnet_reset(structvirtnet_info*vi)+{+structvirtio_device*dev=vi->vdev;+intret;++virtio_config_disable(dev);+dev->failed=dev->config->get_status(dev)&VIRTIO_CONFIG_S_FAILED;+virtnet_freeze_down(dev);+_remove_vq_common(vi);++dev->config->reset(dev);+virtio_add_status(dev,VIRTIO_CONFIG_S_ACKNOWLEDGE);+virtio_add_status(dev,VIRTIO_CONFIG_S_DRIVER);++ret=virtio_finalize_features(dev);+if(ret)+gotoerr;++ret=virtnet_restore_up(dev);+if(ret)+gotoerr;+ret=_virtnet_set_queues(vi,vi->curr_queue_pairs);+if(ret)+gotoerr;++virtio_add_status(dev,VIRTIO_CONFIG_S_DRIVER_OK);+virtio_config_enable(dev);+return0;+err:+virtio_add_status(dev,VIRTIO_CONFIG_S_FAILED);+returnret;+}+staticintvirtnet_xdp_set(structnet_device*dev,structbpf_prog*prog){unsignedlongintmax_sz=PAGE_SIZE-sizeof(structpadded_vnet_hdr);structvirtnet_info*vi=netdev_priv(dev);structbpf_prog*old_prog;-u16xdp_qp=0,curr_qp;+u16oxdp_qp,xdp_qp=0,curr_qp;inti,err;if(virtio_has_feature(vi->vdev,VIRTIO_NET_F_GUEST_TSO4)||
@@ -1764,21 +1830,32 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)return-ENOMEM;}+if(prog){+prog=bpf_prog_add(prog,vi->max_queue_pairs-1);+if(IS_ERR(prog))+returnPTR_ERR(prog);+}+err=_virtnet_set_queues(vi,curr_qp+xdp_qp);if(err){dev_warn(&dev->dev,"XDP Device queue allocation failure.\n");-returnerr;+gotovirtio_queue_err;}-if(prog){-prog=bpf_prog_add(prog,vi->max_queue_pairs-1);-if(IS_ERR(prog)){-_virtnet_set_queues(vi,curr_qp);-returnPTR_ERR(prog);-}+oxdp_qp=vi->xdp_queue_pairs;++/* Changing the headroom in buffers is a disruptive operation because+*existingbuffersmustbeflushedandreallocated.Thiswillhappen+*whenaxdpprogramisinitiallyaddedorxdpisdisabledbyremoving+*thexdpprogramresultinginnumberofXDPqueueschanging.+*/+if(vi->xdp_queue_pairs!=xdp_qp){+vi->xdp_queue_pairs=xdp_qp;+err=virtnet_reset(vi);+if(err)+gotovirtio_reset_err;}-vi->xdp_queue_pairs=xdp_qp;netif_set_real_num_rx_queues(dev,curr_qp+xdp_qp);for(i=0;i<vi->max_queue_pairs;i++){
@@ -1789,6 +1866,21 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)}return0;++virtio_reset_err:+/* On reset error do our best to unwind XDP changes inflight and return+*erroruptouserspaceforresolution.Theunderlyingresethungon+*ussonotmuchwecandohere.+*/+dev_warn(&dev->dev,"XDP reset failure and queues unstable\n");+vi->xdp_queue_pairs=oxdp_qp;+virtio_queue_err:+/* On queue set error we can unwind bpf ref count and user space can+*retrythisismostlikelyanallocationfailure.+*/+if(prog)+bpf_prog_sub(prog,vi->max_queue_pairs-1);+returnerr;}staticboolvirtnet_xdp_query(structnet_device*dev)
@@ -2382,6 +2474,15 @@ static int virtnet_probe(struct virtio_device *vdev)returnerr;}+staticvoid_remove_vq_common(structvirtnet_info*vi)+{+vi->vdev->config->reset(vi->vdev);+free_unused_bufs(vi);+_free_receive_bufs(vi);+free_receive_page_frags(vi);+virtnet_del_vqs(vi);+}+staticvoidremove_vq_common(structvirtnet_info*vi){vi->vdev->config->reset(vi->vdev);
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2017-01-18 15:48:39
On Tue, Jan 17, 2017 at 02:19:27PM -0800, John Fastabend wrote:
This has a fix to handle small buffer free logic correctly and then
also adds adjust head support.
I pushed adjust head at net (even though its rc3) to avoid having
to push another exception case into virtio_net to catch if the
program uses adjust_head and then block it. If there are any strong
objections to this we can push it at net-next and use a patch from
Jakub to add the exception handling but then user space has to deal
with it either via try/fail logic or via kernel version checks. Granted
we already have some cases that need to be configured to enable XDP
but I don't see any reason to have yet another one when we can fix it
now vs delaying a kernel version.
1, 3 and 4 definitely look good to me.
I don't like the big hammer approach that other patches
take though. Sent some comments, and I'd like to ponder it for a
couple of days.
v2: fix spelling error, convert unsigned -> unsigned int
v3: v2 git crashed during send so retrying sorry for the noise
v4: changed layout of rtnl_lock fixes (Stephen)
moved reset logic into virtio core with new patch (MST)
fixed up linearize and some code cleanup (Jason)
Otherwise did some generic code cleanup so might be a bit
cleaner this time at least that is the hope.
v5: fixed rtnl_lock issue (DaveM)
In order to fix rtnl_lock issue and also to address Jason's
comment questioning the need for a generic virtio_device_reset
routine I exported some virtio core routines and then wrote
virtio_net reset routine. This is the cleanest solution I
came up with today and I do not at this time have any need
for a more generic reset. If folks don't like this I could
revert back to v3 variant but Stephen pointed out that the
pattern used there is also not ideal.
Thanks for the review.
---
John Fastabend (6):
virtio_net: use dev_kfree_skb for small buffer XDP receive
virtio_net: wrap rtnl_lock in test for calling with lock already held
virtio_net: factor out xdp handler for readability
virtio_net: remove duplicate queue pair binding in XDP
virtio_net: refactor freeze/restore logic into virtnet reset logic
virtio_net: XDP support for adjust_head
drivers/net/virtio_net.c | 332 ++++++++++++++++++++++++++++++----------------
drivers/virtio/virtio.c | 42 +++---
include/linux/virtio.h | 4 +
3 files changed, 247 insertions(+), 131 deletions(-)
--
Signature
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2017-01-18 15:48:49
On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
In the small buffer case during driver unload we currently use
put_page instead of dev_kfree_skb. Resolve this by adding a check
for virtnet mode when checking XDP queue type. Also name the
function so that the code reads correctly to match the additional
check.
Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
Signed-off-by: John Fastabend <redacted>
Acked-by: Jason Wang <redacted>
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2017-01-18 15:48:59
On Tue, Jan 17, 2017 at 02:21:07PM -0800, John Fastabend wrote:
At this point the do_xdp_prog is mostly if/else branches handling
the different modes of virtio_net. So remove it and handle running
the program in the per mode handlers.
Signed-off-by: John Fastabend <redacted>
@@ -576,6 +544,9 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,xdp_prog=rcu_dereference(rq->xdp_prog);if(xdp_prog){structpage*xdp_page;+structxdp_buffxdp;+unsignedintqp;+void*data;u32act;/* This happens when rx buffer size is underestimated */
@@ -598,8 +569,10 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,if(unlikely(hdr->hdr.gso_type))gotoerr_xdp;-act=do_xdp_prog(vi,rq,xdp_prog,-page_address(xdp_page)+offset,len);+data=page_address(xdp_page)+offset;+xdp.data=data+vi->hdr_len;+xdp.data_end=xdp.data+(len-vi->hdr_len);+act=bpf_prog_run_xdp(xdp_prog,&xdp);switch(act){caseXDP_PASS:/* We can only create skb based on xdp_page. */
@@ -332,15 +332,19 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,staticvoidvirtnet_xdp_xmit(structvirtnet_info*vi,structreceive_queue*rq,-structsend_queue*sq,structxdp_buff*xdp,void*data){structvirtio_net_hdr_mrg_rxbuf*hdr;unsignedintnum_sg,len;+structsend_queue*sq;+unsignedintqp;void*xdp_sent;interr;+qp=vi->curr_queue_pairs-vi->xdp_queue_pairs+smp_processor_id();+sq=&vi->sq[qp];+/* Free up any pending old buffers before queueing new ones. */while((xdp_sent=virtqueue_get_buf(sq->vq,&len))!=NULL){if(vi->mergeable_rx_bufs){
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2017-01-18 15:51:05
On Tue, Jan 17, 2017 at 02:22:23PM -0800, John Fastabend wrote:
quoted hunk
For XDP we will need to reset the queues to allow for buffer headroom
to be configured. In order to do this we need to essentially run the
freeze()/restore() code path. Unfortunately the locking requirements
between the freeze/restore and reset paths are different however so
we can not simply reuse the code.
This patch refactors the code path and adds a reset helper routine.
Signed-off-by: John Fastabend <redacted>
---
drivers/net/virtio_net.c | 75 ++++++++++++++++++++++++++++------------------
drivers/virtio/virtio.c | 42 ++++++++++++++------------
include/linux/virtio.h | 4 ++
3 files changed, 73 insertions(+), 48 deletions(-)
@@ -1684,6 +1684,49 @@ static void virtnet_init_settings(struct net_device *dev).set_settings=virtnet_set_settings,};+staticvoidvirtnet_freeze_down(structvirtio_device*vdev)+{+structvirtnet_info*vi=vdev->priv;+inti;++/* Make sure no work handler is accessing the device */+flush_work(&vi->config_work);++netif_device_detach(vi->dev);+cancel_delayed_work_sync(&vi->refill);++if(netif_running(vi->dev)){+for(i=0;i<vi->max_queue_pairs;i++)+napi_disable(&vi->rq[i].napi);+}+}++staticintinit_vqs(structvirtnet_info*vi);
I dislike forward declarations for static functions -
if you are trying to make the diff more readable
(understandable) then pls move this function to before use
in a follow-up patch. Same applies to the next patch.
quoted hunk
+
+static int virtnet_restore_up(struct virtio_device *vdev)
+{
+ struct virtnet_info *vi = vdev->priv;
+ int err, i;
+
+ err = init_vqs(vi);
+ if (err)
+ return err;
+
+ virtio_device_ready(vdev);
+
+ if (netif_running(vi->dev)) {
+ for (i = 0; i < vi->curr_queue_pairs; i++)
+ if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
+ schedule_delayed_work(&vi->refill, 0);
+
+ for (i = 0; i < vi->max_queue_pairs; i++)
+ virtnet_napi_enable(&vi->rq[i]);
+ }
+
+ netif_device_attach(vi->dev);
+ return err;
+}
+
static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
{
unsigned long int max_sz = PAGE_SIZE - sizeof(struct padded_vnet_hdr);
@@ -2374,21 +2417,9 @@ static void virtnet_remove(struct virtio_device *vdev) static int virtnet_freeze(struct virtio_device *vdev) { struct virtnet_info *vi = vdev->priv;- int i; virtnet_cpu_notif_remove(vi);-- /* Make sure no work handler is accessing the device */- flush_work(&vi->config_work);-- netif_device_detach(vi->dev);- cancel_delayed_work_sync(&vi->refill);-- if (netif_running(vi->dev)) {- for (i = 0; i < vi->max_queue_pairs; i++)- napi_disable(&vi->rq[i].napi);- }-+ virtnet_freeze_down(vdev); remove_vq_common(vi); return 0;
@@ -2397,25 +2428,11 @@ static int virtnet_freeze(struct virtio_device *vdev) static int virtnet_restore(struct virtio_device *vdev) { struct virtnet_info *vi = vdev->priv;- int err, i;+ int err;- err = init_vqs(vi);+ err = virtnet_restore_up(vdev); if (err) return err;-- virtio_device_ready(vdev);-- if (netif_running(vi->dev)) {- for (i = 0; i < vi->curr_queue_pairs; i++)- if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))- schedule_delayed_work(&vi->refill, 0);-- for (i = 0; i < vi->max_queue_pairs; i++)- virtnet_napi_enable(&vi->rq[i]);- }-- netif_device_attach(vi->dev);- virtnet_set_queues(vi, vi->curr_queue_pairs); err = virtnet_cpu_notif_add(vi);
@@ -182,6 +185,7 @@ static int virtio_finalize_features(struct virtio_device *dev)}return0;}+EXPORT_SYMBOL_GPL(virtio_finalize_features);staticintvirtio_dev_probe(structdevice*_d){
@@ -193,7 +197,7 @@ static int virtio_dev_probe(struct device *_d)u64driver_features_legacy;/* We have a driver! */-add_status(dev,VIRTIO_CONFIG_S_DRIVER);+virtio_add_status(dev,VIRTIO_CONFIG_S_DRIVER);/* Figure out what features the device supports. */device_features=dev->config->get_features(dev);
@@ -247,7 +251,7 @@ static int virtio_dev_probe(struct device *_d)return0;err:-add_status(dev,VIRTIO_CONFIG_S_FAILED);+virtio_add_status(dev,VIRTIO_CONFIG_S_FAILED);returnerr;}
@@ -265,7 +269,7 @@ static int virtio_dev_remove(struct device *_d)WARN_ON_ONCE(dev->config->get_status(dev));/* Acknowledge the device's existence again. */-add_status(dev,VIRTIO_CONFIG_S_ACKNOWLEDGE);+virtio_add_status(dev,VIRTIO_CONFIG_S_ACKNOWLEDGE);return0;}
@@ -316,7 +320,7 @@ int register_virtio_device(struct virtio_device *dev)dev->config->reset(dev);/* Acknowledge that we've seen the device. */-add_status(dev,VIRTIO_CONFIG_S_ACKNOWLEDGE);+virtio_add_status(dev,VIRTIO_CONFIG_S_ACKNOWLEDGE);INIT_LIST_HEAD(&dev->vqs);
@@ -325,7 +329,7 @@ int register_virtio_device(struct virtio_device *dev)err=device_register(&dev->dev);out:if(err)-add_status(dev,VIRTIO_CONFIG_S_FAILED);+virtio_add_status(dev,VIRTIO_CONFIG_S_FAILED);returnerr;}EXPORT_SYMBOL_GPL(register_virtio_device);
@@ -365,18 +369,18 @@ int virtio_device_restore(struct virtio_device *dev)dev->config->reset(dev);/* Acknowledge that we've seen the device. */-add_status(dev,VIRTIO_CONFIG_S_ACKNOWLEDGE);+virtio_add_status(dev,VIRTIO_CONFIG_S_ACKNOWLEDGE);/* Maybe driver failed before freeze.*Restorethefailedstatus,fordebugging.*/if(dev->failed)-add_status(dev,VIRTIO_CONFIG_S_FAILED);+virtio_add_status(dev,VIRTIO_CONFIG_S_FAILED);if(!drv)return0;/* We have a driver! */-add_status(dev,VIRTIO_CONFIG_S_DRIVER);+virtio_add_status(dev,VIRTIO_CONFIG_S_DRIVER);ret=virtio_finalize_features(dev);if(ret)
@@ -389,14 +393,14 @@ int virtio_device_restore(struct virtio_device *dev)}/* Finally, tell the device we're all set */-add_status(dev,VIRTIO_CONFIG_S_DRIVER_OK);+virtio_add_status(dev,VIRTIO_CONFIG_S_DRIVER_OK);virtio_config_enable(dev);return0;err:-add_status(dev,VIRTIO_CONFIG_S_FAILED);+virtio_add_status(dev,VIRTIO_CONFIG_S_FAILED);returnret;}EXPORT_SYMBOL_GPL(virtio_device_restore);
From: Jason Wang <hidden> Date: 2017-01-19 03:22:38
On 2017年01月18日 23:15, Michael S. Tsirkin wrote:
On Tue, Jan 17, 2017 at 02:22:59PM -0800, John Fastabend wrote:
quoted
Add support for XDP adjust head by allocating a 256B header region
that XDP programs can grow into. This is only enabled when a XDP
program is loaded.
In order to ensure that we do not have to unwind queue headroom push
queue setup below bpf_prog_add. It reads better to do a prog ref
unwind vs another queue setup call.
At the moment this code must do a full reset to ensure old buffers
without headroom on program add or with headroom on program removal
are not used incorrectly in the datapath. Ideally we would only
have to disable/enable the RX queues being updated but there is no
API to do this at the moment in virtio so use the big hammer. In
practice it is likely not that big of a problem as this will only
happen when XDP is enabled/disabled changing programs does not
require the reset. There is some risk that the driver may either
have an allocation failure or for some reason fail to correctly
negotiate with the underlying backend in this case the driver will
be left uninitialized. I have not seen this ever happen on my test
systems and for what its worth this same failure case can occur
from probe and other contexts in virtio framework.
Signed-off-by: John Fastabend<redacted>
I've been thinking about it - can't we drop
old buffers without the head room which were posted before
xdp attached?
Avoiding the reset would be much nicer.
Thoughts?
As been discussed before, device may use them in the same time so it's
not safe. Or do you mean detect them after xdp were set and drop the
buffer without head room, this looks sub-optimal.
Thanks
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2017-01-19 21:12:00
On Thu, Jan 19, 2017 at 11:05:40AM +0800, Jason Wang wrote:
On 2017年01月18日 23:15, Michael S. Tsirkin wrote:
quoted
On Tue, Jan 17, 2017 at 02:22:59PM -0800, John Fastabend wrote:
quoted
Add support for XDP adjust head by allocating a 256B header region
that XDP programs can grow into. This is only enabled when a XDP
program is loaded.
In order to ensure that we do not have to unwind queue headroom push
queue setup below bpf_prog_add. It reads better to do a prog ref
unwind vs another queue setup call.
At the moment this code must do a full reset to ensure old buffers
without headroom on program add or with headroom on program removal
are not used incorrectly in the datapath. Ideally we would only
have to disable/enable the RX queues being updated but there is no
API to do this at the moment in virtio so use the big hammer. In
practice it is likely not that big of a problem as this will only
happen when XDP is enabled/disabled changing programs does not
require the reset. There is some risk that the driver may either
have an allocation failure or for some reason fail to correctly
negotiate with the underlying backend in this case the driver will
be left uninitialized. I have not seen this ever happen on my test
systems and for what its worth this same failure case can occur
from probe and other contexts in virtio framework.
Signed-off-by: John Fastabend<redacted>
I've been thinking about it - can't we drop
old buffers without the head room which were posted before
xdp attached?
Avoiding the reset would be much nicer.
Thoughts?
As been discussed before, device may use them in the same time so it's not
safe. Or do you mean detect them after xdp were set and drop the buffer
without head room, this looks sub-optimal.
Thanks
Yes, this is what I mean. Why is this suboptimal? It's a single branch
in code. Yes we might lose some packets but the big hammer of device
reset will likely lose more.
--
MST
From: Jason Wang <hidden> Date: 2017-01-20 03:27:15
On 2017年01月20日 05:11, Michael S. Tsirkin wrote:
On Thu, Jan 19, 2017 at 11:05:40AM +0800, Jason Wang wrote:
quoted
On 2017年01月18日 23:15, Michael S. Tsirkin wrote:
quoted
On Tue, Jan 17, 2017 at 02:22:59PM -0800, John Fastabend wrote:
quoted
Add support for XDP adjust head by allocating a 256B header region
that XDP programs can grow into. This is only enabled when a XDP
program is loaded.
In order to ensure that we do not have to unwind queue headroom push
queue setup below bpf_prog_add. It reads better to do a prog ref
unwind vs another queue setup call.
At the moment this code must do a full reset to ensure old buffers
without headroom on program add or with headroom on program removal
are not used incorrectly in the datapath. Ideally we would only
have to disable/enable the RX queues being updated but there is no
API to do this at the moment in virtio so use the big hammer. In
practice it is likely not that big of a problem as this will only
happen when XDP is enabled/disabled changing programs does not
require the reset. There is some risk that the driver may either
have an allocation failure or for some reason fail to correctly
negotiate with the underlying backend in this case the driver will
be left uninitialized. I have not seen this ever happen on my test
systems and for what its worth this same failure case can occur
from probe and other contexts in virtio framework.
Signed-off-by: John Fastabend<redacted>
I've been thinking about it - can't we drop
old buffers without the head room which were posted before
xdp attached?
Avoiding the reset would be much nicer.
Thoughts?
As been discussed before, device may use them in the same time so it's not
safe. Or do you mean detect them after xdp were set and drop the buffer
without head room, this looks sub-optimal.
Thanks
Yes, this is what I mean. Why is this suboptimal? It's a single branch
in code. Yes we might lose some packets but the big hammer of device
reset will likely lose more.
Maybe I was wrong but I think driver should try their best to avoid
dropping packets. (And look at mlx4, it did something similar to this
patch).
Thanks
From: John Fastabend <john.fastabend@gmail.com> Date: 2017-01-20 03:38:41
On 17-01-19 01:11 PM, Michael S. Tsirkin wrote:
On Thu, Jan 19, 2017 at 11:05:40AM +0800, Jason Wang wrote:
quoted
On 2017年01月18日 23:15, Michael S. Tsirkin wrote:
quoted
On Tue, Jan 17, 2017 at 02:22:59PM -0800, John Fastabend wrote:
quoted
Add support for XDP adjust head by allocating a 256B header region
that XDP programs can grow into. This is only enabled when a XDP
program is loaded.
In order to ensure that we do not have to unwind queue headroom push
queue setup below bpf_prog_add. It reads better to do a prog ref
unwind vs another queue setup call.
At the moment this code must do a full reset to ensure old buffers
without headroom on program add or with headroom on program removal
are not used incorrectly in the datapath. Ideally we would only
have to disable/enable the RX queues being updated but there is no
API to do this at the moment in virtio so use the big hammer. In
practice it is likely not that big of a problem as this will only
happen when XDP is enabled/disabled changing programs does not
require the reset. There is some risk that the driver may either
have an allocation failure or for some reason fail to correctly
negotiate with the underlying backend in this case the driver will
be left uninitialized. I have not seen this ever happen on my test
systems and for what its worth this same failure case can occur
from probe and other contexts in virtio framework.
Signed-off-by: John Fastabend<redacted>
I've been thinking about it - can't we drop
old buffers without the head room which were posted before
xdp attached?
Avoiding the reset would be much nicer.
Thoughts?
As been discussed before, device may use them in the same time so it's not
safe. Or do you mean detect them after xdp were set and drop the buffer
without head room, this looks sub-optimal.
Thanks
Yes, this is what I mean. Why is this suboptimal? It's a single branch
in code. Yes we might lose some packets but the big hammer of device
reset will likely lose more.
Maybe I'm not following, is the suggestion to drop the packets after XDP
is setup for all outstanding buffers until we have done a reallocation of
all the buffers? In this case we can't just detach the buffers we have to
wait until the backend retires them by using them correct?
But when XDP setup call returns we need to guarantee that buffers and
driver are setup. Otherwise the next n packets get dropped in the future.
If there is no traffic currently this could be at some undetermined point
in the future. This will be very buggy.
Did I miss something?
Thanks,
John
From: John Fastabend <john.fastabend@gmail.com> Date: 2017-01-20 03:39:59
On 17-01-19 07:26 PM, Jason Wang wrote:
On 2017年01月20日 05:11, Michael S. Tsirkin wrote:
quoted
On Thu, Jan 19, 2017 at 11:05:40AM +0800, Jason Wang wrote:
quoted
On 2017年01月18日 23:15, Michael S. Tsirkin wrote:
quoted
On Tue, Jan 17, 2017 at 02:22:59PM -0800, John Fastabend wrote:
quoted
Add support for XDP adjust head by allocating a 256B header region
that XDP programs can grow into. This is only enabled when a XDP
program is loaded.
In order to ensure that we do not have to unwind queue headroom push
queue setup below bpf_prog_add. It reads better to do a prog ref
unwind vs another queue setup call.
At the moment this code must do a full reset to ensure old buffers
without headroom on program add or with headroom on program removal
are not used incorrectly in the datapath. Ideally we would only
have to disable/enable the RX queues being updated but there is no
API to do this at the moment in virtio so use the big hammer. In
practice it is likely not that big of a problem as this will only
happen when XDP is enabled/disabled changing programs does not
require the reset. There is some risk that the driver may either
have an allocation failure or for some reason fail to correctly
negotiate with the underlying backend in this case the driver will
be left uninitialized. I have not seen this ever happen on my test
systems and for what its worth this same failure case can occur
from probe and other contexts in virtio framework.
Signed-off-by: John Fastabend<redacted>
I've been thinking about it - can't we drop
old buffers without the head room which were posted before
xdp attached?
Avoiding the reset would be much nicer.
Thoughts?
As been discussed before, device may use them in the same time so it's not
safe. Or do you mean detect them after xdp were set and drop the buffer
without head room, this looks sub-optimal.
Thanks
Yes, this is what I mean. Why is this suboptimal? It's a single branch
in code. Yes we might lose some packets but the big hammer of device
reset will likely lose more.
Maybe I was wrong but I think driver should try their best to avoid dropping
packets. (And look at mlx4, it did something similar to this patch).
Thanks
+1 sorry didn't see your reply as I was typing mine. Bottom line when XDP
returns I believe the driver must be ready to accept packets or managing
XDP will be problematic.
.John
From: David Laight <hidden> Date: 2017-01-20 16:59:28
From: Michael S. Tsirkin
Sent: 19 January 2017 21:12
quoted
On 2017?01?18? 23:15, Michael S. Tsirkin wrote:
quoted
On Tue, Jan 17, 2017 at 02:22:59PM -0800, John Fastabend wrote:
quoted
Add support for XDP adjust head by allocating a 256B header region
that XDP programs can grow into. This is only enabled when a XDP
program is loaded.
In order to ensure that we do not have to unwind queue headroom push
queue setup below bpf_prog_add. It reads better to do a prog ref
unwind vs another queue setup call.
At the moment this code must do a full reset to ensure old buffers
without headroom on program add or with headroom on program removal
are not used incorrectly in the datapath. Ideally we would only
have to disable/enable the RX queues being updated but there is no
API to do this at the moment in virtio so use the big hammer. In
practice it is likely not that big of a problem as this will only
happen when XDP is enabled/disabled changing programs does not
require the reset. There is some risk that the driver may either
have an allocation failure or for some reason fail to correctly
negotiate with the underlying backend in this case the driver will
be left uninitialized. I have not seen this ever happen on my test
systems and for what its worth this same failure case can occur
from probe and other contexts in virtio framework.
Signed-off-by: John Fastabend<redacted>
I've been thinking about it - can't we drop
old buffers without the head room which were posted before
xdp attached?
Avoiding the reset would be much nicer.
Thoughts?
As been discussed before, device may use them in the same time so it's not
safe. Or do you mean detect them after xdp were set and drop the buffer
without head room, this looks sub-optimal.
Thanks
Yes, this is what I mean. Why is this suboptimal? It's a single branch
in code. Yes we might lose some packets but the big hammer of device
reset will likely lose more.
Why not leave let the hardware receive into the 'small' buffer (without
headroom) and do a copy when a frame is received.
Replace the buffers with 'big' ones for the next receive.
A data copy on a ring full of buffers won't really be noticed.
David
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2017-01-20 17:48:57
On Fri, Jan 20, 2017 at 04:59:11PM +0000, David Laight wrote:
From: Michael S. Tsirkin
quoted
Sent: 19 January 2017 21:12
quoted
On 2017?01?18? 23:15, Michael S. Tsirkin wrote:
quoted
On Tue, Jan 17, 2017 at 02:22:59PM -0800, John Fastabend wrote:
quoted
Add support for XDP adjust head by allocating a 256B header region
that XDP programs can grow into. This is only enabled when a XDP
program is loaded.
In order to ensure that we do not have to unwind queue headroom push
queue setup below bpf_prog_add. It reads better to do a prog ref
unwind vs another queue setup call.
At the moment this code must do a full reset to ensure old buffers
without headroom on program add or with headroom on program removal
are not used incorrectly in the datapath. Ideally we would only
have to disable/enable the RX queues being updated but there is no
API to do this at the moment in virtio so use the big hammer. In
practice it is likely not that big of a problem as this will only
happen when XDP is enabled/disabled changing programs does not
require the reset. There is some risk that the driver may either
have an allocation failure or for some reason fail to correctly
negotiate with the underlying backend in this case the driver will
be left uninitialized. I have not seen this ever happen on my test
systems and for what its worth this same failure case can occur
from probe and other contexts in virtio framework.
Signed-off-by: John Fastabend<redacted>
I've been thinking about it - can't we drop
old buffers without the head room which were posted before
xdp attached?
Avoiding the reset would be much nicer.
Thoughts?
As been discussed before, device may use them in the same time so it's not
safe. Or do you mean detect them after xdp were set and drop the buffer
without head room, this looks sub-optimal.
Thanks
Yes, this is what I mean. Why is this suboptimal? It's a single branch
in code. Yes we might lose some packets but the big hammer of device
reset will likely lose more.
Why not leave let the hardware receive into the 'small' buffer (without
headroom) and do a copy when a frame is received.
Replace the buffers with 'big' ones for the next receive.
A data copy on a ring full of buffers won't really be noticed.
David
From: Jason Wang <hidden> Date: 2017-01-22 02:52:57
On 2017年01月21日 01:48, Michael S. Tsirkin wrote:
On Fri, Jan 20, 2017 at 04:59:11PM +0000, David Laight wrote:
quoted
From: Michael S. Tsirkin
quoted
Sent: 19 January 2017 21:12
quoted
On 2017?01?18? 23:15, Michael S. Tsirkin wrote:
quoted
On Tue, Jan 17, 2017 at 02:22:59PM -0800, John Fastabend wrote:
quoted
Add support for XDP adjust head by allocating a 256B header region
that XDP programs can grow into. This is only enabled when a XDP
program is loaded.
In order to ensure that we do not have to unwind queue headroom push
queue setup below bpf_prog_add. It reads better to do a prog ref
unwind vs another queue setup call.
At the moment this code must do a full reset to ensure old buffers
without headroom on program add or with headroom on program removal
are not used incorrectly in the datapath. Ideally we would only
have to disable/enable the RX queues being updated but there is no
API to do this at the moment in virtio so use the big hammer. In
practice it is likely not that big of a problem as this will only
happen when XDP is enabled/disabled changing programs does not
require the reset. There is some risk that the driver may either
have an allocation failure or for some reason fail to correctly
negotiate with the underlying backend in this case the driver will
be left uninitialized. I have not seen this ever happen on my test
systems and for what its worth this same failure case can occur
from probe and other contexts in virtio framework.
Signed-off-by: John Fastabend<redacted>
I've been thinking about it - can't we drop
old buffers without the head room which were posted before
xdp attached?
Avoiding the reset would be much nicer.
Thoughts?
As been discussed before, device may use them in the same time so it's not
safe. Or do you mean detect them after xdp were set and drop the buffer
without head room, this looks sub-optimal.
Thanks
Yes, this is what I mean. Why is this suboptimal? It's a single branch
in code. Yes we might lose some packets but the big hammer of device
reset will likely lose more.
Why not leave let the hardware receive into the 'small' buffer (without
headroom) and do a copy when a frame is received.
Replace the buffers with 'big' ones for the next receive.
A data copy on a ring full of buffers won't really be noticed.
David
I like that. John?
This works, I prefer this only if it uses simpler code (but I suspect)
than reset.
Thanks
From: John Fastabend <john.fastabend@gmail.com> Date: 2017-01-22 04:14:39
On 17-01-21 06:51 PM, Jason Wang wrote:
On 2017年01月21日 01:48, Michael S. Tsirkin wrote:
quoted
On Fri, Jan 20, 2017 at 04:59:11PM +0000, David Laight wrote:
quoted
From: Michael S. Tsirkin
quoted
Sent: 19 January 2017 21:12
quoted
On 2017?01?18? 23:15, Michael S. Tsirkin wrote:
quoted
On Tue, Jan 17, 2017 at 02:22:59PM -0800, John Fastabend wrote:
quoted
Add support for XDP adjust head by allocating a 256B header region
that XDP programs can grow into. This is only enabled when a XDP
program is loaded.
In order to ensure that we do not have to unwind queue headroom push
queue setup below bpf_prog_add. It reads better to do a prog ref
unwind vs another queue setup call.
At the moment this code must do a full reset to ensure old buffers
without headroom on program add or with headroom on program removal
are not used incorrectly in the datapath. Ideally we would only
have to disable/enable the RX queues being updated but there is no
API to do this at the moment in virtio so use the big hammer. In
practice it is likely not that big of a problem as this will only
happen when XDP is enabled/disabled changing programs does not
require the reset. There is some risk that the driver may either
have an allocation failure or for some reason fail to correctly
negotiate with the underlying backend in this case the driver will
be left uninitialized. I have not seen this ever happen on my test
systems and for what its worth this same failure case can occur
from probe and other contexts in virtio framework.
Signed-off-by: John Fastabend<redacted>
I've been thinking about it - can't we drop
old buffers without the head room which were posted before
xdp attached?
Avoiding the reset would be much nicer.
Thoughts?
As been discussed before, device may use them in the same time so it's not
safe. Or do you mean detect them after xdp were set and drop the buffer
without head room, this looks sub-optimal.
Thanks
Yes, this is what I mean. Why is this suboptimal? It's a single branch
in code. Yes we might lose some packets but the big hammer of device
reset will likely lose more.
Why not leave let the hardware receive into the 'small' buffer (without
headroom) and do a copy when a frame is received.
Replace the buffers with 'big' ones for the next receive.
A data copy on a ring full of buffers won't really be noticed.
David
I like that. John?
This works, I prefer this only if it uses simpler code (but I suspect) than reset.
Thanks
Before the reset path I looked at doing this but it seems to require tracking
if a buffer had headroom on a per buffer basis. I don't see a good spot to
put a bit like this? It could go in the inbuf 'ctx' added by virtqueue_add_inbuf
but I would need to change the current usage of ctx which in the mergeable case
at least is just a simple pointer today. I don't like this because it
complicates the normal path and the XDP hotpath.
Otherwise we could somehow mark the ring at the point where XDP is enabled so
that it can learn when a full iteration around the ring. But I can't see a
simple way to make this work either.
I don't know the reset look straight forward to me and although not ideal is
fairly common on hardware based drivers during configuration changes. I'm open
to any ideas on where to put the metadata to track headroom though.
Thanks,
John
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2017-01-23 17:02:36
On Sat, Jan 21, 2017 at 08:14:19PM -0800, John Fastabend wrote:
On 17-01-21 06:51 PM, Jason Wang wrote:
quoted
On 2017年01月21日 01:48, Michael S. Tsirkin wrote:
quoted
On Fri, Jan 20, 2017 at 04:59:11PM +0000, David Laight wrote:
quoted
From: Michael S. Tsirkin
quoted
Sent: 19 January 2017 21:12
quoted
On 2017?01?18? 23:15, Michael S. Tsirkin wrote:
quoted
On Tue, Jan 17, 2017 at 02:22:59PM -0800, John Fastabend wrote:
quoted
Add support for XDP adjust head by allocating a 256B header region
that XDP programs can grow into. This is only enabled when a XDP
program is loaded.
In order to ensure that we do not have to unwind queue headroom push
queue setup below bpf_prog_add. It reads better to do a prog ref
unwind vs another queue setup call.
At the moment this code must do a full reset to ensure old buffers
without headroom on program add or with headroom on program removal
are not used incorrectly in the datapath. Ideally we would only
have to disable/enable the RX queues being updated but there is no
API to do this at the moment in virtio so use the big hammer. In
practice it is likely not that big of a problem as this will only
happen when XDP is enabled/disabled changing programs does not
require the reset. There is some risk that the driver may either
have an allocation failure or for some reason fail to correctly
negotiate with the underlying backend in this case the driver will
be left uninitialized. I have not seen this ever happen on my test
systems and for what its worth this same failure case can occur
from probe and other contexts in virtio framework.
Signed-off-by: John Fastabend<redacted>
I've been thinking about it - can't we drop
old buffers without the head room which were posted before
xdp attached?
Avoiding the reset would be much nicer.
Thoughts?
As been discussed before, device may use them in the same time so it's not
safe. Or do you mean detect them after xdp were set and drop the buffer
without head room, this looks sub-optimal.
Thanks
Yes, this is what I mean. Why is this suboptimal? It's a single branch
in code. Yes we might lose some packets but the big hammer of device
reset will likely lose more.
Why not leave let the hardware receive into the 'small' buffer (without
headroom) and do a copy when a frame is received.
Replace the buffers with 'big' ones for the next receive.
A data copy on a ring full of buffers won't really be noticed.
David
I like that. John?
This works, I prefer this only if it uses simpler code (but I suspect) than reset.
Thanks
Before the reset path I looked at doing this but it seems to require tracking
if a buffer had headroom on a per buffer basis. I don't see a good spot to
put a bit like this? It could go in the inbuf 'ctx' added by virtqueue_add_inbuf
but I would need to change the current usage of ctx which in the mergeable case
at least is just a simple pointer today. I don't like this because it
complicates the normal path and the XDP hotpath.
Otherwise we could somehow mark the ring at the point where XDP is enabled so
that it can learn when a full iteration around the ring. But I can't see a
simple way to make this work either.
I don't know the reset look straight forward to me and although not ideal is
fairly common on hardware based drivers during configuration changes. I'm open
to any ideas on where to put the metadata to track headroom though.
Thanks,
John
Well with 4K pages we actually have 4 spare bits to use.
In fact this means we could reduce the mergeable buffer alignment.
It starts getting strange with 64K pages where we are
out of space, and I just noticed that with bigger pages
virtio is actually broken.
So let me fix it up first of all, and on top - maybe we can just
increase the alignment for 64k pages and up?
Truesize alignment to 512 is still reasonable and presumably
these 64k page boxes have lots of memory.
Would it make sense to tweak SK_RMEM_MAX up for larger
page sizes?
--
MST
@@ -41,6 +41,9 @@#define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)#define GOOD_COPY_LEN 128+/* Amount of XDP headroom to prepend to packets for use by xdp_adjust_head */+#define VIRTIO_XDP_HEADROOM 256+/* RX packet size EWMA. The average packet size is used to determine the packet*buffersizewhenrefillingRXrings.AstheentireRXringmayberefilled*atonce,theweightischosensothattheEWMAwillbeinsensitivetoshort-
I wonder where does this number come from? This is quite a lot and
means that using XDP_PASS will slow down any sockets on top of it.
Which in turn means people will try to remove XDP when not in use,
causing resets. E.g. build_skb (which I have a patch to switch to) uses
a much more reasonable NET_SKB_PAD.
--
MST
@@ -41,6 +41,9 @@#define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)#define GOOD_COPY_LEN 128+/* Amount of XDP headroom to prepend to packets for use by xdp_adjust_head */+#define VIRTIO_XDP_HEADROOM 256+/* RX packet size EWMA. The average packet size is used to determine the packet*buffersizewhenrefillingRXrings.AstheentireRXringmayberefilled*atonce,theweightischosensothattheEWMAwillbeinsensitivetoshort-
I wonder where does this number come from? This is quite a lot and
means that using XDP_PASS will slow down any sockets on top of it.
Which in turn means people will try to remove XDP when not in use,
causing resets. E.g. build_skb (which I have a patch to switch to) uses
a much more reasonable NET_SKB_PAD.
--
MST
Let me show you a patch that I've been cooking. What is missing there
is handling corner cases like e.g. when ring size is ~4 entries so
using smaller buffers might mean we no longer have enough space to store
a full packet. So it looks like I have to maintain the skb copy path
for this hardware.
With this patch, standard configuration has NET_SKB_PAD + NET_IP_ALIGN
bytes head padding. Would this be enough for XDP? If yes we do not
need the resets.
Thoughts?
--->
virtio_net: switch to build_skb for mrg_rxbuf
For small packets data copy was observed to
take up about 15% CPU time. Switch to build_skb
and avoid the copy when using mergeable rx buffers.
As a bonus, medium-size skbs that fit in a page will be
completely linear.
Of course, we now need to lower the lower bound on packet size,
to make sure a sane number of skbs fits in rx socket buffer.
By how much? I don't know yet.
It might also be useful to prefetch the packet buffer since
net stack will likely use it soon.
Lightly tested, in particular, I didn't yet test what this
actually does to performance - sending this out for early
feedback/flames.
TODO: it appears that Linux won't handle correctly the case of first
buffer being very small (or consisting exclusively of virtio header).
This is already the case for current code, need to fix.
TODO: might be unfair to the last packet in a fragment as we include
remaining space if any in its truesize.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
----
@@ -38,6 +38,8 @@ module_param(gso, bool, 0444);/* FIXME: MTU in config. */#define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)+//#define MIN_PACKET_ALLOC GOOD_PACKET_LEN+#define MIN_PACKET_ALLOC 128#define GOOD_COPY_LEN 128/* RX packet size EWMA. The average packet size is used to determine the packet
@@ -246,6 +248,9 @@ static void *mergeable_ctx_to_buf_address(unsigned long mrg_ctx)staticunsignedlongmergeable_buf_to_ctx(void*buf,unsignedinttruesize){unsignedintsize=truesize/MERGEABLE_BUFFER_ALIGN;++BUG_ON((unsignedlong)buf&(MERGEABLE_BUFFER_ALIGN-1));+BUG_ON(size-1>=MERGEABLE_BUFFER_ALIGN);return(unsignedlong)buf|(size-1);}
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2017-01-23 21:08:36
On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
In the small buffer case during driver unload we currently use
put_page instead of dev_kfree_skb. Resolve this by adding a check
for virtnet mode when checking XDP queue type. Also name the
function so that the code reads correctly to match the additional
check.
Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
Signed-off-by: John Fastabend <redacted>
Acked-by: Jason Wang <redacted>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
I think we definitely want this one in -net as it's
a bugfix.
From: John Fastabend <john.fastabend@gmail.com> Date: 2017-01-23 21:57:43
On 17-01-23 01:08 PM, Michael S. Tsirkin wrote:
On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
quoted
In the small buffer case during driver unload we currently use
put_page instead of dev_kfree_skb. Resolve this by adding a check
for virtnet mode when checking XDP queue type. Also name the
function so that the code reads correctly to match the additional
check.
Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
Signed-off-by: John Fastabend <redacted>
Acked-by: Jason Wang <redacted>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
I think we definitely want this one in -net as it's
a bugfix.
Agreed, let me pull this fix out of the series and submit it for
net.
@@ -41,6 +41,9 @@#define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)#define GOOD_COPY_LEN 128+/* Amount of XDP headroom to prepend to packets for use by xdp_adjust_head */+#define VIRTIO_XDP_HEADROOM 256+/* RX packet size EWMA. The average packet size is used to determine the packet*buffersizewhenrefillingRXrings.AstheentireRXringmayberefilled*atonce,theweightischosensothattheEWMAwillbeinsensitivetoshort-
I wonder where does this number come from? This is quite a lot and
means that using XDP_PASS will slow down any sockets on top of it.
Which in turn means people will try to remove XDP when not in use,
causing resets. E.g. build_skb (which I have a patch to switch to) uses
a much more reasonable NET_SKB_PAD.
I just used the value Alexei (or someone?) came up with. I think it needs to be
large enough to avoid copy in header encap cases. So minimum
VXLAN_HDR + OUTER_UDP + OUTER_IPV6_HDR + OUTER_MAC =
8 + 8 + 40 + 14 = 70
The choice of VXLAN hdr was sort of arbitrary but seems good for estimates. For
what its worth there is also a ndo_set_rx_headroom could we use that to set it
and choose a reasonable default.
quoted
--
MST
Let me show you a patch that I've been cooking. What is missing there
is handling corner cases like e.g. when ring size is ~4 entries so
using smaller buffers might mean we no longer have enough space to store
a full packet. So it looks like I have to maintain the skb copy path
for this hardware.
With this patch, standard configuration has NET_SKB_PAD + NET_IP_ALIGN
bytes head padding. Would this be enough for XDP? If yes we do not
need the resets.
Based on above seems a bit small (L1_CACHE_BYTES + 2)? How tricky would it
be to add support for ndo_set_rx_headroom.
Thoughts?
I'll take a look at the patch this afternoon. Thanks.
quoted hunk
--->
virtio_net: switch to build_skb for mrg_rxbuf
For small packets data copy was observed to
take up about 15% CPU time. Switch to build_skb
and avoid the copy when using mergeable rx buffers.
As a bonus, medium-size skbs that fit in a page will be
completely linear.
Of course, we now need to lower the lower bound on packet size,
to make sure a sane number of skbs fits in rx socket buffer.
By how much? I don't know yet.
It might also be useful to prefetch the packet buffer since
net stack will likely use it soon.
Lightly tested, in particular, I didn't yet test what this
actually does to performance - sending this out for early
feedback/flames.
TODO: it appears that Linux won't handle correctly the case of first
buffer being very small (or consisting exclusively of virtio header).
This is already the case for current code, need to fix.
TODO: might be unfair to the last packet in a fragment as we include
remaining space if any in its truesize.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
----
@@ -38,6 +38,8 @@ module_param(gso, bool, 0444);/* FIXME: MTU in config. */#define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)+//#define MIN_PACKET_ALLOC GOOD_PACKET_LEN+#define MIN_PACKET_ALLOC 128#define GOOD_COPY_LEN 128/* RX packet size EWMA. The average packet size is used to determine the packet
@@ -246,6 +248,9 @@ static void *mergeable_ctx_to_buf_address(unsigned long mrg_ctx)staticunsignedlongmergeable_buf_to_ctx(void*buf,unsignedinttruesize){unsignedintsize=truesize/MERGEABLE_BUFFER_ALIGN;++BUG_ON((unsignedlong)buf&(MERGEABLE_BUFFER_ALIGN-1));+BUG_ON(size-1>=MERGEABLE_BUFFER_ALIGN);return(unsignedlong)buf|(size-1);}
@@ -41,6 +41,9 @@#define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)#define GOOD_COPY_LEN 128+/* Amount of XDP headroom to prepend to packets for use by xdp_adjust_head */+#define VIRTIO_XDP_HEADROOM 256+/* RX packet size EWMA. The average packet size is used to determine the packet*buffersizewhenrefillingRXrings.AstheentireRXringmayberefilled*atonce,theweightischosensothattheEWMAwillbeinsensitivetoshort-
I wonder where does this number come from? This is quite a lot and
means that using XDP_PASS will slow down any sockets on top of it.
Which in turn means people will try to remove XDP when not in use,
causing resets. E.g. build_skb (which I have a patch to switch to) uses
a much more reasonable NET_SKB_PAD.
I just used the value Alexei (or someone?) came up with. I think it needs to be
large enough to avoid copy in header encap cases. So minimum
VXLAN_HDR + OUTER_UDP + OUTER_IPV6_HDR + OUTER_MAC =
8 + 8 + 40 + 14 = 70
The choice of VXLAN hdr was sort of arbitrary but seems good for estimates. For
what its worth there is also a ndo_set_rx_headroom could we use that to set it
and choose a reasonable default.
quoted
quoted
--
MST
Let me show you a patch that I've been cooking. What is missing there
is handling corner cases like e.g. when ring size is ~4 entries so
using smaller buffers might mean we no longer have enough space to store
a full packet. So it looks like I have to maintain the skb copy path
for this hardware.
With this patch, standard configuration has NET_SKB_PAD + NET_IP_ALIGN
bytes head padding. Would this be enough for XDP? If yes we do not
need the resets.
Based on above seems a bit small (L1_CACHE_BYTES + 2)? How tricky would it
be to add support for ndo_set_rx_headroom.
Donnu but then what? Expose it to userspace and let admin
make the decision for us?
quoted
Thoughts?
I'll take a look at the patch this afternoon. Thanks.
quoted
--->
virtio_net: switch to build_skb for mrg_rxbuf
For small packets data copy was observed to
take up about 15% CPU time. Switch to build_skb
and avoid the copy when using mergeable rx buffers.
As a bonus, medium-size skbs that fit in a page will be
completely linear.
Of course, we now need to lower the lower bound on packet size,
to make sure a sane number of skbs fits in rx socket buffer.
By how much? I don't know yet.
It might also be useful to prefetch the packet buffer since
net stack will likely use it soon.
Lightly tested, in particular, I didn't yet test what this
actually does to performance - sending this out for early
feedback/flames.
TODO: it appears that Linux won't handle correctly the case of first
buffer being very small (or consisting exclusively of virtio header).
This is already the case for current code, need to fix.
TODO: might be unfair to the last packet in a fragment as we include
remaining space if any in its truesize.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
----
@@ -38,6 +38,8 @@ module_param(gso, bool, 0444);/* FIXME: MTU in config. */#define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)+//#define MIN_PACKET_ALLOC GOOD_PACKET_LEN+#define MIN_PACKET_ALLOC 128#define GOOD_COPY_LEN 128/* RX packet size EWMA. The average packet size is used to determine the packet
@@ -246,6 +248,9 @@ static void *mergeable_ctx_to_buf_address(unsigned long mrg_ctx)staticunsignedlongmergeable_buf_to_ctx(void*buf,unsignedinttruesize){unsignedintsize=truesize/MERGEABLE_BUFFER_ALIGN;++BUG_ON((unsignedlong)buf&(MERGEABLE_BUFFER_ALIGN-1));+BUG_ON(size-1>=MERGEABLE_BUFFER_ALIGN);return(unsignedlong)buf|(size-1);}
From: David Miller <davem@davemloft.net> Date: 2017-01-24 19:43:29
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Mon, 23 Jan 2017 23:08:35 +0200
On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
quoted
In the small buffer case during driver unload we currently use
put_page instead of dev_kfree_skb. Resolve this by adding a check
for virtnet mode when checking XDP queue type. Also name the
function so that the code reads correctly to match the additional
check.
Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
Signed-off-by: John Fastabend <redacted>
Acked-by: Jason Wang <redacted>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
I think we definitely want this one in -net as it's
a bugfix.
This whole series is a bug fix, we must have adjust_header XDP
support in the virtio_net driver before v4.10 goes out, it is
a requires base feature for XDP.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2017-01-24 20:08:35
On Tue, Jan 24, 2017 at 02:43:28PM -0500, David Miller wrote:
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Mon, 23 Jan 2017 23:08:35 +0200
quoted
On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
quoted
In the small buffer case during driver unload we currently use
put_page instead of dev_kfree_skb. Resolve this by adding a check
for virtnet mode when checking XDP queue type. Also name the
function so that the code reads correctly to match the additional
check.
Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
Signed-off-by: John Fastabend <redacted>
Acked-by: Jason Wang <redacted>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
I think we definitely want this one in -net as it's
a bugfix.
This whole series is a bug fix, we must have adjust_header XDP
support in the virtio_net driver before v4.10 goes out, it is
a requires base feature for XDP.
I have to say device resets outside probe have a huge potential
to uncover hypervisor bugs. I am rather uncomfortable
doing that after -rc1.
How about a module option to disable it by default?
We can then ship a partial implementation in 4.10
and work on completing it in 4.11.
--
MST
From: David Miller <davem@davemloft.net> Date: 2017-01-24 20:12:05
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Tue, 24 Jan 2017 22:08:33 +0200
On Tue, Jan 24, 2017 at 02:43:28PM -0500, David Miller wrote:
quoted
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Mon, 23 Jan 2017 23:08:35 +0200
quoted
On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
quoted
In the small buffer case during driver unload we currently use
put_page instead of dev_kfree_skb. Resolve this by adding a check
for virtnet mode when checking XDP queue type. Also name the
function so that the code reads correctly to match the additional
check.
Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
Signed-off-by: John Fastabend <redacted>
Acked-by: Jason Wang <redacted>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
I think we definitely want this one in -net as it's
a bugfix.
This whole series is a bug fix, we must have adjust_header XDP
support in the virtio_net driver before v4.10 goes out, it is
a requires base feature for XDP.
I have to say device resets outside probe have a huge potential
to uncover hypervisor bugs. I am rather uncomfortable
doing that after -rc1.
How about a module option to disable it by default?
We can then ship a partial implementation in 4.10
and work on completing it in 4.11.
XDP programmers must be able to assume a base set of features being
present, adjust_header is one of them.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2017-01-24 20:54:42
On Tue, Jan 24, 2017 at 03:11:39PM -0500, David Miller wrote:
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Tue, 24 Jan 2017 22:08:33 +0200
quoted
On Tue, Jan 24, 2017 at 02:43:28PM -0500, David Miller wrote:
quoted
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Mon, 23 Jan 2017 23:08:35 +0200
quoted
On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
quoted
In the small buffer case during driver unload we currently use
put_page instead of dev_kfree_skb. Resolve this by adding a check
for virtnet mode when checking XDP queue type. Also name the
function so that the code reads correctly to match the additional
check.
Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
Signed-off-by: John Fastabend <redacted>
Acked-by: Jason Wang <redacted>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
I think we definitely want this one in -net as it's
a bugfix.
This whole series is a bug fix, we must have adjust_header XDP
support in the virtio_net driver before v4.10 goes out, it is
a requires base feature for XDP.
I have to say device resets outside probe have a huge potential
to uncover hypervisor bugs. I am rather uncomfortable
doing that after -rc1.
How about a module option to disable it by default?
We can then ship a partial implementation in 4.10
and work on completing it in 4.11.
XDP programmers must be able to assume a base set of features being
present, adjust_header is one of them.
Let's make all of XDP depend on this extra_headroom option then?
--
MST
From: Jason Wang <hidden> Date: 2017-01-25 02:57:19
On 2017年01月25日 04:08, Michael S. Tsirkin wrote:
On Tue, Jan 24, 2017 at 02:43:28PM -0500, David Miller wrote:
quoted
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Mon, 23 Jan 2017 23:08:35 +0200
quoted
On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
quoted
In the small buffer case during driver unload we currently use
put_page instead of dev_kfree_skb. Resolve this by adding a check
for virtnet mode when checking XDP queue type. Also name the
function so that the code reads correctly to match the additional
check.
Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
Signed-off-by: John Fastabend <redacted>
Acked-by: Jason Wang <redacted>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
I think we definitely want this one in -net as it's
a bugfix.
This whole series is a bug fix, we must have adjust_header XDP
support in the virtio_net driver before v4.10 goes out, it is
a requires base feature for XDP.
I have to say device resets outside probe have a huge potential
to uncover hypervisor bugs.
Maybe not if it reuses most of current codes? Since we've already used
them in sleep or hibernation?
Thanks
I am rather uncomfortable
doing that after -rc1.
How about a module option to disable it by default?
We can then ship a partial implementation in 4.10
and work on completing it in 4.11.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2017-01-25 03:23:49
On Wed, Jan 25, 2017 at 10:57:12AM +0800, Jason Wang wrote:
On 2017年01月25日 04:08, Michael S. Tsirkin wrote:
quoted
On Tue, Jan 24, 2017 at 02:43:28PM -0500, David Miller wrote:
quoted
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Mon, 23 Jan 2017 23:08:35 +0200
quoted
On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
quoted
In the small buffer case during driver unload we currently use
put_page instead of dev_kfree_skb. Resolve this by adding a check
for virtnet mode when checking XDP queue type. Also name the
function so that the code reads correctly to match the additional
check.
Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
Signed-off-by: John Fastabend <redacted>
Acked-by: Jason Wang <redacted>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
I think we definitely want this one in -net as it's
a bugfix.
This whole series is a bug fix, we must have adjust_header XDP
support in the virtio_net driver before v4.10 goes out, it is
a requires base feature for XDP.
I have to say device resets outside probe have a huge potential
to uncover hypervisor bugs.
Maybe not if it reuses most of current codes? Since we've already used them
in sleep or hibernation?
Thanks
Except almost no one uses sleep or hybernate with VMs. I'm not saying
it's a bad idea, just that it needs a lot of testing before release and
we won't get enough if we merge at this point.
quoted
I am rather uncomfortable
doing that after -rc1.
How about a module option to disable it by default?
We can then ship a partial implementation in 4.10
and work on completing it in 4.11.
To clarify, I'm thinking an option similar to enable_xdp,
and have all packets have a 256 byte headroom for 4.10.
Consider our options for 4.11.
--
MST
From: John Fastabend <john.fastabend@gmail.com> Date: 2017-01-25 04:02:46
On 17-01-24 07:23 PM, Michael S. Tsirkin wrote:
On Wed, Jan 25, 2017 at 10:57:12AM +0800, Jason Wang wrote:
quoted
On 2017年01月25日 04:08, Michael S. Tsirkin wrote:
quoted
On Tue, Jan 24, 2017 at 02:43:28PM -0500, David Miller wrote:
quoted
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Mon, 23 Jan 2017 23:08:35 +0200
quoted
On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
quoted
In the small buffer case during driver unload we currently use
put_page instead of dev_kfree_skb. Resolve this by adding a check
for virtnet mode when checking XDP queue type. Also name the
function so that the code reads correctly to match the additional
check.
Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
Signed-off-by: John Fastabend <redacted>
Acked-by: Jason Wang <redacted>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
I think we definitely want this one in -net as it's
a bugfix.
This whole series is a bug fix, we must have adjust_header XDP
support in the virtio_net driver before v4.10 goes out, it is
a requires base feature for XDP.
I have to say device resets outside probe have a huge potential
to uncover hypervisor bugs.
Maybe not if it reuses most of current codes? Since we've already used them
in sleep or hibernation?
Thanks
Except almost no one uses sleep or hybernate with VMs. I'm not saying
it's a bad idea, just that it needs a lot of testing before release and
we won't get enough if we merge at this point.
Then it would seem like a good thing to have another user of these paths and
find the bugs versus letting them sit there for some poor folks who do use
sleep/hybernate.
quoted
quoted
I am rather uncomfortable
doing that after -rc1.
How about a module option to disable it by default?
We can then ship a partial implementation in 4.10
and work on completing it in 4.11.
Ugh I would prefer to avoid module options. This will only happen if users
push XDP program into driver anyways.
To clarify, I'm thinking an option similar to enable_xdp,
and have all packets have a 256 byte headroom for 4.10.
An option where? In QEMU side, in driver? Is the reset really that bad, coming
from a hardware driver side lots of configuration changes can cause resets. I
agree its not overly elegant but could follow on patches be used to make it
prettier if possible.
I know folks prefer to avoid tuning knobs but I think exposing the headroom
configuration to users might not be a bad idea. After all these same users are
already programming maps and ebpf codes. A simple tuning knob should not be a
big deal and reasonable defaults would of course be used. That is a net-next
debate though.
Consider our options for 4.11.
Finally just to point out here are the drivers with XDP support on latest
net tree,
mlx/mlx5
mlx/mlx4
qlogic/qede
netronome/nfp
virtio_net
And here is the list of adjust header support,
mlx/mlx4
So we currently have the same feature gap on all the other drivers except one.
Although I do not think that is a very good excuse. Lets figure out what we
should do about virtio.
Thanks,
John
From: Jason Wang <hidden> Date: 2017-01-25 05:46:53
On 2017年01月25日 12:02, John Fastabend wrote:
On 17-01-24 07:23 PM, Michael S. Tsirkin wrote:
quoted
On Wed, Jan 25, 2017 at 10:57:12AM +0800, Jason Wang wrote:
quoted
On 2017年01月25日 04:08, Michael S. Tsirkin wrote:
quoted
On Tue, Jan 24, 2017 at 02:43:28PM -0500, David Miller wrote:
quoted
From: "Michael S. Tsirkin"<mst@redhat.com>
Date: Mon, 23 Jan 2017 23:08:35 +0200
quoted
On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
quoted
In the small buffer case during driver unload we currently use
put_page instead of dev_kfree_skb. Resolve this by adding a check
for virtnet mode when checking XDP queue type. Also name the
function so that the code reads correctly to match the additional
check.
Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
Signed-off-by: John Fastabend<redacted>
Acked-by: Jason Wang<redacted>
Acked-by: Michael S. Tsirkin<mst@redhat.com>
I think we definitely want this one in -net as it's
a bugfix.
This whole series is a bug fix, we must have adjust_header XDP
support in the virtio_net driver before v4.10 goes out, it is
a requires base feature for XDP.
I have to say device resets outside probe have a huge potential
to uncover hypervisor bugs.
Maybe not if it reuses most of current codes? Since we've already used them
in sleep or hibernation?
Thanks
Except almost no one uses sleep or hybernate with VMs. I'm not saying
it's a bad idea, just that it needs a lot of testing before release and
we won't get enough if we merge at this point.
Then it would seem like a good thing to have another user of these paths and
find the bugs versus letting them sit there for some poor folks who do use
sleep/hybernate.
Yes, and uncovering hypervisor bugs now is better than uncovering it in
the future.
Thanks
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2017-01-25 14:45:26
On Tue, Jan 24, 2017 at 08:02:29PM -0800, John Fastabend wrote:
On 17-01-24 07:23 PM, Michael S. Tsirkin wrote:
quoted
On Wed, Jan 25, 2017 at 10:57:12AM +0800, Jason Wang wrote:
quoted
On 2017年01月25日 04:08, Michael S. Tsirkin wrote:
quoted
On Tue, Jan 24, 2017 at 02:43:28PM -0500, David Miller wrote:
quoted
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Mon, 23 Jan 2017 23:08:35 +0200
quoted
On Tue, Jan 17, 2017 at 02:19:50PM -0800, John Fastabend wrote:
quoted
In the small buffer case during driver unload we currently use
put_page instead of dev_kfree_skb. Resolve this by adding a check
for virtnet mode when checking XDP queue type. Also name the
function so that the code reads correctly to match the additional
check.
Fixes: bb91accf2733 ("virtio-net: XDP support for small buffers")
Signed-off-by: John Fastabend <redacted>
Acked-by: Jason Wang <redacted>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
I think we definitely want this one in -net as it's
a bugfix.
This whole series is a bug fix, we must have adjust_header XDP
support in the virtio_net driver before v4.10 goes out, it is
a requires base feature for XDP.
I have to say device resets outside probe have a huge potential
to uncover hypervisor bugs.
Maybe not if it reuses most of current codes? Since we've already used them
in sleep or hibernation?
Thanks
Except almost no one uses sleep or hybernate with VMs. I'm not saying
it's a bad idea, just that it needs a lot of testing before release and
we won't get enough if we merge at this point.
Then it would seem like a good thing to have another user of these paths and
find the bugs versus letting them sit there for some poor folks who do use
sleep/hybernate.
Absolutely. But -rc6 is not the time to test waters IMO.
quoted
quoted
quoted
I am rather uncomfortable
doing that after -rc1.
How about a module option to disable it by default?
We can then ship a partial implementation in 4.10
and work on completing it in 4.11.
Ugh I would prefer to avoid module options. This will only happen if users
push XDP program into driver anyways.
Again I agree, it's an idea for a stopgap measure so we can have
something in 4.10 - and also assuming that 256b headroom is a must.
quoted
To clarify, I'm thinking an option similar to enable_xdp,
and have all packets have a 256 byte headroom for 4.10.
An option where? In QEMU side, in driver? Is the reset really that bad, coming
from a hardware driver side lots of configuration changes can cause resets. I
agree its not overly elegant but could follow on patches be used to make it
prettier if possible.
Again I agree and it's not that bad it's just not something we should
do past rc5.
I know folks prefer to avoid tuning knobs but I think exposing the headroom
configuration to users might not be a bad idea. After all these same users are
already programming maps and ebpf codes. A simple tuning knob should not be a
big deal and reasonable defaults would of course be used. That is a net-next
debate though.
No arguments from my side here.
quoted
Consider our options for 4.11.
Finally just to point out here are the drivers with XDP support on latest
net tree,
mlx/mlx5
mlx/mlx4
qlogic/qede
netronome/nfp
virtio_net
And here is the list of adjust header support,
mlx/mlx4
Above seems to imply an interface for userspace to detect the amount
of head space would be benefitial.
So we currently have the same feature gap on all the other drivers except one.
Although I do not think that is a very good excuse. Lets figure out what we
should do about virtio.
Thanks,
John
If we can simply defer adjust_head patches to 4.11 then that's fine.
--
MST
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2017-01-25 14:47:58
On Wed, Jan 25, 2017 at 01:46:46PM +0800, Jason Wang wrote:
quoted
Then it would seem like a good thing to have another user of these paths and
find the bugs versus letting them sit there for some poor folks who do use
sleep/hybernate.
Yes, and uncovering hypervisor bugs now is better than uncovering it in the
future.
Thanks
Not really, all the uncovering should happen in -next or early rc.
Right now we need to fix what has been uncovered so far.
--
MST