| Message ID | 20230827182937.146450-1-lersek@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | vhost-user: call VHOST_USER_SET_VRING_ENABLE synchronously | expand |
On Sun, Aug 27, 2023 at 08:29:30PM +0200, Laszlo Ersek wrote: >The last patch in the series states and fixes the problem; prior patches >only refactor the code. Thanks for the fix and great cleanup! I fully reviewed the series and LGTM. An additional step that we can take (not in this series) crossed my mind, though. In some places we repeat the following pattern: vhost_user_write(dev, &msg, NULL, 0); ... if (reply_supported) { return process_message_reply(dev, &msg); } So what about extending the vhost_user_write_msg() added in this series to support also this cases and remove some code. Or maybe integrate vhost_user_write_msg() in vhost_user_write(). I mean something like this (untested): diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 01e0ca90c5..9ee2a78afa 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -1130,13 +1130,19 @@ static int vhost_user_get_features(struct vhost_dev *dev, uint 64_t *features) return 0; } +typedef enum { + NO_REPLY, + REPLY_IF_SUPPORTED, + REPLY_FORCED, +} VhostUserReply; + /* Note: "msg->hdr.flags" may be modified. */ static int vhost_user_write_msg(struct vhost_dev *dev, VhostUserMsg *msg, - bool wait_for_reply) + VhostUserReply reply) { int ret; - if (wait_for_reply) { + if (reply != NO_REPLY) { bool reply_supported = virtio_has_feature(dev->protocol_features, VHOST_USER_PROTOCOL_F_REPLY_ACK); if (reply_supported) { @@ -1149,7 +1155,7 @@ static int vhost_user_write_msg(struct vhost_dev *dev, VhostUserMsg *msg, return ret; } - if (wait_for_reply) { + if (reply != NO_REPLY) { uint64_t dummy; if (msg->hdr.flags & VHOST_USER_NEED_REPLY_MASK) { @@ -1162,7 +1168,9 @@ static int vhost_user_write_msg(struct vhost_dev *dev, VhostUserMsg *msg, * Send VHOST_USER_GET_FEATURES which makes all backends * send a reply. */ - return vhost_user_get_features(dev, &dummy); + if (reply == REPLY_FORCED) { + return vhost_user_get_features(dev, &dummy); + } } return 0; @@ -2207,9 +2228,6 @@ static bool vhost_user_can_merge(struct vhost_dev *dev, static int vhost_user_net_set_mtu(struct vhost_dev *dev, uint16_t mtu) { VhostUserMsg msg; - bool reply_supported = virtio_has_feature(dev->protocol_features, - VHOST_USER_PROTOCOL_F_REPLY_ACK); - int ret; if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_NET_MTU))) { return 0; @@ -2219,21 +2237,9 @@ static int vhost_user_net_set_mtu(struct vhost_dev *dev, uint16_t mtu) msg.payload.u64 = mtu; msg.hdr.size = sizeof(msg.payload.u64); msg.hdr.flags = VHOST_USER_VERSION; - if (reply_supported) { - msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK; - } - - ret = vhost_user_write(dev, &msg, NULL, 0); - if (ret < 0) { - return ret; - } /* If reply_ack supported, backend has to ack specified MTU is valid */ - if (reply_supported) { - return process_message_reply(dev, &msg); - } - - return 0; + return vhost_user_write_msg(dev, &msg, REPLY_IF_SUPPORTED); } static int vhost_user_send_device_iotlb_msg(struct vhost_dev *dev, @@ -2313,10 +2319,7 @@ static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config, static int vhost_user_set_config(struct vhost_dev *dev, const uint8_t *data, uint32_t offset, uint32_t size, uint32_t flags) { - int ret; uint8_t *p; - bool reply_supported = virtio_has_feature(dev->protocol_features, - VHOST_USER_PROTOCOL_F_REPLY_ACK); VhostUserMsg msg = { .hdr.request = VHOST_USER_SET_CONFIG, @@ -2329,10 +2332,6 @@ static int vhost_user_set_config(struct vhost_dev *dev, const uint8_t *data, return -ENOTSUP; } - if (reply_supported) { - msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK; - } - if (size > VHOST_USER_MAX_CONFIG_SIZE) { return -EINVAL; } @@ -2343,16 +2342,7 @@ static int vhost_user_set_config(struct vhost_dev *dev, const uint8_t *data, p = msg.payload.config.region; memcpy(p, data, size); - ret = vhost_user_write(dev, &msg, NULL, 0); - if (ret < 0) { - return ret; - } - - if (reply_supported) { - return process_message_reply(dev, &msg); - } - - return 0; + return vhost_user_write_msg(dev, &msg, REPLY_IF_SUPPORTED); } Thanks, Stefano
On 8/30/23 10:48, Stefano Garzarella wrote: > On Sun, Aug 27, 2023 at 08:29:30PM +0200, Laszlo Ersek wrote: >> The last patch in the series states and fixes the problem; prior patches >> only refactor the code. > > Thanks for the fix and great cleanup! > > I fully reviewed the series and LGTM. > > An additional step that we can take (not in this series) crossed my > mind, though. In some places we repeat the following pattern: > > vhost_user_write(dev, &msg, NULL, 0); > ... > > if (reply_supported) { > return process_message_reply(dev, &msg); > } > > So what about extending the vhost_user_write_msg() added in this series > to support also this cases and remove some code. > Or maybe integrate vhost_user_write_msg() in vhost_user_write(). Good idea, I'd just like someone else to do it -- and as you say, after this series :) This series is relatively packed with "thought" already (in the last patch), plus a week ago I knew absolutely nothing about vhost / vhost-user. (And, I read the whole blog series at <https://www.redhat.com/en/virtio-networking-series> in 1-2 days, while analyzing this issue, to understand the design of vhost.) So I'd prefer keeping my first contribution in this area limited -- what you are suggesting touches on some of the requests that require genuine responses, and I didn't want to fiddle with those. (I think your patch should be fine BTW!) Laszlo