Re: [PATCH RFC 4/4] doc: add note about rte_vhost_enqueue_burst thread safety.

[Ilya Maximets <i.maximets@samsung.com>]

On 23.02.2016 08:56, Xie, Huawei wrote:
> On 2/22/2016 6:16 PM, Thomas Monjalon wrote:
>> 2016-02-22 02:07, Xie, Huawei:
>>> On 2/19/2016 5:05 PM, Ilya Maximets wrote:
>>>> On 19.02.2016 11:36, Xie, Huawei wrote:
>>>>> On 2/19/2016 3:10 PM, Yuanhan Liu wrote:
>>>>>> On Fri, Feb 19, 2016 at 09:32:43AM +0300, Ilya Maximets wrote:
>>>>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>>>>> ---
>>>>>>>  doc/guides/prog_guide/thread_safety_dpdk_functions.rst | 1 +
>>>>>>>  1 file changed, 1 insertion(+)
>>>>>>>
>>>>>>> diff --git a/doc/guides/prog_guide/thread_safety_dpdk_functions.rst b/doc/guides/prog_guide/thread_safety_dpdk_functions.rst
>>>>>>> index 403e5fc..13a6c89 100644
>>>>>>> --- a/doc/guides/prog_guide/thread_safety_dpdk_functions.rst
>>>>>>> +++ b/doc/guides/prog_guide/thread_safety_dpdk_functions.rst
>>>>>>>  The mempool library is based on the DPDK lockless ring library and therefore is also multi-thread safe.
>>>>>>> +rte_vhost_enqueue_burst() is also thread safe because based on lockless ring-buffer algorithm like the ring library.
>>>>>> FYI, Huawei meant to make rte_vhost_enqueue_burst() not be thread-safe,
>>>>>> to aligh with the usage of rte_eth_tx_burst().
>>>>>>
>>>>>> 	--yliu
>>>>> I have a patch to remove the lockless enqueue. Unless there is strong
>>>>> reason, i prefer vhost PMD to behave like other PMDs, with no internal
>>>>> lockless algorithm. In future, for people who really need it, we could
>>>>> have dynamic/static switch to enable it.
>>> Thomas, what is your opinion on this and my patch removing lockless enqueue?
>> The thread safety behaviour is part of the API specification.
>> If we want to enable/disable such behaviour, it must be done with an API
>> function. But it would introduce a conditional statement in the fast path.
>> That's why the priority must be to keep a simple and consistent behaviour
>> and try to build around. An API complexity may be considered only if there
>> is a real (measured) gain.
> 
> Let us put the gain aside temporarily. I would do the measurement.
> Vhost is wrapped as a PMD in Tetsuya's patch. And also in DPDK OVS's
> case, it is wrapped as a vport like all other physical ports. The DPDK
> app/OVS will treat all ports equally.

That is not true. Currently vhost in Open vSwitch implemented as a separate
netdev class. So, to use concurrency of vhost we just need to remove
2 lines (rte_spinlock_lock and rte_spinlock_unlock) from function
__netdev_dpdk_vhost_send(). This will not change behaviour of other types
of ports.

> It will add complexity if the app
> needs to know that some supports concurrency while some not. Since all
> other PMDs doesn't support thread safety, it doesn't make sense for
> vhost PMD to support that. I believe the APP will not use that behavior.
>>From the API's point of view, if we previously implemented it wrongly,
> we need to fix it as early as possible.