From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pawelx.wodkowski@intel.com>
Received: from mga11.intel.com (mga11.intel.com [192.55.52.93])
by dpdk.org (Postfix) with ESMTP id 62DC07DEC
for <dev@dpdk.org>; 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" <pawelx.wodkowski@intel.com>
To: Neil Horman <nhorman@tuxdriver.com>, "Ananyev, Konstantin"
<konstantin.ananyev@intel.com>
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: <F6F2A6264E145F47A18AB6DF8E87425D12B39AFE@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>
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" <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 <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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