Re: [dpdk-dev] [PATCH v1] event/sw: performance improvements

[Bruce Richardson <bruce.richardson@intel.com>]

On Thu, Sep 24, 2020 at 06:02:48PM +0000, Ananyev, Konstantin wrote:
> 
> 
> 
> <snip>
> 
> Add minimum burst throughout the scheduler pipeline and a flush counter.
> Replace ring API calls with local single threaded implementation where
> possible.
> 
> Signed-off-by: Radu Nicolau mailto:radu.nicolau@intel.com
> 
> Thanks for the patch, a few comments inline.
> 
> ---
>  drivers/event/sw/sw_evdev.h           | 11 +++-
>  drivers/event/sw/sw_evdev_scheduler.c | 83
> +++++++++++++++++++++++----
>  2 files changed, 81 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/event/sw/sw_evdev.h b/drivers/event/sw/sw_evdev.h
> index 7c77b2495..95e51065f 100644
> --- a/drivers/event/sw/sw_evdev.h
> +++ b/drivers/event/sw/sw_evdev.h
> @@ -29,7 +29,13 @@
>  /* report dequeue burst sizes in buckets */  #define
> SW_DEQ_STAT_BUCKET_SHIFT 2
>  /* how many packets pulled from port by sched */ -#define
> SCHED_DEQUEUE_BURST_SIZE 32
> +#define SCHED_DEQUEUE_BURST_SIZE 64
> +
> +#define SCHED_MIN_BURST_SIZE 8
> +#define SCHED_NO_ENQ_CYCLE_FLUSH 256
> +/* set SCHED_DEQUEUE_BURST_SIZE to 64 or 128 when setting this to 1*/
> +#define SCHED_REFILL_ONCE_PER_CALL 1
> 
> Is it possible to make the above #define a runtime option?
> Eg, --vdev event_sw,refill_iter=1
> 
> That would allow packaged versions of DPDK to be usable in both modes.
> 
> +
> 
>  #define SW_PORT_HIST_LIST (MAX_SW_PROD_Q_DEPTH) /* size of our
> history list */  #define NUM_SAMPLES 64 /* how many data points use
> for average stats */ @@ -214,6 +220,9 @@ struct sw_evdev {
>      uint32_t xstats_count_mode_port;
>      uint32_t xstats_count_mode_queue;
> 
> +    uint16_t sched_flush_count;
> +    uint16_t sched_min_burst;
> +
>      /* Contains all ports - load balanced and directed */
>      struct sw_port ports[SW_PORTS_MAX] __rte_cache_aligned;
> 
> diff --git a/drivers/event/sw/sw_evdev_scheduler.c
> b/drivers/event/sw/sw_evdev_scheduler.c
> index cff747da8..ca6d1caff 100644
> --- a/drivers/event/sw/sw_evdev_scheduler.c
> +++ b/drivers/event/sw/sw_evdev_scheduler.c
> @@ -26,6 +26,29 @@
>  /* use cheap bit mixing, we only need to lose a few bits */  #define
> SW_HASH_FLOWID(f) (((f) ^ (f >> 10)) & FLOWID_MASK)
> 
> +
> +/* single object enq and deq for non MT ring */ static
> +__rte_always_inline void sw_nonmt_ring_dequeue(struct rte_ring *r,
> +void **obj) {
> +    if ((r->prod.tail - r->cons.tail) < 1)
> +            return;
> +    void **ring = (void **)&r[1];
> +    *obj = ring[r->cons.tail & r->mask];
> +    r->cons.tail++;
> +}
> +static __rte_always_inline int
> +sw_nonmt_ring_enqueue(struct rte_ring *r, void *obj) {
> +    if ((r->capacity + r->cons.tail - r->prod.tail) < 1)
> +            return 0;
> +    void **ring = (void **)&r[1];
> +    ring[r->prod.tail & r->mask] = obj;
> +    r->prod.tail++;
> +    return 1;
> +
> Why not make these APIs part of the rte_ring library? You could further optimize them by keeping the indices on the same cacheline.
> I'm not sure there is any need for non thread-safe rings outside this particular case.
> [Honnappa] I think if we add the APIs, we will find the use cases.
> But, more than that, I understand that rte_ring structure is exposed to the application. The reason for doing that is the inline functions that rte_ring provides. IMO, we should still maintain modularity and should not use the internals of the rte_ring structure outside of the library.
> 
> +1 to that.
> 
> BTW, is there any real perf benefit from such micor-optimisation?

I'd tend to view these as use-case specific, and I'm not sure we should
clutter up the ring library with yet more functions, especially since they
can't be mixed with the existing enqueue/dequeue functions, since they
don't use the head pointers.