Re: [dpdk-dev] [PATCH 9/9] vhost: only use vDPA config workaround if needed

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 6/18/20 8:39 AM, Matan Azrad wrote:
> HI Maxime
> 
> From: Maxime Coquelin:
>> On 6/17/20 1:04 PM, Matan Azrad wrote:
>>
>>>>> Don’t you think that only enabled queues must be fully initialized
>>>>> when
>>>> their status is changed from disabled to enabled?
>>>>> So, you can assume that disabled queues can stay "not fully initialized"...
>>>>
>>>> That may work but might not be following the Virtio spec as with 1.0
>>>> we shouldn't process the rings before DRIVER_OK is set (but we cannot
>>>> be sure we follow it anyway without SET_STATUS support).
>>>>
>>>> I propose to cook a patch doing the following:
>>>> 1. virtio_is_ready() will only ensure the first queue pair is ready
>>>> (i.e. enabled and configured). Meaning that app's new_device callback
>>>> and vDPA drivers dev_conf callback will be called with only the first
>>>> queue pair configured and enabled.
>>>>
>>>> 2. Before handling a new vhost-user request, it saves the ready
>>>> status for every queue pair.
>>>>
>>>> 3. Same handling of the requests, except that we won't notify the
>>>> vdpa driver and the application of vring state changes in the
>>>> VHOST_USER_SET_VRING_ENABLE handler.
>>>>
>>>> 4. Once the Vhost-user request is handled, it compares the new ready
>>>> status foe every queues with the old one and send queue state event
>>>> changes accordingly.
>>>
>>> Looks very nice to me.
>>
>> Cool!
>>
>>> More points:
>>> By this method some queues may be configured by the set_vring_state
>> operation so the next calls are expected to be called for each queue by the
>> driver from the set_vring_state callback :
>>> 1. rte_vhost_enable_guest_notification
>>> 	This one takes datapath lock so we need to be sure that datapath
>> lock is not locked on this queue from the same caller thread (maybe to not
>> takes datapath locks when vdpa is configured at all).
>>
>> Good point, I agree we shouldn't need to use the access lock when vdpa is
>> configured. We may want to document that all the control path is assumed to
>> be single thread though.
>>
>>
>>> 2. rte_vhost_host_notifier_ctrl
>>> 	This function API is per device and not per queue, maybe we need to
>> change this function to be per queue (add new for now and deprecate the
>> old one in 20.11).
>>
>> This one is still experimental, so no issue in reworking the API to make it per
>> queue without deprecation notice.
>>
>>> 3. Need to be sure that if ready queue configuration is changed after
>> dev_conf, we should notify it to the driver. (maybe by
>> set_vring_state(disabl) and set_vring_state(enable)).
>>
>> Agree, I'm not sure yet if we should just toggle set_vring_state as you
>> proposes, or if we should have a new callback for this.
> 
> Actually, when the queue configuration is changed, there is one moment that configuration is not valid (in the write time).
> So maybe it makes sense to toggle.
> 
> But there is one more option:
> 
> It doesn't make sense that after configuration change the QEMU will not send VHOST_USER_SET_VRING_ENABLE massage.

Agree.
> So maybe we need to call set_vring_state in the next events:
> 	1. queue becomes ready (enabled and fully configured) - set_vring_state(enable).
> 	2. queue becomes not ready - set_vring_state(disable).
> 	3. queue stay ready and VHOST_USER_SET_VRING_ENABLE massage was handled - set_vring_state(enable).
> 
> Then we need to document that every set_vring_state call may point on configuration changes in the queue even if the state was not changed.
> 
> What do you think?

I think it is worth a try.

Thanks,
Maxime