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

[Michael S. Tsirkin]

On Thu, Mar 12, 2020 at 09:37:06AM +0200, Yuri Benditovich wrote:
> 
> 
> On Wed, Mar 11, 2020 at 10:21 PM Michael S. Tsirkin <address@hidden> 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 <address@hidden>
>     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 <address@hidden>
>     >     > ---
>     >     >  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.

But that's shared between all VMs. Table is per VM.

> 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
> 

Let's just make the code future-proof for when we want to change it.

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