RE: [EXT] Re: [PATCH 1/2] doc: add enqueue depth for new event type

["Van Haaren, Harry" <harry.van.haaren@intel.com>]

> -----Original Message-----
> From: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>
> Sent: Thursday, July 14, 2022 7:01 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; mattias.ronnblom
> <mattias.ronnblom@ericsson.com>; Thomas Monjalon <thomas@monjalon.net>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Ray Kinsella <mdr@ashroe.eu>;
> dev@dpdk.org; McDaniel, Timothy <timothy.mcdaniel@intel.com>; Hemant
> Agrawal <hemant.agrawal@nxp.com>; sachin.saxena@oss.nxp.com;
> liangma@liangbit.com; Mccarthy, Peter <peter.mccarthy@intel.com>; Carrillo, Erik
> G <erik.g.carrillo@intel.com>; Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>;
> Jayatheerthan, Jay <jay.jayatheerthan@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>
> Subject: RE: [EXT] Re: [PATCH 1/2] doc: add enqueue depth for new event type
> 
> 
> 
> > -----Original Message-----
> > From: Van Haaren, Harry <harry.van.haaren@intel.com>
> > Sent: Thursday, July 14, 2022 10:24 PM
> > To: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>;
> > mattias.ronnblom <mattias.ronnblom@ericsson.com>; Thomas Monjalon
> > <thomas@monjalon.net>
> > Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Ray Kinsella
> > <mdr@ashroe.eu>; dev@dpdk.org; McDaniel, Timothy
> > <timothy.mcdaniel@intel.com>; Hemant Agrawal
> > <hemant.agrawal@nxp.com>; sachin.saxena@oss.nxp.com;
> > liangma@liangbit.com; Mccarthy, Peter <peter.mccarthy@intel.com>;
> > Carrillo, Erik G <erik.g.carrillo@intel.com>; Gujjar, Abhinandan S
> > <abhinandan.gujjar@intel.com>; Jayatheerthan, Jay
> > <jay.jayatheerthan@intel.com>; Burakov, Anatoly
> > <anatoly.burakov@intel.com>
> > Subject: RE: [EXT] Re: [PATCH 1/2] doc: add enqueue depth for new event
> > type
> >
> > > -----Original Message-----
> > > From: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>
> > > Sent: Thursday, July 14, 2022 5:42 PM
> > > To: mattias.ronnblom <mattias.ronnblom@ericsson.com>; Thomas
> > Monjalon
> > > <thomas@monjalon.net>
> > > Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Ray Kinsella
> > <mdr@ashroe.eu>;
> > > dev@dpdk.org; McDaniel, Timothy <timothy.mcdaniel@intel.com>;
> > Hemant
> > > Agrawal <hemant.agrawal@nxp.com>; sachin.saxena@oss.nxp.com;
> > > liangma@liangbit.com; Mccarthy, Peter <peter.mccarthy@intel.com>; Van
> > Haaren,
> > > Harry <harry.van.haaren@intel.com>; Carrillo, Erik G
> > <erik.g.carrillo@intel.com>;
> > > Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Jayatheerthan, Jay
> > > <jay.jayatheerthan@intel.com>; Burakov, Anatoly
> > <anatoly.burakov@intel.com>
> > > Subject: RE: [EXT] Re: [PATCH 1/2] doc: add enqueue depth for new event
> > type
> >
> > <snip old conversation>
> >
> > > > >> If the underlying hardware has some limitations,
> > > > >> why not let the driver loop until back pressure occurs? Then you can
> > > >
> > > > You didn't answer this question. Why not let the driver loop, until you
> > > > for some reason or the other can't accept more events?
> > >
> > > CNXK event driver cannot accept forwarding(enq) more than one event
> > that has
> > > been dequeued. Enqueueing more than one event for
> > forwarding/releasing
> > > is a violation from HW perspective, this is currently announced by BURST
> > capability.
> > > But It can enqueue a burst if new events.
> >
> > Can't the driver just backpressure NEW events? that's what the event/sw
> > driver
> > does in order to limit "new" inflight events. App attempts to enq FWD/REL,
> > no
> > problem. App enqueues burst of NEW (and there's only N spaces) then the
> > first N events pass, and the rest are returned to the application.
> >
> 
> Yes, driver can backpressure NEW events, in-fact that’s what we do today even
> with burst size 1 as we need to check if target queue has space.
> 
> The main problem is app needs to know that enqueue NEW supports
> burst of events even when capability doesn't report BURST support.

If this is the "main problem", then 2 steps:
1) Let the driver report it supports BURST, and app will try to use it
2A) Let user enq bursts of FWD/REL, and accept only 1 (expecting app to retry with rest of burst, as is common)
2B) Put a retry loop inside the PMD, until actual backpressure is hit in HW, then return to App.


> 
> Currently burst check is done as follows:
> http://git.dpdk.org/dpdk/tree/app/test-eventdev/test_perf_common.c#n545
> http://git.dpdk.org/dpdk/tree/app/test-eventdev/evt_common.h#n99
> 
> > > If you see the current example implementation we pick the worker based
> > on
> > > BURST capability for optimizing the enqueue/dequeue by providing a hint
> > > to the driver layer.
> >
> > Please provide a link to the code? Others are not familiar with the CNXK
> > driver,
> > or the sample code you're referring to...
> >
> 
> See above.
> 
> >
> > > Although, we could live with aggregating the events at driver layer based
> > on
> > > queue. We would still require announce burst capability for new events i.e.
> > > changes to the info structure.
> >
> > As per above, I still don't see a reason why this HW optimization/limitation
> > needs to be pushed to the application layer. Why can the driver not handle
> > things by allowing/backpressure to the events it can/can't handle?
> >
> 
> We can handle aggregation in the driver i.e. the new API is not needed although
> doing so is inefficient, our synthetic benchmark shows ~20% drop.
> 
> The main issue is that application needs to know that burst enqueue is supported
> for event with op_type as NEW even when capability doesn’t report BURST support.
> I think this can only be done if driver reports it via info structure.

See above suggestion; the application should already be burst-capable (if it wants to be)
and hence there's "nothing to do" at the app level, if the PMD is reworked to 1 and 2B?


> > In this email thread[1] you've suggested reworking the rx_burst API with a
> > flag to indicate "same destination". This still pushes the problem to the
> > application,
> > and exposes more HW/PMD specific options. This impl is *slightly* better
> > because it
> > wont' require new APIs for each mode, but also *breaks all existing apps*!?
> >
> > I'm just not understanding why the application needs to change, and why it
> > cannot be optimized/handled in the driver layer.
> >
> > [1] https://urldefense.proofpoint.com/v2/url?u=http-
> > 3A__mails.dpdk.org_archives_dev_2022-
> > 2DJuly_246717.html&d=DwIGaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=E3SgYMjt
> > KCMVsB-fmvgGV3o-
> > g_fjLhk5Pupi9ijohpc&m=CyiCnnBdRFmd0maK3yHCkM7_3fDnVGGCeHteXAb
> > I6DvehYrkk6BvyrMsV_NKsUGs&s=SbdMMotdrG_yzjCRgJc7h_Oq9Jtfl_8V06
> > QsyPqUfro&e=
> >
> > <snip old conversation>