Message-ID: <586F0ED7.7060508@gmail.com>
Date:   Thu, 5 Jan 2017 19:28:23 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     Jason Wang <jasowang@...hat.com>, john.r.fastabend@...el.com,
        netdev@...r.kernel.org, alexei.starovoitov@...il.com,
        daniel@...earbox.net
Subject: 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:
>> On 17-01-03 02:16 PM, Michael S. Tsirkin wrote:
>>> On Tue, Jan 03, 2017 at 02:01:27PM +0800, Jason Wang wrote:
>>>>
>>>>
>>>> On 2017年01月03日 03:44, 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.
>>>>>
>>>>> : 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.
>>>
>>>
>>>> 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.

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

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

>>         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.

>>         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

Powered by blists - more mailing lists