Re: [dpdk-stable] [PATCH] net/softnic: fix pipeline time calculation

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

> -----Original Message-----
> From: Wang, Xiao W
> Sent: Tuesday, July 9, 2019 3:00 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Singh, Jasvinder
> <jasvinder.singh@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: RE: [PATCH] net/softnic: fix pipeline time calculation
> 
> 
> 
> > -----Original Message-----
> > From: Dumitrescu, Cristian
> > Sent: Tuesday, July 9, 2019 6:32 PM
> > To: Wang, Xiao W <xiao.w.wang@intel.com>; Singh, Jasvinder
> > <jasvinder.singh@intel.com>
> > Cc: dev@dpdk.org; stable@dpdk.org
> > Subject: RE: [PATCH] net/softnic: fix pipeline time calculation
> >
> >
> >
> > > -----Original Message-----
> > > From: Wang, Xiao W
> > > Sent: Sunday, June 2, 2019 11:46 AM
> > > To: Singh, Jasvinder <jasvinder.singh@intel.com>
> > > Cc: dev@dpdk.org; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>;
> > > stable@dpdk.org
> > > Subject: RE: [PATCH] net/softnic: fix pipeline time calculation
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Singh, Jasvinder
> > > > Sent: Friday, May 31, 2019 10:46 PM
> > > > To: Wang, Xiao W <xiao.w.wang@intel.com>
> > > > Cc: dev@dpdk.org; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>;
> > > > stable@dpdk.org
> > > > Subject: RE: [PATCH] net/softnic: fix pipeline time calculation
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Wang, Xiao W
> > > > > Sent: Wednesday, May 15, 2019 2:59 PM
> > > > > To: Singh, Jasvinder <jasvinder.singh@intel.com>
> > > > > Cc: dev@dpdk.org; Dumitrescu, Cristian
> > > <cristian.dumitrescu@intel.com>;
> > > > > Wang, Xiao W <xiao.w.wang@intel.com>; stable@dpdk.org
> > > > > Subject: [PATCH] net/softnic: fix pipeline time calculation
> > > > >
> > > > > When a new pipeline is added to a thread, the "time_next_min"
> value
> > > may
> > > > > need update, otherwise this pipeline won't get served timely.
> > > > >
> > > > > Fixes: 70709c78fda6 ("net/softnic: add command to enable/disable
> > > pipeline")
> > > > > Cc: stable@dpdk.org
> > > > >
> > > > > Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
> > > > > ---
> > > > >  drivers/net/softnic/rte_eth_softnic_thread.c | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/drivers/net/softnic/rte_eth_softnic_thread.c
> > > > > b/drivers/net/softnic/rte_eth_softnic_thread.c
> > > > > index 855408e98..2b482117d 100644
> > > > > --- a/drivers/net/softnic/rte_eth_softnic_thread.c
> > > > > +++ b/drivers/net/softnic/rte_eth_softnic_thread.c
> > > > > @@ -337,6 +337,9 @@ softnic_thread_pipeline_enable(struct
> > > > pmd_internals
> > > > > *softnic,
> > > > >  		tdp->timer_period = (rte_get_tsc_hz() * p-
> > > >timer_period_ms)
> > > > /
> > > > > 1000;
> > > > >  		tdp->time_next = rte_get_tsc_cycles() + tdp-
> >timer_period;
> > > > >
> > > > > +		if (tdp->time_next < td->time_next_min)
> > > > > +			td->time_next_min = tdp->time_next;
> > > > > +
> > > > >  		td->n_pipelines++;
> > > > >
> > > > >  		/* Pipeline */
> > > > > @@ -522,6 +525,9 @@ thread_msg_handle_pipeline_enable(struct
> > > > > softnic_thread_data *t,
> > > > >  		(rte_get_tsc_hz() * req-
> >pipeline_enable.timer_period_ms)
> > > /
> > > > > 1000;
> > > > >  	p->time_next = rte_get_tsc_cycles() + p->timer_period;
> > > > >
> > > > > +	if (p->time_next < t->time_next_min)
> > > > > +		t->time_next_min = p->time_next;
> > > > > +
> > > > >  	t->n_pipelines++;
> > > > >
> > > > >  	/* Response */
> > > > > --
> > > > > 2.15.1
> > > >
> > > >
> > > > Hi Wang,
> > > >
> > > > Timer values for pipelines and thread level message handlers are
> already
> > > > adjusted in runtime function rte_pmd_softnic_run_internal(). In
> runtime
> > > > function, the values of t->time_next_min is updated as well. IMO,
> above
> > > > changes not needed. Could you help with the case where timer
> > > adjustments in
> > > > runtime not working?
> > >
> > > Hi Jasvinder,
> > >
> > > the values of t->time_next_min is updated only when the pipeline
> message
> > > and thread message get handled, but not when the pipeline is added to
> > that
> > > thread. E.g. when a thread t->time_next_min is ~100ms later, and a new
> > > pipeline is added to that thread with timer_period_ms parameter set to
> > > 10ms, then this pipeline's control message will not be served until 100ms
> > > later.
> > >
> > > BRs,
> > > Xiao
> > >
> > > >
> > > > Thanks,
> > > > Jasvinder
> > > >
> >
> > NAK
> >
> > There are no bugs/issues fixed by this patch, but there are side effects
> > introduced that maybe you did not anticipate.
> >
> > - Yes, the first message handler for a newly added pipeline might be slightly
> > delayed, but this is harmless.
> 
> As the above example shows, the new pipeline handling may get ~90ms
> delayed, and
> maybe the user wants this newly added pipeline to be real-time and 90ms
> delay is too long
> for that application.
> 

The example you provide is probably not realistic, as typically the thread period should be one order of magnitude smaller than the pipeline period, while your example is the other way around.

If the pipeline period is smaller than the thread period, the pipeline period is essentially upgraded to the thread period, as the thread is going to sample for pipeline messages every thread period without ever meeting the higher sampling frequency that the pipeline ideally wants, right?

So basically set thread period to 10 ms and the pipeline period to 100 ms, which means once every 10 thread check this pipeline is picked for message work.


> >
> > - For a given thread, we periodically iterate through all pipelines the current
> > thread is running and check if there are any pending messages for each
> > pipeline (function rte_pmd_softnic_run_internal). We also update the
> > deadline for the next message handling session for the thread (thread-
> > >time_next_min), which should only be changed by the thread (existing
> 
> Yeah it's about (thead->time_next_min), if current time passes it, then we
> would
> Iteratively check each pipepine, only when pipeline's time_next also passes
> it, we
> handle this pipeline.
> 
> If the thread is running, the softnic_thread_data should only be changed by
> the
> thread itself. And this patch doesn't break this.
> 

Yes, it is the thread changing this variable based on configuration of pipeline periods for each pipeline, but its value needs to be stable during the thread iteration through the pipelines. If you change this in the middle of the for loop (by handling the message to add a pipeline), you are basically changing the behavior of remaining iterations.

> > code); if this is changed by the pipeline message handler, then there is the
> > risk that some pipelines will run their message handler again very soon
> after,
> > as the deadline had been brought earlier. Makes sense?
> 
> The THREAD_REQ_PIPELINE_ENABLE message is handled in
> thread_msg_handle(),
> not in pipeline_msg_handle().
> 
> Thanks for your comments.
> 
> -Xiao

Thanks for your proposal and for this discussion!

Cristian