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

[Nicolau, Radu]

On 10/6/2020 11:13 AM, Ananyev, Konstantin wrote:
-----Original Message-----
From: Jerin Jacob <jerinjac...@gmail.com>
Sent: Monday, October 5, 2020 5:35 PM
To: Nicolau, Radu <radu.nico...@intel.com>
Cc: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>; Richardson, Bruce
<bruce.richard...@intel.com>; Ananyev, Konstantin
<konstantin.anan...@intel.com>; Van Haaren, Harry
<harry.van.haa...@intel.com>; dev@dpdk.org; jer...@marvell.com; nd
<n...@arm.com>
Subject: Re: [dpdk-dev] [PATCH v1] event/sw: performance improvements

On Tue, Sep 29, 2020 at 2:32 PM Nicolau, Radu <radu.nico...@intel.com> wrote:

On 9/28/2020 5:02 PM, Honnappa Nagarahalli 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.nico...@intel.com

Thanks for the patch, a few comments inline.

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.
IMO, the ring library is pretty organized with the recent addition of HTS/RTS
modes. This can be one of the modes and should allow us to use the existing
functions (though additional functions are required as well).
The other concern I have is, this implementation can be further optimized by
using a single cache line for the pointers. It uses 2 cache lines just because 
of the
layout of the rte_ring structure.
There was a question earlier about the performance improvements of this
patch? Are there any % performance improvements that can be shared?
It is also possible to change the above functions to use the head/tail pointers
from producer or the consumer cache line alone to check for perf differences.
I don't have a % for the final improvement for this change alone, but
there was some improvement in the memory overhead measurable during
development, which very likely resulted in the whole optimization having
more headroom.

I agree that this may be further optimized, maybe by having a local
implementation of a ring-like container instead.
Have we decided on the next steps for this patch? Is the plan to
supersede this patch and have different
one in rte_ring subsystem,
My preference is to merge this version of the patch;
1) The ring helper functions are stripped to the SW PMD usage, and not valid to 
use in the general.
2) Adding static inline APIs in an LTS without extensive doesn't seem a good 
idea.

If Honnappa is OK with the above solution for 20.11, we can see about moving 
the rings part of the
code to rte_ring library location in 21.02, and give ourselves some time to 
settle the usage/API before
the next LTS.

As ring library maintainer I share Honnappa concern that another library not 
uses public ring API,
but instead accesses ring internals directly. Obviously such coding practice is 
not welcomed
as it makes harder to maintain/extend ring library in future.
About 2) - these new API can(/shoud) be marked an experimental anyway.
As another thing - it is still unclear what a performance gain we are talking 
about here.
Is it really worth it comparing to just using SP/SC?

The change itself came after I analyzed the memory bound sections of the 
code, and I just did a quick test, I got about 3.5% improvement in 
throughput,  maybe not so much but significant for such a small change, 
and depending on the usecase it may be more.

As for the implementation itself, I would favour having a custom ring 
like container in the PMD code, this will solve the issue of using 
rte_ring internals while still allow for full optimisation. If this is 
acceptable, I will follow up by tomorrow.