Re: [PATCH v3 5/6] virtio-net: add migration support for RSS and hast report

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

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

On Wed, Mar 11, 2020 at 10:21 PM Michael S. Tsirkin <mst@redhat.com> wrote:

> On Wed, Mar 11, 2020 at 04:00:44PM +0200, Yuri Benditovich wrote:
> >
> >
> > On Wed, Mar 11, 2020 at 3:48 PM Michael S. Tsirkin <mst@redhat.com>
> wrote:
> >
> >     On Wed, Mar 11, 2020 at 02:35:17PM +0200, Yuri Benditovich wrote:
> >     > Save and restore RSS/hash report configuration.
> >     >
> >     > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> >     > ---
> >     >  hw/net/virtio-net.c | 9 +++++++++
> >     >  1 file changed, 9 insertions(+)
> >     >
> >     > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >     > index 7b6a929e8c..c8d97d45cd 100644
> >     > --- a/hw/net/virtio-net.c
> >     > +++ b/hw/net/virtio-net.c
> >     > @@ -2869,6 +2869,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();
> >     > +    }
> >     >      return 0;
> >     >  }
> >     >
> >     > @@ -3094,6 +3101,8 @@ static const VMStateDescription
> >     vmstate_virtio_net_device = {
> >     >                           vmstate_virtio_net_tx_waiting),
> >     >          VMSTATE_UINT64_TEST(curr_guest_offloads, VirtIONet,
> >     >                              has_ctrl_guest_offloads),
> >     > +        VMSTATE_UINT8_ARRAY(rss_data_migration, VirtIONet,
> >     > +                            sizeof(VirtioNetRssData)),
> >     >          VMSTATE_END_OF_LIST()
> >     >     },
> >
> >
> >     I think we should migrate the length too. Avoid arbitrary limits.
> >
> >
> > The length of what?
>
> Of the tables.
> > The structure is fixed-length and the intention is just to
> > keep/restore it.
> > The length of indirection table and the table itself are part of the
> structure.
>
>
> And that's a problem, because
> 1. we are wasting memory for a rarely used feature
> 2. if we want to make the table bigger, we'll need to break
>    migration compatibility
>
> Just allocate these dynamically as needed, and migrate length.
>

Unfortunately, this does not make things much better.
The maximum table size is 128, i.e we have persistent allocation of 256
bytes.
1. Addition of the code to make the allocation dynamic and migrate it will
eat most of this.
2. If we decide to change the maximum size if future, we anyway create
incompatibility. The driver asks what is maximum indirection table size at
the initialization time and the OS provides a table according to this. If
we migrate between two different implementations we find ourselves with
queue mask that is not compatible with maximum size. I'd rather add the
comment "do not change these numbers".
3. Size of key for Toeplitz is always 40

Please confirm you want to make it dynamic anyway





>
>
> >
> >     Yes this means we should allocate the indirection arrays on the fly.
> >     But that's probably a good idea anyway.
> >
> >     >  };
> >     > --
> >     > 2.17.1
> >
> >
>
>

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