doc: announce API change in timer
diff mbox series

Message ID 20200803112059.23328-1-sarosh.arif@emumba.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • doc: announce API change in timer
Related show

Checks

Context Check Description
ci/travis-robot success Travis build: passed
ci/Intel-compilation success Compilation OK
ci/checkpatch warning coding style issues

Commit Message

Sarosh Arif Aug. 3, 2020, 11:20 a.m. UTC
If the user tries to reset/stop some other timer in it's callback
function, which is also about to expire, using 
rte_timer_reset_sync/rte_timer_stop_sync the application goes into
an infinite loop. This happens because 
rte_timer_reset_sync/rte_timer_stop_sync loop until the timer 
resets/stops and there is check inside timer_set_config_state which
prevents a running timer from being reset/stopped by not it's own 
timer_cb. Therefore timer_set_config_state returns -1 due to which 
rte_timer_reset returns -1 and rte_timer_reset_sync goes into an 
infinite loop

To to prevent this rte_timer_reset_sync and rte_timer_stop_sync should
have int return types, so that -1 can be returned if the above condition
occurs

Signed-off-by: Sarosh Arif <sarosh.arif@emumba.com>
---
 doc/guides/rel_notes/deprecation.rst | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Erik Gabriel Carrillo Aug. 3, 2020, 4:45 p.m. UTC | #1
> -----Original Message-----
> From: Sarosh Arif <sarosh.arif@emumba.com>
> Sent: Monday, August 3, 2020 6:21 AM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>; rsanford@akamai.com
> Cc: dev@dpdk.org; Sarosh Arif <sarosh.arif@emumba.com>
> Subject: [PATCH] doc: announce API change in timer
> 
> If the user tries to reset/stop some other timer in it's callback function, which
> is also about to expire, using rte_timer_reset_sync/rte_timer_stop_sync the
> application goes into an infinite loop. This happens because
> rte_timer_reset_sync/rte_timer_stop_sync loop until the timer resets/stops
> and there is check inside timer_set_config_state which prevents a running
> timer from being reset/stopped by not it's own timer_cb. Therefore
> timer_set_config_state returns -1 due to which rte_timer_reset returns -1
> and rte_timer_reset_sync goes into an infinite loop
> 
> To to prevent this rte_timer_reset_sync and rte_timer_stop_sync should
> have int return types, so that -1 can be returned if the above condition
> occurs
> 
> Signed-off-by: Sarosh Arif <sarosh.arif@emumba.com>
> ---
>  doc/guides/rel_notes/deprecation.rst | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> index ea4cfa7a4..ed93a707d 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -151,3 +151,9 @@ Deprecation Notices
>    Python 2 support will be completely removed in 20.11.
>    In 20.08, explicit deprecation warnings will be displayed when running
>    scripts with Python 2.
> +
> +* timer: Since timer can get stuck in an infinite loop if the
> +application tries to
> +  reset/stop some other timer in it's callback function, which is also
> +about to
> +  expire. The function ``rte_timer_stop_sync`` and

It looks like this should be rte_timer_reset_sync.  Maybe something like:

timer:  Timers can get stuck in an infinite loop if their callback tries to synchronously reset/stop some other timer that is also about to expire.  The functions ``rte_timer_reset_sync`` and ``rte_timer_stop_sync`` will updated with an int return type so that an error code can be returned when this condition occurs.

Thanks,
Erik

> +``rte_timer_stop_sync``  will
> +  have a int return type in order to return with -1 in when this
> +condition
> +  occures.
> --
> 2.17.1
Honnappa Nagarahalli Aug. 3, 2020, 11:16 p.m. UTC | #2
<snip>

> 
> If the user tries to reset/stop some other timer in it's callback function, which
Is there any use case for this? Why not just say document that the user is not allowed to reset some other timer in the call back function? How does the user get access to some other timer in the call back function?
Not sure if this was discussed earlier, I might have missed.

> is also about to expire, using rte_timer_reset_sync/rte_timer_stop_sync the
> application goes into an infinite loop. This happens because
> rte_timer_reset_sync/rte_timer_stop_sync loop until the timer resets/stops
> and there is check inside timer_set_config_state which prevents a running
> timer from being reset/stopped by not it's own timer_cb. Therefore
> timer_set_config_state returns -1 due to which rte_timer_reset returns -1 and
> rte_timer_reset_sync goes into an infinite loop
> 
> To to prevent this rte_timer_reset_sync and rte_timer_stop_sync should have
> int return types, so that -1 can be returned if the above condition occurs
> 
> Signed-off-by: Sarosh Arif <sarosh.arif@emumba.com>
> ---
>  doc/guides/rel_notes/deprecation.rst | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> index ea4cfa7a4..ed93a707d 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -151,3 +151,9 @@ Deprecation Notices
>    Python 2 support will be completely removed in 20.11.
>    In 20.08, explicit deprecation warnings will be displayed when running
>    scripts with Python 2.
> +
> +* timer: Since timer can get stuck in an infinite loop if the
> +application tries to
> +  reset/stop some other timer in it's callback function, which is also
> +about to
> +  expire. The function ``rte_timer_stop_sync`` and
> +``rte_timer_stop_sync``  will
> +  have a int return type in order to return with -1 in when this
> +condition
> +  occures.
> --
> 2.17.1
Sarosh Arif Aug. 4, 2020, 3:28 a.m. UTC | #3
Thank you Eric, I will fix the mistakes in v2

On Tue, Aug 4, 2020 at 4:16 AM Honnappa Nagarahalli
<Honnappa.Nagarahalli@arm.com> wrote:
>
> <snip>
>
> >
> > If the user tries to reset/stop some other timer in it's callback function, which
> Is there any use case for this? Why not just say document that the user is not allowed to reset some other timer in the call back function? How does the user get access to some other timer in the call back function?
> Not sure if this was discussed earlier, I might have missed.
The issue is more clearly described in bug 491 here is a link:
https://bugs.dpdk.org/show_bug.cgi?id=491
further discussion on this issue was done on the following patch:
https://patches.dpdk.org/patch/73683/

>
> > is also about to expire, using rte_timer_reset_sync/rte_timer_stop_sync the
> > application goes into an infinite loop. This happens because
> > rte_timer_reset_sync/rte_timer_stop_sync loop until the timer resets/stops
> > and there is check inside timer_set_config_state which prevents a running
> > timer from being reset/stopped by not it's own timer_cb. Therefore
> > timer_set_config_state returns -1 due to which rte_timer_reset returns -1 and
> > rte_timer_reset_sync goes into an infinite loop
> >
> > To to prevent this rte_timer_reset_sync and rte_timer_stop_sync should have
> > int return types, so that -1 can be returned if the above condition occurs
> >
> > Signed-off-by: Sarosh Arif <sarosh.arif@emumba.com>
> > ---
> >  doc/guides/rel_notes/deprecation.rst | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/doc/guides/rel_notes/deprecation.rst
> > b/doc/guides/rel_notes/deprecation.rst
> > index ea4cfa7a4..ed93a707d 100644
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > @@ -151,3 +151,9 @@ Deprecation Notices
> >    Python 2 support will be completely removed in 20.11.
> >    In 20.08, explicit deprecation warnings will be displayed when running
> >    scripts with Python 2.
> > +
> > +* timer: Since timer can get stuck in an infinite loop if the
> > +application tries to
> > +  reset/stop some other timer in it's callback function, which is also
> > +about to
> > +  expire. The function ``rte_timer_stop_sync`` and
> > +``rte_timer_stop_sync``  will
> > +  have a int return type in order to return with -1 in when this
> > +condition
> > +  occures.
> > --
> > 2.17.1
>
Erik Gabriel Carrillo Aug. 4, 2020, 2:24 p.m. UTC | #4
> -----Original Message-----
> From: Sarosh Arif <sarosh.arif@emumba.com>
> Sent: Monday, August 3, 2020 10:29 PM
> To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Carrillo, Erik G
> <erik.g.carrillo@intel.com>
> Cc: rsanford@akamai.com; dev@dpdk.org; nd <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH] doc: announce API change in timer
> 
> Thank you Eric, I will fix the mistakes in v2
> 
> On Tue, Aug 4, 2020 at 4:16 AM Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com> wrote:
> >
> > <snip>
> >
> > >
> > > If the user tries to reset/stop some other timer in it's callback
> > > function, which
> > Is there any use case for this? Why not just say document that the user is
> not allowed to reset some other timer in the call back function? How does
> the user get access to some other timer in the call back function?
> > Not sure if this was discussed earlier, I might have missed.
> The issue is more clearly described in bug 491 here is a link:
> https://bugs.dpdk.org/show_bug.cgi?id=491
> further discussion on this issue was done on the following patch:
> https://patches.dpdk.org/patch/73683/
> 

I like Honnappa's suggestion... we could document that the *_sync functions shouldn't be used inside timer callbacks since there are cases where it could hang.  Instead, if doing this was desired, the regular versions could be used there, and the return values could be checked in that case.

> >
> > > is also about to expire, using
> > > rte_timer_reset_sync/rte_timer_stop_sync the application goes into
> > > an infinite loop. This happens because
> > > rte_timer_reset_sync/rte_timer_stop_sync loop until the timer
> > > resets/stops and there is check inside timer_set_config_state which
> > > prevents a running timer from being reset/stopped by not it's own
> > > timer_cb. Therefore timer_set_config_state returns -1 due to which
> > > rte_timer_reset returns -1 and rte_timer_reset_sync goes into an
> > > infinite loop
> > >
> > > To to prevent this rte_timer_reset_sync and rte_timer_stop_sync
> > > should have int return types, so that -1 can be returned if the
> > > above condition occurs
> > >
> > > Signed-off-by: Sarosh Arif <sarosh.arif@emumba.com>
> > > ---
> > >  doc/guides/rel_notes/deprecation.rst | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/doc/guides/rel_notes/deprecation.rst
> > > b/doc/guides/rel_notes/deprecation.rst
> > > index ea4cfa7a4..ed93a707d 100644
> > > --- a/doc/guides/rel_notes/deprecation.rst
> > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > @@ -151,3 +151,9 @@ Deprecation Notices
> > >    Python 2 support will be completely removed in 20.11.
> > >    In 20.08, explicit deprecation warnings will be displayed when running
> > >    scripts with Python 2.
> > > +
> > > +* timer: Since timer can get stuck in an infinite loop if the
> > > +application tries to
> > > +  reset/stop some other timer in it's callback function, which is
> > > +also about to
> > > +  expire. The function ``rte_timer_stop_sync`` and
> > > +``rte_timer_stop_sync``  will
> > > +  have a int return type in order to return with -1 in when this
> > > +condition
> > > +  occures.
> > > --
> > > 2.17.1
> >
Honnappa Nagarahalli Aug. 4, 2020, 8:50 p.m. UTC | #5
<snip>
[
> > Subject: Re: [dpdk-dev] [PATCH] doc: announce API change in timer
> >
> > Thank you Eric, I will fix the mistakes in v2
> >
> > On Tue, Aug 4, 2020 at 4:16 AM Honnappa Nagarahalli
> > <Honnappa.Nagarahalli@arm.com> wrote:
> > >
> > > <snip>
> > >
> > > >
> > > > If the user tries to reset/stop some other timer in it's callback
> > > > function, which
> > > Is there any use case for this? Why not just say document that the
> > > user is
> > not allowed to reset some other timer in the call back function? How
> > does the user get access to some other timer in the call back function?
> > > Not sure if this was discussed earlier, I might have missed.
> > The issue is more clearly described in bug 491 here is a link:
> > https://bugs.dpdk.org/show_bug.cgi?id=491
> > further discussion on this issue was done on the following patch:
> > https://patches.dpdk.org/patch/73683/
Thanks for the links.

> >
> 
> I like Honnappa's suggestion... we could document that the *_sync functions
> shouldn't be used inside timer callbacks since there are cases where it could
> hang.  Instead, if doing this was desired, the regular versions could be used
> there, and the return values could be checked in that case.
Agree, non sync functions can be used.

> 
> > >
> > > > is also about to expire, using
> > > > rte_timer_reset_sync/rte_timer_stop_sync the application goes into
> > > > an infinite loop. This happens because
> > > > rte_timer_reset_sync/rte_timer_stop_sync loop until the timer
> > > > resets/stops and there is check inside timer_set_config_state
> > > > which prevents a running timer from being reset/stopped by not
> > > > it's own timer_cb. Therefore timer_set_config_state returns -1 due
> > > > to which rte_timer_reset returns -1 and rte_timer_reset_sync goes
> > > > into an infinite loop
> > > >
> > > > To to prevent this rte_timer_reset_sync and rte_timer_stop_sync
> > > > should have int return types, so that -1 can be returned if the
> > > > above condition occurs
> > > >
> > > > Signed-off-by: Sarosh Arif <sarosh.arif@emumba.com>
> > > > ---
> > > >  doc/guides/rel_notes/deprecation.rst | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/doc/guides/rel_notes/deprecation.rst
> > > > b/doc/guides/rel_notes/deprecation.rst
> > > > index ea4cfa7a4..ed93a707d 100644
> > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > @@ -151,3 +151,9 @@ Deprecation Notices
> > > >    Python 2 support will be completely removed in 20.11.
> > > >    In 20.08, explicit deprecation warnings will be displayed when running
> > > >    scripts with Python 2.
> > > > +
> > > > +* timer: Since timer can get stuck in an infinite loop if the
> > > > +application tries to
> > > > +  reset/stop some other timer in it's callback function, which is
> > > > +also about to
> > > > +  expire. The function ``rte_timer_stop_sync`` and
> > > > +``rte_timer_stop_sync``  will
> > > > +  have a int return type in order to return with -1 in when this
> > > > +condition
> > > > +  occures.
> > > > --
> > > > 2.17.1
> > >

Patch
diff mbox series

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index ea4cfa7a4..ed93a707d 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -151,3 +151,9 @@  Deprecation Notices
   Python 2 support will be completely removed in 20.11.
   In 20.08, explicit deprecation warnings will be displayed when running
   scripts with Python 2.
+
+* timer: Since timer can get stuck in an infinite loop if the application tries to
+  reset/stop some other timer in it's callback function, which is also about to
+  expire. The function ``rte_timer_stop_sync`` and ``rte_timer_stop_sync``  will
+  have a int return type in order to return with -1 in when this condition
+  occures.