On 9/25/23 16:23, Stefan Hajnoczi wrote: > On Fri, 25 Aug 2023 at 13:04, Ilya Maximets <i.maxim...@ovn.org> wrote: >> >> We do not need the most up to date number of heads, we only want to >> know if there is at least one. >> >> Use shadow variable as long as it is not equal to the last available >> index checked. This avoids expensive qatomic dereference of the >> RCU-protected memory region cache as well as the memory access itself >> and the subsequent memory barrier. >> >> The change improves performance of the af-xdp network backend by 2-3%. >> >> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org> >> --- >> hw/virtio/virtio.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> index 309038fd46..04bf7cc977 100644 >> --- a/hw/virtio/virtio.c >> +++ b/hw/virtio/virtio.c >> @@ -999,7 +999,15 @@ void virtqueue_push(VirtQueue *vq, const >> VirtQueueElement *elem, >> /* Called within rcu_read_lock(). */ >> static int virtqueue_num_heads(VirtQueue *vq, unsigned int idx) >> { >> - uint16_t num_heads = vring_avail_idx(vq) - idx; >> + uint16_t num_heads; >> + >> + if (vq->shadow_avail_idx != idx) { >> + num_heads = vq->shadow_avail_idx - idx; >> + >> + return num_heads; > > This still needs to check num_heads > vq->vring.num and return -EINVAL > as is done below.
Hmm, yeas, you're right. If the value was incorrect initially, the shadow will be incorrect. However, I think we should just not return here in this case and let vring_avail_idx() to grab an actual new value below. Otherwise we may never break out of this error. Does that make sense? > >> + } >> + >> + num_heads = vring_avail_idx(vq) - idx; >> >> /* Check it isn't doing very strange things with descriptor numbers. */ >> if (num_heads > vq->vring.num) { >> -- >> 2.40.1 >> >>