Re: [dpdk-dev] [RFC PATCH 0/3] ethdev: few changes in rte_ethdev layer

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi Jerin,

> 
> Hi Konstantin,
> 
> > The series introduces 2 main changes:
> >
> > 1.Introduce a separate data structure (rte_eth_queue_local)
> > to store local to given process (i.e. no-shareable) information
> > for each configured rx/tx queue.
> > Memory for that structure is allocated/freed dynamically during
> > rte_eth_dev_configure().
> > Reserve a space for queue specific (rx|tx)_pkt_burst(),
> > tx_pkt_prepare() function pointers inside that structure.
> > Move rx/tx callback related information inside that structure.	
> > That introduces a change in current behavior: all callbacks for
> > un-configured queues will be automatically removed.
> > Also as size of struct rte_eth_dev changes that patch is an ABI breakage,
> > so deprecation notice for 18.05 is filled.
> > Further suggestions how to introduce the same functionality
> > without ABI breakage are welcome.
> 
> Are we doing this to enable, Tx/Rx queue specific burst functions to
> invoke based on new TX/RX queue specific offloads framework.
> Meaning, if a offload configured for the given queue,
> driver can select a different function pointer for Rx/Tx burst functions.

Yes that's the stretch goal.
We do need some extra space for rx/tx callbacks (use field) that needs to
be cache aligned and we plan to make rx/tx function per queue.
So I thought it would be a good opportunity to do these 2 things in one go -
to minimize number of ABI breakages. 

> 
> >
> > 2. Make it safe to remove rx/tx callback at runtime.
> > Right now it is not possible for the application to figure out
> > when it is safe to free removed callback handle and
> > associated with it resources(unless the queue is stopped).
> > That's probably not a big problem if all callbacks are static
> > hange through whole application lifetime)
> > and/or application doesn't allocate any resources for the callback handler.
> > Though if callbacks have to be added/removed dynamically and
> > callback handler would require more resources to operate properly -
> > then it might become an issue.
> > So patch #2 fixes that problem - now as soon as
> > rte_eth_remove_(rx|tx)_callback() completes successfully, application
> > can safely free all associated with the removed callback resources.
> >
> > Performance impact:
> > If application doesn't use RX/TX callbacks, then the tests I run didn't
> > reveal any performance degradation.
> > Though if application do use RX/TX callbacks - patch #2 does introduce
> > some slowdown.
> >
> > To be more specific here, on BDW (E5-2699 v4) 2.2GHz, 4x10Gb (X520-4)
> > with http://dpdk.org/dev/patchwork/patch/31864/ patch installed I got:
> > 1) testpmd ... --latencystats=1 - slowdown < 1%
> > 2) examples//l3fwd ... --parse-ptype - - slowdown < 1%
> > 3) examples/rxtx_callbacks - slowdown ~8%
> > All that in terms of packet throughput (Mpps).
> 
> We tried on an arm64 machine; We got following result on host and guest
> using l3fwd.

Thanks for testing it.
So these numbers below - are for l3fwd running with or without rx callback installed?
i.e. with or without '--parse-ptype' option (and on a NIC with/without ptype HW support)?

> 
> 
> Note:
> +ve number means "Performance regression with patches"
> -ve number means "Performance improvement with patches"
> 
> Relative performance difference in percentage:
> 
> % difference on host (2 x 40G)
> pkt_sz  1c      2c      4c      8c
> 64	1.41	0.18	0.51	0.29
> 72	0.50	0.53	0.09	0.19
> 128	0.31	0.31	0.42	0.00
> 256	-0.44	-0.44	0.00	0.00
> 512	1.94	1.94	0.01	0.01
> 1024	0.00	0.01	0.00	0.01
> 1518	0.02	0.01	0.02	0.02
> 
> % difference on guest (2 x 40G)
> pkt_sz  1c      2c      4c      8c
> 64      5.78	2.30	-2.45	-0.01
> 72	1.13	1.13	1.31	-4.29
> 128	1.36	1.36	1.54	-0.82
> 256	2.02	2.03	-0.01	-0.35
> 512	4.05	4.04	0.00	-0.46
> 1024	0.39	0.38	0.00	-0.74
> 1518	0.00	0.00	-0.05	-4.20
> 
> I think, it is because we added more code under
> the RTE_ETHDEV_RXTX_CALLBACKS and it is enabled by
> default.
> 
> I think, the impact will vary based on
> architecture and underneath i-cache, d-cache and IPC.
> 
> I am just thinking, How we can avoid such impact?
> How about,
> # Remove RTE_ETHDEV_RXTX_CALLBACKS option
> # Hook Rx/TX callbacks under TX/RX offload flags
> # ethdev layer gives helper function to invoke
> callbacks to driver. 

I suppose you suggest that user would have to decide at queue_setup()
stage does he wants a callback(s) for RX/TX or no?
As I can see, the main drawback with that approach - it would introduce new limitation.
Right now it is possible to add/remove RX/TX callback at runtime
(without stopping and reconfiguring the queue), and I suppose we do want to keep that ability.
Again in some cases user can't decide does he wants a callback installed or not,
before he actually calls dev_start().
l3fwd ptype-parse callback is an example of such situation:
right now user has to start device first to figure out which ptypes are supported by this PMD
with desired configuration, then based on this information he will (or will not) setup the callback.
What I thought in same direction - introduce a notion of 'static' and 'dynamic' callbacks.
Static ones can't be removed while queue is not stopped while dynamic can.
But that also requires user to select at queue_setup() stage does he wants dynamic
callbacks for that queue or not (new offload flag or extra field in rxconf/txconf structure). 
So again - same limitation for the user.
Another possibility here - have a flag 'dynamic' or 'static' not per queue, but per individual callback.
Yes, that would mean API change for add/remove callback functions.

Konstantin

> But, don't invoke from common code
> # When application needs the callback support, it
> configured the given RX/TX queue offloads
> # If the Rx/TX callback configure by the application
> then driver calls the helper functions exposed by
> the common code to invoke RX callbacks.


> 
> Jerin
> 
> >
> > Ability to safely remove callbacks at runtime implies
> > some sort of synchronization.
> > Even I tried to make it as light as possible,
> > probably some slowdown is unavoidable.
> > Of course instead of introducing these changes at rte_ethdev layer
> > similar technique could be applied on individual callback basis.
> > In that case it would be up to callback writer/installer to decide
> > does he/she need a removable at runtime callback or not.
> > Though in that case, each installed callback might introduce extra
> > synchronization overhead and slowdown.
> >
> > Konstantin Ananyev (3):
> >   ethdev: introduce eth_queue_local
> >   ethdev: make it safe to remove rx/tx callback at runtime
> >   doc: ethdev ABI change deprecation notice
> >
> >  doc/guides/rel_notes/deprecation.rst |   5 +
> >  lib/librte_ether/rte_ethdev.c        | 390 ++++++++++++++++++++++-------------
> >  lib/librte_ether/rte_ethdev.h        | 174 ++++++++++++----
> >  3 files changed, 387 insertions(+), 182 deletions(-)
> >
> > --
> > 2.13.5
> >