Re: [PATCH v4 04/25] virtio: defer config changed notifications

[Michael S. Tsirkin]

On Tue, Oct 14, 2014 at 11:01:12AM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@xxxxxxxxxx> writes:
> > Defer config changed notifications that arrive during
> > probe/scan/freeze/restore.
> >
> > This will allow drivers to set DRIVER_OK earlier, without worrying about
> > racing with config change interrupts.
> >
> > This change will also benefit old hypervisors (before 2009)
> > that send interrupts without checking DRIVER_OK: previously,
> > the callback could race with driver-specific initialization.
> >
> > This will also help simplify drivers.
> 
> But AFAICT you never *read* dev->config_changed.
> 
> You unconditionally trigger a config_changed event in
> virtio_config_enable().  That's a bit weird, but probably OK.
> 
> How about the following change (on top of your patch).  I
> think the renaming is clearer, and note the added if() test in
> virtio_config_enable().
> 
> If you approve, I'll fold it in.
> 
> Cheers,
> Rusty.

Hi Rusty,
I'm okay with both changes.
You can fold it in if you prefer, or just make it a patch on top.

> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 2536701b098b..df598dd8c5c8 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -122,7 +122,7 @@ static void __virtio_config_changed(struct virtio_device *dev)
>  	struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
>  
>  	if (!dev->config_enabled)
> -		dev->config_changed = true;
> +		dev->config_change_pending = true;
>  	else if (drv && drv->config_changed)
>  		drv->config_changed(dev);
>  }
> @@ -148,8 +148,9 @@ static void virtio_config_enable(struct virtio_device *dev)
>  {
>  	spin_lock_irq(&dev->config_lock);
>  	dev->config_enabled = true;
> -	__virtio_config_changed(dev);
> -	dev->config_changed = false;
> +	if (dev->config_change_pending)
> +		__virtio_config_changed(dev);
> +	dev->config_change_pending = false;
>  	spin_unlock_irq(&dev->config_lock);
>  }
>  
> @@ -253,7 +254,7 @@ int register_virtio_device(struct virtio_device *dev)
>  
>  	spin_lock_init(&dev->config_lock);
>  	dev->config_enabled = false;
> -	dev->config_changed = false;
> +	dev->config_change_pending = false;
>  
>  	/* We always start by resetting the device, in case a previous
>  	 * driver messed it up.  This also tests that code path a little. */
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 5636b119dc25..65261a7244fc 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -80,7 +80,7 @@ bool virtqueue_is_broken(struct virtqueue *vq);
>   * @index: unique position on the virtio bus
>   * @failed: saved value for CONFIG_S_FAILED bit (for restore)
>   * @config_enabled: configuration change reporting enabled
> - * @config_changed: configuration change reported while disabled
> + * @config_change_pending: configuration change reported while disabled
>   * @config_lock: protects configuration change reporting
>   * @dev: underlying device.
>   * @id: the device type identification (used to match it with a driver).
> @@ -94,7 +94,7 @@ struct virtio_device {
>  	int index;
>  	bool failed;
>  	bool config_enabled;
> -	bool config_changed;
> +	bool config_change_pending;
>  	spinlock_t config_lock;
>  	struct device dev;
>  	struct virtio_device_id id;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/