diff mbox series

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

Message ID 1657067022-54373-8-git-send-email-nicolas.chautru@intel.com (mailing list archive)
State Superseded, archived
Delegated to: akhil goyal
Headers
Series bbdev changes for 22.11 |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/github-robot: build fail github build: failed
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-abi-testing warning Testing issues
ci/intel-Testing success Testing PASS

Commit Message

Chautru, Nicolas July 6, 2022, 12:23 a.m. UTC 
  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@intel.com>
---
 lib/bbdev/rte_bbdev.h | 2 ++
 1 file changed, 2 insertions(+)

Comments

Tom Rix July 6, 2022, 7:01 p.m. UTC | #1 
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@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

>   };
>   
>   /** @internal Enqueue encode operations for processing on queue of a device. */
Stephen Hemminger July 6, 2022, 7:20 p.m. UTC | #2 
On Wed, 6 Jul 2022 12:01:19 -0700
Tom Rix <trix@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@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

Having two locks on same cacheline will create lots of ping/pong false sharing.

Also, unless the reader is holding the lock for a significant fraction of the time
a regular spin lock will be faster.
Chautru, Nicolas July 6, 2022, 8:21 p.m. UTC | #3 
Hi Stephen, Tom., 

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> 
> On Wed, 6 Jul 2022 12:01:19 -0700
> Tom Rix <trix@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@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).  

> 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
Tom Rix July 7, 2022, 12:47 p.m. UTC | #4 
On 7/6/22 1:21 PM, Chautru, Nicolas wrote:
> Hi Stephen, Tom.,
>
>> -----Original Message-----
>> From: Stephen Hemminger <stephen@networkplumber.org>
>>
>> On Wed, 6 Jul 2022 12:01:19 -0700
>> Tom Rix <trix@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@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
>
>
diff mbox series

Patch

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 */
 };
 
 /** @internal Enqueue encode operations for processing on queue of a device. */