Re: [PATCH v5 7/7] virtio-net: add migration support for RSS and hash report

[Juan Quintela <quintela@redhat.com>]

Hi Yuri

Yuri Benditovich <yuri.benditovich@daynix.com> wrote:
> On Wed, Mar 18, 2020 at 12:48 PM Dr. David Alan Gilbert <dgilbert@redhat.com>
> wrote:
>
>  * Yuri Benditovich (yuri.benditovich@daynix.com) wrote:
>  > Save and restore RSS/hash report configuration.
>  > 
>  > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
>  > ---
>  >  hw/net/virtio-net.c | 26 ++++++++++++++++++++++++++
>  >  1 file changed, 26 insertions(+)
>  > 
>  > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>  > index a0614ad4e6..0b058aae9f 100644
>  > --- a/hw/net/virtio-net.c
>  > +++ b/hw/net/virtio-net.c
>  > @@ -2842,6 +2842,13 @@ static int virtio_net_post_load_device(void
>  *opaque, int version_id)
>  >          }
>  >      }
>  >  
>  > +    if (n->rss_data.enabled) {
>  > +        trace_virtio_net_rss_enable(n->rss_data.hash_types,
>  > +                                    n->rss_data.indirections_len,
>  > +                                    sizeof(n->rss_data.key));
>  > +    } else {
>  > +        trace_virtio_net_rss_disable();
>  > +    }

This is the bigger "abuser" that I have ever seen for a post_load
function.  Just to add a trace depending on a value O:-)

>  >      return 0;
>  >  }
>  >  
>  > @@ -3019,6 +3026,24 @@ static const VMStateDescription
>  vmstate_virtio_net_has_vnet = {
>  >      },
>  >  };
>  >  
>  > +static const VMStateDescription vmstate_rss = {
>  > +    .name      = "vmstate_rss",
>
>  You need to do something to avoid breaking migration compatibility
>  from/to old QEMU's and from/to QEMU's on hosts without the new virtio
>  features.
>  Probably adding a .needed =   here pointing to a function that
>  checks 'enabled' might do it.
>
> Does VMSTATE_STRUCT_TEST(..,..,checker_procedure,...) result the same thing?

It is just a similar thing, not the same.
If you add a new field, you need to increase the version number.  And
that make backward compatibility really annoying.
With subsections, you can make it work correctly with old versions
always that you don't use rss.

> Another question about migration support:
> What is expected/required behavior?
> Possible cases:
> old qemu -> new qemu

That should always work.
If you use an optional subsection this works for free.  Old qemu has no
rss subsection.

> new qemu (new feature off) -> old qemu

This is desirable.  And with the optional subsection it just works, no
chang eneeded.

> new qemu (new feature on) -> old qemu

This obviosly will not work, and we are fine.  There will appear a new
subsection that old qemu don't undertand.  Destination will give one
error and give up.

For one example, just look at something like:

hw/virtio/virtio.c:: vmstate_virtio

There are lots of subscitnsios there.

Later, Juan.