Re: [dpdk-dev] [PATCH v3] rte_ring: clarify preemptible nature of ring algorithm

[Olivier Matz <olivier.matz@6wind.com>]

Hi,

On Mon, Jul 09, 2018 at 11:00:47PM -0500, Honnappa Nagarahalli wrote:
> rte_ring implementation is not preemptible only under certain
> circumstances. This clarification is helpful for data plane and
> control plane communication using rte_ring.
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
> ---
> v3:
> * Corrected known issues for rte_ring
> * Referred to known issues in rte_ring.h (Burakov, Oliver)
> 
> v2:
> * Fixed checkpatch warnings
> 
>  doc/guides/prog_guide/env_abstraction_layer.rst | 27 ++++++++++++++-----------
>  lib/librte_ring/rte_ring.h                      |  5 +++--
>  2 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/env_abstraction_layer.rst b/doc/guides/prog_guide/env_abstraction_layer.rst
> index a22640d..f47a4be 100644
> --- a/doc/guides/prog_guide/env_abstraction_layer.rst
> +++ b/doc/guides/prog_guide/env_abstraction_layer.rst
> @@ -435,23 +435,26 @@ Known Issues
>  
>      The "non-preemptive" constraint means:
>  
> -    - a pthread doing multi-producers enqueues on a given ring must not
> -      be preempted by another pthread doing a multi-producer enqueue on
> -      the same ring.
> -    - a pthread doing multi-consumers dequeues on a given ring must not
> -      be preempted by another pthread doing a multi-consumer dequeue on
> -      the same ring.
> +      A preempted pthread can block other pthreads (operating on the same ring)
> +      from completing their operations, only if those pthreads are performing
> +      the same ring operation (enqueue/dequeue) as the preempted pthread.
> +      In other words, a preempted consumer pthread will not block any producer
> +      pthreads and vice versa.
>  
> -    Bypassing this constraint may cause the 2nd pthread to spin until the 1st one is scheduled again.
> -    Moreover, if the 1st pthread is preempted by a context that has an higher priority, it may even cause a dead lock.
> +    Bypassing this constraint may cause other pthreads to spin until the preempted pthread is scheduled again.
> +    Moreover, if the pthread is preempted by a context that has a higher priority, it may even cause a dead lock.

Your reword makes this section more generic, but I'm not sure it is
really clearer. IMO it can stay as it was.

John, Anatoly, do you have any opinion?


> -  This does not mean it cannot be used, simply, there is a need to narrow down the situation when it is used by multi-pthread on the same core.
> +  This means, use cases involving preemptible pthreads should consider using rte_ring carefully.
>  
> -  1. It CAN be used for any single-producer or single-consumer situation.
> +  1. It CAN be used for preemptible single-producer and single-consumer use case.
>  
> -  2. It MAY be used by multi-producer/consumer pthread whose scheduling policy are all SCHED_OTHER(cfs). User SHOULD be aware of the performance penalty before using it.
> +  2. It CAN be used for non-preemptible multi-producer and preemptible single-consumer use case.
>  
> -  3. It MUST not be used by multi-producer/consumer pthreads, whose scheduling policies are SCHED_FIFO or SCHED_RR.
> +  3. It CAN be used for preemptible single-producer and non-preemptible multi-consumer use case.
> +
> +  4. It MAY be used by preemptible multi-producer and/or preemptible multi-consumer pthreads whose scheduling policy are all SCHED_OTHER(cfs), SCHED_IDLE or SCHED_BATCH. User SHOULD be aware of the performance penalty before using it.
> +
> +  5. It MUST not be used by multi-producer/consumer pthreads, whose scheduling policies are SCHED_FIFO or SCHED_RR.
>  
>  + rte_timer
>  
> diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
> index 1245822..e680101 100644
> --- a/lib/librte_ring/rte_ring.h
> +++ b/lib/librte_ring/rte_ring.h
> @@ -26,8 +26,9 @@
>   * - Bulk dequeue.
>   * - Bulk enqueue.
>   *
> - * Note: the ring implementation is not preemptable. A lcore must not
> - * be interrupted by another task that uses the same ring.
> + * Note: the ring implementation is not preemptible. Refer to Programmer's
> + * guide/Environment Abstraction Layer/Multiple pthread/Known Issues/rte_ring
> + * for more information.


The other parts looks good to me, thank you.