From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755947AbcJZKyP (ORCPT ); Wed, 26 Oct 2016 06:54:15 -0400 Received: from merlin.infradead.org ([205.233.59.134]:36428 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753618AbcJZKyN (ORCPT ); Wed, 26 Oct 2016 06:54:13 -0400 Date: Wed, 26 Oct 2016 12:54:07 +0200 From: Peter Zijlstra To: Vincent Guittot Cc: mingo@kernel.org, linux-kernel@vger.kernel.org, dietmar.eggemann@arm.com, yuyang.du@intel.com, Morten.Rasmussen@arm.com, linaro-kernel@lists.linaro.org, pjt@google.com, bsegall@google.com, kernellwp@gmail.com Subject: Re: [PATCH 4/6 v5] sched: propagate load during synchronous attach/detach Message-ID: <20161026105407.GE3102@twins.programming.kicks-ass.net> References: <1476695653-12309-1-git-send-email-vincent.guittot@linaro.org> <1476695653-12309-5-git-send-email-vincent.guittot@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1476695653-12309-5-git-send-email-vincent.guittot@linaro.org> User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Oct 17, 2016 at 11:14:11AM +0200, Vincent Guittot wrote: > /* > + * Signed add and clamp on underflow. > + * > + * Explicitly do a load-store to ensure the intermediate value never hits > + * memory. This allows lockless observations without ever seeing the negative > + * values. > + */ > +#define add_positive(_ptr, _val) do { \ > + typeof(_ptr) ptr = (_ptr); \ > + typeof(_val) res, val = (_val); \ > + typeof(*ptr) var = READ_ONCE(*ptr); \ > + res = var + val; \ > + if (res < 0) \ > + res = 0; \ I think this is broken, and inconsistent with sub_positive(). The thing is, util_avg, on which you use this, is an unsigned type. Checking for unsigned underflow can be done by comparing against either one of the terms. > + WRITE_ONCE(*ptr, res); \ > +} while (0) > + add_positive(&cfs_rq->avg.util_avg, delta);