From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Wodkowski, PawelX" Subject: Re: [PATCH v2] Change alarm cancel function to thread-safe: Date: Fri, 26 Sep 2014 06:33:12 +0000 Message-ID: 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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev-VfR2kkLFssw@public.gmane.org" To: Neil Horman , "Ananyev, Konstantin" Return-path: In-Reply-To: <20140925172358.GG32725-B26myB8xz7F8NnZeBjwnZQMhkBWG/bsMQH7oEaQurus@public.gmane.org> Content-Language: en-US List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" > Given what you said above, I agree, at least in the current implementatio= n. It > still seems like theres a simpler solution that doesn't require all the > comparative gymnastics. Yes, there is simpler solution, but this solution involve recursive locking= . DPDK recursive spinlocks are no an option in here, so only option is posix = recursive mutex, which I think is even worst option than this gymnastics. >=20 > What if, instead of testing if you're the callback thread, we turn the ex= ecuting > field of alarm_entry into a bitfield, where bit 0 represents the former > "executing" state, and bit 1 is defined as a "cancelled" bit. Then > rte_eal_alarm_cancel becomes a search that, when an alarm is found simply= or's > in the cancelled bit to the executing bit field. When the callback threa= d runs, > it skips executing any alarm that is marked as cancelled, but frees all a= larm > entries that are executed or cancelled. That gives us a single point at = which > frees of alarm entires happen? Something like the patch below (completel= y > untested)? >=20 > It also seems like the alarm api as a whole could use some improvement. = The > way its written right now, theres no way to refer to a specific alarm (i.= e. > cancelation relies on the specification of a function and data pointer, w= hich > may refer to multiple timers). Shouldn't rte_eal_alarm_set return an opa= que > handle to a unique timer instance that can be store by a caller and used = to > specfically cancel that timer? Thats how both the bsd and linux timer > subsystems model timers. >=20 Goal was to not break user applications that use this library. >=20 >=20 > 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) >=20 > +#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); Removing unlock here introduce deadlock. > + ap->executing |=3D ALARM_EXECUTING; > + if (likely(!(ap->executing & ALARM_CANCELLED)) { > + rte_spinlock_unlock(&alarm_list_lk); >=20 > - ap->cb_fn(ap->cb_arg); > + ap->cb_fn(ap->cb_arg); >=20 > - 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; > } Pawel