Re: [PATCH] virtio_blk: always post notifications under the lock

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Jan 22, 2025 at 05:49:18PM +0000, Boyer, Andrew wrote:
> 
> 
> > On Jan 22, 2025, at 12:33 PM, Christian Borntraeger <borntraeger@linux.ibm.com> wrote:
> > 
> > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> > 
> > 
> > Am 22.01.25 um 15:44 schrieb Boyer, Andrew:
> > [...]
> > 
> >>>>> --- a/drivers/block/virtio_blk.c
> >>>>> +++ b/drivers/block/virtio_blk.c
> >>>>> @@ -379,14 +379,10 @@ static void virtio_commit_rqs(struct blk_mq_hw_ctx *hctx)
> >>>>> {
> >>>>>   struct virtio_blk *vblk = hctx->queue->queuedata;
> >>>>>   struct virtio_blk_vq *vq = &vblk->vqs[hctx->queue_num];
> >>>>> -   bool kick;
> >>>>>   spin_lock_irq(&vq->lock);
> >>>>> -   kick = virtqueue_kick_prepare(vq->vq);
> >>>>> +   virtqueue_kick(vq->vq);
> >>>>>   spin_unlock_irq(&vq->lock);
> >>>>> -
> >>>>> -   if (kick)
> >>>>> -           virtqueue_notify(vq->vq);
> >>>>> }
> >>>> 
> >>>> I would assume this will be a performance nightmare for normal IO.
> >>> 
> >> 
> >> Hello Michael and Christian and Jason,
> >> Thank you for taking a look.
> >> 
> >> Is the performance concern that the vmexit might lead to the underlying virtual storage stack doing the work immediately? Any other job posting to the same queue would presumably be blocked on a vmexit when it goes to attempt its own notification. That would be almost the same as having the other job block on a lock during the operation, although I guess if you are skipping notifications somehow it would look different.
> > 
> > The performance concern is that you hold a lock and then exit. Exits are expensive and can schedule so you will increase the lock holding time significantly. This is begging for lock holder preemption.
> > Really, dont do it.
> 
> If holding a lock at all is unworkable, do you have a suggestion for how we might fix the correctness issue?
> Because this is definitely a correctness issue.
> 
> I don't see anything in the spec about requiring support for out-of-order notifications.
> 
> -Andrew

Yes we didn't think it through in the spec :(
So driver made one assumption, for performance,
and the device made another one, for simplicity.

The data needed for the device to solve it, is there in the
notification. We can make the safe choice with the lock, and add a
feature flag for devices who can handle out of order,
or make the current choice and add a feature flag for
devices that need a lock.

Let's continue the discussion in another thread whether
a hardware workaround is possible.


-- 
MST