Re: [net PATCH v3 5/5] virtio_net: XDP support for adjust_head

On 17-01-13 12:08 PM, John Fastabend wrote:

On 17-01-12 11:41 PM, Jason Wang wrote:

quoted

On 2017年01月13日 10:52, 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>
---
  drivers/net/virtio_net.c |  155 ++++++++++++++++++++++++++++++++++++++++------
  drivers/virtio/virtio.c  |    9 ++-
  include/linux/virtio.h   |    3 +
  3 files changed, 144 insertions(+), 23 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6041828..8b897e7 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -28,6 +28,7 @@
  #include <linux/slab.h>
  #include <linux/cpu.h>
  #include <linux/average.h>
+#include <linux/pci.h>
  #include <net/busy_poll.h>
    static int napi_weight = NAPI_POLL_WEIGHT;

@@ -159,6 +160,9 @@ struct virtnet_info {
      /* Ethtool settings */
      u8 duplex;
      u32 speed;
+
+    /* Headroom allocated in RX Queue */
+    unsigned int headroom;

If this could not be changed in anyway, better use a macro instead of a filed
here. And there's even no need to add an extra parameter to
add_recvbuf_mergeable().

OK originally I thought this might be dynamic but I agree no need
for it here.

Well there is a bit of an order of operation issue that means we need at
least some bit here to tell us an enablement is pending.

The problem is when we do the reset we need to know that headroom for XDP
is needed. But we can't use the xdp_prog values because xdp_prog can not
be added on an device that is up without headroom otherwise the program
could fail. Plus reset via freeze/restore tears these structures down and
rebuilds them.

How about a boolean bit here instead of an unsigned int,

	'bool xdp_headroom_needed'

seems better than an int.

Thanks,
John