| Message ID | 20200318091525.27044-8-yuri.benditovich@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | reference implementation of RSS and hash report | expand |
* 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(); > + } > 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. Dave > + .fields = (VMStateField[]) { > + VMSTATE_BOOL(enabled, VirtioNetRssData), > + VMSTATE_BOOL(redirect, VirtioNetRssData), > + VMSTATE_BOOL(populate_hash, VirtioNetRssData), > + VMSTATE_UINT32(hash_types, VirtioNetRssData), > + VMSTATE_UINT16(indirections_len, VirtioNetRssData), > + VMSTATE_UINT16(default_queue, VirtioNetRssData), > + VMSTATE_UINT8_ARRAY(key, VirtioNetRssData, > + VIRTIO_NET_RSS_MAX_KEY_SIZE), > + VMSTATE_VARRAY_UINT16_ALLOC(indirections_table, VirtioNetRssData, > + indirections_len, 0, > + vmstate_info_uint16, uint16_t), > + VMSTATE_END_OF_LIST() > + }, > +}; > + > static const VMStateDescription vmstate_virtio_net_device = { > .name = "virtio-net-device", > .version_id = VIRTIO_NET_VM_VERSION, > @@ -3067,6 +3092,7 @@ static const VMStateDescription vmstate_virtio_net_device = { > vmstate_virtio_net_tx_waiting), > VMSTATE_UINT64_TEST(curr_guest_offloads, VirtIONet, > has_ctrl_guest_offloads), > + VMSTATE_STRUCT(rss_data, VirtIONet, 1, vmstate_rss, VirtioNetRssData), > VMSTATE_END_OF_LIST() > }, > }; > -- > 2.17.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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(); > > + } > > 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? Another question about migration support: What is expected/required behavior? Possible cases: old qemu -> new qemu new qemu (new feature off) -> old qemu new qemu (new feature on) -> old qemu > Dave > > > > + .fields = (VMStateField[]) { > > + VMSTATE_BOOL(enabled, VirtioNetRssData), > > + VMSTATE_BOOL(redirect, VirtioNetRssData), > > + VMSTATE_BOOL(populate_hash, VirtioNetRssData), > > + VMSTATE_UINT32(hash_types, VirtioNetRssData), > > + VMSTATE_UINT16(indirections_len, VirtioNetRssData), > > + VMSTATE_UINT16(default_queue, VirtioNetRssData), > > + VMSTATE_UINT8_ARRAY(key, VirtioNetRssData, > > + VIRTIO_NET_RSS_MAX_KEY_SIZE), > > + VMSTATE_VARRAY_UINT16_ALLOC(indirections_table, > VirtioNetRssData, > > + indirections_len, 0, > > + vmstate_info_uint16, uint16_t), > > + VMSTATE_END_OF_LIST() > > + }, > > +}; > > + > > static const VMStateDescription vmstate_virtio_net_device = { > > .name = "virtio-net-device", > > .version_id = VIRTIO_NET_VM_VERSION, > > @@ -3067,6 +3092,7 @@ static const VMStateDescription > vmstate_virtio_net_device = { > > vmstate_virtio_net_tx_waiting), > > VMSTATE_UINT64_TEST(curr_guest_offloads, VirtIONet, > > has_ctrl_guest_offloads), > > + VMSTATE_STRUCT(rss_data, VirtIONet, 1, vmstate_rss, > VirtioNetRssData), > > VMSTATE_END_OF_LIST() > > }, > > }; > > -- > > 2.17.1 > > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > >
On Thu, Mar 19, 2020 at 07:19:26PM +0200, Yuri Benditovich 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(); > > + } > > 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? > > Another question about migration support: > What is expected/required behavior? > Possible cases: > old qemu -> new qemu > new qemu (new feature off) -> old qemu works > new qemu (new feature on) -> old qemu > fails gracefully > > Dave > > > > + .fields = (VMStateField[]) { > > + VMSTATE_BOOL(enabled, VirtioNetRssData), > > + VMSTATE_BOOL(redirect, VirtioNetRssData), > > + VMSTATE_BOOL(populate_hash, VirtioNetRssData), > > + VMSTATE_UINT32(hash_types, VirtioNetRssData), > > + VMSTATE_UINT16(indirections_len, VirtioNetRssData), > > + VMSTATE_UINT16(default_queue, VirtioNetRssData), > > + VMSTATE_UINT8_ARRAY(key, VirtioNetRssData, > > + VIRTIO_NET_RSS_MAX_KEY_SIZE), > > + VMSTATE_VARRAY_UINT16_ALLOC(indirections_table, > VirtioNetRssData, > > + indirections_len, 0, > > + vmstate_info_uint16, uint16_t), > > + VMSTATE_END_OF_LIST() > > + }, > > +}; > > + > > static const VMStateDescription vmstate_virtio_net_device = { > > .name = "virtio-net-device", > > .version_id = VIRTIO_NET_VM_VERSION, > > @@ -3067,6 +3092,7 @@ static const VMStateDescription > vmstate_virtio_net_device = { > > vmstate_virtio_net_tx_waiting), > > VMSTATE_UINT64_TEST(curr_guest_offloads, VirtIONet, > > has_ctrl_guest_offloads), > > + VMSTATE_STRUCT(rss_data, VirtIONet, 1, vmstate_rss, > VirtioNetRssData), > > VMSTATE_END_OF_LIST() > > }, > > }; > > -- > > 2.17.1 > > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > >
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.
* 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(); > > > + } > > > 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? > > Another question about migration support: > What is expected/required behavior? > Possible cases: > old qemu -> new qemu > new qemu (new feature off) -> old qemu Just works. Also be careful about the definition of 'new feature off'; normally we tie these things to machine types, so that with the old machine type the guest doesn't even see the feature; it can't turn it on. > new qemu (new feature on) -> old qemu Fails; hopefully nicely. Dave > > > > Dave > > > > > > > + .fields = (VMStateField[]) { > > > + VMSTATE_BOOL(enabled, VirtioNetRssData), > > > + VMSTATE_BOOL(redirect, VirtioNetRssData), > > > + VMSTATE_BOOL(populate_hash, VirtioNetRssData), > > > + VMSTATE_UINT32(hash_types, VirtioNetRssData), > > > + VMSTATE_UINT16(indirections_len, VirtioNetRssData), > > > + VMSTATE_UINT16(default_queue, VirtioNetRssData), > > > + VMSTATE_UINT8_ARRAY(key, VirtioNetRssData, > > > + VIRTIO_NET_RSS_MAX_KEY_SIZE), > > > + VMSTATE_VARRAY_UINT16_ALLOC(indirections_table, > > VirtioNetRssData, > > > + indirections_len, 0, > > > + vmstate_info_uint16, uint16_t), > > > + VMSTATE_END_OF_LIST() > > > + }, > > > +}; > > > + > > > static const VMStateDescription vmstate_virtio_net_device = { > > > .name = "virtio-net-device", > > > .version_id = VIRTIO_NET_VM_VERSION, > > > @@ -3067,6 +3092,7 @@ static const VMStateDescription > > vmstate_virtio_net_device = { > > > vmstate_virtio_net_tx_waiting), > > > VMSTATE_UINT64_TEST(curr_guest_offloads, VirtIONet, > > > has_ctrl_guest_offloads), > > > + VMSTATE_STRUCT(rss_data, VirtIONet, 1, vmstate_rss, > > VirtioNetRssData), > > > VMSTATE_END_OF_LIST() > > > }, > > > }; > > > -- > > > 2.17.1 > > > > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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(); + } return 0; } @@ -3019,6 +3026,24 @@ static const VMStateDescription vmstate_virtio_net_has_vnet = { }, }; +static const VMStateDescription vmstate_rss = { + .name = "vmstate_rss", + .fields = (VMStateField[]) { + VMSTATE_BOOL(enabled, VirtioNetRssData), + VMSTATE_BOOL(redirect, VirtioNetRssData), + VMSTATE_BOOL(populate_hash, VirtioNetRssData), + VMSTATE_UINT32(hash_types, VirtioNetRssData), + VMSTATE_UINT16(indirections_len, VirtioNetRssData), + VMSTATE_UINT16(default_queue, VirtioNetRssData), + VMSTATE_UINT8_ARRAY(key, VirtioNetRssData, + VIRTIO_NET_RSS_MAX_KEY_SIZE), + VMSTATE_VARRAY_UINT16_ALLOC(indirections_table, VirtioNetRssData, + indirections_len, 0, + vmstate_info_uint16, uint16_t), + VMSTATE_END_OF_LIST() + }, +}; + static const VMStateDescription vmstate_virtio_net_device = { .name = "virtio-net-device", .version_id = VIRTIO_NET_VM_VERSION, @@ -3067,6 +3092,7 @@ static const VMStateDescription vmstate_virtio_net_device = { vmstate_virtio_net_tx_waiting), VMSTATE_UINT64_TEST(curr_guest_offloads, VirtIONet, has_ctrl_guest_offloads), + VMSTATE_STRUCT(rss_data, VirtIONet, 1, vmstate_rss, 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 | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)