OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.

 


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-dev message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]


Subject: RE: [PATCH v2] Add device reset timeout field



> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Thursday, October 7, 2021 2:23 AM
> On Wed, Oct 06, 2021 at 04:11:19PM +0000, Parav Pandit wrote:
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Wednesday, October 6, 2021 8:52 PM
> > >
> > > On Wed, Oct 06, 2021 at 05:10:33PM +0300, Parav Pandit wrote:
> > > > Motivation:
> > > > ==========
> > > > Currently when driver initiates a device reset operation, a device
> > > > may not be able finish the reset operation immediately. In such
> > > > case driver waits for an arbitrary amount of time in a hope that
> > > > device will finish the reset.
> > >
> > > Hmm does it? Where does spec say that it does? Does not linux really
> > > wait forever?
> > This proposal doesn't claim that spec says so.
> > Linux pci transport waits infinitely.
> > Infinite is a valid arbitrary amount. Isn't it ?:-)

[..]
> > > First, do not use should outside normative sections. e.g. you can
> > > say "is expected".
> > >
So this proposal used the SHOULD in the section 2.1.1 which is the normative section as per the driver conformance clause 1.
Or I misunderstood?

> > should is used in same section which has used _must_ in it.
> 
> yea, some of these things slip through.
> 
> > Can you please help me understand what makes a section normative?
> 
> It starts with
> \devicenormative
> or
> \drivernormative
> 
> and is linked from the conformance section.
>
Ok. I understand better now.

> >
> > > second, I don't see a story around compatibility here.
> > Device reset timeout is supported only when device offers the new
> VIRTIO_DEV_RESET_TIMEOUT.
> > When such feature is not offered, driver follows the guidance provided in
> this proposal.
> > Compatibility is covered with the feature bit.
> 
> Well hmm. Problem is, devices need to be reset before feature bits are read.
>
Yeah, this means that we should remove the feature bit and instead follow your suggestion that if the register value is zero, the feature is not offered,
If it is non zero, driver should follow the timeout.
 
> > > previously pci drivers did wait, other drivers didn't.
> > >
> > The newer driver that implements updated spec, will adhere to newer
> content of the spec.
> 
> right but we can't just unconditionally say driver must wait, this would declare
> old driver non-compliant and we promised not to do that.
> 
It is not said unconditionally. It comes with spec version, isn't it?
This change should increment the spec version so old driver are compliant to some older version of the spec.
Hence, there is no breakage with some old driver.

> > > I think drivers that actually do wait should somehow let the device
> > > know it's ready to wait.
> > >
> > Why should driver tell device that it is waiting?
> > Driver initiated a reset, device may take its time for reset and eventually
> comes out.
> > What can device do differently by knowing that driver is waiting for reset to
> finish?
> > Device usually follow its standard steps to complete the reset action.
> > I do not anticipate that device will do reset differently if it knows that no one
> is waiting for reset to finish.
> 
> I think it might matter for hypervisors, these can thinkably block the write until
> reset completes if driver is not going to wait.  For the hardware - if driver is not
> waiting then hardware has to block the response to the following non posted
> transaction whatever that is.
Hardware will continue to follow reset semantics regardless of advertising the timeout. (and hence 

> Doing that blocks the cpu and generally breaks the whole timeout idea. Which
> you seem to want for some reason ...
No. advertising the reset timeout value doesn't change the reset flow of device or driver.
So it doesn't break anything.
It only means that linux driver code doing vp_reset() infinitely, will instead do it for the timeout given by device.

> 
> 
> >
> > > Third how about making e.g. 0 a special value meaning no limit wait
> forever?
> > The whole idea is to have some finite/deterministic behavior,
> 
> I guess I'm being dense, I just don't yet understand the motivation.
> The cost is difficulty in migration since each and every piece of hardware will
> have a different timeout.
>
Such timeout is already there. Advertising timeout doesn't change the LM flow.
A migrating device at the destination is anyway out of the reset when migration occurs.
 
> The idea is to have finite behaviour, I got that, but I don't really know where
> does the requirement come from, in what sense it's deterministic and why do
> we worry about reset specifically.
> 
> Recovering from failure when hardware is broken?
>
I provided few examples in previous email.
Terminating a probe sequence of the device which is already on its path of hotunplug.
 
> Can we even trust timeout values hardware gives us then?
> 
Sure, why not. Device usually going to provide the reset timeout value based on its backend implementation.
If its completely broken device (driver can always do min() of timeout value given by device and its own).

> What about a ton of other stuff like command vq commands timing out?
>
VQ commands timeout handling is indeed needed but it is a separate enhancement in itself.
This one is self-contained enough that doesn't need to combined with command timeout.
 
> > and hence the reasonable finite value.
> > When value is not specified, driver is expected to choose reasonable value
> terminate the reset operation.
> 
> No one has any idea what's reasonable.
I am sure a finite timeout in driver is reasonable than current infinite one.
This is because it is making system more resilient to occasional device errors.

> There's no reason for a driver to choose any value - it has nothing else to do.
>
At least for the devices that we are seeing, driver choosing a default value of 2 to 3 minutes is good enough and of course useful.

> > Do you propose 0 as no limit to follow current driver behavior?
> 
> Or maybe 0 will make driver poll.
Poll infinitely? Didn't follow the suggestion of 0.

> 
> --
> MST



[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]