From owner-svn-src-all@freebsd.org Fri Dec 9 18:41:24 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 8F6CCC6FA7D; Fri, 9 Dec 2016 18:41:24 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 30A35102D; Fri, 9 Dec 2016 18:41:24 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id uB9IfHMc011099 (version=TLSv1 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Fri, 9 Dec 2016 20:41:17 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua uB9IfHMc011099 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id uB9IfHRs011098; Fri, 9 Dec 2016 20:41:17 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Fri, 9 Dec 2016 20:41:17 +0200 From: Konstantin Belousov To: Gleb Smirnoff Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r309745 - in head: share/man/man9 sys/kern sys/sys Message-ID: <20161209184117.GJ54029@kib.kiev.ua> References: <201612091758.uB9HwYC0087384@repo.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201612091758.uB9HwYC0087384@repo.freebsd.org> User-Agent: Mutt/1.7.1 (2016-10-04) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 09 Dec 2016 18:41:24 -0000 On Fri, Dec 09, 2016 at 05:58:34PM +0000, Gleb Smirnoff wrote: > Author: glebius > Date: Fri Dec 9 17:58:34 2016 > New Revision: 309745 > URL: https://svnweb.freebsd.org/changeset/base/309745 > > Log: > Provide counter_ratecheck(), a MP-friendly substitution to ppsratecheck(). > When rated event happens at a very quick rate, the ppsratecheck() is not > only racy, but also becomes a performance bottleneck. > > Together with: rrs, jtl > > Modified: > head/share/man/man9/counter.9 > head/sys/kern/subr_counter.c > head/sys/sys/counter.h > > Modified: head/share/man/man9/counter.9 > ============================================================================== > --- head/share/man/man9/counter.9 Fri Dec 9 17:58:07 2016 (r309744) > +++ head/share/man/man9/counter.9 Fri Dec 9 17:58:34 2016 (r309745) > @@ -25,7 +25,7 @@ > .\" > .\" $FreeBSD$ > .\" > -.Dd March 14, 2016 > +.Dd December 8, 2016 > .Dt COUNTER 9 > .Os > .Sh NAME > @@ -51,6 +51,8 @@ > .Fn counter_u64_fetch "counter_u64_t c" > .Ft void > .Fn counter_u64_zero "counter_u64_t c" > +.Ft int64_t > +.Fn counter_ratecheck "struct counter_rate *cr" "int64_t limit" > .In sys/sysctl.h > .Fn SYSCTL_COUNTER_U64 parent nbr name access ptr descr > .Fn SYSCTL_ADD_COUNTER_U64 ctx parent nbr name access ptr descr > @@ -128,6 +130,18 @@ value for any moment. > Clear the counter > .Fa c > and set it to zero. > +.It Fn counter_ratecheck cr limit > +The function is a multiprocessor-friendly version of > +.Fn ppsratecheck , > +which uses > +.Nm > +internally. > +Returns non-negative value if the rate isn't yet reached during the current > +second, and a negative value otherwise. > +If the limit was reached on previous second, but was just reset back to zero, > +then > +.Fn counter_ratecheck > +returns number of events since previous reset. > .It Fn SYSCTL_COUNTER_U64 parent nbr name access ptr descr > Declare a static > .Xr sysctl > > Modified: head/sys/kern/subr_counter.c > ============================================================================== > --- head/sys/kern/subr_counter.c Fri Dec 9 17:58:07 2016 (r309744) > +++ head/sys/kern/subr_counter.c Fri Dec 9 17:58:34 2016 (r309745) > @@ -119,3 +119,57 @@ sysctl_handle_counter_u64_array(SYSCTL_H > > return (0); > } > + > +/* > + * MP-friendly version of ppsratecheck(). > + * > + * Returns non-negative if we are in the rate, negative otherwise. > + * 0 - rate limit not reached. > + * -1 - rate limit reached. > + * >0 - rate limit was reached before, and was just reset. The return value > + * is number of events since last reset. > + */ > +int64_t > +counter_ratecheck(struct counter_rate *cr, int64_t limit) > +{ > + int64_t val; > + int now; > + > + val = cr->cr_over; > + now = ticks; > + > + if (now - cr->cr_ticks >= hz) { > + /* > + * Time to clear the structure, we are in the next second. > + * First try unlocked read, and then proceed with atomic. > + */ > + if ((cr->cr_lock == 0) && > + atomic_cmpset_int(&cr->cr_lock, 0, 1)) { This should be cmpset_acq to avoid reordering of the operations from locked region outside of it, and to make _rel below to sync-with. Or call atomic_thread_fence_acq() after cmpset(), then unneeded barrier is not executed if atomic failed. > + /* > + * Check if other thread has just went through the > + * reset sequence before us. > + */ > + if (now - cr->cr_ticks >= hz) { > + val = counter_u64_fetch(cr->cr_rate); > + counter_u64_zero(cr->cr_rate); > + cr->cr_over = 0; > + cr->cr_ticks = now; > + } > + atomic_store_rel_int(&cr->cr_lock, 0); > + } else > + /* > + * We failed to lock, in this case other thread may > + * be running counter_u64_zero(), so it is not safe > + * to do an update, we skip it. > + */ > + return (val); > + } > + > + counter_u64_add(cr->cr_rate, 1); What prevents this thread to fail the check for now - cr->cr_ticks >= hz above, then get off the cpu long enough for the next second to tick, so that another thread starts the cleanup, while this thread performs the increment ? The current thread update is lost then. > + if (cr->cr_over != 0) > + return (-1); > + if (counter_u64_fetch(cr->cr_rate) > limit) > + val = cr->cr_over = -1; > + > + return (val); > +} > > Modified: head/sys/sys/counter.h > ============================================================================== > --- head/sys/sys/counter.h Fri Dec 9 17:58:07 2016 (r309744) > +++ head/sys/sys/counter.h Fri Dec 9 17:58:34 2016 (r309745) > @@ -59,5 +59,18 @@ uint64_t counter_u64_fetch(counter_u64_t > for (int i = 0; i < (n); i++) \ > counter_u64_zero((a)[i]); \ > } while (0) > + > +/* > + * counter(9) based rate checking. > + */ > +struct counter_rate { > + counter_u64_t cr_rate; /* Events since last second */ > + volatile int cr_lock; /* Lock to clean the struct */ > + int cr_ticks; /* Ticks on last clean */ > + int cr_over; /* Over limit since cr_ticks? */ > +}; > + > +int64_t counter_ratecheck(struct counter_rate *, int64_t); > + > #endif /* _KERNEL */ > #endif /* ! __SYS_COUNTER_H__ */