Re: [Qemu-devel] [PATCH v3 8/8] vmxnet3: remove unnecessary internal msix state flag

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Dmitry Fleytman (dmitry@daynix.com) wrote:
> 
> > On 30 Sep 2016, at 15:08 PM, Markus Armbruster <armbru@redhat.com> wrote:
> > 
> > Cao jin <caoj.fnst@cn.fujitsu.com> writes:
> > 
> >> On 09/29/2016 10:42 PM, Markus Armbruster wrote:
> >>> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
> >>> 
> >> 
> >>>>  static int vmxnet3_post_load(void *opaque, int version_id)
> >>>>  {
> >>>>      VMXNET3State *s = opaque;
> >>>> -    PCIDevice *d = PCI_DEVICE(s);
> >>>> 
> >>>>      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);
> >>>> 
> >>>> -    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 = false;
> >>>> -            return -1;
> >>>> -        }
> >>>> -    }
> >>>> -
> >>>>      vmxnet3_validate_queues(s);
> >>>>      vmxnet3_validate_interrupts(s);
> >>> 
> >>> This hunk isn't obvious.  Can you explain the change?
> >>> 
> >> 
> >> flag msix_used is used in VMStateDescription.post_Load().
> >> 
> >> 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?
> >> 
> >> 2nd, this patch is going to remove this flag, so I removed the hunk.
> > 
> > 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.
> > 
> > If that's true, removing becomes a bug fix that should be a separate
> > patch.
> > 
> > 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.
> > 
> > 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.
> > 
> > Dmitry, can you help us find out?
> 
> Hello,
> 
> Yes, from what I see, this call is wrong and leads to
> reference leaks on device unload at migration target.
> It should be removed.

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 = &vmstate_vmxnet3)

What's the history behind that? Is there some ordering requirement
etc about the order the two get loaded/saved?

Dave

> Best regards,
> Dmitry
> 
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK