From owner-freebsd-mips@FreeBSD.ORG Sat Jul 31 00:47:58 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 CB853106566C; Sat, 31 Jul 2010 00:47:58 +0000 (UTC) (envelope-from neelnatu@gmail.com) Received: from mail-qw0-f54.google.com (mail-qw0-f54.google.com [209.85.216.54]) by mx1.freebsd.org (Postfix) with ESMTP id 486498FC13; Sat, 31 Jul 2010 00:47:58 +0000 (UTC) Received: by qwk3 with SMTP id 3so506079qwk.13 for ; Fri, 30 Jul 2010 17:47:57 -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=Zy3tsS/Hh/9MtD0Rsol9Isu57my1lGp2cNoEi/UtdWU=; b=PjVptfbwt4Ncclj5k+iTWlJnEhLRfh0/l3a/aKlumA7lEsgjqLB1LxFCAES25DM4Nk KBYREeppfMGDS8++BAdbVwA2hT2su4NURVbltGY+LIiKNDSNxFy1PEOGQSrdcylSBJS4 Z8t24VDp0GNwdyEHmQhbVmDJoPLodN25jqgKo= 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=kvUbcNbNKG2QdBrr4b8PSEkK1ZOgZHkPw5xvl9bi+ACqLxdRqAi0okPHcAxdlQSAEb NNygamUYqleKxsmTpf8gW8hxRznLZdd3BOxmt8NPN6wi8EM3WCn904abwMClEIJ7Ypah hNJD9FXRjMBjtvSFa9oEbUn1TRMI/VgVcgLe0= MIME-Version: 1.0 Received: by 10.224.66.23 with SMTP id l23mr544853qai.152.1280537277169; Fri, 30 Jul 2010 17:47:57 -0700 (PDT) Received: by 10.224.36.201 with HTTP; Fri, 30 Jul 2010 17:47:56 -0700 (PDT) In-Reply-To: References: <4C41A248.8090605@FreeBSD.org> <4C41B4CF.6080409@FreeBSD.org> <4C4205CC.6080700@FreeBSD.org> <4C4ED247.80701@FreeBSD.org> Date: Fri, 30 Jul 2010 17:47:56 -0700 Message-ID: From: Neel Natu To: "Jayachandran C." Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: Randall Stewart , Alexander Motin , Neel Natu , 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: Sat, 31 Jul 2010 00:47:58 -0000 Hi, On Tue, Jul 27, 2010 at 7:43 AM, Jayachandran C. wrote: > On Tue, Jul 27, 2010 at 6:04 PM, Alexander Motin wrote: >> Jayachandran C. wrote: >>> On Sun, Jul 18, 2010 at 1:04 AM, Alexander Motin wrot= e: >>>> Jayachandran C. wrote: >>>>> On XLR we would like to use the count/compare which is faster but les= s >>>>> accurate on all cpus - we can have upto 32 cpus now. =A0We also have = a >>>>> PIC which can provide a better timestamp and timer interrupts. =A0Thi= s >>>>> PIC timestamp can be read from all CPUs but the timer interrupt can b= e >>>>> delivered to just one CPU at a time. =A0I think this is how we ended = up >>>>> with the current implementation, but any suggestions on how to improv= e >>>>> this is welcome. >>> >>> As a first step, I have copied the count /compare code from mips with >>> minor modifications into mips/rmi, this lets me boot up (checked in as >>> r210528). >>> >>> I would like to add the PIC based clock next. >> >> Thanks. >> >>>> I would prefer to not mix the things. >>>> >>>> I think: >>>> =A0- PIC timestamp looks like the best candidate for system timecounte= r. >>>> =A0- per-CPU counters could be registered as per-CPU timecounters with >>>> set_cputicker() - the main criteria there is a speed. >>>> =A0- if per-CPU counters are synchronized between CPUs - they could be >>>> registered as alternative timecounter for people who wish fastest >>>> timecounting; if they are not - they are useless in that role. >>>> =A0- both PIC timer and per-CPU comparators should be independently >>>> registered as eventtimers - it is better to have two of them to from >>>> accounting correctness PoV, and it will allow user to experiment which >>>> one he likes more. >>>> =A0- if there is any other timer hardware - it also should be register= ed - >>>> it will give additional flexibility. >>> >>> The per-cpu count/compare counters are not synchronized on XLR. >> >> Then tick_ticker() function looks broken. counter_lower_last and >> counter_upper should be tracked per-CPU. Otherwise you will have huge >> forward jumps due to false overflows. >> >>> So your suggestion would be to add a PIC based clock which calls >>> tc_init() and et_register(), and to leave the set_cputicker() to be >>> the count/compare? >> >> Yes. And I would leave count/compare also calling tc_init() and >> et_register() as it is now. It won't hurt. >> >>> Also, with just the count/compare, I get these print on early mutiuser = bootup. >>> --- >>> calcru: runtime went backwards from 85936878 usec to 236488 usec for >>> pid 1286 (rpcbind) >>> calcru: runtime went backwards from 7158742 usec to 19700 usec for pid >>> 1285 (nfsiod 0) >>> calcru: runtime went backwards from 111005442 usec to 305474 usec for >>> pid 1257 (syslogd) >>> calcru: runtime went backwards from 10740196 usec to 29555 usec for >>> pid 1048 (devd) >>> -- >>> Did not get much time to investigate, any idea what the cause =A0can be= ? >> >> I think it can easily be result of broken tick_ticker(). > > I'm planning to check-in the attached patch for mips/rmi, I think > mips/mips would need something similar. > 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. - 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'. - Get rid of the superfluous critical section in clock_intr(). There is no reason for it because clock_intr() executes in hard interrupt context. Comments? best Neel Index: sys/mips/sibyte/sb_machdep.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- sys/mips/sibyte/sb_machdep.c (revision 210664) +++ sys/mips/sibyte/sb_machdep.c (working copy) @@ -454,6 +454,4 @@ mips_init(); mips_timer_init_params(sb_cpu_speed(), 0); - - set_cputicker(sb_zbbus_cycle_count, sb_cpu_speed() / 2, 1); } Index: sys/mips/mips/tick.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- sys/mips/mips/tick.c (revision 210664) +++ sys/mips/mips/tick.c (working copy) @@ -60,9 +60,8 @@ static DPCPU_DEFINE(uint32_t, cycles_per_tick); static uint32_t cycles_per_usec; -static u_int32_t counter_upper =3D 0; -static u_int32_t counter_lower_last =3D 0; - +static DPCPU_DEFINE(uint32_t, counter_upper); +static DPCPU_DEFINE(uint32_t, counter_lower_last); static DPCPU_DEFINE(uint32_t, compare_ticks); static DPCPU_DEFINE(uint32_t, lost_ticks); @@ -104,21 +103,32 @@ { uint64_t ret; uint32_t ticktock; + uint32_t t_lower_last, t_upper; /* - * XXX: MIPS64 platforms can read 64-bits of counter directly. - * Also: the tc code is supposed to cope with things wrapping - * from the time counter, so I'm not sure why all these hoops - * are even necessary. + * Disable preemption because we are working with cpu specific data. */ + critical_enter(); + + /* + * Note that even though preemption is disabled, interrupts are + * still enabled. In particular there is a race with clock_intr() + * reading the values of 'counter_upper' and 'counter_lower_last'. + */ + do { + t_upper =3D DPCPU_GET(counter_upper); + t_lower_last =3D DPCPU_GET(counter_lower_last); + } while (t_upper !=3D DPCPU_GET(counter_upper)); + ticktock =3D mips_rd_count(); - critical_enter(); - if (ticktock < counter_lower_last) - counter_upper++; - counter_lower_last =3D ticktock; + critical_exit(); - ret =3D ((uint64_t) counter_upper << 32) | counter_lower_last; + /* COUNT register wrapped around */ + if (ticktock < t_lower_last) + t_upper++; + + ret =3D ((uint64_t)t_upper << 32) | ticktock; return (ret); } @@ -262,11 +272,11 @@ } else /* In one-shot mode timer should be stopped after the event. */ mips_wr_compare(0xffffffff); - critical_enter(); - if (count < counter_lower_last) { - counter_upper++; - counter_lower_last =3D count; + /* COUNT register wrapped around */ + if (count < DPCPU_GET(counter_lower_last)) { + DPCPU_SET(counter_upper, DPCPU_GET(counter_upper) + 1); } + DPCPU_SET(counter_lower_last, count); if (cycles_per_tick > 0) { @@ -296,7 +306,6 @@ } if (sc->et.et_active) sc->et.et_event_cb(&sc->et, sc->et.et_arg); - critical_exit(); return (FILTER_HANDLED); } > JC. >