From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.5 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6083EC2D0DD for ; Thu, 2 Jan 2020 07:10:11 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 2647020672 for ; Thu, 2 Jan 2020 07:10:11 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=daynix-com.20150623.gappssmtp.com header.i=@daynix-com.20150623.gappssmtp.com header.b="YOeywBh+" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2647020672 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=daynix.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:37510 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1imucU-0002bU-7c for qemu-devel@archiver.kernel.org; Thu, 02 Jan 2020 02:10:10 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:59713) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1imubg-0002AI-JG for qemu-devel@nongnu.org; Thu, 02 Jan 2020 02:09:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1imubf-0005Of-58 for qemu-devel@nongnu.org; Thu, 02 Jan 2020 02:09:20 -0500 Received: from mail-io1-xd43.google.com ([2607:f8b0:4864:20::d43]:36357) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1imubf-0005OS-06 for qemu-devel@nongnu.org; Thu, 02 Jan 2020 02:09:19 -0500 Received: by mail-io1-xd43.google.com with SMTP id r13so27410033ioa.3 for ; Wed, 01 Jan 2020 23:09:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=daynix-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=pUfGEOZubPWzi7339q0Mwt55Ltq2QCLf9l7khOCJ7bk=; b=YOeywBh+DmzM0n9AU0FTQXA4quLhIoFFjuN4tkFZNkqARi/hZAhtuWis2IlsvBNC68 4HdNWTH00llx7atXqmSli65apY91QbEvCnlB0oD2orhbbJjlBSr/WOJXi0AuLccLEPIr LUONMZofcOZAFHhOlgcY4akY3YAEANPONojxyaQFBbMGmInR/F4kLJxkITSllbBBl2c0 SevOzoF6/95oNssdMYMFJPt3SWTkJllTcgTt3libY9j8JjV52slkqj5WJ4ssQZHBk52T YD3vqjsQueSBiWeRMijyzTZELyexo6d3VTqMZ+UkxnvNl+fsCjnOj2Cd1fgYzvweo8nQ g4ZQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=pUfGEOZubPWzi7339q0Mwt55Ltq2QCLf9l7khOCJ7bk=; b=loPVsMEBiyYboWlenOVmxFYaWWakL1gR4JIz0mSz2roByRI2EVQCzKAPgNzsx3eSKq Noo16AN+qdfisz9BxyGYha2ymkLWNpMN3At9BUlx/cWxCvANmHtyTW8TNQgzlcp2Ywfy dhJw/XajHyHYkbCyb2667ugmQgobQ6JASplbqJtJ8AElM2AHr9Z7Vyfej0iRrwzlnl+6 tfLxeVUTY+2Js9aR/FiVo5QCKKJ3WD3K27IjD0etiXYgYwRh5d1Y9qZD20N0sFFBW0h1 ji0kN9VFO9FX7Ln0ov3vnxIHgu86WVjxrjVzQEXBmGwoCMnVKxpxW7g4cuou08oYizGE SiUw== X-Gm-Message-State: APjAAAWifSKZpq/qJb6Kq2i/37vHTGB/1Uvmzwce2B4VvozGQr/gaPzR EZ0CnS3Qcz0bNnHd46y7MO/AQL7p70WiLt6CBppgFw== X-Google-Smtp-Source: APXvYqyO3HqVBpW61z1V4lXNpLbKRlIi/jzMtN5OTRIjgu2EeO/PyPQ+ky50gmZ2MbEUH0Sf9zCjCHOnJ5+ZJYAUwp0= X-Received: by 2002:a6b:ec0f:: with SMTP id c15mr50218412ioh.149.1577948958168; Wed, 01 Jan 2020 23:09:18 -0800 (PST) MIME-Version: 1.0 References: <20191226043649.14481-1-yuri.benditovich@daynix.com> <20191226043649.14481-2-yuri.benditovich@daynix.com> <05ead321-e93f-1b07-01cc-e0b023be8168@redhat.com> <20200101184425-mutt-send-email-mst@kernel.org> In-Reply-To: <20200101184425-mutt-send-email-mst@kernel.org> From: Yuri Benditovich Date: Thu, 2 Jan 2020 09:09:04 +0200 Message-ID: Subject: Re: [PATCH 1/2] virtio: reset region cache when on queue deletion To: "Michael S. Tsirkin" Content-Type: multipart/alternative; boundary="000000000000202d4c059b22df16" X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2607:f8b0:4864:20::d43 X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Yan Vugenfirer , pbonzini@redhat.com, Jason Wang , qemu-devel@nongnu.org Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --000000000000202d4c059b22df16 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, Jan 2, 2020 at 1:50 AM Michael S. Tsirkin 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 wrote= : > > > > > > > > > On 2019/12/26 =E4=B8=8B=E5=8D=8812:36, Yuri Benditovich wrote: > > > > https://bugzilla.redhat.com/show_bug.cgi?id=3D1708480 > > > > 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 > > > > --- > > > > 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, in= t > n) > > > > vdev->vq[n].vring.num_default =3D 0; > > > > vdev->vq[n].handle_output =3D NULL; > > > > vdev->vq[n].handle_aio_output =3D NULL; > > > > + /* > > > > + * with vring.num =3D 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. > > > > g_free(vdev->vq[n].used_elems); > > > > } > > > > > > > > > --000000000000202d4c059b22df16 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Thu, Jan 2, 2020 at 1:50 AM Michae= l S. Tsirkin <mst@redhat.com> w= rote:
On Thu, De= c 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 =E4=B8=8B=E5=8D=8812:36, Yuri Benditovich wrote: > > > https://bugzilla.redhat.com/show= _bug.cgi?id=3D1708480
> > > 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 t= his
> > 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.<= br>


> >
> > >
> > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>= ;
> > > ---
> > >=C2=A0 =C2=A0hw/virtio/virtio.c | 5 +++++
> > >=C2=A0 =C2=A01 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 *v= dev, int n)
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0vdev->vq[n].vring.num_default = =3D 0;
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0vdev->vq[n].handle_output =3D N= ULL;
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0vdev->vq[n].handle_aio_output = =3D NULL;
> > > +=C2=A0 =C2=A0 /*
> > > +=C2=A0 =C2=A0 =C2=A0* with vring.num =3D 0 the queue will b= e ignored
> > > +=C2=A0 =C2=A0 =C2=A0* in later loops of region cache reset<= br> > > > +=C2=A0 =C2=A0 =C2=A0*/
> >
> >
> > I can't get the meaning of this comment.
> >
> > Thanks
> >
> >
> > > +=C2=A0 =C2=A0 virtio_virtqueue_reset_region_cache(&vdev= ->vq[n]);


Do we need to drop this from virtio_device_free_virtqueues then?


Not mandatory. Repetitive=C2=A0 virtio_virtqueue_reset_region_cache=C2=A0does not do anything bad.
Some of virtio devices do not do 'virtio_del_queue' at all. Curre= ntly=C2=A0 virtio_device_free_virtqueues resets region cache for them.
IMO, = not calling=20 'virtio_del_queue' is a bug, but not in the scope of current series= , I'll take care of that later.
=C2=A0
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0g_free(vdev->vq[n].used_elems);=
> > >=C2=A0 =C2=A0}
> > >
> >

--000000000000202d4c059b22df16--