Hi Harry, Thanks for the review.
On Tue, Jan 16, 2018 at 12:05:48PM +0000, Van Haaren, Harry wrote: > > From: Pavan Nikhilesh [mailto:pbhagavat...@caviumnetworks.com] > > Sent: Friday, January 12, 2018 4:44 PM > > To: jerin.ja...@caviumnetworks.com; santosh.shu...@caviumnetworks.com; Van > > Haaren, Harry <harry.van.haa...@intel.com>; Eads, Gage > > <gage.e...@intel.com>; hemant.agra...@nxp.com; nipun.gu...@nxp.com; Ma, > > Liang J <liang.j...@intel.com> > > Cc: dev@dpdk.org; Pavan Nikhilesh <pbhagavat...@caviumnetworks.com> > > Subject: [dpdk-dev] [PATCH v4 10/13] app/eventdev: add pipeline queue worker > > functions > > > > Signed-off-by: Pavan Nikhilesh <pbhagavat...@caviumnetworks.com> > > Some of the code feels like duplication - but there are differences in each > implementation as far as I can see. The common initialization parts are > "macro-ed" to ensure minimal duplication. In short, I'm not aware of a better > solution/implementation. > > Please see note on branch-optimization in the code below. > > Acked-by: Harry van Haaren <harry.van.haa...@intel.com> > > > > static int > > worker_wrapper(void *arg) > > { > > - RTE_SET_USED(arg); > > + struct worker_data *w = arg; > > + struct evt_options *opt = w->t->opt; > > + const bool burst = evt_has_burst_mode(w->dev_id); > > + const bool mt_safe = !w->t->mt_unsafe; > > + const uint8_t nb_stages = opt->nb_stages; > > + RTE_SET_USED(opt); > > + > > + /* allow compiler to optimize */ > > + if (nb_stages == 1) { > > + if (!burst && mt_safe) > > + return pipeline_queue_worker_single_stage_tx(arg); > > + else if (!burst && !mt_safe) > > + return pipeline_queue_worker_single_stage_fwd(arg); > > + else if (burst && mt_safe) > > + return pipeline_queue_worker_single_stage_burst_tx(arg); > > + else if (burst && !mt_safe) > > + return pipeline_queue_worker_single_stage_burst_fwd( > > + arg); > > + } else { > > + if (!burst && mt_safe) > > + return pipeline_queue_worker_multi_stage_tx(arg); > > + else if (!burst && !mt_safe) > > + return pipeline_queue_worker_multi_stage_fwd(arg); > > + else if (burst && mt_safe) > > + return pipeline_queue_worker_multi_stage_burst_tx(arg); > > + else if (burst && !mt_safe) > > + return pipeline_queue_worker_multi_stage_burst_fwd(arg); > > + > > + } > > > I don't think the compiler will optimize the below, as the nb_stages value > comes from opt, which is a uint8 originating from a pointer-dereference... > > As far as I know, an optimizing compiler will do "constant propagation" > through function calls, but I would be amazed if it found a pointer > initialized to a specific value, and optimized the branches away here based > on the contents of that pointer deref. > > So I see 2 options; > 1) accept as is - I'll ack this patch here > 2) Rework so that the worker_wrapper() accepts nb_stages, burst and mt_safe > as function arguments, allowing constant propagation for optimizing away the > branches. > > @Pavan, I'll leave that choice up to you! As the worker functions are not affected, I will keep the conditions as is and remove the comment for now. Cheers, Pavan. >
