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

["Wang, Xiao W" <xiao.w.wang@intel.com>]

> -----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.

> 
> - 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.

> 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