[PATCH v4 7/7] bbdev: add a lock option for enqueue/dequeue operation

[Tom Rix]

On 7/6/22 1:21 PM, Chautru, Nicolas wrote:
> Hi Stephen, Tom.,
>
>> -----Original Message-----
>> From: Stephen Hemminger <stephen at networkplumber.org>
>>
>> On Wed, 6 Jul 2022 12:01:19 -0700
>> Tom Rix <trix at redhat.com> wrote:
>>
>>> On 7/5/22 5:23 PM, Nicolas Chautru wrote:
>>>> Locking is not explicitly required but can be valuable in case the
>>>> application cannot guarantee to be thread-safe, or specifically is
>>>> at risk of using the same queue from multiple threads.
>>>> This is an option for PMD to use this.
>>>>
>>>> Signed-off-by: Nicolas Chautru <nicolas.chautru at intel.com>
>>>> ---
>>>>    lib/bbdev/rte_bbdev.h | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/lib/bbdev/rte_bbdev.h b/lib/bbdev/rte_bbdev.h index
>>>> b7ecf94..8e7ca86 100644
>>>> --- a/lib/bbdev/rte_bbdev.h
>>>> +++ b/lib/bbdev/rte_bbdev.h
>>>> @@ -407,6 +407,8 @@ struct rte_bbdev_queue_data {
>>>>    	struct rte_bbdev_stats queue_stats;  /**< Queue statistics */
>>>>    	enum rte_bbdev_enqueue_status enqueue_status; /**< Enqueue
>> status when op is rejected */
>>>>    	bool started;  /**< Queue state */
>>>> +	rte_rwlock_t lock_enq; /**< lock protection for the Enqueue */
>>>> +	rte_rwlock_t lock_deq; /**< lock protection for the Dequeue */
>>> No.
>>>
>>> This is a good idea but needs a use before introducing another
>>> element, particularly a complicated one like locking
>>>
>>> Tom
> The actual usage would be implemented within the PMD. Basically this to prevent the corner case when a queue is being accessed from multiple thread for which there is no protection in DPDK (but application does not necessarily behaves well).
> In normal operation there would never be a case when there is a conflict on the lock.
> This is not something which was considered for any other PMD?
>  From DPDK doc : "If multiple threads are to use the same hardware queue on the same NIC port, then locking, or some other form of mutual exclusion, is necessary."
> Basically for AC100 we would purely enforce the lock for any enqueue/dequeue operation for a given queue (distinct lock for enqueue and dequeue, since these would run on different threads).

I am fine with locking, just have to use them.

For me, this would mean adding them to every public interface so the 
changes would be involved.

This is a big change and if pressed to get this patchset into 22.11, 
then defer this patch to later.

Tom

>
>> Having two locks on same cacheline will create lots of ping/pong false sharing.
> You would recommend to purely spread them within the structure? Or something else?
>   
>> Also, unless the reader is holding the lock for a significant fraction of the time a
>> regular spin lock will be faster.
> OK Thanks. It should in principle never have to wait for the lock for the usage above, only to catch misbehaving application risk.
>
> Nic
>
>