[dpdk-dev] [EXT] Re: [PATCH 32/39] eventdev: add routine to launch eventmode workers

[Anoob Joseph]

Hi Erik,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: dev <dev-bounces at dpdk.org> On Behalf Of Carrillo, Erik G
> Sent: Monday, June 10, 2019 8:02 PM
> To: Anoob Joseph <anoobj at marvell.com>; Jerin Jacob Kollanukkaran
> <jerinj at marvell.com>; Rao, Nikhil <nikhil.rao at intel.com>; Gujjar,
> Abhinandan S <abhinandan.gujjar at intel.com>; Richardson, Bruce
> <bruce.richardson at intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch at intel.com>
> Cc: Narayana Prasad Raju Athreya <pathreya at marvell.com>; dev at dpdk.org;
> Lukas Bartosik <lbartosik at marvell.com>; Pavan Nikhilesh Bhagavatula
> <pbhagavatula at marvell.com>; Hemant Agrawal
> <hemant.agrawal at nxp.com>; Nipun Gupta <nipun.gupta at nxp.com>; Van
> Haaren, Harry <harry.van.haaren at intel.com>; Mattias Rönnblom
> <mattias.ronnblom at ericsson.com>; Ma, Liang J <liang.j.ma at intel.com>
> Subject: [EXT] Re: [dpdk-dev] [PATCH 32/39] eventdev: add routine to launch
> eventmode workers
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi Anoob,
> 
> I've listed a few notes in-line.
> 
> > -----Original Message-----
> > From: Anoob Joseph <anoobj at marvell.com>
> > Sent: Monday, June 3, 2019 12:33 PM
> > To: Jerin Jacob <jerinj at marvell.com>; Rao, Nikhil
> > <nikhil.rao at intel.com>; Carrillo, Erik G <erik.g.carrillo at intel.com>;
> > Gujjar, Abhinandan S <abhinandan.gujjar at intel.com>; Richardson, Bruce
> > <bruce.richardson at intel.com>; De Lara Guarch, Pablo
> > <pablo.de.lara.guarch at intel.com>
> > Cc: Anoob Joseph <anoobj at marvell.com>; Narayana Prasad
> > <pathreya at marvell.com>; dev at dpdk.org; Lukasz Bartosik
> > <lbartosik at marvell.com>; Pavan Nikhilesh <pbhagavatula at marvell.com>;
> > Hemant Agrawal <hemant.agrawal at nxp.com>; Nipun Gupta
> > <nipun.gupta at nxp.com>; Van Haaren, Harry
> <harry.van.haaren at intel.com>;
> > Mattias Rönnblom <mattias.ronnblom at ericsson.com>; Ma, Liang J
> > <liang.j.ma at intel.com>
> > Subject: [PATCH 32/39] eventdev: add routine to launch eventmode
> > workers
> >
> > With eventmode, workers could be drafted differently according to the
> > capabilities of the underlying event device. The added function would
> > receive an array of such workers and probes the eventmode properties
> > to choose the worker.
> >
> > Signed-off-by: Anoob Joseph <anoobj at marvell.com>
> > Signed-off-by: Lukasz Bartosik <lbartosik at marvell.com>
> > ---
> 
> <...snipped...>
> 
> > +
> > +/* Event mode packet tx types */
> > +enum rte_eventmode_helper_tx_types {
> > +	RTE_EVETNMODE_HELPER_TX_TYPE_INVALID = 0,
> 
> A couple of characters are transposed in the above.

[Anoob] Will fix it in v2.
> 
> > +	RTE_EVENTMODE_HELPER_TX_TYPE_MAX = 16 };
> > +
> >  struct rte_eventmode_helper_conf {
> >  	enum rte_eventmode_helper_pkt_transfer_mode mode;
> >  		/**< Packet transfer mode of the application */ @@ -41,6
> > +55,20 @@ struct rte_eventmode_helper_event_link_info {
> >  		/**< Lcore to be polling on this port */  };
> >
> 
> I believe anonymous unions and structures should be annotated with
> RTE_STD_C11 below and in other places throughout the series.

[Anoob] Will fix in v2.

> 
> > +/* Workers registered by the application */ struct
> > +rte_eventmode_helper_app_worker_params {
> > +	union {
> > +		struct {
> > +			uint64_t burst : 4;
> > +			/**< Specify status of rx type burst */
> > +		};
> > +		uint64_t u64;
> > +	} cap;
> > +			/**< Capabilities of this worker */
> > +	void (*worker_thread)(void *mode_conf);
> > +			/**< Worker thread */
> > +};
> > +
> >  /* Common helper functions for command line parsing */
> >
> >  /**
> > @@ -157,6 +185,27 @@ uint8_t __rte_experimental
> > rte_eventmode_helper_get_tx_queue(struct
> rte_eventmode_helper_conf
> > *mode_conf,
> >  		uint8_t eventdev_id);
> >
> 
> The doxygen documentation for __rte_experimental functions in general
> should have:
> 
> * @warning
> * @b EXPERIMENTAL: this API may change without prior notice
> 
> as well.
> 

[Anoob] Will add in v2.

> > +/**
> > + * Launch eventmode worker
> > + *
> > + * The application can request the eventmode helper subsystem to
> > +launch the
> > + * worker based on the capabilities of event device and the options
> > +selected
> > + * while initializing the eventmode.
> > + *
> > + * @param mode_conf
> > + *   Configuration of the mode in which app is doing packet handling
> > + * @param app_wrkr
> > + *   List of all the workers registered by application, along with it's
> > + *   capabilities
> > + * @param nb_wrkr_param
> > + *   Number of workers passed by the application
> > + *
> > + */
> > +void __rte_experimental
> > +rte_eventmode_helper_launch_worker(struct
> > rte_eventmode_helper_conf *mode_conf,
> > +		struct rte_eventmode_helper_app_worker_params
> > *app_wrkr,
> > +		uint8_t nb_wrkr_param);
> > +
> >  #ifdef __cplusplus
> >  }
> >  #endif
> 
> <...snipped...>
> 
> Regards,
> Erik