Re: [dpdk-dev] [PATCH v4 10/13] app/eventdev: add pipeline queue worker functions

[Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>]

Hi Harry,

Thanks for the review.

On Tue, Jan 16, 2018 at 12:05:48PM +0000, Van Haaren, Harry wrote:
> > From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com]
> > Sent: Friday, January 12, 2018 4:44 PM
> > To: jerin.jacob@caviumnetworks.com; santosh.shukla@caviumnetworks.com; Van
> > Haaren, Harry <harry.van.haaren@intel.com>; Eads, Gage
> > <gage.eads@intel.com>; hemant.agrawal@nxp.com; nipun.gupta@nxp.com; Ma,
> > Liang J <liang.j.ma@intel.com>
> > Cc: dev@dpdk.org; Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> > Subject: [dpdk-dev] [PATCH v4 10/13] app/eventdev: add pipeline queue worker
> > functions
> >
> > Signed-off-by: Pavan Nikhilesh <pbhagavatula@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.haaren@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.
>