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

["Wodkowski, PawelX" <pawelx.wodkowski@intel.com>]

> >
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_alarm.c
> > b/lib/librte_eal/linuxapp/eal/eal_alarm.c
> > index 480f0cb..73b6dc5 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal_alarm.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_alarm.c
> > @@ -64,6 +64,9 @@
> >  #define MS_PER_S 1000
> >  #define US_PER_S (US_PER_MS * MS_PER_S)
> >
> > +#define ALARM_EXECUTING (1 << 0)
> > +#define ALARM_CANCELLED (1 << 1)
> > +
> >  struct alarm_entry {
> >  	LIST_ENTRY(alarm_entry) next;
> >  	struct timeval time;
> > @@ -107,12 +110,14 @@ eal_alarm_callback(struct rte_intr_handle *hdl
> > __rte_unused,
> >  			gettimeofday(&now, NULL) == 0 &&
> >  			(ap->time.tv_sec < now.tv_sec || (ap->time.tv_sec ==
> > now.tv_sec &&
> >  						ap->time.tv_usec <=
> > now.tv_usec))){
> > -		ap->executing = 1;
> > -		rte_spinlock_unlock(&alarm_list_lk);
> 
> Removing unlock here introduce deadlock.

I does no spotted unlocking bellow so above is invalid.

> 
> > +		ap->executing |= ALARM_EXECUTING;
> > +		if (likely(!(ap->executing & ALARM_CANCELLED)) {
> > +			rte_spinlock_unlock(&alarm_list_lk);
> >
> > -		ap->cb_fn(ap->cb_arg);
> > +			ap->cb_fn(ap->cb_arg);
> >
> > -		rte_spinlock_lock(&alarm_list_lk);
> > +			rte_spinlock_lock(&alarm_list_lk);
> > +		}
> >  		LIST_REMOVE(ap, next);
> >  		rte_free(ap);
> >  	}
> > @@ -209,10 +214,9 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn,
> > void *cb_arg)
> >  	rte_spinlock_lock(&alarm_list_lk);
> >  	/* remove any matches at the start of the list */
> >  	while ((ap = LIST_FIRST(&alarm_list)) != NULL &&
> > -			cb_fn == ap->cb_fn && ap->executing == 0 &&
> > +			cb_fn == ap->cb_fn &&
> >  			(cb_arg == (void *)-1 || cb_arg == ap->cb_arg)) {
> > -		LIST_REMOVE(ap, next);
> > -		rte_free(ap);
> > +		ap->executing |= ALARM_CANCELLED;
> >  		count++;
> >  	}
> >  	ap_prev = ap;
> > @@ -220,10 +224,9 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn,
> > void *cb_arg)
> >  	/* now go through list, removing entries not at start */
> >  	LIST_FOREACH(ap, &alarm_list, next) {
> >  		/* this won't be true first time through */
> > -		if (cb_fn == ap->cb_fn &&  ap->executing == 0 &&
> > +		if (cb_fn == ap->cb_fn &&
> >  				(cb_arg == (void *)-1 || cb_arg == ap->cb_arg))
> > {
> > -			LIST_REMOVE(ap,next);
> > -			rte_free(ap);
> > +			ap->executing |= ALARM_CANCELLED;
> >  			count++;
> >  			ap = ap_prev;
> >  		}
> 
> Pawel