Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <53831b99-6c63-7ca9-9b95-bc3f8c5e015c@redhat.com>
Date:   Thu, 7 Apr 2022 14:25:44 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.7.0
Subject: Re: [PATCH V2 3/5] virtio: introduce config op to synchronize vring
 callbacks
Content-Language: en-US
To:     "Michael S. Tsirkin" <mst@redhat.com>
Cc:     virtualization@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org, maz@kernel.org, tglx@linutronix.de,
        peterz@infradead.org, sgarzare@redhat.com,
        "Paul E. McKenney" <paulmck@kernel.org>
References: <20220406083538.16274-1-jasowang@redhat.com>
 <20220406083538.16274-4-jasowang@redhat.com>
 <20220406075405-mutt-send-email-mst@kernel.org>
From:   Jason Wang <jasowang@redhat.com>
In-Reply-To: <20220406075405-mutt-send-email-mst@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


在 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@linutronix.de>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: "Paul E. McKenney" <paulmck@kernel.org>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   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