| Message ID | 20200311123518.4025-6-yuri.benditovich@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | reference implementation of RSS and hash report | expand |
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. Yes this means we should allocate the indirection arrays on the fly. But that's probably a good idea anyway. > }; > -- > 2.17.1
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? 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. > Yes this means we should allocate the indirection arrays on the fly. > But that's probably a good idea anyway. > > > }; > > -- > > 2.17.1 > >
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. > > Yes this means we should allocate the indirection arrays on the fly. > But that's probably a good idea anyway. > > > }; > > -- > > 2.17.1 > >
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 > > > > > >
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 <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. 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 > > > > > >
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() }, };
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(+)