Messages in this thread | | | Date | Mon, 23 May 2022 17:01:27 -0700 | Subject | Re: [PATCH 1/4] vdpa: Add stop operation | From | Si-Wei Liu <> |
| |
On 5/23/2022 4:54 PM, Si-Wei Liu wrote: > > > On 5/23/2022 12:20 PM, Eugenio Perez Martin wrote: >> On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@oracle.com> >> wrote: >>> >>> >>> On 5/20/2022 10:23 AM, Eugenio Pérez wrote: >>>> This operation is optional: It it's not implemented, backend >>>> feature bit >>>> will not be exposed. >>>> >>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com> >>>> --- >>>> include/linux/vdpa.h | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h >>>> index 15af802d41c4..ddfebc4e1e01 100644 >>>> --- a/include/linux/vdpa.h >>>> +++ b/include/linux/vdpa.h >>>> @@ -215,6 +215,11 @@ struct vdpa_map_file { >>>> * @reset: Reset device >>>> * @vdev: vdpa device >>>> * Returns integer: success (0) or >>>> error (< 0) >>>> + * @stop: Stop or resume the device (optional, >>>> but it must >>>> + * be implemented if require device stop) >>>> + * @vdev: vdpa device >>>> + * @stop: stop (true), not stop (false) >>>> + * Returns integer: success (0) or error >>>> (< 0) >>> Is this uAPI meant to address all use cases described in the full blown >>> _F_STOP virtio spec proposal, such as: >>> >>> --------------%<-------------- >>> >>> ...... the device MUST finish any in flight >>> operations after the driver writes STOP. Depending on the device, it >>> can do it >>> in many ways as long as the driver can recover its normal operation >>> if it >>> resumes the device without the need of resetting it: >>> >>> - Drain and wait for the completion of all pending requests until a >>> convenient avail descriptor. Ignore any other posterior descriptor. >>> - Return a device-specific failure for these descriptors, so the driver >>> can choose to retry or to cancel them. >>> - Mark them as done even if they are not, if the kind of device can >>> assume to lose them. >>> --------------%<-------------- >>> >> Right, this is totally underspecified in this series. >> >> I'll expand on it in the next version, but that text proposed to >> virtio-comment was complicated and misleading. I find better to get >> the previous version description. Would the next description work? >> >> ``` >> After the return of ioctl, the device MUST finish any pending >> operations like >> in flight requests. It must also preserve all the necessary state (the >> virtqueue vring base plus the possible device specific states) > Hmmm, "possible device specific states" is a bit vague. Does it > require the device to save any device internal state that is not > defined in the virtio spec - such as any failed in-flight requests to > resubmit upon resume? Or you would lean on SVQ to intercept it in > depth and save it with some other means? I think network device also > has internal state such as flow steering state that needs bookkeeping > as well. Noted that I understand you may introduce additional feature call similar to VHOST_USER_GET_INFLIGHT_FD for (failed) in-flight request, but since that's is a get interface, I assume the actual state preserving should still take place in this STOP call.
-Siwei
> > A follow-up question is what is the use of the `stop` argument of > false, does it require the device to support resume? I seem to recall > this is something to abandon in favor of device reset plus setting > queue base/addr after. Or it's just a optional feature that may be > device specific (if one can do so in simple way). > > -Siwei > >> that is required >> for restoring in the future. >> >> In the future, we will provide features similar to >> VHOST_USER_GET_INFLIGHT_FD >> so the device can save pending operations. >> ``` >> >> Thanks for pointing it out! >> >> >> >> >> >>> E.g. do I assume correctly all in flight requests are flushed after >>> return from this uAPI call? Or some of pending requests may be subject >>> to loss or failure? How does the caller/user specify these various >>> options (if there are) for device stop? >>> >>> BTW, it would be nice to add the corresponding support to vdpa_sim_blk >>> as well to demo the stop handling. To just show it on vdpa-sim-net IMHO >>> is perhaps not so convincing. >>> >>> -Siwei >>> >>>> * @get_config_size: Get the size of the configuration space >>>> includes >>>> * fields that are conditional on >>>> feature bits. >>>> * @vdev: vdpa device >>>> @@ -316,6 +321,7 @@ struct vdpa_config_ops { >>>> u8 (*get_status)(struct vdpa_device *vdev); >>>> void (*set_status)(struct vdpa_device *vdev, u8 status); >>>> int (*reset)(struct vdpa_device *vdev); >>>> + int (*stop)(struct vdpa_device *vdev, bool stop); >>>> size_t (*get_config_size)(struct vdpa_device *vdev); >>>> void (*get_config)(struct vdpa_device *vdev, unsigned int >>>> offset, >>>> void *buf, unsigned int len); >
| |