From owner-freebsd-mips@FreeBSD.ORG Tue Aug 3 15:15:28 2010 Return-Path: Delivered-To: freebsd-mips@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D61F01065675 for ; Tue, 3 Aug 2010 15:15:28 +0000 (UTC) (envelope-from neelnatu@gmail.com) Received: from mail-wy0-f182.google.com (mail-wy0-f182.google.com [74.125.82.182]) by mx1.freebsd.org (Postfix) with ESMTP id 4D7228FC16 for ; Tue, 3 Aug 2010 15:15:27 +0000 (UTC) Received: by wyj26 with SMTP id 26so5490749wyj.13 for ; Tue, 03 Aug 2010 08:15:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:received:in-reply-to :references:date:message-id:subject:from:to:cc:content-type; bh=avEVTZNW5pd7cctHh+66q95QUKSKpS3FWk6zPwniGZo=; b=NPV+dmDSoafckPGAD1xp2h03GJtshOYaRzy1iQPAQSOIQt8rTqG88mBIF7tlrn2swH 9q626m8MqqOl4VLr6Lbvy9JhtPqR6veTXAyUGq9J5wXc1PjJ+xYWY9/rANyhTr2X8M3a 6qYZANhI5tmsnEwS61sYwIq7gsIMc07YrPit8= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; b=XJlqlBfdZPwbRLc3HePI2DWreVbVqK1LmEx8Df1HuTUTaTChwoDvWekov4SyQabXAj Q1nl2EMTTQpiRoyQ86s5Ii2l1sezYo5AHODp+FFvGpjTRBnYbE9d5JHwm/P6qn1EUJZC d0BKeXOHUKaS3371lrPtX85gPCVEHd/qvozvI= MIME-Version: 1.0 Received: by 10.216.35.138 with SMTP id u10mr888540wea.80.1280848527091; Tue, 03 Aug 2010 08:15:27 -0700 (PDT) Received: by 10.216.80.8 with HTTP; Tue, 3 Aug 2010 08:15:26 -0700 (PDT) In-Reply-To: <4C555CF7.5080101@FreeBSD.org> References: <4C41A248.8090605@FreeBSD.org> <4C41B4CF.6080409@FreeBSD.org> <4C4205CC.6080700@FreeBSD.org> <4C4ED247.80701@FreeBSD.org> <4C555CF7.5080101@FreeBSD.org> Date: Tue, 3 Aug 2010 08:15:26 -0700 Message-ID: From: Neel Natu To: Alexander Motin Content-Type: text/plain; charset=ISO-8859-1 Cc: Randall Stewart , freebsd-mips@freebsd.org, Neel Natu Subject: Re: [RFC] Event timers on MIPS X-BeenThere: freebsd-mips@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Porting FreeBSD to MIPS List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 03 Aug 2010 15:15:29 -0000 Hi Alexander, Thanks for taking the time to review the patch. Here is the updated patch: http://people.freebsd.org/~neel/tick_diff.txt On Sun, Aug 1, 2010 at 4:39 AM, Alexander Motin wrote: > Neel Natu wrote: >> Here is the patch for mips/mips/tick.c to fix tick_ticker(). >> >> In addition to incorporating the changes made in rmi/tick.c it fixes >> the following: >> >> - There is a race between clock_intr() and tick_ticker() updating >> counter_upper and counter_lower_last. This race exists because >> interrupts are enabled even though tick_ticker() executes in a >> critical section. > > While there is indeed a possible issue, I am not sure your solution is > reliable. I haven't looked how DPCPU_GET implemented on MIPS, but can't > compiler reorder them? I would thought about some lock or at least some > atomics with barriers. > Good catch. I have changed the type of 'counter_upper' and 'counter_lower_last' from 'uint32_t' to 'volatile uint32_t'. That should prevent the compiler from doing any reordering or loop optimization. I also verified the assembly listing of tick_ticker() and made sure that the such optimizations are not occurring. > "t_upper++;" there looks a bit strange, as it is not written back. The > wrapping stuff won't work if this timer interrupts were not used. > This part is intentional. I wanted only clock_intr() to update the cached values of 'counter_upper' and 'counter_lower_last' and tick_ticker() to sample a consistent snapshot of the tuple and then operate on it. I have added an XXX comment to describe the dependency. We can revisit this if we change the default timer in mips. >> - Fix a bug in clock_intr() in how it updates counter_upper and >> counter_lower_last. It updates it only once every time the COUNT >> register wraps around. More interestingly it will *never* update the >> cached values of 'counter_upper' and 'counter_lower_last' if the >> previous value of 'counter_lower_last' happens to be '0'. > > Reasonable. It would be nice if both wrapping places were implemented > alike or the same way. > This was intentional. See above. >> - Get rid of the superfluous critical section in clock_intr(). There >> is no reason for it because clock_intr() executes in hard interrupt >> context. > > Seems OK. > best Neel > -- > Alexander Motin >