From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from prod-mail-xrelay07.akamai.com (unknown [23.79.238.175]) by dpdk.org (Postfix) with ESMTP id 37B35C572 for ; Mon, 27 Jul 2015 17:46:13 +0200 (CEST) Received: from prod-mail-xrelay07.akamai.com (localhost.localdomain [127.0.0.1]) by postfix.imss70 (Postfix) with ESMTP id BB2E448118; Mon, 27 Jul 2015 15:46:12 +0000 (GMT) Received: from prod-mail-relay06.akamai.com (prod-mail-relay06.akamai.com [172.17.120.126]) by prod-mail-xrelay07.akamai.com (Postfix) with ESMTP id A3C5447EF0; Mon, 27 Jul 2015 15:46:12 +0000 (GMT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=akamai.com; s=a1; t=1438011972; bh=Gq5SK/X/7CylQ1pUdt1Jkx59KdbBPTUUs/touLK6rcs=; h=From:To:Subject:Date:References:In-Reply-To:From; b=WiEoFSBflZ3ZU5NKyHr7xfg5IMwXtQBsHj6u7hFP5eisKDb8y1cw3W/I5eMy4z2Zo 5uQM8y5hJuj7O8mBc3d0bEAthmJAMCYjw050ojGGH8XiHzhk51GbxVABTPUwDjkMjG bGD0IoD/NBeg+9omE7lzPz+dJmbZ3m1nPVIC6FVY= Received: from email.msg.corp.akamai.com (ustx2ex-cas4.msg.corp.akamai.com [172.27.25.33]) by prod-mail-relay06.akamai.com (Postfix) with ESMTP id 7DA072027; Mon, 27 Jul 2015 15:46:12 +0000 (GMT) Received: from USTX2EX-DAG1MB3.msg.corp.akamai.com (172.27.27.103) by ustx2ex-dag1mb4.msg.corp.akamai.com (172.27.27.104) with Microsoft SMTP Server (TLS) id 15.0.1076.9; Mon, 27 Jul 2015 10:46:11 -0500 Received: from USTX2EX-DAG1MB3.msg.corp.akamai.com ([172.27.27.103]) by ustx2ex-dag1mb3.msg.corp.akamai.com ([172.27.27.103]) with mapi id 15.00.1076.000; Mon, 27 Jul 2015 10:46:11 -0500 From: "Sanford, Robert" To: Thomas Monjalon , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH 0/3] timer: fix rte_timer_manage and improve unit tests Thread-Index: AQHQyINc7Z3poR3vXUeWbiEyKD8PZg== Date: Mon, 27 Jul 2015 15:46:11 +0000 Message-ID: References: <1437691347-58708-1-git-send-email-rsanford2@gmail.com> <4708068.jtpnxJe3Ir@xps13> In-Reply-To: <4708068.jtpnxJe3Ir@xps13> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: user-agent: Microsoft-MacOutlook/14.4.3.140616 x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [172.19.132.110] Content-Type: text/plain; charset="us-ascii" Content-ID: Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 0/3] timer: fix rte_timer_manage and improve unit tests 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: Mon, 27 Jul 2015 15:46:13 -0000 Hi Thomas, > Please, could you re-send this serie after having added the description >of > each patch in the commit messages? Yes, I will move the paragraphs that begin with "Patch n" from patch 0 to their respective patches. > It seems you fix 2 bugs in the first patch. It may be clearer to split it > in 2 patches with separate explanations. No, I respectfully disagree. The only bug that we address in patch 1 is that the slaves become out of sync with the master. The section that begins with "Description of rte_timer_manage() race condition bug" is a general description to give background info for both patches 2 and 3. I would like to leave that section in part 0, if it's OK with you. -- Thanks, Robert >2015-07-23 18:42, rsanford2@gmail.com: >> From: Robert Sanford >>=20 >> This patchset fixes a bug in timer stress test 2, adds a new stress test >> to expose a race condition bug in API rte_timer_manage(), and then fixes >> the rte_timer_manage() bug. >> -- >>=20 >> Patch 1, app/test timer stress test 2: Sometimes this test fails and >> seg-faults because the slave lcores get out of phase with the master. >> The master uses a single int, 'ready', to synchronize multiple slave >> lcores through multiple phases of the test. >>=20 >> To resolve, we construct simple synchronization primitives that use one >> atomic-int state variable per slave. The master tells the slaves when to >> start, and then waits for all of them to finish. Each slave waits for >> the master to tell it to start, and then tells the master when it has >> finished. >> -- >>=20 >> Description of rte_timer_manage() race condition bug: Through code >> inspection, we noticed a potential problem in rte_timer_manage() that >> leads to corruption of per-lcore pending-lists (implemented as >> skip-lists). The race condition occurs when rte_timer_manage() expires >> multiple timers on lcore A, while lcore B simultaneously invokes >> rte_timer_reset() for one of the expiring timers (other than the first >> one). >>=20 >> Lcore A splits its pending-list, creating a local list of expired timers >> linked through their sl_next[0] pointers, and sets the first expired >> timer to the RUNNING state, all during one list-lock round trip. Lcore A >> then unlocks the list-lock to run the first callback, and that is when >> lcore B can misinterpret the subsequent expired timers' true states. >> Lcore B sees an expired timer still in the PENDING state, locks A's >> list-lock, and reinserts the timer into A's pending-list. We are trying >> to use the same pointer to belong to both lists! >>=20 >> One solution is to remove timers from the pending-list and set them to >> the RUNNING state in one atomic step, i.e., we should perform these two >> actions within one ownership of the list-lock. >> -- >>=20 >> Patch 2, new timer-manage race-condition test: We wrote a test to >> confirm our suspicion that we could crash rte_timer_manage() (RTM) under >> the right circumstances. We repeatedly set several timers to expire at >> roughly the same time on the master core. The master lcore just delays >> and runs RTM about ten times per second. The slave lcores all watch the >> first timer (timer-0) to see when RTM is running on the master, i.e., >> timer-0's state is not PENDING. At this point, each slave attempts to >> reset a subset of the timers to a later expiration time. The goal here >> is to have the slaves moving most of the timers to a different place in >> the master's pending-list, while the master is working with the same >> next-pointers and running callback functions. This eventually results in >> the master traversing a corrupted linked-list. In our observations, it >> results in an infinite loop. >> -- >>=20 >> Patch 3, eliminate race condition in rte_timer_manage(): After splitting >> the pending-list at the current time point, we try to set all expired >> timers to the RUNNING state, and put back into the pending-list any >> timers that we fail to set to the RUNNING state, all during one >> list-lock round trip. It is then safe to run the callback functions >> for all expired timers that remain on our local list, without the lock. > >Please, could you re-send this serie after having added the description of >each patch in the commit messages? >It seems you fix 2 bugs in the first patch. It may be clearer to split it >in 2 patches with separate explanations. > >Thanks