Re: [PATCH v2 3/6] virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support
On Mon, May 20, 2024 at 3:01 PM Jonah Palmer wrote:
>
> Add VIRTIO_F_IN_ORDER feature support for the virtqueue_fill operation.
>
> The goal of the virtqueue_ordered_fill operation when the
> VIRTIO_F_IN_ORDER feature has been negotiated is to search for this
> now-used element, set its length, and mark the element as filled in
> the VirtQueue's used_elems array.
>
> By marking the element as filled, it will indicate that this element has
> been processed and is ready to be flushed, so long as the element is
> in-order.
>
> Signed-off-by: Jonah Palmer
> ---
> hw/virtio/virtio.c | 36 +++-
> 1 file changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 7456d61bc8..01b6b32460 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -873,6 +873,38 @@ static void virtqueue_packed_fill(VirtQueue *vq, const
> VirtQueueElement *elem,
> vq->used_elems[idx].ndescs = elem->ndescs;
> }
>
> +static void virtqueue_ordered_fill(VirtQueue *vq, const VirtQueueElement
> *elem,
> + unsigned int len)
> +{
> +unsigned int i, steps, max_steps;
> +
> +i = vq->used_idx;
> +steps = 0;
> +/*
> + * We shouldn't need to increase 'i' by more than the distance
> + * between used_idx and last_avail_idx.
> + */
> +max_steps = (vq->last_avail_idx + vq->vring.num - vq->used_idx)
> +% vq->vring.num;
I may be missing something, but (+vq->vring.num) is redundant if we (%
vq->vring.num), isn't it?
> +
> +/* Search for element in vq->used_elems */
> +while (steps <= max_steps) {
> +/* Found element, set length and mark as filled */
> +if (vq->used_elems[i].index == elem->index) {
> +vq->used_elems[i].len = len;
> +vq->used_elems[i].in_order_filled = true;
> +break;
> +}
> +
> +i += vq->used_elems[i].ndescs;
> +steps += vq->used_elems[i].ndescs;
> +
> +if (i >= vq->vring.num) {
> +i -= vq->vring.num;
> +}
> +}
> +}
> +
Let's report an error if we finish the loop. I think:
qemu_log_mask(LOG_GUEST_ERROR,
"%s: %s cannot fill buffer id %u\n",
__func__, vdev->name, elem->index);
(or similar) should do.
apart form that,
Reviewed-by: Eugenio Pérez
> static void virtqueue_packed_fill_desc(VirtQueue *vq,
> const VirtQueueElement *elem,
> unsigned int idx,
> @@ -923,7 +955,9 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement
> *elem,
> return;
> }
>
> -if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
> +if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_IN_ORDER)) {
> +virtqueue_ordered_fill(vq, elem, len);
> +} else if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
> virtqueue_packed_fill(vq, elem, len, idx);
> } else {
> virtqueue_split_fill(vq, elem, len, idx);
> --
> 2.39.3
>
Re: [PATCH v2 2/6] virtio: virtqueue_pop - VIRTIO_F_IN_ORDER support
On Mon, May 20, 2024 at 3:01 PM Jonah Palmer wrote:
>
> Add VIRTIO_F_IN_ORDER feature support in virtqueue_split_pop and
> virtqueue_packed_pop.
>
> VirtQueueElements popped from the available/descritpor ring are added to
> the VirtQueue's used_elems array in-order and in the same fashion as
> they would be added the used and descriptor rings, respectively.
>
> This will allow us to keep track of the current order, what elements
> have been written, as well as an element's essential data after being
> processed.
>
> Tested-by: Lei Yang
> Signed-off-by: Jonah Palmer
> ---
> hw/virtio/virtio.c | 17 -
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 893a072c9d..7456d61bc8 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1506,7 +1506,7 @@ static void *virtqueue_alloc_element(size_t sz,
> unsigned out_num, unsigned in_nu
>
> static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
> {
> -unsigned int i, head, max;
> +unsigned int i, head, max, prev_avail_idx;
> VRingMemoryRegionCaches *caches;
> MemoryRegionCache indirect_desc_cache;
> MemoryRegionCache *desc_cache;
> @@ -1539,6 +1539,8 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t
> sz)
> goto done;
> }
>
> +prev_avail_idx = vq->last_avail_idx;
> +
> if (!virtqueue_get_head(vq, vq->last_avail_idx++, &head)) {
> goto done;
> }
> @@ -1630,6 +1632,12 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t
> sz)
> elem->in_sg[i] = iov[out_num + i];
> }
>
> +if (virtio_vdev_has_feature(vdev, VIRTIO_F_IN_ORDER)) {
I think vq->last_avail_idx - 1 could be more clear here.
Either way,
Reviewed-by: Eugenio Pérez
> +vq->used_elems[prev_avail_idx].index = elem->index;
> +vq->used_elems[prev_avail_idx].len = elem->len;
> +vq->used_elems[prev_avail_idx].ndescs = elem->ndescs;
> +}
> +
> vq->inuse++;
>
> trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num);
> @@ -1758,6 +1766,13 @@ static void *virtqueue_packed_pop(VirtQueue *vq,
> size_t sz)
>
> elem->index = id;
> elem->ndescs = (desc_cache == &indirect_desc_cache) ? 1 : elem_entries;
> +
> +if (virtio_vdev_has_feature(vdev, VIRTIO_F_IN_ORDER)) {
> +vq->used_elems[vq->last_avail_idx].index = elem->index;
> +vq->used_elems[vq->last_avail_idx].len = elem->len;
> +vq->used_elems[vq->last_avail_idx].ndescs = elem->ndescs;
> +}
> +
> vq->last_avail_idx += elem->ndescs;
> vq->inuse += elem->ndescs;
>
> --
> 2.39.3
>
Re: [PATCH v2 1/6] virtio: Add bool to VirtQueueElement
On Mon, May 20, 2024 at 3:01 PM Jonah Palmer wrote: > > Add the boolean 'in_order_filled' member to the VirtQueueElement structure. > The use of this boolean will signify whether the element has been processed > and is ready to be flushed (so long as the element is in-order). This > boolean is used to support the VIRTIO_F_IN_ORDER feature. > > Tested-by: Lei Yang The code has changed from the version that Lei tested, so we should drop this tag until he re-test again. Reviewed-by: Eugenio Pérez > Signed-off-by: Jonah Palmer > --- > include/hw/virtio/virtio.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index 7d5ffdc145..88e70c1ae1 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -69,6 +69,8 @@ typedef struct VirtQueueElement > unsigned int ndescs; > unsigned int out_num; > unsigned int in_num; > +/* Element has been processed (VIRTIO_F_IN_ORDER) */ > +bool in_order_filled; > hwaddr *in_addr; > hwaddr *out_addr; > struct iovec *in_sg; > -- > 2.39.3 >
Re: [PULL 10/38] tests/qtest/migration: Add a test for the analyze-migration script
Fabiano Rosas writes:
> Alex Bennée writes:
>
>> Juan Quintela writes:
>>
>>> From: Fabiano Rosas
>>>
>>> Add a smoke test that migrates to a file and gives it to the
>>> script. It should catch the most annoying errors such as changes in
>>> the ram flags.
>>>
>>> After code has been merged it becomes way harder to figure out what is
>>> causing the script to fail, the person making the change is the most
>>> likely to know right away what the problem is.
>>>
>>> Signed-off-by: Fabiano Rosas
>>> Acked-by: Thomas Huth
>>> Reviewed-by: Juan Quintela
>>> Signed-off-by: Juan Quintela
>>> Message-ID: <20231009184326.15777-7-faro...@suse.de>
>>
>> I bisected the failures I'm seeing on s390x to the introduction of this
>> script. I don't know if its simply a timeout on a relatively slow VM:
>
> What's the range of your bisect? That test has been disabled and then
> reenabled on s390x. It could be tripping the bisect.
>
> 04131e0009 ("tests/qtest/migration-test: Disable the analyze-migration.py
> test on s390x")
> 81c2c9dd5d ("tests/qtest/migration-test: Fix analyze-migration.py for
> s390x")
I ran between v8.1.0 and master.
But it is still failing @ HEAD 01782d6b294f95bcde334386f0aaac593cd28c0d
as you can see from the run I did at the end.
>
> I don't think that test itself could be timing out. It's a very simple
> test. It runs a migration and then uses the output to validate the
> script.
>
> I don't have a Z machine at hand and the migration tests only run with
> KVM for s390x, so it would be useful to take a look at meson's
> testlog.txt so we can see which test is failing and hopefully in what
> way it is failing.
>
> If you're up for it, running this in a loop is usually the best way to
> catch any intermittent issues:
>
> QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test
>
> And once you figure out which test, there's this monstrosity:
>
> QTEST_QEMU_BINARY='gdb -q --ex "set pagination off" \
> --ex "set print thread-events off" \
> --ex "handle SIGUSR1 noprint" \
> --ex "handle SIGPIPE noprint" \
> --ex "run" --ex "quit \$_exitcode" \
> --args ./qemu-system-x86_64' \
> gdb -q --ex "set prompt (qtest) " \
> --ex "handle SIGPIPE noprint" \
> --args ./tests/qtest/migration-test -p
> /x86_64/migration//
>
>> Summary of Failures:
>>
>> 36/546 qemu:qtest+qtest-s390x / qtest-s390x/migration-test
>> ERROR 93.51s killed by signal 6 SIGABRT
>>
>> It seems to be unstable as we pass sometimes:
>>
>> 11:26:42 [ajb@qemu01:~/l/q/b/system] master|… + ./pyvenv/bin/meson test
>> --repeat 100 qtest-s390x/migration-test
>> ninja: Entering directory `/home/ajb/lsrc/qemu.git/builds/system'
>> [1/9] Generating qemu-version.h with a custom command (wrapped by meson to
>> capture output)
>> 1/100 qemu:qtest+qtest-s390x / qtest-s390x/migration-test ERROR
>>251.98s killed by signal 6 SIGABRT
> MALLOC_PERTURB_=9
> PYTHON=/home/ajb/lsrc/qemu.git/builds/system/pyvenv/bin/python3
> G_TEST_DBUS_DAEMON=/home/ajb/lsrc/qemu.git/tests/dbus-vmstate-daemon.sh
> QTEST_QEMU_BINARY=./qemu-system-s390x QTEST_QEMU_IMG=./qemu-img
> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon
> /home/ajb/lsrc/qemu.git/builds/system/tests/qtest/migration-test
> --tap -k
>>
>> 2/100 qemu:qtest+qtest-s390x / qtest-s390x/migration-test ERROR
>>258.71s killed by signal 6 SIGABRT
> PYTHON=/home/ajb/lsrc/qemu.git/builds/system/pyvenv/bin/python3
> MALLOC_PERTURB_=205
> G_TEST_DBUS_DAEMON=/home/ajb/lsrc/qemu.git/tests/dbus-vmstate-daemon.sh
> QTEST_QEMU_BINARY=./qemu-system-s390x QTEST_QEMU_IMG=./qemu-img
> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon
> /home/ajb/lsrc/qemu.git/builds/system/tests/qtest/migration-test
> --tap -k
>>
>> 3/100 qemu:qtest+qtest-s390x / qtest-s390x/migration-test OK
>>302.53s 46 subtests passed
>> 4/100 qemu:qtest+qtest-s390x / qtest-s390x/migration-test OK
>>319.56s 46 subtests passed
>> 5/100 qemu:qtest+qtest-s390x / qtest-s390x/migration-test OK
>>320.11s 46 subtests passed
>> 6/100 qemu:qtest+qtest-s390x / qtest-s390x/migration-test OK
>>328.40s 46 subtests passed
>>
>> Ok: 4
>> Expected Fail: 0
>> Fail: 2
>> Unexpected Pass:0
>> Skipped:0
>> Timeout:0
>>
>>> ---
>>> tests/qtest/migration-test.c | 60
>>> tests/qtest/meson.build | 2 ++
>>> 2 files changed, 62 insertions(+)
>>>
>>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>>> i
Re: [PULL 10/38] tests/qtest/migration: Add a test for the analyze-migration script
On 22/05/2024 14.48, Fabiano Rosas wrote:
Thomas Huth writes:
On 21/05/2024 14.46, Fabiano Rosas wrote:
Alex Bennée writes:
Juan Quintela writes:
From: Fabiano Rosas
Add a smoke test that migrates to a file and gives it to the
script. It should catch the most annoying errors such as changes in
the ram flags.
After code has been merged it becomes way harder to figure out what is
causing the script to fail, the person making the change is the most
likely to know right away what the problem is.
Signed-off-by: Fabiano Rosas
Acked-by: Thomas Huth
Reviewed-by: Juan Quintela
Signed-off-by: Juan Quintela
Message-ID: <20231009184326.15777-7-faro...@suse.de>
I bisected the failures I'm seeing on s390x to the introduction of this
script. I don't know if its simply a timeout on a relatively slow VM:
What's the range of your bisect? That test has been disabled and then
reenabled on s390x. It could be tripping the bisect.
04131e0009 ("tests/qtest/migration-test: Disable the analyze-migration.py test on
s390x")
81c2c9dd5d ("tests/qtest/migration-test: Fix analyze-migration.py for s390x")
I don't think that test itself could be timing out. It's a very simple
test. It runs a migration and then uses the output to validate the
script.
Agreed, the analyze-migration.py is unlikely to be the issue - especially
since it seems to have been disabled again in commit 6f0771de903b ...
Fabiano, why did you disable it here again? The reason is not mentioned in
the commit description.
Your patch 81c2c9dd5d was merged between my v1 and v2 on the list and I
didn't notice so I messed up the rebase. I'll send a patch soon to fix
that.
Thanks, but I already sent a patch earlier today that should fix the issue:
https://lore.kernel.org/qemu-devel/20240522091255.417263-1-th...@redhat.com/T/#u
Thomas
Re: [PULL 10/38] tests/qtest/migration: Add a test for the analyze-migration script
Thomas Huth writes:
> On 21/05/2024 14.46, Fabiano Rosas wrote:
>> Alex Bennée writes:
>>
>>> Juan Quintela writes:
>>>
From: Fabiano Rosas
Add a smoke test that migrates to a file and gives it to the
script. It should catch the most annoying errors such as changes in
the ram flags.
After code has been merged it becomes way harder to figure out what is
causing the script to fail, the person making the change is the most
likely to know right away what the problem is.
Signed-off-by: Fabiano Rosas
Acked-by: Thomas Huth
Reviewed-by: Juan Quintela
Signed-off-by: Juan Quintela
Message-ID: <20231009184326.15777-7-faro...@suse.de>
>>>
>>> I bisected the failures I'm seeing on s390x to the introduction of this
>>> script. I don't know if its simply a timeout on a relatively slow VM:
>>
>> What's the range of your bisect? That test has been disabled and then
>> reenabled on s390x. It could be tripping the bisect.
>>
>> 04131e0009 ("tests/qtest/migration-test: Disable the analyze-migration.py
>> test on s390x")
>> 81c2c9dd5d ("tests/qtest/migration-test: Fix analyze-migration.py for s390x")
>>
>> I don't think that test itself could be timing out. It's a very simple
>> test. It runs a migration and then uses the output to validate the
>> script.
>
> Agreed, the analyze-migration.py is unlikely to be the issue - especially
> since it seems to have been disabled again in commit 6f0771de903b ...
> Fabiano, why did you disable it here again? The reason is not mentioned in
> the commit description.
Your patch 81c2c9dd5d was merged between my v1 and v2 on the list and I
didn't notice so I messed up the rebase. I'll send a patch soon to fix
that.
