From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 62DC07DEC for ; Fri, 26 Sep 2014 08:26:57 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga102.fm.intel.com with ESMTP; 25 Sep 2014 23:33:17 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.97,862,1389772800"; d="scan'208";a="391871812" Received: from irsmsx101.ger.corp.intel.com ([163.33.3.153]) by FMSMGA003.fm.intel.com with ESMTP; 25 Sep 2014 23:27:11 -0700 Received: from irsmsx102.ger.corp.intel.com ([169.254.2.200]) by IRSMSX101.ger.corp.intel.com ([169.254.1.201]) with mapi id 14.03.0195.001; Fri, 26 Sep 2014 07:33:13 +0100 From: "Wodkowski, PawelX" To: Neil Horman , "Ananyev, Konstantin" Thread-Topic: [dpdk-dev] [PATCH v2] Change alarm cancel function to thread-safe: Thread-Index: AQHP2MAapayPUkT5UUGj9XqXyFJ3T5wR4sKAgAAPjwCAABZmAIAA6q+A 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> In-Reply-To: <20140925172358.GG32725@hmsreliant.think-freely.org> Accept-Language: pl-PL, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v2] Change alarm cancel function to thread-safe: X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 26 Sep 2014 06:26:58 -0000 > 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