[Qemu-devel] [PATCH v2] vhost-user: pass message as a pointer to process_message_reply()

Maxime Coquelin posted 1 patch 6 years, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170524090520.321-1-maxime.coquelin@redhat.com
Test checkpatch passed
Test docker passed
Test s390x passed
hw/virtio/vhost-user.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
[Qemu-devel] [PATCH v2] vhost-user: pass message as a pointer to process_message_reply()
Posted by Maxime Coquelin 6 years, 10 months ago
process_message_reply() was recently updated to get full message
content instead of only its request field.

There is no need to copy all the struct content into the stack,
so just pass its pointer as const.

Cc: Zhiyong Yang <zhiyong.yang@intel.com>
Fixes: 60cd11024f41 ("hw/virtio: fix vhost user fails to startup when MQ")
Reviewed-by: Jens Freimann <jfreiman@redhat.com>
Reviewed-by: Zhiyong Yang <zhiyong.yang@intel.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
V2:
 - Make msg pointer const (Marc-Andre)
 - Apply R-b's

 hw/virtio/vhost-user.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index b87a176..dde094a 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -162,11 +162,11 @@ fail:
 }
 
 static int process_message_reply(struct vhost_dev *dev,
-                                 VhostUserMsg msg)
+                                 const VhostUserMsg *msg)
 {
     VhostUserMsg msg_reply;
 
-    if ((msg.flags & VHOST_USER_NEED_REPLY_MASK) == 0) {
+    if ((msg->flags & VHOST_USER_NEED_REPLY_MASK) == 0) {
         return 0;
     }
 
@@ -174,10 +174,10 @@ static int process_message_reply(struct vhost_dev *dev,
         return -1;
     }
 
-    if (msg_reply.request != msg.request) {
+    if (msg_reply.request != msg->request) {
         error_report("Received unexpected msg type."
                      "Expected %d received %d",
-                     msg.request, msg_reply.request);
+                     msg->request, msg_reply.request);
         return -1;
     }
 
@@ -324,7 +324,7 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
     }
 
     if (reply_supported) {
-        return process_message_reply(dev, msg);
+        return process_message_reply(dev, &msg);
     }
 
     return 0;
@@ -716,7 +716,7 @@ static int vhost_user_net_set_mtu(struct vhost_dev *dev, uint16_t mtu)
 
     /* If reply_ack supported, slave has to ack specified MTU is valid */
     if (reply_supported) {
-        return process_message_reply(dev, msg);
+        return process_message_reply(dev, &msg);
     }
 
     return 0;
-- 
2.9.4


Re: [Qemu-devel] [PATCH v2] vhost-user: pass message as a pointer to process_message_reply()
Posted by Marc-André Lureau 6 years, 10 months ago
Hi

On Wed, May 24, 2017 at 1:06 PM Maxime Coquelin <maxime.coquelin@redhat.com>
wrote:

> process_message_reply() was recently updated to get full message
> content instead of only its request field.
>
> There is no need to copy all the struct content into the stack,
> so just pass its pointer as const.
>
> Cc: Zhiyong Yang <zhiyong.yang@intel.com>
> Fixes: 60cd11024f41 ("hw/virtio: fix vhost user fails to startup when MQ")
> Reviewed-by: Jens Freimann <jfreiman@redhat.com>
> Reviewed-by: Zhiyong Yang <zhiyong.yang@intel.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
> V2:
>  - Make msg pointer const (Marc-Andre)
>  - Apply R-b's
>
>  hw/virtio/vhost-user.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index b87a176..dde094a 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -162,11 +162,11 @@ fail:
>  }
>
>  static int process_message_reply(struct vhost_dev *dev,
> -                                 VhostUserMsg msg)
> +                                 const VhostUserMsg *msg)
>  {
>      VhostUserMsg msg_reply;
>
> -    if ((msg.flags & VHOST_USER_NEED_REPLY_MASK) == 0) {
> +    if ((msg->flags & VHOST_USER_NEED_REPLY_MASK) == 0) {
>          return 0;
>      }
>
> @@ -174,10 +174,10 @@ static int process_message_reply(struct vhost_dev
> *dev,
>          return -1;
>      }
>
> -    if (msg_reply.request != msg.request) {
> +    if (msg_reply.request != msg->request) {
>          error_report("Received unexpected msg type."
>                       "Expected %d received %d",
> -                     msg.request, msg_reply.request);
> +                     msg->request, msg_reply.request);
>          return -1;
>      }
>
> @@ -324,7 +324,7 @@ static int vhost_user_set_mem_table(struct vhost_dev
> *dev,
>      }
>
>      if (reply_supported) {
> -        return process_message_reply(dev, msg);
> +        return process_message_reply(dev, &msg);
>      }
>
>      return 0;
> @@ -716,7 +716,7 @@ static int vhost_user_net_set_mtu(struct vhost_dev
> *dev, uint16_t mtu)
>
>      /* If reply_ack supported, slave has to ack specified MTU is valid */
>      if (reply_supported) {
> -        return process_message_reply(dev, msg);
> +        return process_message_reply(dev, &msg);
>      }
>
>      return 0;
> --
> 2.9.4
>
>
> --
Marc-André Lureau
Re: [Qemu-devel] [PATCH v2] vhost-user: pass message as a pointer to process_message_reply()
Posted by Michael S. Tsirkin 6 years, 10 months ago
On Wed, May 24, 2017 at 11:05:20AM +0200, Maxime Coquelin wrote:
> process_message_reply() was recently updated to get full message
> content instead of only its request field.
> 
> There is no need to copy all the struct content into the stack,
> so just pass its pointer as const.
> 
> Cc: Zhiyong Yang <zhiyong.yang@intel.com>
> Fixes: 60cd11024f41 ("hw/virtio: fix vhost user fails to startup when MQ")
> Reviewed-by: Jens Freimann <jfreiman@redhat.com>
> Reviewed-by: Zhiyong Yang <zhiyong.yang@intel.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Why "Fixes"? It's not a bugfix, is it? Passing a pointer is
slightly cleaner but it's not a big deal IMHO. I'll apply
but would like to get clarification on this tag.

> ---
> V2:
>  - Make msg pointer const (Marc-Andre)
>  - Apply R-b's
> 
>  hw/virtio/vhost-user.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index b87a176..dde094a 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -162,11 +162,11 @@ fail:
>  }
>  
>  static int process_message_reply(struct vhost_dev *dev,
> -                                 VhostUserMsg msg)
> +                                 const VhostUserMsg *msg)
>  {
>      VhostUserMsg msg_reply;
>  
> -    if ((msg.flags & VHOST_USER_NEED_REPLY_MASK) == 0) {
> +    if ((msg->flags & VHOST_USER_NEED_REPLY_MASK) == 0) {
>          return 0;
>      }
>  
> @@ -174,10 +174,10 @@ static int process_message_reply(struct vhost_dev *dev,
>          return -1;
>      }
>  
> -    if (msg_reply.request != msg.request) {
> +    if (msg_reply.request != msg->request) {
>          error_report("Received unexpected msg type."
>                       "Expected %d received %d",
> -                     msg.request, msg_reply.request);
> +                     msg->request, msg_reply.request);
>          return -1;
>      }
>  
> @@ -324,7 +324,7 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
>      }
>  
>      if (reply_supported) {
> -        return process_message_reply(dev, msg);
> +        return process_message_reply(dev, &msg);
>      }
>  
>      return 0;
> @@ -716,7 +716,7 @@ static int vhost_user_net_set_mtu(struct vhost_dev *dev, uint16_t mtu)
>  
>      /* If reply_ack supported, slave has to ack specified MTU is valid */
>      if (reply_supported) {
> -        return process_message_reply(dev, msg);
> +        return process_message_reply(dev, &msg);
>      }
>  
>      return 0;
> -- 
> 2.9.4

Re: [Qemu-devel] [PATCH v2] vhost-user: pass message as a pointer to process_message_reply()
Posted by Maxime Coquelin 6 years, 10 months ago

On 05/24/2017 11:58 PM, Michael S. Tsirkin wrote:
> On Wed, May 24, 2017 at 11:05:20AM +0200, Maxime Coquelin wrote:
>> process_message_reply() was recently updated to get full message
>> content instead of only its request field.
>>
>> There is no need to copy all the struct content into the stack,
>> so just pass its pointer as const.
>>
>> Cc: Zhiyong Yang <zhiyong.yang@intel.com>
>> Fixes: 60cd11024f41 ("hw/virtio: fix vhost user fails to startup when MQ")
>> Reviewed-by: Jens Freimann <jfreiman@redhat.com>
>> Reviewed-by: Zhiyong Yang <zhiyong.yang@intel.com>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Why "Fixes"? It's not a bugfix, is it? Passing a pointer is
> slightly cleaner but it's not a big deal IMHO. I'll apply
> but would like to get clarification on this tag.

Right, this is not a bug fix.
I noticed this while rebasing my Vhost-user IOMMU series,
which will call this function much more often than currently.
That said, I haven't done any measurements, and I don't believe it
will have a noticeable impact.

Feel free to remove the "Fixes:" line when applying, or I can resend
if you prefer.

Maxime

Re: [Qemu-devel] [PATCH v2] vhost-user: pass message as a pointer to process_message_reply()
Posted by Michael S. Tsirkin 6 years, 10 months ago
On Thu, May 25, 2017 at 05:57:43PM +0200, Maxime Coquelin wrote:
> 
> 
> On 05/24/2017 11:58 PM, Michael S. Tsirkin wrote:
> > On Wed, May 24, 2017 at 11:05:20AM +0200, Maxime Coquelin wrote:
> > > process_message_reply() was recently updated to get full message
> > > content instead of only its request field.
> > > 
> > > There is no need to copy all the struct content into the stack,
> > > so just pass its pointer as const.
> > > 
> > > Cc: Zhiyong Yang <zhiyong.yang@intel.com>
> > > Fixes: 60cd11024f41 ("hw/virtio: fix vhost user fails to startup when MQ")
> > > Reviewed-by: Jens Freimann <jfreiman@redhat.com>
> > > Reviewed-by: Zhiyong Yang <zhiyong.yang@intel.com>
> > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > 
> > Why "Fixes"? It's not a bugfix, is it? Passing a pointer is
> > slightly cleaner but it's not a big deal IMHO. I'll apply
> > but would like to get clarification on this tag.
> 
> Right, this is not a bug fix.
> I noticed this while rebasing my Vhost-user IOMMU series,
> which will call this function much more often than currently.
> That said, I haven't done any measurements, and I don't believe it
> will have a noticeable impact.
> 
> Feel free to remove the "Fixes:" line when applying, or I can resend
> if you prefer.
> 
> Maxime

Applied no need to resend.