| Message ID | 20210507170634.2058801-3-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-blk: Convert QEMUBH callback to "bitops.h" API | expand |
On 5/7/21 10:06 AM, Philippe Mathieu-Daudé wrote: > By directly using find_first_bit() and find_next_bit from the > "bitops.h" API to iterate over the bitmap, we can remove the > bitmap[] variable-length array copy on the stack and the complex > manual bit testing/clearing logic. > > Suggested-by: Stefan Hajnoczi<stefanha@redhat.com> > Suggested-by: Richard Henderson<richard.henderson@linaro.org> > Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com> > --- > hw/block/dataplane/virtio-blk.c | 19 ++++--------------- > 1 file changed, 4 insertions(+), 15 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Fri, May 07, 2021 at 07:06:34PM +0200, Philippe Mathieu-Daudé wrote: > By directly using find_first_bit() and find_next_bit from the > "bitops.h" API to iterate over the bitmap, we can remove the > bitmap[] variable-length array copy on the stack and the complex > manual bit testing/clearing logic. > > Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> > Suggested-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/block/dataplane/virtio-blk.c | 19 ++++--------------- > 1 file changed, 4 insertions(+), 15 deletions(-) > > diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c > index e9050c8987e..a7b5bda06fc 100644 > --- a/hw/block/dataplane/virtio-blk.c > +++ b/hw/block/dataplane/virtio-blk.c > @@ -60,23 +60,12 @@ static void notify_guest_bh(void *opaque) > { > VirtIOBlockDataPlane *s = opaque; > unsigned nvqs = s->conf->num_queues; > - unsigned long bitmap[BITS_TO_LONGS(nvqs)]; > - unsigned j; > > - memcpy(bitmap, s->batch_notify_vqs, sizeof(bitmap)); > - memset(s->batch_notify_vqs, 0, sizeof(bitmap)); > + for (unsigned long i = find_first_bit(s->batch_notify_vqs, nvqs); > + i < nvqs; i = find_next_bit(s->batch_notify_vqs, nvqs, i)) { > + VirtQueue *vq = virtio_get_queue(s->vdev, i); > > - for (j = 0; j < nvqs; j += BITS_PER_LONG) { > - unsigned long bits = bitmap[j / BITS_PER_LONG]; > - > - while (bits != 0) { > - unsigned i = j + ctzl(bits); > - VirtQueue *vq = virtio_get_queue(s->vdev, i); > - > - virtio_notify_irqfd(s->vdev, vq); > - > - bits &= bits - 1; /* clear right-most bit */ > - } > + virtio_notify_irqfd(s->vdev, vq); > } > } Bits in s->batch_notify_vqs[] must be cleared. Otherwise we may raise spurious irqs next time this function is called. The memset() can be moved after the loop. Stefan
On 5/10/21 3:25 PM, Stefan Hajnoczi wrote: > On Fri, May 07, 2021 at 07:06:34PM +0200, Philippe Mathieu-Daudé wrote: >> By directly using find_first_bit() and find_next_bit from the >> "bitops.h" API to iterate over the bitmap, we can remove the >> bitmap[] variable-length array copy on the stack and the complex >> manual bit testing/clearing logic. >> >> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> >> Suggested-by: Richard Henderson <richard.henderson@linaro.org> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/block/dataplane/virtio-blk.c | 19 ++++--------------- >> 1 file changed, 4 insertions(+), 15 deletions(-) >> >> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c >> index e9050c8987e..a7b5bda06fc 100644 >> --- a/hw/block/dataplane/virtio-blk.c >> +++ b/hw/block/dataplane/virtio-blk.c >> @@ -60,23 +60,12 @@ static void notify_guest_bh(void *opaque) >> { >> VirtIOBlockDataPlane *s = opaque; >> unsigned nvqs = s->conf->num_queues; >> - unsigned long bitmap[BITS_TO_LONGS(nvqs)]; >> - unsigned j; >> >> - memcpy(bitmap, s->batch_notify_vqs, sizeof(bitmap)); >> - memset(s->batch_notify_vqs, 0, sizeof(bitmap)); >> + for (unsigned long i = find_first_bit(s->batch_notify_vqs, nvqs); >> + i < nvqs; i = find_next_bit(s->batch_notify_vqs, nvqs, i)) { >> + VirtQueue *vq = virtio_get_queue(s->vdev, i); >> >> - for (j = 0; j < nvqs; j += BITS_PER_LONG) { >> - unsigned long bits = bitmap[j / BITS_PER_LONG]; >> - >> - while (bits != 0) { >> - unsigned i = j + ctzl(bits); >> - VirtQueue *vq = virtio_get_queue(s->vdev, i); >> - >> - virtio_notify_irqfd(s->vdev, vq); >> - >> - bits &= bits - 1; /* clear right-most bit */ >> - } >> + virtio_notify_irqfd(s->vdev, vq); >> } >> } > > Bits in s->batch_notify_vqs[] must be cleared. Otherwise we may raise > spurious irqs next time this function is called. The memset() can be > moved after the loop. Doh... good catch! I missed it when removing the previous test_and_clear_bit() call (from v1).
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c index e9050c8987e..a7b5bda06fc 100644 --- a/hw/block/dataplane/virtio-blk.c +++ b/hw/block/dataplane/virtio-blk.c @@ -60,23 +60,12 @@ static void notify_guest_bh(void *opaque) { VirtIOBlockDataPlane *s = opaque; unsigned nvqs = s->conf->num_queues; - unsigned long bitmap[BITS_TO_LONGS(nvqs)]; - unsigned j; - memcpy(bitmap, s->batch_notify_vqs, sizeof(bitmap)); - memset(s->batch_notify_vqs, 0, sizeof(bitmap)); + for (unsigned long i = find_first_bit(s->batch_notify_vqs, nvqs); + i < nvqs; i = find_next_bit(s->batch_notify_vqs, nvqs, i)) { + VirtQueue *vq = virtio_get_queue(s->vdev, i); - for (j = 0; j < nvqs; j += BITS_PER_LONG) { - unsigned long bits = bitmap[j / BITS_PER_LONG]; - - while (bits != 0) { - unsigned i = j + ctzl(bits); - VirtQueue *vq = virtio_get_queue(s->vdev, i); - - virtio_notify_irqfd(s->vdev, vq); - - bits &= bits - 1; /* clear right-most bit */ - } + virtio_notify_irqfd(s->vdev, vq); } }
By directly using find_first_bit() and find_next_bit from the "bitops.h" API to iterate over the bitmap, we can remove the bitmap[] variable-length array copy on the stack and the complex manual bit testing/clearing logic. Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> Suggested-by: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/block/dataplane/virtio-blk.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-)