On Mon, Sep 29, 2025 at 4:22 PM Michael S. Tsirkin <[email protected]> wrote: > > On Sun, Sep 28, 2025 at 07:27:19PM +0100, David Laight wrote: > > On Thu, 25 Sep 2025 18:37:01 +0800 > > Jason Wang <[email protected]> wrote: > > > > > Use u16 for last_used_idx in virtqueue_poll_split() to align with the > > > spec. > > > > If you care about performance you should pretty much never use 'u16' for > > function parameters, return values or any arithmetic. > > Just because the domain of the variable is [0..65535] doesn't mean that > > 'unsigned int' isn't the correct type. > > > > > > > > Acked-by: Eugenio Pérez <[email protected]> > > > Reviewed-by: Xuan Zhuo <[email protected]> > > > Signed-off-by: Jason Wang <[email protected]> > > > > I don't like this because it is inconsistent with virtqueue_poll. > > > If you are going to change this, change virtqueue_enable_cb_prepare_split > too. > > > But really there's no point. > > > > > --- > > > drivers/virtio/virtio_ring.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > index 58c03a8aab85..4679a027dc53 100644 > > > --- a/drivers/virtio/virtio_ring.c > > > +++ b/drivers/virtio/virtio_ring.c > > > @@ -806,7 +806,7 @@ static void detach_buf_split(struct vring_virtqueue > > > *vq, unsigned int head, > > > } > > > > > > static bool virtqueue_poll_split(const struct vring_virtqueue *vq, > > > - unsigned int last_used_idx) > > > + u16 last_used_idx) > > > { > > > return (u16)last_used_idx != virtio16_to_cpu(vq->vq.vdev, > > > > You can't want that (u16) cast now, I doubt it was ever needed. > > Note that the compiler promotes the value to 'signed int', > > so the LHS of the comparison is actually (int)(u16)last_used_idx. > > > > David > > It is not needed because the value is from > virtqueue_enable_cb_prepare_split: > > static unsigned int virtqueue_enable_cb_prepare_split(struct virtqueue *_vq) > { > struct vring_virtqueue *vq = to_vvq(_vq); > u16 last_used_idx; > > START_USE(vq); > > /* We optimistically turn back on interrupts, then check if there was > * more to do. */ > /* Depending on the VIRTIO_RING_F_EVENT_IDX feature, we need to > * either clear the flags bit or point the event index at the next > * entry. Always do both to keep code simple. */ > if (vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) { > vq->split.avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT; > if (!vq->event) > vq->split.vring.avail->flags = > cpu_to_virtio16(_vq->vdev, > vq->split.avail_flags_shadow); > } > vring_used_event(&vq->split.vring) = cpu_to_virtio16(_vq->vdev, > last_used_idx = vq->last_used_idx); > END_USE(vq); > return last_used_idx; > } > > > > > > > > > vq->split.vring.used->idx); >
I will drop this in the next version. Thanks
