Re: [PATCH 1/2] virtio: reset region cache when on queue deletion

[Yuri Benditovich <yuri.benditovich@daynix.com>]

[-- Attachment #1: Type: text/plain, Size: 3516 bytes --]

Michael,
Can you please comment on Jason's question: why we have a problem only with
q35 and not with legacy pc?
If you have a simple answer, it will help us in further work with other hot
plug/unplug problems.

Thanks,
Yuri Benditovich

On Sun, Jan 5, 2020 at 6:21 PM Yuri Benditovich <yuri.benditovich@daynix.com>
wrote:

>
>
> On Sun, Jan 5, 2020 at 1:39 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
>> On Thu, Jan 02, 2020 at 09:09:04AM +0200, Yuri Benditovich wrote:
>> >
>> >
>> > On Thu, Jan 2, 2020 at 1:50 AM Michael S. Tsirkin <mst@redhat.com>
>> wrote:
>> >
>> >     On Thu, Dec 26, 2019 at 11:29:50AM +0200, Yuri Benditovich wrote:
>> >     > On Thu, Dec 26, 2019 at 10:58 AM Jason Wang <jasowang@redhat.com>
>> wrote:
>> >     > >
>> >     > >
>> >     > > On 2019/12/26 下午12:36, Yuri Benditovich wrote:
>> >     > > > https://bugzilla.redhat.com/show_bug.cgi?id=1708480
>> >     > > > Fix leak of region reference that prevents complete
>> >     > > > device deletion on hot unplug.
>> >     > >
>> >     > >
>> >     > > More information is needed here, the bug said only q35 can meet
>> this
>> >     > > issue. What makes q35 different here?
>> >     > >
>> >     >
>> >     > I do not have any ready answer, I did not dig into it too much.
>> >     > Probably Michael Tsirkin or Paolo Bonzini can answer without
>> digging.
>> >
>> >
>> >
>> >     > >
>> >     > > >
>> >     > > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
>> >     > > > ---
>> >     > > >   hw/virtio/virtio.c | 5 +++++
>> >     > > >   1 file changed, 5 insertions(+)
>> >     > > >
>> >     > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> >     > > > index 04716b5f6c..baadec8abc 100644
>> >     > > > --- a/hw/virtio/virtio.c
>> >     > > > +++ b/hw/virtio/virtio.c
>> >     > > > @@ -2340,6 +2340,11 @@ void virtio_del_queue(VirtIODevice
>> *vdev, int
>> >     n)
>> >     > > >       vdev->vq[n].vring.num_default = 0;
>> >     > > >       vdev->vq[n].handle_output = NULL;
>> >     > > >       vdev->vq[n].handle_aio_output = NULL;
>> >     > > > +    /*
>> >     > > > +     * with vring.num = 0 the queue will be ignored
>> >     > > > +     * in later loops of region cache reset
>> >     > > > +     */
>> >     > >
>> >     > >
>> >     > > I can't get the meaning of this comment.
>> >     > >
>> >     > > Thanks
>> >     > >
>> >     > >
>> >     > > > +    virtio_virtqueue_reset_region_cache(&vdev->vq[n]);
>> >
>> >
>> >     Do we need to drop this from virtio_device_free_virtqueues then?
>> >
>> >
>> >
>> > Not mandatory. Repetitive  virtio_virtqueue_reset_region_cache does not
>> do
>> > anything bad.
>> > Some of virtio devices do not do 'virtio_del_queue' at all. Currently
>> > virtio_device_free_virtqueues resets region cache for them.
>> > IMO, not calling 'virtio_del_queue' is a bug, but not in the scope of
>> current
>> > series, I'll take care of that later.
>>
>> Maybe we should just del all queues in virtio_device_unrealize?
>> Will allow us to drop some logic tracking which vqs were created.
>>
>>
> Yes, this is also possible with some rework of
> virtio_device_free_virtqueues.
> virtio-net has some additional operations around queue deletion, it
> deletes queues when switches from single queue to multiple.
>
>
>>
>> >
>> >     > > >       g_free(vdev->vq[n].used_elems);
>> >     > > >   }
>> >     > > >
>> >     > >
>> >
>> >
>>
>>

[-- Attachment #2: Type: text/html, Size: 5447 bytes --]