As the while steps < max_steps is already one less than the vq size, the
right maximum max_steps variable is queue length, not the maximum
possible remainder of % vq->vring.num.

Fixes: b44135daa37 ("virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support")
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
Personally I'd just remove max_steps and let it be vq->vring.num, but
let's make the minimal changes for now.
---
 hw/virtio/virtio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index e9d553295257..17f171551892 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -980,7 +980,7 @@ static void virtqueue_ordered_fill(VirtQueue *vq, const VirtQueueElement *elem,
      * We shouldn't need to increase 'i' by more than or equal to
      * the distance between used_idx and last_avail_idx (max_steps).
      */
-    max_steps = (vq->last_avail_idx - vq->used_idx) % vq->vring.num;
+    max_steps = MIN(vq->last_avail_idx - vq->used_idx, vq->vring.num);
 
     /* Search for element in vq->used_elems */
     while (steps < max_steps) {
-- 
2.53.0

On Thu, Mar 5, 2026 at 1:35 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> As the while steps < max_steps is already one less than the vq size, the
> right maximum max_steps variable is queue length, not the maximum
> possible remainder of % vq->vring.num.
>
> Fixes: b44135daa37 ("virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support")
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
> Personally I'd just remove max_steps and let it be vq->vring.num, but
> let's make the minimal changes for now.
> ---
>  hw/virtio/virtio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index e9d553295257..17f171551892 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -980,7 +980,7 @@ static void virtqueue_ordered_fill(VirtQueue *vq, const VirtQueueElement *elem,
>       * We shouldn't need to increase 'i' by more than or equal to
>       * the distance between used_idx and last_avail_idx (max_steps).
>       */
> -    max_steps = (vq->last_avail_idx - vq->used_idx) % vq->vring.num;
> +    max_steps = MIN(vq->last_avail_idx - vq->used_idx, vq->vring.num);
>
>      /* Search for element in vq->used_elems */
>      while (steps < max_steps) {

Not directly related but I found that the virtqueue_ordered_fill() and
virtqueue_ordered_flush() can "emulate" an in-order device even if it
isn't one. I'm not sure this is good for the device that can benefit
from out of order completion like block devices.

It seems the correct approach is to let the device decide whether it
can operate in order, not the virtio core (as vhost-kernel did).

Thanks

> --
> 2.53.0
>

On Fri, Mar 6, 2026 at 4:26 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Mar 5, 2026 at 1:35 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > As the while steps < max_steps is already one less than the vq size, the
> > right maximum max_steps variable is queue length, not the maximum
> > possible remainder of % vq->vring.num.
> >
> > Fixes: b44135daa37 ("virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support")
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> > Personally I'd just remove max_steps and let it be vq->vring.num, but
> > let's make the minimal changes for now.
> > ---
> >  hw/virtio/virtio.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index e9d553295257..17f171551892 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -980,7 +980,7 @@ static void virtqueue_ordered_fill(VirtQueue *vq, const VirtQueueElement *elem,
> >       * We shouldn't need to increase 'i' by more than or equal to
> >       * the distance between used_idx and last_avail_idx (max_steps).
> >       */
> > -    max_steps = (vq->last_avail_idx - vq->used_idx) % vq->vring.num;
> > +    max_steps = MIN(vq->last_avail_idx - vq->used_idx, vq->vring.num);
> >
> >      /* Search for element in vq->used_elems */
> >      while (steps < max_steps) {
>
> Not directly related but I found that the virtqueue_ordered_fill() and
> virtqueue_ordered_flush() can "emulate" an in-order device even if it
> isn't one. I'm not sure this is good for the device that can benefit
> from out of order completion like block devices.
>

I guess the benefit is significantly reduced if the device cannot use
more than one descriptor in the same used entry.

> It seems the correct approach is to let the device decide whether it
> can operate in order, not the virtio core (as vhost-kernel did).
>

I agree with this.

Currently, `in_order` is off by default but we could enable it by
default per-device.

On Fri, Mar 6, 2026 at 2:23 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> On Fri, Mar 6, 2026 at 4:26 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, Mar 5, 2026 at 1:35 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > >
> > > As the while steps < max_steps is already one less than the vq size, the
> > > right maximum max_steps variable is queue length, not the maximum
> > > possible remainder of % vq->vring.num.
> > >
> > > Fixes: b44135daa37 ("virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support")
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > > Personally I'd just remove max_steps and let it be vq->vring.num, but
> > > let's make the minimal changes for now.
> > > ---
> > >  hw/virtio/virtio.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index e9d553295257..17f171551892 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -980,7 +980,7 @@ static void virtqueue_ordered_fill(VirtQueue *vq, const VirtQueueElement *elem,
> > >       * We shouldn't need to increase 'i' by more than or equal to
> > >       * the distance between used_idx and last_avail_idx (max_steps).
> > >       */
> > > -    max_steps = (vq->last_avail_idx - vq->used_idx) % vq->vring.num;
> > > +    max_steps = MIN(vq->last_avail_idx - vq->used_idx, vq->vring.num);
> > >
> > >      /* Search for element in vq->used_elems */
> > >      while (steps < max_steps) {
> >
> > Not directly related but I found that the virtqueue_ordered_fill() and
> > virtqueue_ordered_flush() can "emulate" an in-order device even if it
> > isn't one. I'm not sure this is good for the device that can benefit
> > from out of order completion like block devices.
> >
>
> I guess the benefit is significantly reduced if the device cannot use
> more than one descriptor in the same used entry.
>
> > It seems the correct approach is to let the device decide whether it
> > can operate in order, not the virtio core (as vhost-kernel did).
> >
>
> I agree with this.
>
> Currently, `in_order` is off by default but we could enable it by
> default per-device.

After further thought, there's another reason. For example, for a
networking device, virtio-net should know nothing about the networking
backend (though some tricks exist in the code). So it can't know if
the backend can handle OOO or not. Starting with this seems to be
fine.

Thanks

>

On Mon, Mar 9, 2026 at 4:17 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Mar 6, 2026 at 2:23 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
> >
> > On Fri, Mar 6, 2026 at 4:26 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Thu, Mar 5, 2026 at 1:35 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > >
> > > > As the while steps < max_steps is already one less than the vq size, the
> > > > right maximum max_steps variable is queue length, not the maximum
> > > > possible remainder of % vq->vring.num.
> > > >
> > > > Fixes: b44135daa37 ("virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support")
> > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > ---
> > > > Personally I'd just remove max_steps and let it be vq->vring.num, but
> > > > let's make the minimal changes for now.
> > > > ---
> > > >  hw/virtio/virtio.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > index e9d553295257..17f171551892 100644
> > > > --- a/hw/virtio/virtio.c
> > > > +++ b/hw/virtio/virtio.c
> > > > @@ -980,7 +980,7 @@ static void virtqueue_ordered_fill(VirtQueue *vq, const VirtQueueElement *elem,
> > > >       * We shouldn't need to increase 'i' by more than or equal to
> > > >       * the distance between used_idx and last_avail_idx (max_steps).
> > > >       */
> > > > -    max_steps = (vq->last_avail_idx - vq->used_idx) % vq->vring.num;
> > > > +    max_steps = MIN(vq->last_avail_idx - vq->used_idx, vq->vring.num);
> > > >
> > > >      /* Search for element in vq->used_elems */
> > > >      while (steps < max_steps) {
> > >
> > > Not directly related but I found that the virtqueue_ordered_fill() and
> > > virtqueue_ordered_flush() can "emulate" an in-order device even if it
> > > isn't one. I'm not sure this is good for the device that can benefit
> > > from out of order completion like block devices.
> > >
> >
> > I guess the benefit is significantly reduced if the device cannot use
> > more than one descriptor in the same used entry.
> >
> > > It seems the correct approach is to let the device decide whether it
> > > can operate in order, not the virtio core (as vhost-kernel did).
> > >
> >
> > I agree with this.
> >
> > Currently, `in_order` is off by default but we could enable it by
> > default per-device.
>
> After further thought, there's another reason. For example, for a
> networking device, virtio-net should know nothing about the networking
> backend (though some tricks exist in the code). So it can't know if
> the backend can handle OOO or not. Starting with this seems to be
> fine.
>

Well, if the backend exposes in_order, I'd assume it works better if
the feature is acknowledged. I was talking just about the emulated
device.

I kind of agree with what you say for virtio-net+kernel tap or
vhost-kernel case, but we're already assuming that the virtio-net
backend works in order in Live Migration anyway. With worse
consecuences that less performance. To be coherent with your mention
here, we should support out of order descriptors in live migration.
Not saying that it is urgent, just pointing it out.

On Mon, Mar 9, 2026 at 2:19 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> On Mon, Mar 9, 2026 at 4:17 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Fri, Mar 6, 2026 at 2:23 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
> > >
> > > On Fri, Mar 6, 2026 at 4:26 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Thu, Mar 5, 2026 at 1:35 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > >
> > > > > As the while steps < max_steps is already one less than the vq size, the
> > > > > right maximum max_steps variable is queue length, not the maximum
> > > > > possible remainder of % vq->vring.num.
> > > > >
> > > > > Fixes: b44135daa37 ("virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support")
> > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > ---
> > > > > Personally I'd just remove max_steps and let it be vq->vring.num, but
> > > > > let's make the minimal changes for now.
> > > > > ---
> > > > >  hw/virtio/virtio.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > > index e9d553295257..17f171551892 100644
> > > > > --- a/hw/virtio/virtio.c
> > > > > +++ b/hw/virtio/virtio.c
> > > > > @@ -980,7 +980,7 @@ static void virtqueue_ordered_fill(VirtQueue *vq, const VirtQueueElement *elem,
> > > > >       * We shouldn't need to increase 'i' by more than or equal to
> > > > >       * the distance between used_idx and last_avail_idx (max_steps).
> > > > >       */
> > > > > -    max_steps = (vq->last_avail_idx - vq->used_idx) % vq->vring.num;
> > > > > +    max_steps = MIN(vq->last_avail_idx - vq->used_idx, vq->vring.num);
> > > > >
> > > > >      /* Search for element in vq->used_elems */
> > > > >      while (steps < max_steps) {
> > > >
> > > > Not directly related but I found that the virtqueue_ordered_fill() and
> > > > virtqueue_ordered_flush() can "emulate" an in-order device even if it
> > > > isn't one. I'm not sure this is good for the device that can benefit
> > > > from out of order completion like block devices.
> > > >
> > >
> > > I guess the benefit is significantly reduced if the device cannot use
> > > more than one descriptor in the same used entry.
> > >
> > > > It seems the correct approach is to let the device decide whether it
> > > > can operate in order, not the virtio core (as vhost-kernel did).
> > > >
> > >
> > > I agree with this.
> > >
> > > Currently, `in_order` is off by default but we could enable it by
> > > default per-device.
> >
> > After further thought, there's another reason. For example, for a
> > networking device, virtio-net should know nothing about the networking
> > backend (though some tricks exist in the code). So it can't know if
> > the backend can handle OOO or not. Starting with this seems to be
> > fine.
> >
>
> Well, if the backend exposes in_order, I'd assume it works better if
> the feature is acknowledged. I was talking just about the emulated
> device.
>
> I kind of agree with what you say for virtio-net+kernel tap or
> vhost-kernel case, but we're already assuming that the virtio-net
> backend works in order in Live Migration anyway.

Could you point the code that has this asumption?

> With worse
> consecuences that less performance. To be coherent with your mention
> here, we should support out of order descriptors in live migration.
> Not saying that it is urgent, just pointing it out.
>

Thanks

On Tue, Mar 10, 2026 at 4:09 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, Mar 9, 2026 at 2:19 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
> >
> > On Mon, Mar 9, 2026 at 4:17 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Fri, Mar 6, 2026 at 2:23 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
> > > >
> > > > On Fri, Mar 6, 2026 at 4:26 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > On Thu, Mar 5, 2026 at 1:35 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > > >
> > > > > > As the while steps < max_steps is already one less than the vq size, the
> > > > > > right maximum max_steps variable is queue length, not the maximum
> > > > > > possible remainder of % vq->vring.num.
> > > > > >
> > > > > > Fixes: b44135daa37 ("virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support")
> > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > ---
> > > > > > Personally I'd just remove max_steps and let it be vq->vring.num, but
> > > > > > let's make the minimal changes for now.
> > > > > > ---
> > > > > >  hw/virtio/virtio.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > > > index e9d553295257..17f171551892 100644
> > > > > > --- a/hw/virtio/virtio.c
> > > > > > +++ b/hw/virtio/virtio.c
> > > > > > @@ -980,7 +980,7 @@ static void virtqueue_ordered_fill(VirtQueue *vq, const VirtQueueElement *elem,
> > > > > >       * We shouldn't need to increase 'i' by more than or equal to
> > > > > >       * the distance between used_idx and last_avail_idx (max_steps).
> > > > > >       */
> > > > > > -    max_steps = (vq->last_avail_idx - vq->used_idx) % vq->vring.num;
> > > > > > +    max_steps = MIN(vq->last_avail_idx - vq->used_idx, vq->vring.num);
> > > > > >
> > > > > >      /* Search for element in vq->used_elems */
> > > > > >      while (steps < max_steps) {
> > > > >
> > > > > Not directly related but I found that the virtqueue_ordered_fill() and
> > > > > virtqueue_ordered_flush() can "emulate" an in-order device even if it
> > > > > isn't one. I'm not sure this is good for the device that can benefit
> > > > > from out of order completion like block devices.
> > > > >
> > > >
> > > > I guess the benefit is significantly reduced if the device cannot use
> > > > more than one descriptor in the same used entry.
> > > >
> > > > > It seems the correct approach is to let the device decide whether it
> > > > > can operate in order, not the virtio core (as vhost-kernel did).
> > > > >
> > > >
> > > > I agree with this.
> > > >
> > > > Currently, `in_order` is off by default but we could enable it by
> > > > default per-device.
> > >
> > > After further thought, there's another reason. For example, for a
> > > networking device, virtio-net should know nothing about the networking
> > > backend (though some tricks exist in the code). So it can't know if
> > > the backend can handle OOO or not. Starting with this seems to be
> > > fine.
> > >
> >
> > Well, if the backend exposes in_order, I'd assume it works better if
> > the feature is acknowledged. I was talking just about the emulated
> > device.
> >
> > I kind of agree with what you say for virtio-net+kernel tap or
> > vhost-kernel case, but we're already assuming that the virtio-net
> > backend works in order in Live Migration anyway.
>
> Could you point the code that has this asumption?
>

Actually, the lack of code to migrate the inflight descriptors is the
fact that the migration code of virtio-net assumes the buffers arrive
in order :).

It is easier to reproduce in the packed vq: When used entries override
the avail descriptors, all the overridden entries for available
buffers are lost. But it's easy to reproduce in the split vq too, as
we only migrate indexes.

It does not happen in virtio-blk, where in-flight virtqueue elements
are migrated.

> > With worse
> > consecuences that less performance. To be coherent with your mention
> > here, we should support out of order descriptors in live migration.
> > Not saying that it is urgent, just pointing it out.
> >
>
> Thanks
>

On 3/10/26 2:21 AM, Eugenio Perez Martin wrote:
> On Tue, Mar 10, 2026 at 4:09 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On Mon, Mar 9, 2026 at 2:19 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>>>
>>> On Mon, Mar 9, 2026 at 4:17 AM Jason Wang <jasowang@redhat.com> wrote:
>>>>
>>>> On Fri, Mar 6, 2026 at 2:23 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>>>>>
>>>>> On Fri, Mar 6, 2026 at 4:26 AM Jason Wang <jasowang@redhat.com> wrote:
>>>>>>
>>>>>> On Thu, Mar 5, 2026 at 1:35 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>>>>>>>
>>>>>>> As the while steps < max_steps is already one less than the vq size, the
>>>>>>> right maximum max_steps variable is queue length, not the maximum
>>>>>>> possible remainder of % vq->vring.num.
>>>>>>>
>>>>>>> Fixes: b44135daa37 ("virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support")
>>>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>>>>> ---
>>>>>>> Personally I'd just remove max_steps and let it be vq->vring.num, but
>>>>>>> let's make the minimal changes for now.
>>>>>>> ---
>>>>>>>   hw/virtio/virtio.c | 2 +-
>>>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>>>>>> index e9d553295257..17f171551892 100644
>>>>>>> --- a/hw/virtio/virtio.c
>>>>>>> +++ b/hw/virtio/virtio.c
>>>>>>> @@ -980,7 +980,7 @@ static void virtqueue_ordered_fill(VirtQueue *vq, const VirtQueueElement *elem,
>>>>>>>        * We shouldn't need to increase 'i' by more than or equal to
>>>>>>>        * the distance between used_idx and last_avail_idx (max_steps).
>>>>>>>        */
>>>>>>> -    max_steps = (vq->last_avail_idx - vq->used_idx) % vq->vring.num;
>>>>>>> +    max_steps = MIN(vq->last_avail_idx - vq->used_idx, vq->vring.num);
>>>>>>>
>>>>>>>       /* Search for element in vq->used_elems */
>>>>>>>       while (steps < max_steps) {

After looking at this again, I realize my original max_steps calculation 
is wrong when the ring is full (outstanding descriptors == vring.num).

virtqueue_ordered_fill should only walk the current in-flight window 
from used_idx, not the entire ring vring.num (of course unless the ring 
is full).

Speaking of which, IIUC, last_avail_idx == used_idx if the ring is full 
and would cause max_steps = 0 when descriptors are in flight. What if we 
did this instead?

	max_steps = MIN(vq->inuse, vq->vring.num);

>>>>>>
>>>>>> Not directly related but I found that the virtqueue_ordered_fill() and
>>>>>> virtqueue_ordered_flush() can "emulate" an in-order device even if it
>>>>>> isn't one. I'm not sure this is good for the device that can benefit
>>>>>> from out of order completion like block devices.
>>>>>>
>>>>>
>>>>> I guess the benefit is significantly reduced if the device cannot use
>>>>> more than one descriptor in the same used entry.
>>>>>
>>>>>> It seems the correct approach is to let the device decide whether it
>>>>>> can operate in order, not the virtio core (as vhost-kernel did).
>>>>>>
>>>>>
>>>>> I agree with this.
>>>>>
>>>>> Currently, `in_order` is off by default but we could enable it by
>>>>> default per-device.
>>>>
>>>> After further thought, there's another reason. For example, for a
>>>> networking device, virtio-net should know nothing about the networking
>>>> backend (though some tricks exist in the code). So it can't know if
>>>> the backend can handle OOO or not. Starting with this seems to be
>>>> fine.
>>>>
>>>
>>> Well, if the backend exposes in_order, I'd assume it works better if
>>> the feature is acknowledged. I was talking just about the emulated
>>> device.
>>>
>>> I kind of agree with what you say for virtio-net+kernel tap or
>>> vhost-kernel case, but we're already assuming that the virtio-net
>>> backend works in order in Live Migration anyway.
>>
>> Could you point the code that has this asumption?
>>
> 
> Actually, the lack of code to migrate the inflight descriptors is the
> fact that the migration code of virtio-net assumes the buffers arrive
> in order :).
> 
> It is easier to reproduce in the packed vq: When used entries override
> the avail descriptors, all the overridden entries for available
> buffers are lost. But it's easy to reproduce in the split vq too, as
> we only migrate indexes.
> 
> It does not happen in virtio-blk, where in-flight virtqueue elements
> are migrated.
> 
>>> With worse
>>> consecuences that less performance. To be coherent with your mention
>>> here, we should support out of order descriptors in live migration.
>>> Not saying that it is urgent, just pointing it out.
>>>
>>
>> Thanks
>>
>