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

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

On Thu, Jan 23, 2025 at 09:39:44AM +0100, Christian Borntraeger wrote:
> 
> Am 22.01.25 um 23:07 schrieb Michael S. Tsirkin:
> > On Wed, Jan 22, 2025 at 06:33:04PM +0100, Christian Borntraeger wrote:
> > > 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.
> > 
> > 
> > The issue is with hardware that wants a copy of an index sent to
> > it in a notification. Now, you have a race:
> > 
> > 
> > thread 1:
> > 
> > 	index = 1
> > 		-> 			-> send 1 to hardware
> > 
> > thread 2:
> > 
> > 	index = 2
> > 		-> send 2 to hardware
> > 
> > 
> > 
> > 
> > the spec unfortunately does not say whether that is legal.
> > 
> > As far as I could tell, the device can easily use the
> > wrap counter inside the notification to detect this
> > and simply discard the "1" notification.
> > 
> > 
> > If not, I'd like to understand why.
> 
> Yes I agree with you here.
> I just want to emphasize that holding a lock during notify is not a workable solution.


Andrew, what do you say?

-- 
MST