Re: [dpdk-dev] [PATCH v2] Change alarm cancel function to thread-safe:

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Neil Horman
> Sent: Thursday, September 25, 2014 4:08 PM
> To: Jastrzebski, MichalX K
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] Change alarm cancel function to thread-safe:
> 
> On Thu, Sep 25, 2014 at 01:56:08PM +0100, Michal Jastrzebski wrote:
> >     Change alarm cancel function to thread-safe.
> >     It eliminates a race between threads using rte_alarm_cancel and
> >     rte_alarm_set.
> >
> > Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
> > Reviewed-by: Michal Jastrzebski <michalx.k.jastrzebski@intel.com>
> >
> > ---
> >  lib/librte_eal/common/include/rte_alarm.h |    3 +-
> >  lib/librte_eal/linuxapp/eal/eal_alarm.c   |   68 ++++++++++++++++++-----------
> >  2 files changed, 45 insertions(+), 26 deletions(-)
> >
> 
> > diff --git a/lib/librte_eal/common/include/rte_alarm.h b/lib/librte_eal/common/include/rte_alarm.h
> > index d451522..e7cbaef 100644
> > --- a/lib/librte_eal/common/include/rte_alarm.h
> > +++ b/lib/librte_eal/common/include/rte_alarm.h
> > @@ -76,7 +76,8 @@ typedef void (*rte_eal_alarm_callback)(void *arg);
> >  int rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb, void *cb_arg);
> >
> >  /**
> > - * Function to cancel an alarm callback which has been registered before.
> > + * Function to cancel an alarm callback which has been registered before. If
> > + * used outside alarm callback it wait for all callbacks to finish its execution.
> >   *
> >   * @param cb_fn
> >   *  alarm callback
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_alarm.c b/lib/librte_eal/linuxapp/eal/eal_alarm.c
> > index 480f0cb..ea8dfb4 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal_alarm.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_alarm.c
> > @@ -69,7 +69,8 @@ struct alarm_entry {
> >  	struct timeval time;
> >  	rte_eal_alarm_callback cb_fn;
> >  	void *cb_arg;
> > -	volatile int executing;
> > +	volatile uint8_t executing;
> > +	volatile pthread_t executing_id;
> >  };
> >
> >  static LIST_HEAD(alarm_list, alarm_entry) alarm_list = LIST_HEAD_INITIALIZER();
> > @@ -108,11 +109,13 @@ eal_alarm_callback(struct rte_intr_handle *hdl __rte_unused,
> >  			(ap->time.tv_sec < now.tv_sec || (ap->time.tv_sec == now.tv_sec &&
> >  						ap->time.tv_usec <= now.tv_usec))){
> >  		ap->executing = 1;
> > +		ap->executing_id = pthread_self();
> How exactly does this work?  From my read all alarm callbacks are handled by the
> thread created in rte_eal_intr_init (which runs forever in
> eal_intr_thread_main()). 

In current implementation - yes.

 So every assignment to the above executing_id value
> will be from that thread.  As such, anytime rte_eal_alarm_cancel is called from
> within a callback we are guaranteed that:
> a) the ap->executing flag is set to 1
> b) the ap->executing_id value should equal whatever is returned from
> pthread_self()

Yes

> 
> That will cause the executing counter local to the cancel function to get
> incremented, meaning we will deadlock withing that do { ... } while (executing
> != 0) loop, no?

No, as for the case when cancel is called from callback:
pthread_equal(ap->executing_id, pthread_self())
would return non-zero value (which means threads ids are equal), so executing will not be incremented. 

> 
> >  		rte_spinlock_unlock(&alarm_list_lk);
> >
> >  		ap->cb_fn(ap->cb_arg);
> >
> >  		rte_spinlock_lock(&alarm_list_lk);
> > +
> >  		LIST_REMOVE(ap, next);
> >  		rte_free(ap);
> >  	}
> > @@ -145,7 +148,7 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
> >  	if (us < 1 || us > (UINT64_MAX - US_PER_S) || cb_fn == NULL)
> >  		return -EINVAL;
> >
> > -	new_alarm = rte_malloc(NULL, sizeof(*new_alarm), 0);
> > +	new_alarm = rte_zmalloc(NULL, sizeof(*new_alarm), 0);
> >  	if (new_alarm == NULL)
> >  		return -ENOMEM;
> >
> > @@ -156,7 +159,6 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
> >  	new_alarm->cb_arg = cb_arg;
> >  	new_alarm->time.tv_usec = (now.tv_usec + us) % US_PER_S;
> >  	new_alarm->time.tv_sec = now.tv_sec + ((now.tv_usec + us) / US_PER_S);
> > -	new_alarm->executing = 0;
> >
> This removes the only place where ->executing is cleared again.  If there is
> only one change to this bits state (which is the case after this patch), it
> seems that you can just use the executing bit as the test in the alarm_cancel
> function, and remove all the pthread_self mess.

I believe we do need executing_id here.
It allows us to distinguish are we executing cancel from a callback or not.

> We still need to address the
> deadlock question, but it seems using this single flag is easier than using
> pthread_self.
> 
> Neil