From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58243) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bsEQG-00058m-CG for qemu-devel@nongnu.org; Thu, 06 Oct 2016 15:33:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bsEQD-0002cG-1V for qemu-devel@nongnu.org; Thu, 06 Oct 2016 15:33:40 -0400 Received: from mail-wm0-x243.google.com ([2a00:1450:400c:c09::243]:35522) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bsEQC-0002b1-H3 for qemu-devel@nongnu.org; Thu, 06 Oct 2016 15:33:36 -0400 Received: by mail-wm0-x243.google.com with SMTP id f193so4932186wmg.2 for ; Thu, 06 Oct 2016 12:33:35 -0700 (PDT) Mime-Version: 1.0 (1.0) From: Dmitry Fleytman In-Reply-To: <20161006154305.GB2091@work-vm> Date: Thu, 6 Oct 2016 21:33:29 +0200 Message-Id: <5EDC40F2-BDCE-4EAA-A78F-F233E1340288@daynix.com> References: <1473839464-8670-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1473839464-8670-9-git-send-email-caoj.fnst@cn.fujitsu.com> <87k2du7oo7.fsf@dusky.pond.sub.org> <57EE0CFB.4070002@cn.fujitsu.com> <878tu9ttzv.fsf@dusky.pond.sub.org> <017DDFF7-F80A-4C07-B533-4BEBEEA641CE@daynix.com> <20161006154305.GB2091@work-vm> Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 8/8] vmxnet3: remove unnecessary internal msix state flag List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" Cc: Markus Armbruster , Marcel Apfelbaum , Cao jin , "Michael S. Tsirkin" , Jason Wang , qemu-devel@nongnu.org > On 6 Oct 2016, at 17:43, Dr. David Alan Gilbert wrot= e: >=20 > * Dmitry Fleytman (dmitry@daynix.com) wrote: >>=20 >>> On 30 Sep 2016, at 15:08 PM, Markus Armbruster wrote= : >>>=20 >>> Cao jin writes: >>>=20 >>>>> On 09/29/2016 10:42 PM, Markus Armbruster wrote: >>>>> Cao jin writes: >>>>>=20 >>>>=20 >>>>>> static int vmxnet3_post_load(void *opaque, int version_id) >>>>>> { >>>>>> VMXNET3State *s =3D opaque; >>>>>> - PCIDevice *d =3D PCI_DEVICE(s); >>>>>>=20 >>>>>> net_tx_pkt_init(&s->tx_pkt, PCI_DEVICE(s), >>>>>> s->max_tx_frags, s->peer_has_vhdr); >>>>>> net_rx_pkt_init(&s->rx_pkt, s->peer_has_vhdr); >>>>>>=20 >>>>>> - if (s->msix_used) { >>>>>> - if (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) { >>>>>> - VMW_WRPRN("Failed to re-use MSI-X vectors"); >>>>>> - msix_uninit(d, &s->msix_bar, &s->msix_bar); >>>>>> - s->msix_used =3D false; >>>>>> - return -1; >>>>>> - } >>>>>> - } >>>>>> - >>>>>> vmxnet3_validate_queues(s); >>>>>> vmxnet3_validate_interrupts(s); >>>>>=20 >>>>> This hunk isn't obvious. Can you explain the change? >>>>>=20 >>>>=20 >>>> flag msix_used is used in VMStateDescription.post_Load(). >>>>=20 >>>> 1st, I think msix's code here is not necessary, because in >>>> destination, device has been realized before incoming migration, So I >>>> don't know why re-use MSI-X vectors here. Dmitry, could help to >>>> explain? >>>>=20 >>>> 2nd, this patch is going to remove this flag, so I removed the hunk. >>>=20 >>> We need to find out whether the call of vmxnet3_use_msix_vectors() is >>> necessary. I suspect it's not only not necessary, but actively wrong. >>>=20 >>> If that's true, removing becomes a bug fix that should be a separate >>> patch. >>>=20 >>> If it's only unnecessary, the removal may stay in this patch, but it >>> needs to be explained. Separate patch might be easier to explain. Your= >>> choice. >>>=20 >>> If it correct and necessary, then this patch needs to be changed not to >>> drop it. Instead, replace s->msix_used by msix_enabled(d) like you do >>> elsewhere. >>>=20 >>> Dmitry, can you help us find out? >>=20 >> Hello, >>=20 >> Yes, from what I see, this call is wrong and leads to >> reference leaks on device unload at migration target. >> It should be removed. >=20 > Talking of oddities in vmxnet3's msix load/save, > vmxnet3 has the honour of being the only device that > has both a register_savevm (which registers vmxnet3-msix) > and also has a ->vmsd entry (dc->vmsd =3D &vmstate_vmxnet3) >=20 > What's the history behind that? Is there some ordering requirement > etc about the order the two get loaded/saved? >=20 > Dave Hi Dave, There is no specific history behind that. Vmxnet3 code was written long time= ago and this part is just a legacy code that was not cleaned up as QEMU cod= e base evolved. ~Dmitry >=20 >> Best regards, >> Dmitry >>=20 >>=20 >>=20 > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK