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=-1.0 required=3.0 tests=MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no 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 49FF1C4BA0B for ; Wed, 26 Feb 2020 09:14:25 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 10F842084E for ; Wed, 26 Feb 2020 09:14:25 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 10F842084E 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 B2FA96B0006; Wed, 26 Feb 2020 04:14:24 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id AE17A6B0007; Wed, 26 Feb 2020 04:14:24 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9A9026B0008; Wed, 26 Feb 2020 04:14:24 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0073.hostedemail.com [216.40.44.73]) by kanga.kvack.org (Postfix) with ESMTP id 813016B0006 for ; Wed, 26 Feb 2020 04:14:24 -0500 (EST) Received: from smtpin01.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 62BE5180AD807 for ; Wed, 26 Feb 2020 09:14:24 +0000 (UTC) X-FDA: 76531717248.01.chin76_43853e77fc634 X-HE-Tag: chin76_43853e77fc634 X-Filterd-Recvd-Size: 5448 Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by imf44.hostedemail.com (Postfix) with ESMTP for ; Wed, 26 Feb 2020 09:14:23 +0000 (UTC) Received: by mail-wr1-f68.google.com with SMTP id c13so1218205wrq.10 for ; Wed, 26 Feb 2020 01:14:23 -0800 (PST) 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=90Gaup4GBl/WZhK+7p3+YI0kruzLG0IEp7O26cMJO1E=; b=Ehgo8PkdTx+XHo1nCE3i7ogXyi1JlSu6nGDGSsgRRIZr5C2viCb9xg202rJX5gOrCw DEY2r9lfLELbulMC9oN+RWMpWmBAZEqNcbGulMB7UbiKa/xWY1vkyL9jqh4V5GOYQXGW 4pQQAPl1wby/+cFnGnKzPT8qcK2IC4ymUydE8ImaT8GG1GgUTKHp61KlhhhZo86qhkDi DRTRmHpACxthkTMX3et79bskJZXUJIICgX86wrRzFI6m6u9lPydCsPo+z3qslHX9E9or tvltULch7yakHbAzFfQ8+fZL5DImkD12i6ARGRoCz3pol7e8OSaaIkvFPLGU0PxuRv9H njPA== X-Gm-Message-State: APjAAAUawB4LWISldXqESx/oHt1q9sUsVY+okMkH9FF1R6lZWiskIdgu PdOpSpq+AcXAWAh6tnIZ//g= X-Google-Smtp-Source: APXvYqyU+AFLfI6C9bXoMHUATVgRno4Q4tuyNnRlEGnnnzJUedVksiuvqUK1i8VPkH85h89Js03b8g== X-Received: by 2002:adf:ecc2:: with SMTP id s2mr4382833wro.263.1582708462718; Wed, 26 Feb 2020 01:14:22 -0800 (PST) Received: from localhost (prg-ext-pat.suse.com. [213.151.95.130]) by smtp.gmail.com with ESMTPSA id o15sm2430534wra.83.2020.02.26.01.14.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 26 Feb 2020 01:14:22 -0800 (PST) Date: Wed, 26 Feb 2020 10:14:21 +0100 From: Michal Hocko To: Baoquan He Cc: David Hildenbrand , linux-kernel@vger.kernel.org, linux-mm@kvack.org, akpm@linux-foundation.org, richardw.yang@linux.intel.com, osalvador@suse.de, dan.j.williams@intel.com, rppt@linux.ibm.com, robin.murphy@arm.com Subject: Re: [PATCH v2 0/7] mm/hotplug: Only use subsection map in VMEMMAP case Message-ID: <20200226091421.GE3771@dhcp22.suse.cz> References: <20200220043316.19668-1-bhe@redhat.com> <20200220103849.GG20509@dhcp22.suse.cz> <20200221142847.GG4937@MiWiFi-R3L-srv> <75b4f840-7454-d6d0-5453-f0a045c852fa@redhat.com> <20200225100226.GM22443@dhcp22.suse.cz> <20200226034236.GD24216@MiWiFi-R3L-srv> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200226034236.GD24216@MiWiFi-R3L-srv> 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 Wed 26-02-20 11:42:36, Baoquan He wrote: > On 02/25/20 at 11:02am, Michal Hocko wrote: > > On Tue 25-02-20 10:10:45, David Hildenbrand wrote: > > > >>> include/linux/mmzone.h | 2 + > > > >>> mm/sparse.c | 178 +++++++++++++++++++++++++++++------------ > > > >>> 2 files changed, 127 insertions(+), 53 deletions(-) > > > >> > > > >> Why do we need to add so much code to remove a functionality from one > > > >> memory model? > > > > > > > > Hmm, Dan also asked this before. > > > > > > > > The adding mainly happens in patch 2, 3, 4, including the two newly > > > > added function defitions, the code comments above them, and those added > > > > dummy functions for !VMEMMAP. > > > > > > AFAIKS, it's mostly a bunch of newly added comments on top of functions. > > > E.g., the comment for fill_subsection_map() alone spans 12 LOC in total. > > > I do wonder if we have to be that verbose. We are barely that verbose on > > > MM code (and usually I don't see much benefit unless it's a function > > > with many users from many different places). > > > > I would tend to agree here. Not that I am against kernel doc > > documentation but these are internal functions and the comment doesn't > > really give any better insight IMHO. I would be much more inclined if > > this was the general pattern in the respective file but it just stands > > out. > > I saw there are internal functions which have code comments, e.g > shrink_slab() in mm/vmscan.c, not only this one place, there are several > places. I personally prefer to see code comment for function if > possible, this can save time, e.g people can skip the bitmap operation > when read code if not necessary. And here I mainly want to tell there > are different returned value to note different behaviour when call them. ... yet nobody really cares about different return code. It is an error that is propagated up the call chain and that's all. I also like when there is a kernel doc comment that helps to understand the intented usage, context the function can be called from, potential side effects, locking requirements and other details people need to know when calling functions. But have a look at /** * clear_subsection_map - Clear subsection map of one memory region * * @pfn - start pfn of the memory range * @nr_pages - number of pfns to add in the region * * This is only intended for hotplug, and clear the related subsection * map inside one section. * * Return: * * -EINVAL - Section already deactived. * * 0 - Subsection map is emptied. * * 1 - Subsection map is not empty. */ the only useful information in here is that this is a hotplug stuff but I would be completely lost about the intention without already knowing what is this whole subsection about. -- Michal Hocko SUSE Labs