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=-13.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,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 9934EC433FE for ; Fri, 4 Dec 2020 16:20:11 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 01D4722838 for ; Fri, 4 Dec 2020 16:20:10 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 01D4722838 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=bytedance.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 8BCD76B0072; Fri, 4 Dec 2020 11:20:10 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 86D4A6B0073; Fri, 4 Dec 2020 11:20:10 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7AB306B0078; Fri, 4 Dec 2020 11:20:10 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0165.hostedemail.com [216.40.44.165]) by kanga.kvack.org (Postfix) with ESMTP id 643866B0072 for ; Fri, 4 Dec 2020 11:20:10 -0500 (EST) Received: from smtpin13.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 227B61EF1 for ; Fri, 4 Dec 2020 16:20:10 +0000 (UTC) X-FDA: 77556111780.13.blow25_07166d1273c5 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin13.hostedemail.com (Postfix) with ESMTP id 162CB18140B70 for ; Fri, 4 Dec 2020 16:20:08 +0000 (UTC) X-HE-Tag: blow25_07166d1273c5 X-Filterd-Recvd-Size: 5808 Received: from mail-pf1-f196.google.com (mail-pf1-f196.google.com [209.85.210.196]) by imf14.hostedemail.com (Postfix) with ESMTP for ; Fri, 4 Dec 2020 16:20:07 +0000 (UTC) Received: by mail-pf1-f196.google.com with SMTP id q22so4021540pfk.12 for ; Fri, 04 Dec 2020 08:20:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Fe+vbNKf2orQSQdQfp84tFWO7dmTK0e8xZVK2kuk9vc=; b=00ymjmeHcRm+t9WqfDypR12xjjGpRELcXY0sIkHfHtMG6GdeFg8EcQt28ccHWoeOIk tOcEiwui9V9z5lzZCtEYUMXKoXacmBh3GI5G/Obk0zIwy7KXlWsq7P8hxvuUC9JLs4cT NatI8Rdmu1F+VnftUvhJ6bJj+11mSL4aVSPk7Qt8f5pEYc29Sva7p5t1SmunuF4uZlLV DAoYBcmG/3chKFux9QqTK0Y+YruaH4fHQglRAaT4ywJ5KetCdUKm5N5kWPd6ultHAUpO G6oupDP9ndJQ4STkntJw/zwi4f1qtk5NJbc4eX9ND0GJAgw6X5UEcw2iFT1Opd4n3cKz V6IA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Fe+vbNKf2orQSQdQfp84tFWO7dmTK0e8xZVK2kuk9vc=; b=m+sP/fLUKHXh22rY8AZxGJL6uqE2xtbQ8Pdt8aOhiu/wYrTAOxxG6zJLqZ1g0s8Bld PNGRpUq1LRpXp0uiq8AhWLXob9Lb3denyAPv7Y/QLJ35MMGRxW1aGWeL1YUhLTqAv2CO YWROGmVtR7L9wTl1/8iN3yUliP3hNbzZdQ0qZHGBqJdlywDslDbUBfR0PqdA4UfYmt1E 37KsJycon2UwRZ6010DNe5ekmd/8E/yqdqlSRY4wmu6A6ZItLI25LFMKkeVp/Glb/cfo UIK1QfGhpmkeQ26vSUK+xsK131CTPENSGjsjliwnTdPahaVkhnwP9TmxFNeRQdKL1Czh 1yDA== X-Gm-Message-State: AOAM533bcwEYWHuISnt0T3EsmM21N1TmQUG3tF8bL8I2Qx1WdBRs9zWN OW7wvhVMqrUyE/SJ9wdBOAZv2mFCVYPCuMzEGrGnFg== X-Google-Smtp-Source: ABdhPJx3djr9fUf8EXR6B02YTCxWp5UKSEi9b1HmR59NDusa/KdvB75cQjsaMRdK5L9unlrT3dQcwY8opF6Bq/+6m5U= X-Received: by 2002:a62:3103:0:b029:19b:d4e8:853c with SMTP id x3-20020a6231030000b029019bd4e8853cmr4524242pfx.59.1607098806291; Fri, 04 Dec 2020 08:20:06 -0800 (PST) MIME-Version: 1.0 References: <20201203031111.3187-1-songmuchun@bytedance.com> <20201204154613.GA176901@cmpxchg.org> In-Reply-To: <20201204154613.GA176901@cmpxchg.org> From: Muchun Song Date: Sat, 5 Dec 2020 00:19:30 +0800 Message-ID: Subject: Re: [External] Re: [PATCH v2] mm/memcontrol: make the slab calculation consistent To: Johannes Weiner Cc: Michal Hocko , Vladimir Davydov , Andrew Morton , Cgroups , Linux Memory Management List , LKML , Roman Gushchin Content-Type: text/plain; charset="UTF-8" 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 Fri, Dec 4, 2020 at 11:48 PM Johannes Weiner wrote: > > On Thu, Dec 03, 2020 at 11:11:11AM +0800, Muchun Song wrote: > > Although the ratio of the slab is one, we also should read the ratio > > from the related memory_stats instead of hard-coding. And the local > > variable of size is already the value of slab_unreclaimable. So we > > do not need to read again. Simplify the code here. > > > > Signed-off-by: Muchun Song > > Acked-by: Roman Gushchin > > I agree that ignoring the ratio right now is not very pretty, but > > size = memcg_page_state(memcg, NR_SLAB_RECLAIMABLE_B) + > memcg_page_state(memcg, NR_SLAB_UNRECLAIMABLE_B); > seq_buf_printf(&s, "slab %llu\n", size); > > is way easier to understand and more robust than using idx and idx + 1 > and then requiring a series of BUG_ONs to ensure these two items are > actually adjacent and in the right order. > > There is a redundant call to memcg_page_state(), granted, but that > function is extremely cheap compared with e.g. seq_buf_printf(). > > > mm/memcontrol.c | 26 +++++++++++++++++++++----- > > 1 file changed, 21 insertions(+), 5 deletions(-) > > IMO this really just complicates the code with little discernible > upside. It's going to be a NAK from me, unfortunately. > > > In retrospect, I think that memory_stats[] table was a mistake. It > would probably be easier to implement this using a wrapper for > memcg_page_state() that has a big switch() for unit > conversion. Something like this: > > /* Translate stat items to the correct unit for memory.stat output */ > static unsigned long memcg_page_state_output(memcg, item) > { > unsigned long value = memcg_page_state(memcg, item); > int unit = PAGE_SIZE; > > switch (item) { > case NR_SLAB_RECLAIMABLE_B: > case NR_SLAB_UNRECLAIMABLE_B: > case WORKINGSET_REFAULT_ANON: > case WORKINGSET_REFAULT_FILE: > case WORKINGSET_ACTIVATE_ANON: > case WORKINGSET_ACTIVATE_FILE: > case WORKINGSET_RESTORE_ANON: > case WORKINGSET_RESTORE_FILE: > case MEMCG_PERCPU_B: > unit = 1; > break; > case NR_SHMEM_THPS: > case NR_FILE_THPS: > case NR_ANON_THPS: > unit = HPAGE_PMD_SIZE; > break; > case NR_KERNEL_STACK_KB: > unit = 1024; > break; > } > > return value * unit; > } > > This would fix the ratio inconsistency, get rid of the awkward mix of > static and runtime initialization of the table, is probably about the > same amount of code, but simpler and more obvious overall. Good idea. I can do that :) Thanks. -- Yours, Muchun