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=-8.4 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 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 813BEC3F2D1 for ; Sun, 1 Mar 2020 05:40:02 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 3F69B21556 for ; Sun, 1 Mar 2020 05:40:02 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="LXAqKmma" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3F69B21556 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id B8C0C6B0005; Sun, 1 Mar 2020 00:40:01 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id B3CE26B0006; Sun, 1 Mar 2020 00:40:01 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A51BF6B0007; Sun, 1 Mar 2020 00:40:01 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0180.hostedemail.com [216.40.44.180]) by kanga.kvack.org (Postfix) with ESMTP id 8B44D6B0005 for ; Sun, 1 Mar 2020 00:40:01 -0500 (EST) Received: from smtpin16.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 2DC3940F0 for ; Sun, 1 Mar 2020 05:40:01 +0000 (UTC) X-FDA: 76545692202.16.can70_1d3952e5c0546 X-HE-Tag: can70_1d3952e5c0546 X-Filterd-Recvd-Size: 5690 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.61]) by imf30.hostedemail.com (Postfix) with ESMTP for ; Sun, 1 Mar 2020 05:40:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1583041200; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=3YIHlD0M+k+yxPJl/fAAm98gZtg1wXJwtbyuwCIFlGI=; b=LXAqKmmabnWn/rHjPi9dKiV6bGPSUzNPn3IfeAy5s7YIidFuVzAhmKqz05QSH3YiUtUQlX oOW8e08dsJ/fe13cUiKbUYq5q8HwE230oReJsicZyV0heUJZqTr+IuTtYA+vTjvwpb3BPS xJdOOxW5j4vpN4mR01362dWX9UpHXnw= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-154-f3ljMdhqNtu_2RzLzpQL9A-1; Sun, 01 Mar 2020 00:39:58 -0500 X-MC-Unique: f3ljMdhqNtu_2RzLzpQL9A-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 8F082800D4E; Sun, 1 Mar 2020 05:39:56 +0000 (UTC) Received: from localhost (ovpn-12-59.pek2.redhat.com [10.72.12.59]) by smtp.corp.redhat.com (Postfix) with ESMTPS id C15605D9C9; Sun, 1 Mar 2020 05:39:53 +0000 (UTC) Date: Sun, 1 Mar 2020 13:39:50 +0800 From: Baoquan He To: David Hildenbrand Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, Segher Boessenkool , Andrew Morton , Oscar Salvador , Michal Hocko , Dan Williams , Wei Yang Subject: Re: [PATCH v2 2/2] mm/memory_hotplug: cleanup __add_pages() Message-ID: <20200301053950.GO24216@MiWiFi-R3L-srv> References: <20200228095819.10750-1-david@redhat.com> <20200228095819.10750-3-david@redhat.com> <20200228103442.GL24216@MiWiFi-R3L-srv> <65186cee-358a-5c23-49e0-5507730941ad@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <65186cee-358a-5c23-49e0-5507730941ad@redhat.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 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 02/28/20 at 12:14pm, David Hildenbrand wrote: > On 28.02.20 11:34, Baoquan He wrote: > > On 02/28/20 at 10:58am, David Hildenbrand wrote: > >> Let's drop the basically unused section stuff and simplify. The logic > >> now matches the logic in __remove_pages(). > >> > >> Cc: Segher Boessenkool > >> Cc: Andrew Morton > >> Cc: Oscar Salvador > >> Cc: Michal Hocko > >> Cc: Baoquan He > >> Cc: Dan Williams > >> Cc: Wei Yang > >> Signed-off-by: David Hildenbrand > >> --- > >> mm/memory_hotplug.c | 18 +++++++----------- > >> 1 file changed, 7 insertions(+), 11 deletions(-) > >> > >> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > >> index 8fe7e32dad48..1a00b5a37ef6 100644 > >> --- a/mm/memory_hotplug.c > >> +++ b/mm/memory_hotplug.c > >> @@ -307,8 +307,9 @@ static int check_hotplug_memory_addressable(unsigned long pfn, > >> int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages, > >> struct mhp_restrictions *restrictions) > >> { > >> + const unsigned long end_pfn = pfn + nr_pages; > >> + unsigned long cur_nr_pages; > >> int err; > >> - unsigned long nr, start_sec, end_sec; > >> struct vmem_altmap *altmap = restrictions->altmap; > >> > >> err = check_hotplug_memory_addressable(pfn, nr_pages); > >> @@ -331,18 +332,13 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages, > >> if (err) > >> return err; > >> > >> - start_sec = pfn_to_section_nr(pfn); > >> - end_sec = pfn_to_section_nr(pfn + nr_pages - 1); > >> - for (nr = start_sec; nr <= end_sec; nr++) { > >> - unsigned long pfns; > >> - > >> - pfns = min(nr_pages, PAGES_PER_SECTION > >> - - (pfn & ~PAGE_SECTION_MASK)); > >> - err = sparse_add_section(nid, pfn, pfns, altmap); > >> + for (; pfn < end_pfn; pfn += cur_nr_pages) { > >> + /* Select all remaining pages up to the next section boundary */ > >> + cur_nr_pages = min(end_pfn - pfn, > >> + SECTION_ALIGN_UP(pfn + 1) - pfn); > >> + err = sparse_add_section(nid, pfn, cur_nr_pages, altmap); > > > > Honestly, I am not a big fan of this kind of code refactoring. The old > > code may span seveal more lines or define several several more veriables, > > but logic is clear, and no visible defect. It's hard to say how much we > > I'm sorry, but iterating over variables and not using a single one in > the body is definitely not clean, at least IMHO. Leftover from Hmm, sometime we do use iterator to loop over only, it's just not so good. People usually have their own preferred coding style, and try to optimize to remove the itch in heart, totally understand. I have no strong objection to this, as long as it gets support from reviewers, it's not a bad thing. > sub-section hotadd support. > > > can benefit from this kind of code simplifying, and reviewing it will take > > people more time. While for the code style consistency with > > __remove_page(), I would like to see it's merged. My personal opinion. > > Thanks! > > > -- > Thanks, > > David / dhildenb