From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Sanford, Robert" Subject: Re: [PATCH 0/3] timer: fix rte_timer_manage and improve unit tests Date: Mon, 27 Jul 2015 15:46:11 +0000 Message-ID: References: <1437691347-58708-1-git-send-email-rsanford2@gmail.com> <4708068.jtpnxJe3Ir@xps13> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable To: Thomas Monjalon , "dev@dpdk.org" 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) In-Reply-To: <4708068.jtpnxJe3Ir@xps13> Content-Language: en-US Content-ID: List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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