[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 0/2] virtio-blk: add iothread-vq-mapping parameter
From: |
Kevin Wolf |
Subject: |
Re: [PATCH v2 0/2] virtio-blk: add iothread-vq-mapping parameter |
Date: |
Tue, 7 Nov 2023 11:20:56 +0100 |
Am 07.11.2023 um 04:00 hat Stefan Hajnoczi geschrieben:
> On Thu, Nov 02, 2023 at 03:10:52PM +0100, Kevin Wolf wrote:
> > Am 18.09.2023 um 18:16 hat Stefan Hajnoczi geschrieben:
> > > virtio-blk and virtio-scsi devices need a way to specify the mapping
> > > between
> > > IOThreads and virtqueues. At the moment all virtqueues are assigned to a
> > > single
> > > IOThread or the main loop. This single thread can be a CPU bottleneck, so
> > > it is
> > > necessary to allow finer-grained assignment to spread the load. With this
> > > series applied, "pidstat -t 1" shows that guests with -smp 2 or higher
> > > are able
> > > to exploit multiple IOThreads.
> > >
> > > This series introduces command-line syntax for the new iothread-vq-mapping
> > > property is as follows:
> > >
> > > --device
> > > '{"driver":"virtio-blk-pci","iothread-vq-mapping":[{"iothread":"iothread0","vqs":[0,1,2]},...]},...'
> > >
> > > IOThreads are specified by name and virtqueues are specified by 0-based
> > > index.
> > >
> > > It will be common to simply assign virtqueues round-robin across a set
> > > of IOThreads. A convenient syntax that does not require specifying
> > > individual virtqueue indices is available:
> > >
> > > --device
> > > '{"driver":"virtio-blk-pci","iothread-vq-mapping":[{"iothread":"iothread0"},{"iothread":"iothread1"},...]},...'
> > >
> > > There is no way to reassign virtqueues at runtime and I expect that to be
> > > a
> > > very rare requirement.
> > >
> > > Note that JSON --device syntax is required for the iothread-vq-mapping
> > > parameter because it's non-scalar.
> > >
> > > Based-on: 20230912231037.826804-1-stefanha@redhat.com ("[PATCH v3 0/5]
> > > block-backend: process I/O in the current AioContext")
> >
> > Does this strictly depend on patch 5/5 of that series, or would it just
> > be a missed opportunity for optimisation by unnecessarily running some
> > requests from a different thread?
>
> "[PATCH v3 5/5] block-coroutine-wrapper: use
> qemu_get_current_aio_context()" is necessary so that
> virtio_blk_sect_range_ok -> blk_get_geometry -> blk_nb_sectors ->
> bdrv_refresh_total_sectors -> bdrv_poll_co can be called without holding
> the AioContext lock.
Ooh, so we only have the whole problem because bdrv_poll_co() wants to
temporarily unlock an AioContext that we don't even hold? That's a real
shame, but I understand now why we need the patch.
> That case only happens when the BlockDriverState is a file-posix host
> CD-ROM or a file-win32 host_device. Most users will never hit this
> problem, but it would be unsafe to proceed merging code without this
> patch.
Yes, I agree.
Kevin
signature.asc
Description: PGP signature