packed-ring: fix example code

PATCH In review Tiwei Bie 3 participants 6 messages Dec 5, 2018

Share & tools

Tiwei Bie

author

2018-12-05 02:18 UTC #0 lore raw

PATCH v3 1/1 packed-ring: fix example code

Driver can't just check whether USED bit equals to the used wrap counter when checking whether a descriptor is a used descriptor, because driver also needs to check whether the descriptor has been made available. Below is an example:

Assuming ring size is 4, ring's initial state will be:

+----+----+----+----+
| 00 | 00 | 00 | 00 |
+----+----+----+----+

00 means AVAIL=0 USED=0, 01 means AVAIL=0 USED=1 10 means AVAIL=1 USED=0, 11 means AVAIL=1 USED=1

After driver made two descriptor chains available and each chain consists of two descriptors, the ring could be:

+----+-----------+----+-----------+
| 10 | 10 (id=0) | 10 | 10 (id=1) |
+----+-----------+----+-----------+

After device processed all the available descriptors and made them used (e.g. in order), the ring could be:

+-----------+----+-----------+----+
| 11 (id=0) | 10 | 11 (id=1) | 10 |
+-----------+----+-----------+----+

After driver processed all the used descriptors and made one descriptor (not chained, just one descriptor) available, the ring could be:

+-----------+----+----+----+
| 01 (id=0) | 10 | 11 | 10 |
+-----------+----+----+----+

After device made that descriptor used, the ring will be:

+-----------+----+----+----+
| 00 (id=0) | 10 | 11 | 10 |
+-----------+----+----+----+

If driver just checks whether USED bit equals to the used wrap counter when checking whether a descriptor is a used descriptor, after processing the first descriptor (whose AVAIL and USED bits are both 0), and advancing vq->next_used pointer, it will then also treat the next descriptor, i.e. the second descriptor (whose AVAIL and USED bits are 1 and 0 respectively) as a used descriptor which is wrong.

Fixes https://github.com/oasis-tcs/virtio-spec/issues/29

Signed-off-by Tiwei Bie <[email protected]>

---
v2:
- Add "Fixes" tag;
- Refine commit log;

v3:
- Compare with vq->used_wrap_count (MST);
- Add comments (MST);
- Refine commit log;

 packed-ring.tex | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

packed-ring.tex +16 -3

index f24f49b..c8cd83b 100644
@@ -687,16 +687,29 @@ vq->driver_event.flags = RING_EVENT_FLAGS_DISABLE;
 for (;;) {
         struct pvirtq_desc *d = vq->desc[vq->next_used];
 
+        /*
+         * Check that
+         * 1. Descriptor has been made available.
+         *    Note: there are many other ways to check this, e.g.
+         *    track the number of outstanding available descriptors or buffers
+         *    and check that it's not 0.
+         * 2. Descriptor has been used by device.
+         */
         flags = d->flags;
+        bool avail = flags & VIRTQ_DESC_F_AVAIL;
         bool used = flags & VIRTQ_DESC_F_USED;
-
-        if (used != vq->used_wrap_count) {
+        if (avail != vq->used_wrap_count || used != vq->used_wrap_count) {
                 vq->driver_event.flags = RING_EVENT_FLAGS_ENABLE;
                 memory_barrier();
 
+                /*
+                 * Re-test in case another thread submitted more descriptors
+                 * and/or device used more descriptors before driver enabled events.
+                 */
                 flags = d->flags;
+                bool avail = flags & VIRTQ_DESC_F_AVAIL;
                 bool used = flags & VIRTQ_DESC_F_USED;
-                if (used != vq->used_wrap_count) {
+                if (avail != vq->used_wrap_count || used != vq->used_wrap_count) {
                         break;
                 }

Signature · 6 lines

2.17.1


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Cornelia Huck

2018-12-05 14:04 UTC #1 lore raw

Quoted 70 lines · On Wed, 5 Dec 2018 10:16:51 +0800 Tiwei Bie <[email protected]> wrote:

Driver can't just check whether USED bit equals to the used wrap counter when checking whether a descriptor is a used descriptor, because driver also needs to check whether the descriptor has been made available. Below is an example:
Assuming ring size is 4, ring's initial state will be:
+----+----+----+----+
| 00 | 00 | 00 | 00 |
+----+----+----+----+
00 means AVAIL=0 USED=0, 01 means AVAIL=0 USED=1 10 means AVAIL=1 USED=0, 11 means AVAIL=1 USED=1
After driver made two descriptor chains available and each chain consists of two descriptors, the ring could be:
+----+-----------+----+-----------+
| 10 | 10 (id=0) | 10 | 10 (id=1) |
+----+-----------+----+-----------+
After device processed all the available descriptors and made them used (e.g. in order), the ring could be:
+-----------+----+-----------+----+
| 11 (id=0) | 10 | 11 (id=1) | 10 |
+-----------+----+-----------+----+
After driver processed all the used descriptors and made one descriptor (not chained, just one descriptor) available, the ring could be:
+-----------+----+----+----+
| 01 (id=0) | 10 | 11 | 10 |
+-----------+----+----+----+
After device made that descriptor used, the ring will be:
+-----------+----+----+----+
| 00 (id=0) | 10 | 11 | 10 |
+-----------+----+----+----+
If driver just checks whether USED bit equals to the used wrap counter when checking whether a descriptor is a used descriptor, after processing the first descriptor (whose AVAIL and USED bits are both 0), and advancing vq->next_used pointer, it will then also treat the next descriptor, i.e. the second descriptor (whose AVAIL and USED bits are 1 and 0 respectively) as a used descriptor which is wrong.
Fixes https://github.com/oasis-tcs/virtio-spec/issues/29
Signed-off-by Tiwei Bie <[email protected]>
---
v2:
- Add "Fixes" tag;
- Refine commit log;
v3:
- Compare with vq->used_wrap_count (MST);
- Add comments (MST);
- Refine commit log;
 packed-ring.tex | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)
packed-ring.tex +0 -0
index f24f49b..c8cd83b 100644

packed-ring.tex vq->driver_event.flags = RING_EVENT_FLAGS_DISABLE; · line 696

Quoted 10 lines

packed-ring.tex +7 -0

@@ -687,16 +687,29 @@ vq->driver_event.flags = RING_EVENT_FLAGS_DISABLE;
 for (;;) {
         struct pvirtq_desc *d = vq->desc[vq->next_used];
 
+        /*
+         * Check that
+         * 1. Descriptor has been made available.
+         *    Note: there are many other ways to check this, e.g.
+         *    track the number of outstanding available descriptors or buffers
+         *    and check that it's not 0.
+         * 2. Descriptor has been used by device.

s/device/the device/

packed-ring.tex vq->driver_event.flags = RING_EVENT_FLAGS_DISABLE; · line 707

Quoted 13 lines

packed-ring.tex +6 -2

+         */
         flags = d->flags;
+        bool avail = flags & VIRTQ_DESC_F_AVAIL;
         bool used = flags & VIRTQ_DESC_F_USED;
-
-        if (used != vq->used_wrap_count) {
+        if (avail != vq->used_wrap_count || used != vq->used_wrap_count) {
                 vq->driver_event.flags = RING_EVENT_FLAGS_ENABLE;
                 memory_barrier();
 
+                /*
+                 * Re-test in case another thread submitted more descriptors
+                 * and/or device used more descriptors before driver enabled events.

s/device/the device/
s/driver/the driver/

(Also in the patch description.)

packed-ring.tex vq->driver_event.flags = RING_EVENT_FLAGS_DISABLE; · line 715

Quoted 9 lines

packed-ring.tex +3 -1

+                 */
                 flags = d->flags;
+                bool avail = flags & VIRTQ_DESC_F_AVAIL;
                 bool used = flags & VIRTQ_DESC_F_USED;
-                if (used != vq->used_wrap_count) {
+                if (avail != vq->used_wrap_count || used != vq->used_wrap_count) {
                         break;
                 }

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]

Tiwei Bie

author

2018-12-05 15:05 UTC #2 lore raw

Quoted 96 lines · Cornelia Huck

Quoted 70 lines · On Wed, 5 Dec 2018 10:16:51 +0800 Tiwei Bie <[email protected]> wrote:
Driver can't just check whether USED bit equals to the used wrap counter when checking whether a descriptor is a used descriptor, because driver also needs to check whether the descriptor has been made available. Below is an example:
Assuming ring size is 4, ring's initial state will be:
+----+----+----+----+
| 00 | 00 | 00 | 00 |
+----+----+----+----+
00 means AVAIL=0 USED=0, 01 means AVAIL=0 USED=1 10 means AVAIL=1 USED=0, 11 means AVAIL=1 USED=1
After driver made two descriptor chains available and each chain consists of two descriptors, the ring could be:
+----+-----------+----+-----------+
| 10 | 10 (id=0) | 10 | 10 (id=1) |
+----+-----------+----+-----------+
After device processed all the available descriptors and made them used (e.g. in order), the ring could be:
+-----------+----+-----------+----+
| 11 (id=0) | 10 | 11 (id=1) | 10 |
+-----------+----+-----------+----+
After driver processed all the used descriptors and made one descriptor (not chained, just one descriptor) available, the ring could be:
+-----------+----+----+----+
| 01 (id=0) | 10 | 11 | 10 |
+-----------+----+----+----+
After device made that descriptor used, the ring will be:
+-----------+----+----+----+
| 00 (id=0) | 10 | 11 | 10 |
+-----------+----+----+----+
If driver just checks whether USED bit equals to the used wrap counter when checking whether a descriptor is a used descriptor, after processing the first descriptor (whose AVAIL and USED bits are both 0), and advancing vq->next_used pointer, it will then also treat the next descriptor, i.e. the second descriptor (whose AVAIL and USED bits are 1 and 0 respectively) as a used descriptor which is wrong.
Fixes https://github.com/oasis-tcs/virtio-spec/issues/29
Signed-off-by Tiwei Bie <[email protected]>
---
v2:
- Add "Fixes" tag;
- Refine commit log;
v3:
- Compare with vq->used_wrap_count (MST);
- Add comments (MST);
- Refine commit log;
 packed-ring.tex | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)
packed-ring.tex +0 -0
index f24f49b..c8cd83b 100644
packed-ring.tex vq->driver_event.flags = RING_EVENT_FLAGS_DISABLE; · line 696
Quoted 10 lines
packed-ring.tex +7 -0
@@ -687,16 +687,29 @@ vq->driver_event.flags = RING_EVENT_FLAGS_DISABLE;
 for (;;) {
         struct pvirtq_desc *d = vq->desc[vq->next_used];
 
+        /*
+         * Check that
+         * 1. Descriptor has been made available.
+         *    Note: there are many other ways to check this, e.g.
+         *    track the number of outstanding available descriptors or buffers
+         *    and check that it's not 0.
+         * 2. Descriptor has been used by device.
s/device/the device/
Quoted 10 lines
packed-ring.tex +3 -2
+         */
         flags = d->flags;
+        bool avail = flags & VIRTQ_DESC_F_AVAIL;
         bool used = flags & VIRTQ_DESC_F_USED;
-
-        if (used != vq->used_wrap_count) {
+        if (avail != vq->used_wrap_count || used != vq->used_wrap_count) {
                 vq->driver_event.flags = RING_EVENT_FLAGS_ENABLE;
                 memory_barrier();
 

Quoted 8 lines

packed-ring.tex vq->driver_event.flags = RING_EVENT_FLAGS_DISABLE; · line 707
Quoted 3 lines
packed-ring.tex +3 -0
+                /*
+                 * Re-test in case another thread submitted more descriptors
+                 * and/or device used more descriptors before driver enabled events.
s/device/the device/
s/driver/the driver/
(Also in the patch description.)

Got it. Will fix them in the next version. Thanks!

Quoted 11 lines

packed-ring.tex vq->driver_event.flags = RING_EVENT_FLAGS_DISABLE; · line 715

Quoted 10 lines

packed-ring.tex +3 -1

+                 */
                 flags = d->flags;
+                bool avail = flags & VIRTQ_DESC_F_AVAIL;
                 bool used = flags & VIRTQ_DESC_F_USED;
-                if (used != vq->used_wrap_count) {
+                if (avail != vq->used_wrap_count || used != vq->used_wrap_count) {
                         break;
                 }

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]

Michael S. Tsirkin

2018-12-05 15:18 UTC #3 lore raw

Quoted 101 lines · Tiwei Bie

Quoted 100 lines · Cornelia Huck
Quoted 70 lines · On Wed, 5 Dec 2018 10:16:51 +0800 Tiwei Bie <[email protected]> wrote:
Driver can't just check whether USED bit equals to the used wrap counter when checking whether a descriptor is a used descriptor, because driver also needs to check whether the descriptor has been made available. Below is an example:
Assuming ring size is 4, ring's initial state will be:
+----+----+----+----+
| 00 | 00 | 00 | 00 |
+----+----+----+----+
00 means AVAIL=0 USED=0, 01 means AVAIL=0 USED=1 10 means AVAIL=1 USED=0, 11 means AVAIL=1 USED=1
After driver made two descriptor chains available and each chain consists of two descriptors, the ring could be:
+----+-----------+----+-----------+
| 10 | 10 (id=0) | 10 | 10 (id=1) |
+----+-----------+----+-----------+
After device processed all the available descriptors and made them used (e.g. in order), the ring could be:
+-----------+----+-----------+----+
| 11 (id=0) | 10 | 11 (id=1) | 10 |
+-----------+----+-----------+----+
After driver processed all the used descriptors and made one descriptor (not chained, just one descriptor) available, the ring could be:
+-----------+----+----+----+
| 01 (id=0) | 10 | 11 | 10 |
+-----------+----+----+----+
After device made that descriptor used, the ring will be:
+-----------+----+----+----+
| 00 (id=0) | 10 | 11 | 10 |
+-----------+----+----+----+
If driver just checks whether USED bit equals to the used wrap counter when checking whether a descriptor is a used descriptor, after processing the first descriptor (whose AVAIL and USED bits are both 0), and advancing vq->next_used pointer, it will then also treat the next descriptor, i.e. the second descriptor (whose AVAIL and USED bits are 1 and 0 respectively) as a used descriptor which is wrong.
Fixes https://github.com/oasis-tcs/virtio-spec/issues/29
Signed-off-by Tiwei Bie <[email protected]>
---
v2:
- Add "Fixes" tag;
- Refine commit log;
v3:
- Compare with vq->used_wrap_count (MST);
- Add comments (MST);
- Refine commit log;
 packed-ring.tex | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)
packed-ring.tex +0 -0
index f24f49b..c8cd83b 100644
packed-ring.tex vq->driver_event.flags = RING_EVENT_FLAGS_DISABLE; · line 696
Quoted 10 lines
packed-ring.tex +7 -0
@@ -687,16 +687,29 @@ vq->driver_event.flags = RING_EVENT_FLAGS_DISABLE;
 for (;;) {
         struct pvirtq_desc *d = vq->desc[vq->next_used];
 
+        /*
+         * Check that
+         * 1. Descriptor has been made available.
+         *    Note: there are many other ways to check this, e.g.
+         *    track the number of outstanding available descriptors or buffers
+         *    and check that it's not 0.
+         * 2. Descriptor has been used by device.
s/device/the device/
Quoted 13 lines
packed-ring.tex +6 -2
+         */
         flags = d->flags;
+        bool avail = flags & VIRTQ_DESC_F_AVAIL;
         bool used = flags & VIRTQ_DESC_F_USED;
-
-        if (used != vq->used_wrap_count) {
+        if (avail != vq->used_wrap_count || used != vq->used_wrap_count) {
                 vq->driver_event.flags = RING_EVENT_FLAGS_ENABLE;
                 memory_barrier();
 
+                /*
+                 * Re-test in case another thread submitted more descriptors
+                 * and/or device used more descriptors before driver enabled events.

Quoted 6 lines

Quoted 4 lines
s/device/the device/
s/driver/the driver/
(Also in the patch description.)
Got it. Will fix them in the next version. Thanks!

And it's fine to mention threads but we really should also write it in terms of driver/device, used/available and parallel processing. E.g.

* Re-test in case the driver made more descriptors available in
* parallel with the used descriptor processing (e.g. from another
* thread) and/or the device used more descriptors before the driver
* enabled events.

Quoted 11 lines

packed-ring.tex vq->driver_event.flags = RING_EVENT_FLAGS_DISABLE; · line 715

Quoted 10 lines

packed-ring.tex +3 -1

+                 */
                 flags = d->flags;
+                bool avail = flags & VIRTQ_DESC_F_AVAIL;
                 bool used = flags & VIRTQ_DESC_F_USED;
-                if (used != vq->used_wrap_count) {
+                if (avail != vq->used_wrap_count || used != vq->used_wrap_count) {
                         break;
                 }

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]

Tiwei Bie

author

2018-12-05 15:47 UTC #4 lore raw

Quoted 109 lines · Michael S. Tsirkin

Quoted 101 lines · Tiwei Bie
Quoted 100 lines · Cornelia Huck
Quoted 70 lines · On Wed, 5 Dec 2018 10:16:51 +0800 Tiwei Bie <[email protected]> wrote:
Driver can't just check whether USED bit equals to the used wrap counter when checking whether a descriptor is a used descriptor, because driver also needs to check whether the descriptor has been made available. Below is an example:
Assuming ring size is 4, ring's initial state will be:
+----+----+----+----+
| 00 | 00 | 00 | 00 |
+----+----+----+----+
00 means AVAIL=0 USED=0, 01 means AVAIL=0 USED=1 10 means AVAIL=1 USED=0, 11 means AVAIL=1 USED=1
After driver made two descriptor chains available and each chain consists of two descriptors, the ring could be:
+----+-----------+----+-----------+
| 10 | 10 (id=0) | 10 | 10 (id=1) |
+----+-----------+----+-----------+
After device processed all the available descriptors and made them used (e.g. in order), the ring could be:
+-----------+----+-----------+----+
| 11 (id=0) | 10 | 11 (id=1) | 10 |
+-----------+----+-----------+----+
After driver processed all the used descriptors and made one descriptor (not chained, just one descriptor) available, the ring could be:
+-----------+----+----+----+
| 01 (id=0) | 10 | 11 | 10 |
+-----------+----+----+----+
After device made that descriptor used, the ring will be:
+-----------+----+----+----+
| 00 (id=0) | 10 | 11 | 10 |
+-----------+----+----+----+
If driver just checks whether USED bit equals to the used wrap counter when checking whether a descriptor is a used descriptor, after processing the first descriptor (whose AVAIL and USED bits are both 0), and advancing vq->next_used pointer, it will then also treat the next descriptor, i.e. the second descriptor (whose AVAIL and USED bits are 1 and 0 respectively) as a used descriptor which is wrong.
Fixes https://github.com/oasis-tcs/virtio-spec/issues/29
Signed-off-by Tiwei Bie <[email protected]>
---
v2:
- Add "Fixes" tag;
- Refine commit log;
v3:
- Compare with vq->used_wrap_count (MST);
- Add comments (MST);
- Refine commit log;
 packed-ring.tex | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)
packed-ring.tex +0 -0
index f24f49b..c8cd83b 100644
packed-ring.tex vq->driver_event.flags = RING_EVENT_FLAGS_DISABLE; · line 696
Quoted 10 lines
packed-ring.tex +7 -0
@@ -687,16 +687,29 @@ vq->driver_event.flags = RING_EVENT_FLAGS_DISABLE;
 for (;;) {
         struct pvirtq_desc *d = vq->desc[vq->next_used];
 
+        /*
+         * Check that
+         * 1. Descriptor has been made available.
+         *    Note: there are many other ways to check this, e.g.
+         *    track the number of outstanding available descriptors or buffers
+         *    and check that it's not 0.
+         * 2. Descriptor has been used by device.
s/device/the device/
Quoted 13 lines
packed-ring.tex +6 -2
+         */
         flags = d->flags;
+        bool avail = flags & VIRTQ_DESC_F_AVAIL;
         bool used = flags & VIRTQ_DESC_F_USED;
-
-        if (used != vq->used_wrap_count) {
+        if (avail != vq->used_wrap_count || used != vq->used_wrap_count) {
                 vq->driver_event.flags = RING_EVENT_FLAGS_ENABLE;
                 memory_barrier();
 
+                /*
+                 * Re-test in case another thread submitted more descriptors
+                 * and/or device used more descriptors before driver enabled events.
Quoted 6 lines
Quoted 4 lines
s/device/the device/
s/driver/the driver/
(Also in the patch description.)
Got it. Will fix them in the next version. Thanks!

Quoted 7 lines

And it's fine to mention threads but we really should also write it in terms of driver/device, used/available and parallel processing. E.g.
* Re-test in case the driver made more descriptors available in
* parallel with the used descriptor processing (e.g. from another
* thread) and/or the device used more descriptors before the driver
* enabled events.

Got it. Thanks!

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines

Quoted 10 lines

packed-ring.tex vq->driver_event.flags = RING_EVENT_FLAGS_DISABLE; · line 715

Quoted 9 lines

packed-ring.tex +3 -1

+                 */
                 flags = d->flags;
+                bool avail = flags & VIRTQ_DESC_F_AVAIL;
                 bool used = flags & VIRTQ_DESC_F_USED;
-                if (used != vq->used_wrap_count) {
+                if (avail != vq->used_wrap_count || used != vq->used_wrap_count) {
                         break;
                 }

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]

Michael S. Tsirkin

2018-12-05 15:14 UTC #5 lore raw

In reply to Tiwei Bie

I'd add to the commit log:
When driver is processing used descriptors in parallel with adding new available descriptors ...

and then as described.

Quoted 71 lines · Tiwei Bie

Driver can't just check whether USED bit equals to the used wrap counter when checking whether a descriptor is a used descriptor, because driver also needs to check whether the descriptor has been made available.
Below is an example:
Assuming ring size is 4, ring's initial state will be:
+----+----+----+----+
| 00 | 00 | 00 | 00 |
+----+----+----+----+
00 means AVAIL=0 USED=0, 01 means AVAIL=0 USED=1 10 means AVAIL=1 USED=0, 11 means AVAIL=1 USED=1
After driver made two descriptor chains available and each chain consists of two descriptors, the ring could be:
+----+-----------+----+-----------+
| 10 | 10 (id=0) | 10 | 10 (id=1) |
+----+-----------+----+-----------+
After device processed all the available descriptors and made them used (e.g. in order), the ring could be:
+-----------+----+-----------+----+
| 11 (id=0) | 10 | 11 (id=1) | 10 |
+-----------+----+-----------+----+
After driver processed all the used descriptors and made one descriptor (not chained, just one descriptor) available, the ring could be:
+-----------+----+----+----+
| 01 (id=0) | 10 | 11 | 10 |
+-----------+----+----+----+
After device made that descriptor used, the ring will be:
+-----------+----+----+----+
| 00 (id=0) | 10 | 11 | 10 |
+-----------+----+----+----+
If driver just checks whether USED bit equals to the used wrap counter when checking whether a descriptor is a used descriptor, after processing the first descriptor (whose AVAIL and USED bits are both 0), and advancing vq->next_used pointer, it will then also treat the next descriptor, i.e. the second descriptor (whose AVAIL and USED bits are 1 and 0 respectively) as a used descriptor which is wrong.
Fixes https://github.com/oasis-tcs/virtio-spec/issues/29
Signed-off-by Tiwei Bie <[email protected]>
---
v2:
- Add "Fixes" tag;
- Refine commit log;
v3:
- Compare with vq->used_wrap_count (MST);
- Add comments (MST);
- Refine commit log;
 packed-ring.tex | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)
packed-ring.tex +0 -0
index f24f49b..c8cd83b 100644

packed-ring.tex vq->driver_event.flags = RING_EVENT_FLAGS_DISABLE; · line 692

Quoted 6 lines

packed-ring.tex +3 -0

@@ -687,16 +687,29 @@ vq->driver_event.flags = RING_EVENT_FLAGS_DISABLE;
 for (;;) {
         struct pvirtq_desc *d = vq->desc[vq->next_used];
 
+        /*
+         * Check that
+         * 1. Descriptor has been made available.

Maybe here as well, we can say
... this check is necessary if driver is making
new descriptors available in parallel with this processing of used descriptors (e.g. from another thread).

Quoted 28 lines

packed-ring.tex +13 -3

+         *    Note: there are many other ways to check this, e.g.
+         *    track the number of outstanding available descriptors or buffers
+         *    and check that it's not 0.
+         * 2. Descriptor has been used by device.
+         */
         flags = d->flags;
+        bool avail = flags & VIRTQ_DESC_F_AVAIL;
         bool used = flags & VIRTQ_DESC_F_USED;
-
-        if (used != vq->used_wrap_count) {
+        if (avail != vq->used_wrap_count || used != vq->used_wrap_count) {
                 vq->driver_event.flags = RING_EVENT_FLAGS_ENABLE;
                 memory_barrier();
 
+                /*
+                 * Re-test in case another thread submitted more descriptors
+                 * and/or device used more descriptors before driver enabled events.
+                 */
                 flags = d->flags;
+                bool avail = flags & VIRTQ_DESC_F_AVAIL;
                 bool used = flags & VIRTQ_DESC_F_USED;
-                if (used != vq->used_wrap_count) {
+                if (avail != vq->used_wrap_count || used != vq->used_wrap_count) {
                         break;
                 }

Signature · 1 line

2.17.1

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]