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

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

[Xiao Wang]

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

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

[Xiao Wang]

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

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

[Singh, Jasvinder]

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

Thanks,
Jasvinder
 

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

[Wang, Xiao W]

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

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

[Singh, Jasvinder]

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

Acked-by: Jasvinder Singh <jasvinder.singh@intel.com>

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

[Dumitrescu, Cristian]

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

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

[Wang, Xiao W]

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

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

[Dumitrescu, Cristian]

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