[PATCH v5 6/7] vmstate.h: provide VMSTATE_VARRAY_UINT16_ALLOC macro

Yuri Benditovich posted 7 patches 5 years, 6 months ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Juan Quintela <quintela@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Jason Wang <jasowang@redhat.com>
There is a newer version of this series
[PATCH v5 6/7] vmstate.h: provide VMSTATE_VARRAY_UINT16_ALLOC macro
Posted by Yuri Benditovich 5 years, 6 months ago
Similar to VMSTATE_VARRAY_UINT32_ALLOC, but the size is
16-bit field.

Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
---
 include/migration/vmstate.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 30667631bc..baaefb6b9b 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -432,6 +432,16 @@ extern const VMStateInfo vmstate_info_qlist;
     .offset     = vmstate_offset_pointer(_state, _field, _type),     \
 }
 
+#define VMSTATE_VARRAY_UINT16_ALLOC(_field, _state, _field_num, _version, _info, _type) {\
+    .name       = (stringify(_field)),                               \
+    .version_id = (_version),                                        \
+    .num_offset = vmstate_offset_value(_state, _field_num, uint16_t),\
+    .info       = &(_info),                                          \
+    .size       = sizeof(_type),                                     \
+    .flags      = VMS_VARRAY_UINT16 | VMS_POINTER | VMS_ALLOC,       \
+    .offset     = vmstate_offset_pointer(_state, _field, _type),     \
+}
+
 #define VMSTATE_VARRAY_UINT16_UNSAFE(_field, _state, _field_num, _version, _info, _type) {\
     .name       = (stringify(_field)),                               \
     .version_id = (_version),                                        \
-- 
2.17.1


Re: [PATCH v5 6/7] vmstate.h: provide VMSTATE_VARRAY_UINT16_ALLOC macro
Posted by Michael S. Tsirkin 5 years, 6 months ago
On Wed, Mar 18, 2020 at 11:15:24AM +0200, Yuri Benditovich wrote:
> Similar to VMSTATE_VARRAY_UINT32_ALLOC, but the size is
> 16-bit field.
> 
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>

Hmm this is exactly my patch isn't it? If yes pls fix up attribution
(if this is not reposted, then when applying):

From: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>


> ---
>  include/migration/vmstate.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 30667631bc..baaefb6b9b 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -432,6 +432,16 @@ extern const VMStateInfo vmstate_info_qlist;
>      .offset     = vmstate_offset_pointer(_state, _field, _type),     \
>  }
>  
> +#define VMSTATE_VARRAY_UINT16_ALLOC(_field, _state, _field_num, _version, _info, _type) {\
> +    .name       = (stringify(_field)),                               \
> +    .version_id = (_version),                                        \
> +    .num_offset = vmstate_offset_value(_state, _field_num, uint16_t),\
> +    .info       = &(_info),                                          \
> +    .size       = sizeof(_type),                                     \
> +    .flags      = VMS_VARRAY_UINT16 | VMS_POINTER | VMS_ALLOC,       \
> +    .offset     = vmstate_offset_pointer(_state, _field, _type),     \
> +}
> +
>  #define VMSTATE_VARRAY_UINT16_UNSAFE(_field, _state, _field_num, _version, _info, _type) {\
>      .name       = (stringify(_field)),                               \
>      .version_id = (_version),                                        \
> -- 
> 2.17.1


Re: [PATCH v5 6/7] vmstate.h: provide VMSTATE_VARRAY_UINT16_ALLOC macro
Posted by Yuri Benditovich 5 years, 6 months ago
On Wed, Mar 18, 2020 at 11:42 AM Michael S. Tsirkin <mst@redhat.com> wrote:

> On Wed, Mar 18, 2020 at 11:15:24AM +0200, Yuri Benditovich wrote:
> > Similar to VMSTATE_VARRAY_UINT32_ALLOC, but the size is
> > 16-bit field.
> >
> > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
>
> Hmm this is exactly my patch isn't it? If yes pls fix up attribution
> (if this is not reposted, then when applying):
>

Of course, it is similar to the one you wrote inline.
Unlike one you wrote inline this patch does not fail on checkpatch.
But the idea is the same, hard to invent something.
Please just let me know what exactly should I do: resubmit or not and
whether it is possible to fix it without resubmission.


>
> From: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
>
> > ---
> >  include/migration/vmstate.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > index 30667631bc..baaefb6b9b 100644
> > --- a/include/migration/vmstate.h
> > +++ b/include/migration/vmstate.h
> > @@ -432,6 +432,16 @@ extern const VMStateInfo vmstate_info_qlist;
> >      .offset     = vmstate_offset_pointer(_state, _field, _type),     \
> >  }
> >
> > +#define VMSTATE_VARRAY_UINT16_ALLOC(_field, _state, _field_num,
> _version, _info, _type) {\
> > +    .name       = (stringify(_field)),                               \
> > +    .version_id = (_version),                                        \
> > +    .num_offset = vmstate_offset_value(_state, _field_num, uint16_t),\
> > +    .info       = &(_info),                                          \
> > +    .size       = sizeof(_type),                                     \
> > +    .flags      = VMS_VARRAY_UINT16 | VMS_POINTER | VMS_ALLOC,       \
> > +    .offset     = vmstate_offset_pointer(_state, _field, _type),     \
> > +}
> > +
> >  #define VMSTATE_VARRAY_UINT16_UNSAFE(_field, _state, _field_num,
> _version, _info, _type) {\
> >      .name       = (stringify(_field)),                               \
> >      .version_id = (_version),                                        \
> > --
> > 2.17.1
>
>
Re: [PATCH v5 6/7] vmstate.h: provide VMSTATE_VARRAY_UINT16_ALLOC macro
Posted by Michael S. Tsirkin 5 years, 6 months ago
On Thu, Mar 19, 2020 at 07:12:20PM +0200, Yuri Benditovich wrote:
> 
> 
> On Wed, Mar 18, 2020 at 11:42 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> 
>     On Wed, Mar 18, 2020 at 11:15:24AM +0200, Yuri Benditovich wrote:
>     > Similar to VMSTATE_VARRAY_UINT32_ALLOC, but the size is
>     > 16-bit field.
>     >
>     > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> 
>     Hmm this is exactly my patch isn't it? If yes pls fix up attribution
>     (if this is not reposted, then when applying):
> 
> 
> Of course, it is similar to the one you wrote inline.
> Unlike one you wrote inline this patch does not fail on checkpatch.

If you feel you modified it significantly enough, you can write
"based on a patch by mst".

> But the idea is the same, hard to invent something.
> Please just let me know what exactly should I do: resubmit or not and whether
> it is possible to fix it without resubmission.
>  
> 
> 
>     From: Michael S. Tsirkin <mst@redhat.com>
>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> 
>     > ---
>     >  include/migration/vmstate.h | 10 ++++++++++
>     >  1 file changed, 10 insertions(+)
>     >
>     > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>     > index 30667631bc..baaefb6b9b 100644
>     > --- a/include/migration/vmstate.h
>     > +++ b/include/migration/vmstate.h
>     > @@ -432,6 +432,16 @@ extern const VMStateInfo vmstate_info_qlist;
>     >      .offset     = vmstate_offset_pointer(_state, _field, _type),     \
>     >  }
>     > 
>     > +#define VMSTATE_VARRAY_UINT16_ALLOC(_field, _state, _field_num,
>     _version, _info, _type) {\
>     > +    .name       = (stringify(_field)),                               \
>     > +    .version_id = (_version),                                        \
>     > +    .num_offset = vmstate_offset_value(_state, _field_num, uint16_t),\
>     > +    .info       = &(_info),                                          \
>     > +    .size       = sizeof(_type),                                     \
>     > +    .flags      = VMS_VARRAY_UINT16 | VMS_POINTER | VMS_ALLOC,       \
>     > +    .offset     = vmstate_offset_pointer(_state, _field, _type),     \
>     > +}
>     > +
>     >  #define VMSTATE_VARRAY_UINT16_UNSAFE(_field, _state, _field_num,
>     _version, _info, _type) {\
>     >      .name       = (stringify(_field)),                               \
>     >      .version_id = (_version),                                        \
>     > --
>     > 2.17.1
> 
> 


Re: [PATCH v5 6/7] vmstate.h: provide VMSTATE_VARRAY_UINT16_ALLOC macro
Posted by Dr. David Alan Gilbert 5 years, 6 months ago
* Yuri Benditovich (yuri.benditovich@daynix.com) wrote:
> Similar to VMSTATE_VARRAY_UINT32_ALLOC, but the size is
> 16-bit field.
> 
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> ---
>  include/migration/vmstate.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 30667631bc..baaefb6b9b 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -432,6 +432,16 @@ extern const VMStateInfo vmstate_info_qlist;
>      .offset     = vmstate_offset_pointer(_state, _field, _type),     \
>  }
>  
> +#define VMSTATE_VARRAY_UINT16_ALLOC(_field, _state, _field_num, _version, _info, _type) {\
> +    .name       = (stringify(_field)),                               \
> +    .version_id = (_version),                                        \
> +    .num_offset = vmstate_offset_value(_state, _field_num, uint16_t),\
> +    .info       = &(_info),                                          \
> +    .size       = sizeof(_type),                                     \
> +    .flags      = VMS_VARRAY_UINT16 | VMS_POINTER | VMS_ALLOC,       \
> +    .offset     = vmstate_offset_pointer(_state, _field, _type),     \
> +}
> +

Once you agree attribution with mst,

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

>  #define VMSTATE_VARRAY_UINT16_UNSAFE(_field, _state, _field_num, _version, _info, _type) {\
>      .name       = (stringify(_field)),                               \
>      .version_id = (_version),                                        \
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


Re: [PATCH v5 6/7] vmstate.h: provide VMSTATE_VARRAY_UINT16_ALLOC macro
Posted by Juan Quintela 5 years, 6 months ago
Yuri Benditovich <yuri.benditovich@daynix.com> wrote:
> Similar to VMSTATE_VARRAY_UINT32_ALLOC, but the size is
> 16-bit field.
>
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Same caveat about attribution to MST.

Once told tha, I don't understand why you are using a unit16_t.
You define indirections_len as:

+    uint16_t indirections_len;

But its maximum value right now is:

+#define VIRTIO_NET_RSS_MAX_TABLE_LEN    128

So, are we planning to increase that value in the next future, or we
just want to give enough space?

Later, Juan.


Re: [PATCH v5 6/7] vmstate.h: provide VMSTATE_VARRAY_UINT16_ALLOC macro
Posted by Michael S. Tsirkin 5 years, 6 months ago
On Wed, Mar 18, 2020 at 02:02:37PM +0100, Juan Quintela wrote:
> Yuri Benditovich <yuri.benditovich@daynix.com> wrote:
> > Similar to VMSTATE_VARRAY_UINT32_ALLOC, but the size is
> > 16-bit field.
> >
> > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> Same caveat about attribution to MST.
> 
> Once told tha, I don't understand why you are using a unit16_t.
> You define indirections_len as:
> 
> +    uint16_t indirections_len;
> 
> But its maximum value right now is:
> 
> +#define VIRTIO_NET_RSS_MAX_TABLE_LEN    128
> 
> So, are we planning to increase that value in the next future, or we
> just want to give enough space?
> 
> Later, Juan.

The max size according to spec is u16. Using that limits
the size and makes it forward compatible.