Re: [PATCH] VIRTIO_F_USED_EVENT_AUTO_DISABLE: add new used buffer notification suppression mechanism

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

On Fri, Jul 05, 2024 at 11:12:47AM +0000, Lege Wang wrote:
> hi,
> 
> > >
> > > > -----Original Message-----
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Wednesday, July 3, 2024 8:34 PM
> > > > To: Lege Wang <lege.wang@jaguarmicro.com>
> > > > Cc: virtio-comment@lists.linux.dev; vattunuru@marvell.com;
> > > > ndabilpuram@marvell.com; parav@nvidia.com; Leo Liu
> > > > <leo.liu@jaguarmicro.com>; Angus Chen <angus.chen@jaguarmicro.com>
> > > > Subject: Re: [PATCH] VIRTIO_F_USED_EVENT_AUTO_DISABLE: add new used
> > > > buffer notification suppression mechanism
> > > >
> > > > External Mail: This email originated from OUTSIDE of the organization!
> > > > Do not click links, open attachments or provide ANY information unless you
> > > > recognize the sender and know the content is safe.
> > > >
> > > >
> > > > On Wed, Jul 03, 2024 at 12:14:01PM +0000, Lege Wang wrote:
> > > > > hi,
> > > > >
> > > > > > > > After reading this and the text, I can't figure out how is this feature
> > > > > > > > supposed to work without races.  What if the device uses a buffer
> > > > while
> > > > > > > > the driver notification is in flight to the device?
> > > > > > > Hmm, I think it doesn't matter, Only the used buffer enable notification
> > is
> > > > > > > transmitted to device successfully, device starts to judge whether it's
> > time
> > > > > > > to send used buffer notification to driver according to its internal
> > > > interrupt
> > > > > > policy.
> > > > > >
> > > > > > I don't understand what you are saying. If device has some magical
> > > > > > interrupt policy why do we need a notification from the driver
> > > > > > at all.
> > > > > >
> > > > > > Conversely, if device relies on the notification and thinks that an
> > > > > > interrupt is not needed (because it did not get the notification yet),
> > > > > > while at the same time the driver is blocked waiting for an interrupt,
> > > > > > we get a deadlock.
> > > > > No, I'm not saying that our interrupt policy is magical?? Let me explain a
> > bit
> > > > > more about its simple mechanism here, there're two basic parameters to
> > > > control
> > > > > whether device should send used buffer notification to driver:
> > > > >   1. number of used ring entries written since last used buffer notification
> > to
> > > > driver,
> > > > >     say threshold value is N.
> > > > >   2. Max wait time to send a used buffer notification to driver.
> > > > >
> > > > > The work flow would look like below:
> > > > > 1. driver enables used buffer notification initially when enabling virtio
> > device
> > > > > 2. driver is blocked waiting interrupt.
> > > > > 3. if the number of used ring entries written by device is equal to or
> > greater
> > > > than above threshold N,
> > > > >   a used buffered notification is sent to driver immediately, or if this
> > > > condition is not met, but the period
> > > > >   specified by max wait time has reached, device will still send one used
> > > > buffer notification to driver.
> > > > >   And device will recount number of used ring entries written by device
> > > > from zero and start another
> > > > >   Max wait time timer.
> > > > > 4. driver is waken up by interrupt, after it has processed used ring entries,
> > it'll
> > > > start to wait for interrupt
> > > > >   again and enable used buffer notification.
> > > > > 5. go to step 2.
> > > > >
> > > > > >From above work flow, I don't think the deadlock you describe would
> > > > happen.
> > > > >
> > > > > Regards,
> > > > > Xiaoguang Wang
> > > >
> > > > 1. device sends an interrupt
> > > > 2. device writes a used entry
> > > >
> > > > Will another interrupt be sent, with driver doing nothing at all?
> > > Unless driver enables device's used buffer notification, the device will
> > > not send an interrupt.
> > >
> > > Regards,
> > > Xiaoguang Wang
> > 
> > how about this:
> > 1. device sends an interrupt
> > 2. device writes a used entry
> > 2. driver enables interrupt
> 
> Indeed enabling used buffer notification and sending used buffer notification are matched operations.
> Time-sequence |  driver           |  device
> T1           | enable notification  |
> T2           |                  |  send notification
> T3           |                  |
> T4           | enable notification  |
> T5           |                  |  send notification
> 
> That means once driver enables interrupt, device will ensure that it will send an
> Interrupt if there are used events written. If driver does not enable interrupt at all,
> Device will not send interrupt.
> 
> But yet there is a case to take care, which driver does not handle all used buffer events it received,
> parav explained It would happen before. As I said in this proposal, when driver enables interrupt, it'll
> also send driver's last_used_idx info to device, see below handling:
>   1 driver enable interrupt
>   2 driver submit 5 requests
>   3 all 5 request are completed, device sends interrupt, now device's used_idx is 5.
>   4 driver gets interrupt, and it just handled 3 requests, so last_used_idx is 3.
>   5 driver enables interrupt and attach last_used_idx(3) info.
>   6 device will know that driver does not handle 2 requests and will add these two requests to
>     the number of used ring entries written since last used buffer notification, that means
>     though if there is no new used ring entries written, because of these two outstanding requests,
>     device will still send an interrupt.
> 
> So I still think the deadlock you describe would not happen.
> 
> Regards,
> Xiaoguang Wang

Hmm this is different from event index (which simple triggers when
device is using a buffer at a given index).

We will need to define exactly what is the index sent in the
request and how does device handle it. Here you described
an example, but I don't really know what exactly is expected
generally. So device gets some index, how does it decide whether
to send this immediate notification?


Also looks like this is less "enabling notifications" and more "request
notifications".





> > to
> 
> Regards,
> Xiaoguang Wang
> > 
> > 
> > 
> > > >
> > > >
> > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > MST