[PATCH v3 4/4] vhost: add device op to offload the interrupt kick

[Maxime Coquelin]

On 6/1/23 10:49, Eelco Chaudron wrote:
> 
> 
> On 1 Jun 2023, at 10:29, Maxime Coquelin wrote:
> 
>> On 6/1/23 10:15, Eelco Chaudron wrote:
>>>
>>>
>>> On 1 Jun 2023, at 4:18, Xia, Chenbo wrote:
>>>
>>>>> -----Original Message-----
>>>>> From: Maxime Coquelin <maxime.coquelin at redhat.com>
>>>>> Sent: Wednesday, May 31, 2023 5:29 PM
>>>>> To: Xia, Chenbo <chenbo.xia at intel.com>; Thomas Monjalon
>>>>> <thomas at monjalon.net>; Eelco Chaudron <echaudro at redhat.com>;
>>>>> david.marchand at redhat.com
>>>>> Cc: dev at dpdk.org
>>>>> Subject: Re: [PATCH v3 4/4] vhost: add device op to offload the interrupt
>>>>> kick
>>>>>
>>>>>
>>>>>
>>>>> On 5/31/23 08:19, Xia, Chenbo wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: Maxime Coquelin <maxime.coquelin at redhat.com>
>>>>>>> Sent: Tuesday, May 30, 2023 11:17 PM
>>>>>>> To: Thomas Monjalon <thomas at monjalon.net>; Eelco Chaudron
>>>>>>> <echaudro at redhat.com>; Xia, Chenbo <chenbo.xia at intel.com>;
>>>>>>> david.marchand at redhat.com
>>>>>>> Cc: dev at dpdk.org
>>>>>>> Subject: Re: [PATCH v3 4/4] vhost: add device op to offload the
>>>>> interrupt
>>>>>>> kick
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 5/30/23 15:16, Thomas Monjalon wrote:
>>>>>>>> 30/05/2023 15:02, Maxime Coquelin:
>>>>>>>>>
>>>>>>>>> On 5/17/23 11:09, Eelco Chaudron wrote:
>>>>>>>>>> This patch adds an operation callback which gets called every time
>>>>> the
>>>>>>>>>> library wants to call eventfd_write(). This eventfd_write() call
>>>>> could
>>>>>>>>>> result in a system call, which could potentially block the PMD
>>>>> thread.
>>>>>>>>>>
>>>>>>>>>> The callback function can decide whether it's ok to handle the
>>>>>>>>>> eventfd_write() now or have the newly introduced function,
>>>>>>>>>> rte_vhost_notify_guest(), called at a later time.
>>>>>>>>>>
>>>>>>>>>> This can be used by 3rd party applications, like OVS, to avoid
>>>>> system
>>>>>>>>>> calls being called as part of the PMD threads.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Eelco Chaudron <echaudro at redhat.com>
>>>>>>>>>> ---
>>>>>>>>>>       lib/vhost/meson.build |    2 ++
>>>>>>>>>>       lib/vhost/rte_vhost.h |   23 +++++++++++++++++-
>>>>>>>>>>       lib/vhost/socket.c    |   63
>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++---
>>>>>>>>>>       lib/vhost/version.map |    9 +++++++
>>>>>>>>>>       lib/vhost/vhost.c     |   38 ++++++++++++++++++++++++++++++
>>>>>>>>>>       lib/vhost/vhost.h     |   58 ++++++++++++++++++++++++++++++++---
>>>>> ---
>>>>>>> -------
>>>>>>>>>>       6 files changed, 171 insertions(+), 22 deletions(-)
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The patch looks good to me, but that's the first time we use function
>>>>>>>>> versioning in Vhost library, so I'd like another pair of eyes to be
>>>>>>> sure
>>>>>>>>> I don't miss anything.
>>>>>>>>>
>>>>>>>>> Reviewed-by: Maxime Coquelin <maxime.coquelin at redhat.com>
>>>>>>>>>
>>>>>>>>> Thomas, do we need to mention it somewhere in the release note?
>>>>>>>>
>>>>>>>> If compatibility is kept, I think we don't need to mention it.
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> Thanks Thomas for the information.
>>>>>>>
>>>>>>> Maxime
>>>>>>
>>>>>> About release note, except the versioning, there is also one new API
>>>>> introduced
>>>>>> in this patch, so we still need to mention this in release note
>>>>>
>>>>> Right, good catch.
>>>>> Eelco, let me know what you would put, I'll add it while applying (No
>>>>> need for a new revision).
>>>>
>>>> Btw, the vhost_lib.rst also needs a new item..
>>>
>>> What about the following?
>>>
>>> diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
>>> index e8bb8c9b7b..0f964d7a4a 100644
>>> --- a/doc/guides/prog_guide/vhost_lib.rst
>>> +++ b/doc/guides/prog_guide/vhost_lib.rst
>>> @@ -334,6 +334,11 @@ The following is an overview of some key Vhost API functions:
>>>      Clean DMA vChannel finished to use. After this function is called,
>>>      the specified DMA vChannel should no longer be used by the Vhost library.
>>>
>>> +* ``rte_vhost_notify_guest(int vid, uint16_t queue_id)``
>>> +
>>> +  Inject the offloaded interrupt received by the 'guest_notify' callback,
>>> +  into the vhost device's queue.
>>> +
>>>    Vhost-user Implementations
>>>    --------------------------
>>>
>>> Maxime do you want to add it, or do you want a new rev?
>>
>> That's fine, I just added it now :) Can you check the next-virtio main
>> branch and confirm all is OK for your series?
>>
>> https://git.dpdk.org/next/dpdk-next-virtio/log/
>>
> 
> One small spelling issue, the rest looks good:
> 
> +* **Added callback API support for interrupt handling in the vhost library.**
> +
> +  A new callback, guest_notify, is introduced that can be used to handle the
> +  interrupt kick outside of the datapath fast path. In addition, a new API,
> +  rte_vhost_notify_guest(), is added to raise the interrupt outside of the
> +  past path.
> 
> Last sentence spells ‘past path’ instead of ‘fast path’.

I edited the line but it didn't caught my eye!
Fixing it now.

Thanks,
Maxime

> 
>> Thanks,
>> Maxime
>>
>>>
>>>> Thanks,
>>>> Chenbo
>>>>
>>>>>
>>>>> Thanks,
>>>>> Maxime
>>>>>
>>>>>> Thanks,
>>>>>> Chenbo
>>>
>