From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Wodkowski, PawelX" <pawelx.wodkowski-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Subject: Re: [PATCH v2] Change alarm cancel function to
 thread-safe:
Date: Fri, 26 Sep 2014 09:49:54 +0000
Message-ID: <F6F2A6264E145F47A18AB6DF8E87425D12B39DE5@IRSMSX102.ger.corp.intel.com>
References: <1411649768-8084-1-git-send-email-michalx.k.jastrzebski@intel.com>
 <20140925150807.GD32725@hmsreliant.think-freely.org>
 <2601191342CEEE43887BDE71AB977258213769DE@IRSMSX105.ger.corp.intel.com>
 <20140925172358.GG32725@hmsreliant.think-freely.org>
 <F6F2A6264E145F47A18AB6DF8E87425D12B39AFE@IRSMSX102.ger.corp.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Cc: "dev-VfR2kkLFssw@public.gmane.org" <dev-VfR2kkLFssw@public.gmane.org>
To: "Wodkowski, PawelX" <pawelx.wodkowski-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>, Neil Horman
 <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>, "Ananyev, Konstantin" <konstantin.ananyev-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Return-path: <dev-bounces-VfR2kkLFssw@public.gmane.org>
In-Reply-To: <F6F2A6264E145F47A18AB6DF8E87425D12B39AFE-kPTMFJFq+rFP9JyJpTNKArfspsVTdybXVpNB7YpNyf8@public.gmane.org>
Content-Language: en-US
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev-VfR2kkLFssw@public.gmane.org>
List-Help: <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=subscribe>
Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org
Sender: "dev" <dev-bounces-VfR2kkLFssw@public.gmane.org>

> >
> > 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) =3D=3D 0 &&
> >  			(ap->time.tv_sec < now.tv_sec || (ap->time.tv_sec =3D=3D
> > now.tv_sec &&
> >  						ap->time.tv_usec <=3D
> > now.tv_usec))){
> > -		ap->executing =3D 1;
> > -		rte_spinlock_unlock(&alarm_list_lk);
>=20
> Removing unlock here introduce deadlock.

I does no spotted unlocking bellow so above is invalid.

>=20
> > +		ap->executing |=3D 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 =3D LIST_FIRST(&alarm_list)) !=3D NULL &&
> > -			cb_fn =3D=3D ap->cb_fn && ap->executing =3D=3D 0 &&
> > +			cb_fn =3D=3D ap->cb_fn &&
> >  			(cb_arg =3D=3D (void *)-1 || cb_arg =3D=3D ap->cb_arg)) {
> > -		LIST_REMOVE(ap, next);
> > -		rte_free(ap);
> > +		ap->executing |=3D ALARM_CANCELLED;
> >  		count++;
> >  	}
> >  	ap_prev =3D 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 =3D=3D ap->cb_fn &&  ap->executing =3D=3D 0 &&
> > +		if (cb_fn =3D=3D ap->cb_fn &&
> >  				(cb_arg =3D=3D (void *)-1 || cb_arg =3D=3D ap->cb_arg))
> > {
> > -			LIST_REMOVE(ap,next);
> > -			rte_free(ap);
> > +			ap->executing |=3D ALARM_CANCELLED;
> >  			count++;
> >  			ap =3D ap_prev;
> >  		}
>=20
> Pawel