From owner-freebsd-mips@FreeBSD.ORG Thu Jul 22 06:33:13 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 994681065676; Thu, 22 Jul 2010 06:33:13 +0000 (UTC) (envelope-from neelnatu@gmail.com) Received: from mail-px0-f182.google.com (mail-px0-f182.google.com [209.85.212.182]) by mx1.freebsd.org (Postfix) with ESMTP id 680B08FC24; Thu, 22 Jul 2010 06:33:13 +0000 (UTC) Received: by pxi8 with SMTP id 8so3554475pxi.13 for ; Wed, 21 Jul 2010 23:33:12 -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 :content-transfer-encoding; bh=FL4L/Ll2QM+zxg8vobWxYDNyYPDJxxTOLCi4fbrLAps=; b=eNtKP1l9Bnx5F4Jy3pgZKEVd/zj7BHsI6J8Ra3WznDv4+QfXN7EQYCMPyLDOHKWWch Bn+O9zuH2fHB2bDwEeur8ef9pdGzGgQvDd4kQYSFKf2c7e2RJrFMduWAqU5/qyMzmq+Y S66Aq5gXu8xIhxbg4nULY4lp6IM+xI+FbwuBU= 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:content-transfer-encoding; b=EFH7TeWHk3aan9SMvRmY7Ip4Jwo45zjU1mDhcJ+vh9877VRRw2cRVmn2mXF0loyiF7 2NlP9cwwV6wJIkxP0zxwutNqxb3HGKJsrX8M4sQR+rHVlHk9UAj6lRTz3n76vnOTLhik 3Damw/Zprt4gjhkMI5o8YG8s3+DZbPV3qrwk8= MIME-Version: 1.0 Received: by 10.142.199.21 with SMTP id w21mr1749937wff.140.1279780391179; Wed, 21 Jul 2010 23:33:11 -0700 (PDT) Received: by 10.142.139.19 with HTTP; Wed, 21 Jul 2010 23:33:10 -0700 (PDT) In-Reply-To: <4C47D8CD.7020209@FreeBSD.org> References: <4C41A248.8090605@FreeBSD.org> <4C4698D6.2090104@FreeBSD.org> <4C47D8CD.7020209@FreeBSD.org> Date: Wed, 21 Jul 2010 23:33:10 -0700 Message-ID: From: Neel Natu To: Alexander Motin Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: freebsd-mips@freebsd.org 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: Thu, 22 Jul 2010 06:33:13 -0000 Hi Alexander, On Wed, Jul 21, 2010 at 10:36 PM, Alexander Motin wrote: > Hi. > > Neel Natu wrote: >> Your patch works fine on a Sibyte. I verified that wall clock time is >> progressing as expected. Is there any other test that you recommend? >> >> I have a few comments about the patch: >> >> 1. set_cputicker() in clock_attach() should be moved back up to >> =A0 =A0 mips_timer_init_params(). Otherwise the platform-specific CPU ti= ckers >> =A0 =A0used by Sibyte, Octeon and RMI will be overwritten by the mips32 = ticker. > > Fixed. > >> 2. The local variable 'cycles_per_tick' in clock_intr() can now be a uin= t32_t. > > Fixed. > >> 3. Is there a race between setting 'cycles_per_tick' in clock_start() an= d >> =A0 =A0 clock_intr()? Would it be better to write the compare register f= irst >> =A0 =A0 and then set 'cycles_per_tick' to a non-zero value? > > And how does it help? > So, the timeline I have in mind is this: A: clock_stop() sets compare register to 0xffffffff and cycles_per_tick to = 0 B: clock_start() is called and we update cycles_per_tick to a non-zero value but we have not updated the compare register yet C: the COUNT register is a free running counter and it happens to reach 0xffffffff exactly at this time D: the clock_intr() handler sees a non-zero cycles_per_tick value and proceeds as if this was a legitimate interrupt (when in reality it is not) If we update the cycles_per_tick at the very end then it is guaranteed that any interrupt that happens while it is zero is essentially ignored. And any interrupt that happens after it has been updated to a non-zero value is legitimate. >> =A0 =A0 If I understand the code correctly we are liable to get clock in= terrupts >> =A0 =A0 even afer the clock is stopped when the CP0 COUNT register match= es >> =A0 =A0 0xffffffff. > > You mean we can receive interrupt from previous clock_start() after new > one? Theoretically I think it is possible, but what can we do about it? > I don't think we can do anything about it. I was referring to the race when the clock was stopped and we are in the process of restarting it. See above. >> 4. We need to update the DPCPU(compare_ticks) value when we start the ti= mer >> =A0 =A0 in clock_start(). > > Fixed. > >> 5. I think the entire 'lost_ticks' processing in clock_intr() should be >> =A0 =A0 conditional on (cycles_per_tick > 0). Without this we may incorr= ectly >> =A0 =A0 update the value of DPCPU(lost_ticks). > > Fixed. Indeed, in one-shot mode extra callback can be confusing. > >> 6. Can you a couple of lines of explaining the computatation of >> =A0 =A0 'et_min_period' and 'et_max_period'? It is not completely obviou= s to me. > > Minimal period is set artificialy to reduce possibility to lost very > short time interval during comparator programming. I've done it after > marius@ asked to do the same for sparc64. If this comparator is more > clever to handle missed time, it may not be needed. > Maximal period is calculated from maximal counter value, minus one to > possibly avoid some rounding overflows. Again, if this comparator is > "clever" may be it need to be reduced. > I don't have documentation for this hardware, so fix me if I am wrong. > I see. Thanks for the explanation. > New patch: http://people.freebsd.org/~mav/timers_mips3.patch > In clock_intr() it would seem that the last 'et_event_cb()' should be called conditionally only if (cycles_per_tick > 0). Of course, this is necessary only if my explanation about spurious clock_intr() invocations is correct. I have tested the latest patch on the Sibyte as well and it works correctly= . best Neel > Thank you! > > -- > Alexander Motin >