Re: [dpdk-dev] [PATCH v4 1/8] lib/librte_vhost: add external backend support

["Tan, Jianfeng" <jianfeng.tan@intel.com>]

On 4/3/2018 9:44 PM, Maxime Coquelin wrote:
> Hi Pawel, Fan,
>
> On 04/01/2018 09:53 PM, Zhang, Roy Fan wrote:
>> Hi Pawel,
>>
>>> -----Original Message-----
>>> From: Wodkowski, PawelX
>>> Sent: Thursday, March 29, 2018 2:48 PM
>>> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; dev@dpdk.org
>>> Cc: maxime.coquelin@redhat.com; jianjay.zhou@huawei.com; Tan, Jianfeng
>>> <jianfeng.tan@intel.com>
>>> Subject: RE: [dpdk-dev] [PATCH v4 1/8] lib/librte_vhost: add external
>>> backend support
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Fan Zhang
>>>> Sent: Thursday, March 29, 2018 2:53 PM
>>>> To: dev@dpdk.org
>>>> Cc: maxime.coquelin@redhat.com; jianjay.zhou@huawei.com; Tan,
>>> Jianfeng
>>>> <jianfeng.tan@intel.com>
>>>> Subject: [dpdk-dev] [PATCH v4 1/8] lib/librte_vhost: add external
>>>> backend support
>>>>
>>>> This patch adds external backend support to vhost library. The patch
>>>> provides new APIs for the external backend to register pre and post
>>>> vhost-user message handlers.
>>>>
>>>> Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
>>>> ---
>>>>   lib/librte_vhost/rte_vhost.h           | 64
>>>> +++++++++++++++++++++++++++++++++-
>>>>   lib/librte_vhost/rte_vhost_version.map |  6 ++++
>>>>   lib/librte_vhost/vhost.c               | 17 ++++++++-
>>>>   lib/librte_vhost/vhost.h               |  8 +++--
>>>>   lib/librte_vhost/vhost_user.c          | 33 +++++++++++++++++-
>>>>   5 files changed, 123 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/lib/librte_vhost/rte_vhost.h
>>>> b/lib/librte_vhost/rte_vhost.h index d332069..b902c44 100644
>>>> --- a/lib/librte_vhost/rte_vhost.h
>>>> +++ b/lib/librte_vhost/rte_vhost.h
>>>> @@ -1,5 +1,5 @@
>
> <snip/>
>
>>
>>>> + * @param require_reply
>>>> + *  If the handler requires sending a reply, this varaible shall be
>>>> +written 1,
>>>> + *  otherwise 0.
>>>> + * @return
>>>> + *  0 on success, -1 on failure
>>>> + */
>>>> +typedef int (*rte_vhost_msg_post_handle)(int vid, void *msg,
>>>> +        uint32_t *require_reply);
>>>> +
>>>
>>> What mean 'Message pointer' Is this const for us? Is this payload? 
>>> Making
>>> msg 'void *' is not a way to go here. Those pre and post handlers 
>>> need to see
>>> exactly the same structures like vhost_user.c file. Otherwise we can 
>>> get into
>>> troubles when ABI changes.
>>
>> It is the pointer to the vhost_user message. It cannot be const as 
>> the backend
>> may change the payload.
>>
>>>
>>> Also you can easily merge pre and post handlers into one handler 
>>> with one
>>> Parameter describing what phase of message processing we are now.
>>>
>>
>> No I don't think so. To do so it will be quite unclear in the future 
>> as we are
>> using one function to do two totally different things.
>
> Time is running out for v18.05 integration deadline (April 6th), and 
> we haven't reached a consensus.
>
> Except this API point, I think vhost-crypto is at the right level.
> Since vhost-crypto lives in librte_vhost, I propose Fan cooks an
> intermediate solution that does not need API change.
>
> Doing this, we postpone the API change to v18.08, so we have time to
> discuss what the right API should be. Once agreed, vhost-crypto moves to
> the new API.
>
> Pawel, Jianfeng, Fan, is it fine for you?

+1. This can avoid blocking this patch set, and give more time for 
discussing new APIs and external structs.

Thanks,
Jianfeng

>
> Thanks,
> Maxime