From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,USER_AGENT_NEOMUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 56796C10F13 for ; Mon, 8 Apr 2019 11:49:19 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 28AC2204EC for ; Mon, 8 Apr 2019 11:49:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726602AbfDHLtS (ORCPT ); Mon, 8 Apr 2019 07:49:18 -0400 Received: from foss.arm.com ([217.140.101.70]:46904 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726119AbfDHLtR (ORCPT ); Mon, 8 Apr 2019 07:49:17 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 0C29E15BF; Mon, 8 Apr 2019 04:49:17 -0700 (PDT) Received: from e110439-lin (e110439-lin.cambridge.arm.com [10.1.194.43]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 168953F718; Mon, 8 Apr 2019 04:49:13 -0700 (PDT) Date: Mon, 8 Apr 2019 12:49:08 +0100 From: Patrick Bellasi To: Suren Baghdasaryan Cc: LKML , linux-pm@vger.kernel.org, linux-api@vger.kernel.org, Ingo Molnar , Peter Zijlstra , Tejun Heo , "Rafael J . Wysocki" , Vincent Guittot , Viresh Kumar , Paul Turner , Quentin Perret , Dietmar Eggemann , Morten Rasmussen , Juri Lelli , Todd Kjos , Joel Fernandes , Steve Muckle Subject: Re: [PATCH v8 01/16] sched/core: uclamp: Add CPU's clamp buckets refcounting Message-ID: <20190408114908.evij7pqml6ltqtnl@e110439-lin> References: <20190402104153.25404-1-patrick.bellasi@arm.com> <20190402104153.25404-2-patrick.bellasi@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20180716 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06-Apr 16:51, Suren Baghdasaryan wrote: > On Tue, Apr 2, 2019 at 3:42 AM Patrick Bellasi wrote: [...] > > + * The first few values calculated by this routine: > > + * bf(0) = 1 > > + * bf(1) = 1 > > + * bf(2) = 2 > > + * bf(3) = 2 > > + * bf(4) = 3 > > + * ... and so on. > > + */ > > +#define bits_per(n) \ > > +( \ > > + __builtin_constant_p(n) ? ( \ > > + ((n) == 0 || (n) == 1) ? 1 : ( \ > > + ((n) & (n - 1)) == 0 ? \ > > missing braces around 'n' > - ((n) & (n - 1)) == 0 ? \ > + ((n) & ((n) - 1)) == 0 ? \ > > > + ilog2((n) - 1) + 2 : \ > > + ilog2((n) - 1) + 1 \ > > Isn't this "((n) & ((n) - 1)) == 0 ? ilog2((n) - 1) + 2 : ilog2((n) - > 1) + 1" expression equivalent to a simple "ilog2(n) + 1"? Right, since we already have n=0 and n=1 as special cases, what you propose should work for all n>=2. > > > + ) \ > > + ) : \ > > + __bits_per(n) \ > > +) > > #endif /* _LINUX_LOG2_H */ [...] > > +static inline unsigned int uclamp_bucket_base_value(unsigned int clamp_value) > > Where are you using uclamp_bucket_base_value()? I would expect its > usage somewhere inside uclamp_rq_dec_id() when the last task in the > bucket is dequeued but I don't see it... This behavior is not move into a dedicated patch, as per Peter request: Message-ID: <20190314111849.gx6bl6myfjtaan7r@e110439-lin> This functions was left here to support a the intialization code in init_uclamp() but... I notice know I'm doing the initialization in a different way thus, I'll move it into the following patch. > > +{ > > + return UCLAMP_BUCKET_DELTA * uclamp_bucket_id(clamp_value); > > +} > > + [...] > > +static inline void uclamp_rq_dec_id(struct task_struct *p, struct rq *rq, > > + unsigned int clamp_id) > > +{ > > + struct uclamp_rq *uc_rq = &rq->uclamp[clamp_id]; > > + struct uclamp_se *uc_se = &p->uclamp[clamp_id]; > > + struct uclamp_bucket *bucket; > > + unsigned int rq_clamp; > > + > > + bucket = &uc_rq->bucket[uc_se->bucket_id]; > > + SCHED_WARN_ON(!bucket->tasks); > > + if (likely(bucket->tasks)) > > + bucket->tasks--; > > + > > + if (likely(bucket->tasks)) > > Shouldn't you adjust bucket->value if the remaining tasks in the > bucket have a lower clamp value than the task that was just removed? No, this is never done. As long as a bucket is not empty/idle we never reset it to its nominal value. In this patch specifically, the value is never changed since we moved the "local max tracking" bits into a dedicated patch. > > + return; > > + > > + rq_clamp = READ_ONCE(uc_rq->value); > > + /* > > + * Defensive programming: this should never happen. If it happens, > > + * e.g. due to future modification, warn and fixup the expected value. > > + */ > > + SCHED_WARN_ON(bucket->value > rq_clamp); > > + if (bucket->value >= rq_clamp) > > + WRITE_ONCE(uc_rq->value, uclamp_rq_max_value(rq, clamp_id)); > > +} [...] > > +static void __init init_uclamp(void) > > +{ > > + unsigned int clamp_id; > > + int cpu; > > + > > + for_each_possible_cpu(cpu) { > > + struct uclamp_bucket *bucket; > > + struct uclamp_rq *uc_rq; > > + unsigned int bucket_id; > > + > > + memset(&cpu_rq(cpu)->uclamp, 0, sizeof(struct uclamp_rq)); > > + > > + for (clamp_id = 0; clamp_id < UCLAMP_CNT; ++clamp_id) { > > + uc_rq = &cpu_rq(cpu)->uclamp[clamp_id]; > > + > > + bucket_id = 1; > > + while (bucket_id < UCLAMP_BUCKETS) { > > + bucket = &uc_rq->bucket[bucket_id]; > > + bucket->value = bucket_id * UCLAMP_BUCKET_DELTA; > > + ++bucket_id; > > + } > > + } > > + } All the initialization code above is not more required after the next patch introducing "local max tracking". > > + > > + for (clamp_id = 0; clamp_id < UCLAMP_CNT; ++clamp_id) { > > + struct uclamp_se *uc_se = &init_task.uclamp[clamp_id]; > > + > > + uc_se->value = uclamp_none(clamp_id); > > + uc_se->bucket_id = uclamp_bucket_id(uc_se->value); > > + } > > +} [...] -- #include Patrick Bellasi