| Message ID | 13b3a36cc33c443a47525957ea38e80594d90595.1693287885.git.yin31149@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/8] vhost: Add count argument to vhost_svq_poll() | expand |
On Tue, Aug 29, 2023 at 7:55 AM Hawkins Jiawei <yin31149@gmail.com> wrote: > > Next patches in this series will delay the polling > and checking of buffers until either the SVQ is > full or control commands shadow buffers are full, > no longer perform an immediate poll and check of > the device's used buffers for each CVQ state load command. > > To achieve this, this patch exposes > vhost_svq_available_slots() and introduces a helper function, > allowing QEMU to know whether the SVQ is full. > > Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> > Acked-by: Eugenio Pérez <eperezma@redhat.com> > --- > hw/virtio/vhost-shadow-virtqueue.c | 2 +- > hw/virtio/vhost-shadow-virtqueue.h | 1 + > net/vhost-vdpa.c | 9 +++++++++ > 3 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > index e731b1d2ea..fc5f408f77 100644 > --- a/hw/virtio/vhost-shadow-virtqueue.c > +++ b/hw/virtio/vhost-shadow-virtqueue.c > @@ -66,7 +66,7 @@ bool vhost_svq_valid_features(uint64_t features, Error **errp) > * > * @svq: The svq > */ > -static uint16_t vhost_svq_available_slots(const VhostShadowVirtqueue *svq) > +uint16_t vhost_svq_available_slots(const VhostShadowVirtqueue *svq) > { > return svq->num_free; > } > diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h > index 5bce67837b..19c842a15b 100644 > --- a/hw/virtio/vhost-shadow-virtqueue.h > +++ b/hw/virtio/vhost-shadow-virtqueue.h > @@ -114,6 +114,7 @@ typedef struct VhostShadowVirtqueue { > > bool vhost_svq_valid_features(uint64_t features, Error **errp); > > +uint16_t vhost_svq_available_slots(const VhostShadowVirtqueue *svq); > void vhost_svq_push_elem(VhostShadowVirtqueue *svq, > const VirtQueueElement *elem, uint32_t len); > int vhost_svq_add(VhostShadowVirtqueue *svq, const struct iovec *out_sg, I think it is ok to split this export in its own patch. If you decide to do it that way, you can add my Acked-by. > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > index a875767ee9..e6342b213f 100644 > --- a/net/vhost-vdpa.c > +++ b/net/vhost-vdpa.c > @@ -620,6 +620,13 @@ static ssize_t vhost_vdpa_net_cvq_add(VhostVDPAState *s, > return vhost_svq_poll(svq, 1); > } > > +/* Convenience wrapper to get number of available SVQ descriptors */ > +static uint16_t vhost_vdpa_net_svq_available_slots(VhostVDPAState *s) > +{ > + VhostShadowVirtqueue *svq = g_ptr_array_index(s->vhost_vdpa.shadow_vqs, 0); This is not really generic enough for all VhostVDPAState, as dataplane ones have two svqs. I think the best is to just inline the function in the caller, as there is only one, isn't it? If not, would it work to just replace _net_ by _cvq_ or similar? > + return vhost_svq_available_slots(svq); > +} > + > static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, uint8_t class, > uint8_t cmd, const struct iovec *data_sg, > size_t data_num) > @@ -640,6 +647,8 @@ static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, uint8_t class, > }; > > assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - sizeof(ctrl)); > + /* Each CVQ command has one out descriptor and one in descriptor */ > + assert(vhost_vdpa_net_svq_available_slots(s) >= 2); > I think we should remove this assertion. By the end of the series there is an "if" checks explicitly for the opposite condition, and flushing the queue in that case, so the code can never reach it. > /* pack the CVQ command header */ > memcpy(s->cvq_cmd_out_buffer, &ctrl, sizeof(ctrl)); > -- > 2.25.1 >
在 2023/10/4 01:44, Eugenio Perez Martin 写道: > On Tue, Aug 29, 2023 at 7:55 AM Hawkins Jiawei <yin31149@gmail.com> wrote: >> >> Next patches in this series will delay the polling >> and checking of buffers until either the SVQ is >> full or control commands shadow buffers are full, >> no longer perform an immediate poll and check of >> the device's used buffers for each CVQ state load command. >> >> To achieve this, this patch exposes >> vhost_svq_available_slots() and introduces a helper function, >> allowing QEMU to know whether the SVQ is full. >> >> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> >> Acked-by: Eugenio Pérez <eperezma@redhat.com> >> --- >> hw/virtio/vhost-shadow-virtqueue.c | 2 +- >> hw/virtio/vhost-shadow-virtqueue.h | 1 + >> net/vhost-vdpa.c | 9 +++++++++ >> 3 files changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c >> index e731b1d2ea..fc5f408f77 100644 >> --- a/hw/virtio/vhost-shadow-virtqueue.c >> +++ b/hw/virtio/vhost-shadow-virtqueue.c >> @@ -66,7 +66,7 @@ bool vhost_svq_valid_features(uint64_t features, Error **errp) >> * >> * @svq: The svq >> */ >> -static uint16_t vhost_svq_available_slots(const VhostShadowVirtqueue *svq) >> +uint16_t vhost_svq_available_slots(const VhostShadowVirtqueue *svq) >> { >> return svq->num_free; >> } >> diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h >> index 5bce67837b..19c842a15b 100644 >> --- a/hw/virtio/vhost-shadow-virtqueue.h >> +++ b/hw/virtio/vhost-shadow-virtqueue.h >> @@ -114,6 +114,7 @@ typedef struct VhostShadowVirtqueue { >> >> bool vhost_svq_valid_features(uint64_t features, Error **errp); >> >> +uint16_t vhost_svq_available_slots(const VhostShadowVirtqueue *svq); >> void vhost_svq_push_elem(VhostShadowVirtqueue *svq, >> const VirtQueueElement *elem, uint32_t len); >> int vhost_svq_add(VhostShadowVirtqueue *svq, const struct iovec *out_sg, > > I think it is ok to split this export in its own patch. If you decide > to do it that way, you can add my Acked-by. I will split this in its own patch, thanks for your suggestion! > >> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c >> index a875767ee9..e6342b213f 100644 >> --- a/net/vhost-vdpa.c >> +++ b/net/vhost-vdpa.c >> @@ -620,6 +620,13 @@ static ssize_t vhost_vdpa_net_cvq_add(VhostVDPAState *s, >> return vhost_svq_poll(svq, 1); >> } >> >> +/* Convenience wrapper to get number of available SVQ descriptors */ >> +static uint16_t vhost_vdpa_net_svq_available_slots(VhostVDPAState *s) >> +{ >> + VhostShadowVirtqueue *svq = g_ptr_array_index(s->vhost_vdpa.shadow_vqs, 0); > > This is not really generic enough for all VhostVDPAState, as dataplane > ones have two svqs. > > I think the best is to just inline the function in the caller, as > there is only one, isn't it? If not, would it work to just replace > _net_ by _cvq_ or similar? > Yes, there should be only one user for this function, I will inline the function in the caller. >> + return vhost_svq_available_slots(svq); >> +} >> + >> static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, uint8_t class, >> uint8_t cmd, const struct iovec *data_sg, >> size_t data_num) >> @@ -640,6 +647,8 @@ static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, uint8_t class, >> }; >> >> assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - sizeof(ctrl)); >> + /* Each CVQ command has one out descriptor and one in descriptor */ >> + assert(vhost_vdpa_net_svq_available_slots(s) >= 2); >> > > I think we should remove this assertion. By the end of the series > there is an "if" checks explicitly for the opposite condition, and > flushing the queue in that case, so the code can never reach it. > Yes, you are right. I will remove this assertion. Thanks! >> /* pack the CVQ command header */ >> memcpy(s->cvq_cmd_out_buffer, &ctrl, sizeof(ctrl)); >> -- >> 2.25.1 >> >
diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c index e731b1d2ea..fc5f408f77 100644 --- a/hw/virtio/vhost-shadow-virtqueue.c +++ b/hw/virtio/vhost-shadow-virtqueue.c @@ -66,7 +66,7 @@ bool vhost_svq_valid_features(uint64_t features, Error **errp) * * @svq: The svq */ -static uint16_t vhost_svq_available_slots(const VhostShadowVirtqueue *svq) +uint16_t vhost_svq_available_slots(const VhostShadowVirtqueue *svq) { return svq->num_free; } diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h index 5bce67837b..19c842a15b 100644 --- a/hw/virtio/vhost-shadow-virtqueue.h +++ b/hw/virtio/vhost-shadow-virtqueue.h @@ -114,6 +114,7 @@ typedef struct VhostShadowVirtqueue { bool vhost_svq_valid_features(uint64_t features, Error **errp); +uint16_t vhost_svq_available_slots(const VhostShadowVirtqueue *svq); void vhost_svq_push_elem(VhostShadowVirtqueue *svq, const VirtQueueElement *elem, uint32_t len); int vhost_svq_add(VhostShadowVirtqueue *svq, const struct iovec *out_sg, diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c index a875767ee9..e6342b213f 100644 --- a/net/vhost-vdpa.c +++ b/net/vhost-vdpa.c @@ -620,6 +620,13 @@ static ssize_t vhost_vdpa_net_cvq_add(VhostVDPAState *s, return vhost_svq_poll(svq, 1); } +/* Convenience wrapper to get number of available SVQ descriptors */ +static uint16_t vhost_vdpa_net_svq_available_slots(VhostVDPAState *s) +{ + VhostShadowVirtqueue *svq = g_ptr_array_index(s->vhost_vdpa.shadow_vqs, 0); + return vhost_svq_available_slots(svq); +} + static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, uint8_t class, uint8_t cmd, const struct iovec *data_sg, size_t data_num) @@ -640,6 +647,8 @@ static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, uint8_t class, }; assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - sizeof(ctrl)); + /* Each CVQ command has one out descriptor and one in descriptor */ + assert(vhost_vdpa_net_svq_available_slots(s) >= 2); /* pack the CVQ command header */ memcpy(s->cvq_cmd_out_buffer, &ctrl, sizeof(ctrl));