From owner-freebsd-hackers@FreeBSD.ORG Tue Jun 9 11:07:59 2015 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 095F2F43 for ; Tue, 9 Jun 2015 11:07:59 +0000 (UTC) (envelope-from sebastian.huber@embedded-brains.de) Received: from mail.embedded-brains.de (host-82-135-62-35.customer.m-online.net [82.135.62.35]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 7134B1ACE for ; Tue, 9 Jun 2015 11:07:58 +0000 (UTC) (envelope-from sebastian.huber@embedded-brains.de) Received: from localhost (localhost.localhost [127.0.0.1]) by mail.embedded-brains.de (Postfix) with ESMTP id 7E4EE2A194C; Tue, 9 Jun 2015 13:08:05 +0200 (CEST) Received: from mail.embedded-brains.de ([127.0.0.1]) by localhost (zimbra.eb.localhost [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id mg9TypOI0gpa; Tue, 9 Jun 2015 13:08:05 +0200 (CEST) Received: from localhost (localhost.localhost [127.0.0.1]) by mail.embedded-brains.de (Postfix) with ESMTP id 075482A1992; Tue, 9 Jun 2015 13:08:05 +0200 (CEST) X-Virus-Scanned: amavisd-new at zimbra.eb.localhost Received: from mail.embedded-brains.de ([127.0.0.1]) by localhost (zimbra.eb.localhost [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id 3oz4TesUzRYs; Tue, 9 Jun 2015 13:08:04 +0200 (CEST) Received: from [192.168.96.129] (unknown [192.168.96.129]) by mail.embedded-brains.de (Postfix) with ESMTPSA id DEEFE2A194C; Tue, 9 Jun 2015 13:08:04 +0200 (CEST) Message-ID: <5576C90A.9010807@embedded-brains.de> Date: Tue, 09 Jun 2015 13:07:54 +0200 From: Sebastian Huber User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Konstantin Belousov CC: freebsd-hackers@freebsd.org, phk@phk.freebsd.dk Subject: Re: [PATCH v3] timecounters: Fix timehand generation read/write References: <1433331966-27548-1-git-send-email-sebastian.huber@embedded-brains.de> <1433838715-22850-1-git-send-email-sebastian.huber@embedded-brains.de> <20150609105526.GK2499@kib.kiev.ua> In-Reply-To: <20150609105526.GK2499@kib.kiev.ua> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: quoted-printable X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Jun 2015 11:07:59 -0000 On 09/06/15 12:55, Konstantin Belousov wrote: > On Tue, Jun 09, 2015 at 10:31:55AM +0200, Sebastian Huber wrote: >> The compiler is free to re-order load/store instructions to non-volati= le >> variables around a load/store of a volatile variable. So the volatile >> generation counter is insufficent. In addition tests on a Freescale >> T4240 platform with 24 PowerPC processors showed that real memory >> barriers are required. Compiler memory barriers are not enough. > This looks fine. The only detail I changed is that I do not see a reas= on > to use atomics or barriers on UP machines. See the fragment at the end > of the message for the exact diff. You still need the compiler memory barriers in the UP case (at least=20 with GCC 4.9 on PowerPC), e.g. something like this: static u_int tc_getgen(struct timehands *th) { u_int gen; gen =3D th->th_generation; __compiler_membar(); return (gen); } static void tc_setgen(struct timehands *th, u_int newgen) { __compiler_membar(); th->th_generation =3D newgen; } > >> For the test the timehand count was reduced to one with 10000 >> tc_windup() calls per second. The timehand memory location was adjust= ed >> so that the th_generation field was on its own cache line. > You mean, that the 'th_generation for its own cache line' was done for > testing only ? Yes, this was done for testing only. In case the timehand data is in one=20 cache line I don't think its possible to see the error case if no=20 barriers are present. > >> --- >> >> v2: Don't use tc_getgen() in tc_windup() since in the only writer ther= e is no >> need for a read memory barrier. >> >> v3: Use atomic load/store instead of explicit memory barriers. >> > If you do not have any comments, I will commit this version. > > diff --git a/sys/kern/kern_tc.c b/sys/kern/kern_tc.c > index 9dca0e8..fabfe03 100644 > --- a/sys/kern/kern_tc.c > +++ b/sys/kern/kern_tc.c > @@ -189,6 +190,28 @@ tc_delta(struct timehands *th) > tc->tc_counter_mask); > } > =20 > +static u_int > +tc_getgen(struct timehands *th) > +{ > + > +#ifdef SMP > + return (atomic_load_acq_int(&th->th_generation)); > +#else > + return (th->th_generation); > +#endif > +} > + > +static void > +tc_setgen(struct timehands *th, u_int newgen) > +{ > + > +#ifdef SMP > + atomic_store_rel_int(&th->th_generation, newgen); > +#else > + th->th_generation =3D newgen; > +#endif > +} > + > /* > * Functions for reading the time. We have to loop until we are sure= that > * the timehands that we operated on was not updated under our feet. = See --=20 Sebastian Huber, embedded brains GmbH Address : Dornierstr. 4, D-82178 Puchheim, Germany Phone : +49 89 189 47 41-16 Fax : +49 89 189 47 41-09 E-Mail : sebastian.huber@embedded-brains.de PGP : Public key available on request. Diese Nachricht ist keine gesch=E4ftliche Mitteilung im Sinne des EHUG.