Re: [PATCH V2 3/5] virtio: introduce config op to synchronize vring callbacks

[Jason Wang]

在 2022/4/6 下午7:59, Michael S. Tsirkin 写道:
> On Wed, Apr 06, 2022 at 04:35:36PM +0800, Jason Wang wrote:
> > This patch introduce
> introduces
>
> > a new
> new
>
> > virtio config ops to vring
> > callbacks. Transport specific method is required to call
> > synchornize_irq() on the IRQs. For the transport that doesn't provide
> > synchronize_vqs(), use synchornize_rcu() as a fallback.
> >
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Cc: "Paul E. McKenney" <paulmck@xxxxxxxxxx>
> > Cc: Marc Zyngier <maz@xxxxxxxxxx>
> > Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
> > ---
> >   include/linux/virtio_config.h | 16 ++++++++++++++++
> >   1 file changed, 16 insertions(+)
> >
> > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> > index b341dd62aa4d..08b73d9bbff2 100644
> > --- a/include/linux/virtio_config.h
> > +++ b/include/linux/virtio_config.h
> > @@ -57,6 +57,8 @@ struct virtio_shm_region {
> >    *		include a NULL entry for vqs unused by driver
> >    *	Returns 0 on success or error status
> >    * @del_vqs: free virtqueues found by find_vqs().
> > + * @synchronize_vqs: synchronize with the virtqueue callbacks.
> > + *	vdev: the virtio_device
> I think I prefer synchronize_callbacks


Ok, I will rename it.


> >    * @get_features: get the array of feature bits for this device.
> >    *	vdev: the virtio_device
> >    *	Returns the first 64 feature bits (all we currently need).
> > @@ -89,6 +91,7 @@ struct virtio_config_ops {
> >   			const char * const names[], const bool *ctx,
> >   			struct irq_affinity *desc);
> >   	void (*del_vqs)(struct virtio_device *);
> > +	void (*synchronize_vqs)(struct virtio_device *);
> >   	u64 (*get_features)(struct virtio_device *vdev);
> >   	int (*finalize_features)(struct virtio_device *vdev);
> >   	const char *(*bus_name)(struct virtio_device *vdev);
> > @@ -217,6 +220,19 @@ int virtio_find_vqs_ctx(struct virtio_device *vdev, unsigned nvqs,
> >   				      desc);
> >   }
> >   
> > +/**
> > + * virtio_synchronize_vqs - synchronize with virtqueue callbacks
> > + * @vdev: the device
> > + */
> > +static inline
> > +void virtio_synchronize_vqs(struct virtio_device *dev)
> > +{
> > +	if (dev->config->synchronize_vqs)
> > +		dev->config->synchronize_vqs(dev);
> > +	else
> > +		synchronize_rcu();
> I am not sure about this fallback and the latency impact.


Unless each transport can implement their own synchronization routine, 
we need something that can do best effort as fallback here.


> Maybe synchronize_rcu_expedited is better here.


Not sure, it might lead IPIs and according to the 
Documentation/RCU/checklist.rst:


"""

        The expedited forms of these primitives have the same semantics
        as the non-expedited forms, but expediting is both expensive and
        (with the exception of synchronize_srcu_expedited()) unfriendly
        to real-time workloads.  Use of the expedited primitives should
        be restricted to rare configuration-change operations that would
        not normally be undertaken while a real-time workload is running.
        However, real-time workloads can use rcupdate.rcu_normal kernel
        boot parameter to completely disable expedited grace periods,
        though this might have performance implications.

"""

It will be expensive for real time workloads.

Thanks


> > +}
> > +
> >   /**
> >    * virtio_device_ready - enable vq use in probe function
> >    * @vdev: the device
> > --
> > 2.25.1