[dpdk-dev] [PATCH 01/17] vhost: introduce driver features related APIs

[Maxime Coquelin]

On 03/16/2017 08:08 AM, Yuanhan Liu wrote:
> On Tue, Mar 14, 2017 at 10:53:23AM +0100, Maxime Coquelin wrote:
>>
>>
>> On 03/14/2017 10:46 AM, Maxime Coquelin wrote:
>>>
>>>
>>> On 03/03/2017 10:51 AM, Yuanhan Liu wrote:
>>>> Introduce few APIs to set/get/enable/disable driver features.
>>>>
>>>> Signed-off-by: Yuanhan Liu <yuanhan.liu at linux.intel.com>
>>>> ---
>>>> lib/librte_vhost/rte_vhost_version.map | 10 ++++
>>>> lib/librte_vhost/rte_virtio_net.h      |  9 ++++
>>>> lib/librte_vhost/socket.c              | 90
>>>> ++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 109 insertions(+)
>>>
>>> Nice!
>>> Reviewed-by: Maxime Coquelin <maxime.coquelin at redhat.com>
>>>
>>> I wonder whether we could protect from setting/enabling/disabling
>>> features once negotiation is done?
>
> Those APIs won't be able to change the negotiated features. They are
> just some interfaces before the vhost-user connection is established.

Right.
I meant it could return an error is the connection is already
established. Else, the caller might think the feature has been
successfully enabled/disabled, whereas it is not the case.
But this is maybe over-engineering to handle this case.

> Ideally, we could/should get rid of the enabling/disabling functions:
> let the vhost-user driver to handle the features directly (say, for
> vhost-user pmd, we could use vdev options to disable/enable few specific
> features). Once that is done, use the rte_vhost_driver_set_features()
> set the features once.

Ok, but what about vhost lib users and TSO (I'm thinking of OVS).

> The reason I introduced the enable/disable_features() is to keep the
> compatability with the builtin vhost-user net driver (virtio_net.c).
> If there is a plan to move it into vhost-pmd, they won't be needed.
> And I don't think that will happen soon.

I agree with you that would be ideal, but unlikely to happen soon.

>> Oh, I forgot one comment on this patch.
>> Since these new functions are part to the API, shouldn't them be
>> documented?
>
> Yes, indeed, it's also noted in my cover letter.

Oops, I missed that!

Thanks,
Maxime
> 	--yliu
>