| Message ID | 20170524090520.321-1-maxime.coquelin@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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.
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;
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(-)