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=-7.0 required=3.0 tests=INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 3B24ACA90AF for ; Wed, 13 May 2020 08:06:13 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 01DD2206F5 for ; Wed, 13 May 2020 08:06:12 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 01DD2206F5 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 8A5A5900110; Wed, 13 May 2020 04:06:12 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 82FB89000F3; Wed, 13 May 2020 04:06:12 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6D1E2900110; Wed, 13 May 2020 04:06:12 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0080.hostedemail.com [216.40.44.80]) by kanga.kvack.org (Postfix) with ESMTP id 50A539000F3 for ; Wed, 13 May 2020 04:06:12 -0400 (EDT) Received: from smtpin01.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 0CBB1180AD807 for ; Wed, 13 May 2020 08:06:12 +0000 (UTC) X-FDA: 76810962984.01.spot02_18fef7653224a X-HE-Tag: spot02_18fef7653224a X-Filterd-Recvd-Size: 5371 Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by imf30.hostedemail.com (Postfix) with ESMTP for ; Wed, 13 May 2020 08:06:11 +0000 (UTC) Received: by mail-wm1-f65.google.com with SMTP id y24so27390476wma.4 for ; Wed, 13 May 2020 01:06:11 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=uMZjq/iOl4YhzI3MtVxiCP69dEbqt/r5VFCUz+vIP04=; b=HWbLiLGgPqkL6bnySmLBl7eU/qmgrIsk3Z0V9BAyTScPtG8+dEzbTKJqyJfvp64pOX ZPiMZ3lxzZM0EHgKZ38KoVujBIgBUkrtdw02AglH/lweODJ3cih4M5q0m/k4EhIZHD4T Co38nBDJVaI6N9EGMNORHdE33MegWZRn4g4mLut8mpHSkbm+84TRsfnZS644llSqPBev rhSpqGGgNEpZ+QJnh4KFuta3SBTHiWyOrlEgHmaq5d67EuC2UL6KYxIrBF4A1SCM5IJX nNX//NlTa/eXsafr83YFB7D6NG05YMJSP+4lL/ZUIVyOODMY8dhqHbEds3F3lQ6Z5R0P IdsQ== X-Gm-Message-State: AGi0PubVHZrBcV36pw17KKQ9nVstuUNWidylQDkbZn2e3hBy1Vq0l66p nwtpaYZXNDDdcVHLLMwKHCY= X-Google-Smtp-Source: APiQypLW0LASPxxt94QEDeFIjzHOhswaS3MCP1BE7S64KLKZMPIgWdL9Zeo/M+k3TyqJp7tuCQHmhw== X-Received: by 2002:a1c:2d14:: with SMTP id t20mr42849836wmt.28.1589357170591; Wed, 13 May 2020 01:06:10 -0700 (PDT) Received: from localhost (ip-37-188-249-36.eurotel.cz. [37.188.249.36]) by smtp.gmail.com with ESMTPSA id s12sm24086332wmb.3.2020.05.13.01.06.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 13 May 2020 01:06:09 -0700 (PDT) Date: Wed, 13 May 2020 10:06:07 +0200 From: Michal Hocko To: Jakub Kicinski Cc: akpm@linux-foundation.org, linux-mm@kvack.org, kernel-team@fb.com, tj@kernel.org, hannes@cmpxchg.org, chris@chrisdown.name, cgroups@vger.kernel.org, shakeelb@google.com Subject: Re: [PATCH mm v2 1/3] mm: prepare for swap over-high accounting and penalty calculation Message-ID: <20200513080607.GR29153@dhcp22.suse.cz> References: <20200511225516.2431921-1-kuba@kernel.org> <20200511225516.2431921-2-kuba@kernel.org> <20200512070858.GO29153@dhcp22.suse.cz> <20200512102819.4858a60a@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200512102819.4858a60a@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue 12-05-20 10:28:19, Jakub Kicinski wrote: > On Tue, 12 May 2020 09:08:58 +0200 Michal Hocko wrote: > > On Mon 11-05-20 15:55:14, Jakub Kicinski wrote: > > > Slice the memory overage calculation logic a little bit so we can > > > reuse it to apply a similar penalty to the swap. The logic which > > > accesses the memory-specific fields (use and high values) has to > > > be taken out of calculate_high_delay(). > > > > > > Signed-off-by: Jakub Kicinski > > > > Acked-by: Michal Hocko > > > > some recommendations below. > > Thank you! > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > > index 05dcb72314b5..8a9b671c3249 100644 > > > --- a/mm/memcontrol.c > > > +++ b/mm/memcontrol.c > > > @@ -2321,41 +2321,48 @@ static void high_work_func(struct work_struct *work) > > > #define MEMCG_DELAY_PRECISION_SHIFT 20 > > > #define MEMCG_DELAY_SCALING_SHIFT 14 > > > > > > -/* > > > - * Get the number of jiffies that we should penalise a mischievous cgroup which > > > - * is exceeding its memory.high by checking both it and its ancestors. > > > - */ > > > -static unsigned long calculate_high_delay(struct mem_cgroup *memcg, > > > - unsigned int nr_pages) > > > +static u64 calculate_overage(unsigned long usage, unsigned long high) > > > > the naming is slightly confusing. I would concider the return value > > to be in memory units rather than time because I would read it as > > overrage of high. calculate_throttle_penalty would be more clear to me. > > Hm. The unit is the fraction of high. Here is the code, it's quite hard > to read in diff form (I should have used --histogram, sorry): Yeah, I have checked the resulting code. > static u64 calculate_overage(unsigned long usage, unsigned long high) > { > u64 overage; > > if (usage <= high) > return 0; > > /* > * Prevent division by 0 in overage calculation by acting as if > * it was a threshold of 1 page > */ > high = max(high, 1UL); > > overage = usage - high; > overage <<= MEMCG_DELAY_PRECISION_SHIFT; > return div64_u64(overage, high); > } > > calculate_throttle_penalty() sounds like it returns time. How about > something like calc_overage_frac() ? Or calc_overage_perc()? > (abbreviating to "calc" so the caller fits on a line) heh, naming is hard and not the most important thing in the world. So if _penalty doesn't really sound good to you then let's just stick with what you've had. I do not really like the _perc/_frac much more because this is more about the implementation of the function than the intention. We shouldn't really care whether the throttling is based on overage scaled linearly (aka fraction) or by other means. The implementation might change in the future. -- Michal Hocko SUSE Labs