Re: [RFC PATCH] virtio_net: XDP support for adjust_head

On 17-01-05 04:39 PM, Michael S. Tsirkin wrote:

On Thu, Jan 05, 2017 at 02:57:23PM -0800, John Fastabend wrote:

quoted

On 17-01-03 02:16 PM, Michael S. Tsirkin wrote:

quoted

On Tue, Jan 03, 2017 at 02:01:27PM +0800, Jason Wang wrote:

quoted

On 2017年01月03日 03:44, 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.

: There is a problem with this patch as is. When xdp prog is loaded
   the old buffers without the 256B headers need to be flushed so that
   the bpf prog has the necessary headroom. This patch does this by
   calling the virtqueue_detach_unused_buf() and followed by the
   virtnet_set_queues() call to reinitialize the buffers. However I
   don't believe this is safe per comment in virtio_ring this API
   is not valid on an active queue and the only thing we have done
   here is napi_disable/napi_enable wrappers which doesn't do anything
   to the emulation layer.

   So the RFC is really to find the best solution to this problem.
   A couple things come to mind, (a) always allocate the necessary
   headroom but this is a bit of a waste (b) add some bit somewhere
   to check if the buffer has headroom but this would mean XDP programs
   would be broke for a cycle through the ring, (c) figure out how
   to deactivate a queue, free the buffers and finally reallocate.
   I think (c) is the best choice for now but I'm not seeing the
   API to do this so virtio/qemu experts anyone know off-hand
   how to make this work? I started looking into the PCI callbacks
   reset() and virtio_device_ready() or possibly hitting the right
   set of bits with vp_set_status() but my first attempt just hung
   the device.

Hi John:

AFAIK, disabling a specific queue was supported only by virtio 1.0 through
queue_enable field in pci common cfg.

In fact 1.0 only allows enabling queues selectively.
We can add disabling by a spec enhancement but
for now reset is the only way.

quoted

But unfortunately, qemu does not
emulate this at all and legacy device does not even support this. So the
safe way is probably reset the device and redo the initialization here.

You will also have to re-apply rx filtering if you do this.
Probably sending notification uplink.

The following seems to hang the device on the next virtnet_send_command()
I expected this to meet the reset requirements from the spec because I
believe its the same flow coming out of restore(). For a real patch we
don't actually need to kfree all the structs and reallocate them but
I was expecting the below to work. Any ideas/hints?

Restore assumes device was previously reset.
You want to combine freeze+restore.

Yep the below is actually freeze+restore I just omitted the freeze portion
from the description.

quoted

static int virtnet_xdp_reset(struct virtnet_info *vi)
{
        int i, ret;

//  insert flush_work here doesn't seem to help hang.

quoted

        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);
        }

        remove_vq_common(vi, false);

// everything above is freeze sans flush_work and virtnet_cpu_notif_remove
// the rest below is restore where I call virtnet_set_queues later outside
// the reset function.

quoted

        ret = init_vqs(vi);
        if (ret)
                return ret;
        virtio_device_ready(vi->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 0;
}

Could be a locking problem I'm missing so I'll look at it a bit more but
good to know we expect freeze/restore to reset the device.

.John