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
