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

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

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

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