From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR02-AM5-obe.outbound.protection.outlook.com (mail-eopbgr00049.outbound.protection.outlook.com [40.107.0.49]) by dpdk.org (Postfix) with ESMTP id 6265A1B727 for ; Wed, 19 Dec 2018 08:57:05 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector1-arm-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=FgkijD8FdcdNAxYYLp7Y37UnBFAGsyikBTlDJY3V1EQ=; b=Gykw5n97tZBTgqzqa6lK5uB1LMgNVT6eW/Jttlp+o4/EO+Se/acV2S7X6sCYvhBamXDI20TcchnKH2NdyuCJCSR4DUOOGds1sZhKGSZByhZgclxk0xbuJeTnZd//inOUCVll/cFiIRhx4bM//lpsO5OkN/f1cISeKl5z94uqrSw= Received: from VI1PR08MB3167.eurprd08.prod.outlook.com (52.133.15.142) by VI1PR08MB1248.eurprd08.prod.outlook.com (10.166.198.143) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1425.19; Wed, 19 Dec 2018 07:57:03 +0000 Received: from VI1PR08MB3167.eurprd08.prod.outlook.com ([fe80::b5a5:e179:34f1:7d21]) by VI1PR08MB3167.eurprd08.prod.outlook.com ([fe80::b5a5:e179:34f1:7d21%5]) with mapi id 15.20.1446.018; Wed, 19 Dec 2018 07:57:03 +0000 From: "Gavin Hu (Arm Technology China)" To: Thomas Monjalon , "dev@dpdk.org" CC: Erik Gabriel Carrillo , "rsanford@akamai.com" , "olivier.matz@6wind.com" , "stephen@networkplumber.org" , "bruce.richardson@intel.com" Thread-Topic: [dpdk-dev] [PATCH 1/1] timer: fix race condition Thread-Index: AQHUiBTayCDO2QHSckOpfhtSADMFb6WFiACAgABH20A= Date: Wed, 19 Dec 2018 07:57:03 +0000 Message-ID: References: <1543517626-142526-1-git-send-email-erik.g.carrillo@intel.com> <3302607.WHYlCW4uPu@xps> In-Reply-To: <3302607.WHYlCW4uPu@xps> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=Gavin.Hu@arm.com; x-originating-ip: [113.29.88.7] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; VI1PR08MB1248; 6:QhLRl049gWj95U6ltX8eHaRrhG0KPm6sozx3FPg7DcfwTeVaKuLMkwYnDpiqJgtV48O3XP09FJYXOWgyDJHa9FB3iZ3ZrUq+D+nozumB9T9uwS2E0VSRtnTMKxsgEXi5BbfVDYNj7c9+5Q1mwhLYK3xI85a1ffvj1zUR5dOElrdB1X788iyGG9qBL1hDi7OR7PNvygk7Pe8AyoMZ81eWjP81MDUUP7aI8csJck9P6y8fLyy11p67FkciXDDooV5bZgdEajYXD+8yGuiADu7s8B5xYVD21e2aMX5dbdrEqNYcGX6UZd3wsq2Dls99qqY2XI62HSn0XeTrS9RdQ7+EMB25RkBTBmTYZ5iWiDSEGv9yYmBStaOx6j1Cd1hwcdQii4lreik3/ftXWptIEEXErO9tRe1vUtEKzj+khjEnQHfiI7aFLULUy+NgEh7hsSHxif30B+65KtphA4mhkTHnWw==; 5:QjWxo6+AvdVJa5qHjhfoYceUcfvrUI3F8LdNsPHLPPQPCrG4nCtUewp62QXrjog9mSBMEdQ7//kpbsqIANKrMDqVP8oxoJKzTyNtNuVtYB7JKrGS0o5ClAxz5R7ddZv/ClezjYyni5Yd4HAMUSWIf3Z57ohA434NfJsxhY91ABQ=; 7:f1SWuKDMepvETMy98u8QTdrM0JHhiHa/IopGHsiInCHHuYaC8Gzftr4jZeDbhyFlSLmA8mF0dj+A8ZN/aRBTDZbJ1hFTahxqjo1exxBDxVoVXh3AZ/ME/LtoU4PB2d3mxCYjbheTWXssVkVEMeQk2w== x-ms-exchange-antispam-srfa-diagnostics: SOS; x-ms-office365-filtering-correlation-id: 22b2f3cf-fd28-431e-9aab-08d665879024 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0; PCL:0; RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(7168020)(4627221)(201703031133081)(201702281549075)(8990200)(5600074)(711020)(4618075)(2017052603328)(7153060)(7193020); SRVR:VI1PR08MB1248; x-ms-traffictypediagnostic: VI1PR08MB1248: x-microsoft-antispam-prvs: x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(3230021)(999002)(6040522)(2401047)(5005006)(8121501046)(3231475)(944501520)(52105112)(93006095)(93001095)(10201501046)(3002001)(6055026)(148016)(149066)(150057)(6041310)(20161123558120)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123564045)(20161123562045)(20161123560045)(201708071742011)(7699051)(76991095); SRVR:VI1PR08MB1248; BCL:0; PCL:0; RULEID:; SRVR:VI1PR08MB1248; x-forefront-prvs: 0891BC3F3D x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(376002)(396003)(346002)(39860400002)(366004)(136003)(13464003)(189003)(199004)(40434004)(6116002)(7736002)(9686003)(8676002)(3846002)(55236004)(55016002)(81156014)(5660300001)(53936002)(71190400001)(71200400001)(2906002)(33656002)(110136005)(7696005)(99286004)(53546011)(26005)(316002)(6506007)(8936002)(81166006)(186003)(54906003)(229853002)(86362001)(575784001)(76176011)(102836004)(6436002)(2501003)(97736004)(68736007)(478600001)(25786009)(446003)(66066001)(74316002)(6246003)(305945005)(4326008)(476003)(105586002)(486006)(106356001)(14444005)(11346002)(14454004)(5024004)(256004)(72206003); DIR:OUT; SFP:1101; SCL:1; SRVR:VI1PR08MB1248; H:VI1PR08MB3167.eurprd08.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; MX:1; A:1; received-spf: None (protection.outlook.com: arm.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: rF6IS1k66Uk8tXozrEw4mMiY+bWvZ7roqbHDJOXSro8iwDXQuVgvHnOcfEQ8C/fdpEvrhLRzHFSvVDFx18hHz7FIekhNtDJNqiOiTVWLVy9SSu2iIf0bjQcDP+8VVYLjUxIciwCSWkP4RGx/JWX3L2KL2B2VQyH0QHmaPBdkg5ncqKRF/+RYdyT4hl/nV8hWCe1tjKUvtNMxOS/UKkjnvGJJKQqGXu2/ArkeDoJUYEeKD2ScRwglg6ttgMP6noscfA1ZZl01zKlYYBoDxh+yyh5KTalhyxOP2DHPI2LIb14lvuREfvNyBG4lD1OnTXKs spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-Network-Message-Id: 22b2f3cf-fd28-431e-9aab-08d665879024 X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Dec 2018 07:57:03.1010 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR08MB1248 Subject: Re: [dpdk-dev] [PATCH 1/1] timer: fix race condition X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 19 Dec 2018 07:57:05 -0000 > -----Original Message----- > From: dev On Behalf Of Thomas Monjalon > Sent: Wednesday, December 19, 2018 11:37 AM > To: dev@dpdk.org > Cc: Erik Gabriel Carrillo ; rsanford@akamai.co= m; > olivier.matz@6wind.com; stephen@networkplumber.org; > bruce.richardson@intel.com > Subject: Re: [dpdk-dev] [PATCH 1/1] timer: fix race condition > > Who could review this fix please? > > 29/11/2018 19:53, Erik Gabriel Carrillo: > > rte_timer_manage() adds expired timers to a "run list", and walks the > > list, transitioning each timer from the PENDING to the RUNNING state. > > If another lcore resets or stops the timer at precisely this moment, > > the timer state would instead be set to CONFIG by that other lcore, > > which would cause timer_manage() to skip over it. This is expected > > behavior. > > > > However, if a timer expires quickly enough, there exists the following > > race condition that causes the timer_manage() routine to misinterpret > > a timer in CONFIG state, resulting in lost timers: > > > > - Thread A: > > - starts a timer with rte_timer_reset() > > - the timer is moved to CONFIG state > > - the spinlock associated with the appropriate skiplist is acquired > > - timer is inserted into the skiplist > > - the spinlock is released > > - Thread B: > > - executes rte_timer_manage() > > - find above timer as expired, add it to run list > > - walk run list, see above timer still in CONFIG state, unlink it fro= m > > run list and continue on > > - Thread A: > > - move timer to PENDING state > > - return from rte_timer_reset() > > - timer is now in PENDING state, but not actually linked into skiplis= t Add "nor the run list"? > > and will never get processed further by rte_timer_manage() > > > > This commit fixes this race condition by only releasing the spinlock > > after the timer state has been transitioned from CONFIG to PENDING, > > which prevents rte_timer_manage() from seeing an incorrect state. > > > > Fixes: 9b15ba895b9f ("timer: use a skip list") > > Signed-off-by: Erik Gabriel Carrillo > > --- > > lib/librte_timer/rte_timer.c | 28 ++++++++++++++-------------- > > 1 file changed, 14 insertions(+), 14 deletions(-) > > > > diff --git a/lib/librte_timer/rte_timer.c > > b/lib/librte_timer/rte_timer.c index 590488c..30c7b0a 100644 > > --- a/lib/librte_timer/rte_timer.c > > +++ b/lib/librte_timer/rte_timer.c > > @@ -241,24 +241,17 @@ timer_get_prev_entries_for_node(struct > rte_timer *tim, unsigned tim_lcore, > > } > > } > > > > -/* > > - * add in list, lock if needed > > +/* call with lock held as necessary > > + * add in list > > * timer must be in config state > > * timer must not be in a list > > */ > > static void > > -timer_add(struct rte_timer *tim, unsigned tim_lcore, int > > local_is_locked) > > +timer_add(struct rte_timer *tim, unsigned int tim_lcore) > > { > > -unsigned lcore_id =3D rte_lcore_id(); > > unsigned lvl; > > struct rte_timer *prev[MAX_SKIPLIST_DEPTH+1]; > > > > -/* if timer needs to be scheduled on another core, we need to > > - * lock the list; if it is on local core, we need to lock if > > - * we are not called from rte_timer_manage() */ > > -if (tim_lcore !=3D lcore_id || !local_is_locked) > > -rte_spinlock_lock(&priv_timer[tim_lcore].list_lock); > > - > > /* find where exactly this element goes in the list of elements > > * for each depth. */ > > timer_get_prev_entries(tim->expire, tim_lcore, prev); @@ -282,9 > > +275,6 @@ timer_add(struct rte_timer *tim, unsigned tim_lcore, int > local_is_locked) > > * NOTE: this is not atomic on 32-bit*/ > > priv_timer[tim_lcore].pending_head.expire =3D priv_timer[tim_lcore].\ > > pending_head.sl_next[0]->expire; > > - > > -if (tim_lcore !=3D lcore_id || !local_is_locked) > > -rte_spinlock_unlock(&priv_timer[tim_lcore].list_lock); > > } > > > > /* > > @@ -379,8 +369,15 @@ __rte_timer_reset(struct rte_timer *tim, > uint64_t expire, > > tim->f =3D fct; > > tim->arg =3D arg; > > > > +/* if timer needs to be scheduled on another core, we need to > > + * lock the destination list; if it is on local core, we need to lock = if > > + * we are not called from rte_timer_manage() > > + */ > > +if (tim_lcore !=3D lcore_id || !local_is_locked) > > +rte_spinlock_lock(&priv_timer[tim_lcore].list_lock); > > + > > __TIMER_STAT_ADD(pending, 1); > > -timer_add(tim, tim_lcore, local_is_locked); > > +timer_add(tim, tim_lcore); > > > > /* update state: as we are in CONFIG state, only us can modify > > * the state so we don't need to use cmpset() here */ @@ -389,6 > > +386,9 @@ __rte_timer_reset(struct rte_timer *tim, uint64_t expire, > > status.owner =3D (int16_t)tim_lcore; > > tim->status.u32 =3D status.u32; > > > > +if (tim_lcore !=3D lcore_id || !local_is_locked) > > +rte_spinlock_unlock(&priv_timer[tim_lcore].list_lock); > > + > > return 0; > > } > > Other than the minor comment, Reviewed-by: Gavin Hu > > > > IMPORTANT NOTICE: The contents of this email and any attachments are confid= ential and may also be privileged. If you are not the intended recipient, p= lease notify the sender immediately and do not disclose the contents to any= other person, use it for any purpose, or store or copy the information in = any medium. Thank you.