Linux-RISC-V Archive on lore.kernel.org
 help / color / Atom feed
WARNING: multiple messages refer to this Message-ID
From: logang@deltatee.com (Logan Gunthorpe)
To: linux-riscv@lists.infradead.org
Subject: [PATCH 2/2] mm/sparse: add common helper to mark all memblocks present
Date: Wed, 7 Nov 2018 13:36:34 -0700
Message-ID: <b1cc442e-7314-4a8e-3eec-9adc200d7582@deltatee.com> (raw)
In-Reply-To: <alpine.DEB.2.21.1811072125280.1666@nanos.tec.linutronix.de>



On 2018-11-07 1:26 p.m., Thomas Gleixner wrote:
> Logan,
> 
> On Wed, 7 Nov 2018, Logan Gunthorpe wrote:
>> On 2018-11-07 1:12 p.m., Andrew Morton wrote:
>>>> +void __init memblocks_present(void)
>>>> +{
>>>> +	struct memblock_region *reg;
>>>> +
>>>> +	for_each_memblock(memory, reg) {
>>>> +		memory_present(memblock_get_region_node(reg),
>>>> +			       memblock_region_memory_base_pfn(reg),
>>>> +			       memblock_region_memory_end_pfn(reg));
>>>> +	}
>>>> +}
>>>> +
>>>
>>> I don't like the name much.  To me, memblocks_present means "are
>>> memblocks present" whereas this actually means "memblocks are present".
>>> But whatever.  A little covering comment which describes what this
>>> does and why it does it would be nice.
>>
>> The same argument can be made about the existing memory_present()
>> function and I think it's worth keeping the naming consistent. I'll add
>> a comment and resend shortly.
> 
> Actually if both names suck, then there also is the option to rename both
> instead of adding a comment to explain the suckage.

Ok, well, I wasn't expecting to take on a big rename like that as it
would create a patch touching a bunch of arches and mm files... But if
we can come to some agreement on a better name and someone is willing to
take that patch without significant delay then I'd be happy to create
the patch and add it to the start of my series.

Some ideas for new names:

mark_memory_present() / mark_memblocks_present()
set_memory_present() / set_memblocks_present()
memory_register() / memblocks_register()
register_memory() / register_memblocks()

Logan

From: Logan Gunthorpe <logang@deltatee.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-arch@vger.kernel.org, Albert Ou <aou@eecs.berkeley.edu>,
	Arnd Bergmann <arnd@arndb.de>,
	linux-sh@vger.kernel.org, Palmer Dabbelt <palmer@sifive.com>,
	linux-kernel@vger.kernel.org, Stephen Bates <sbates@raithlin.com>,
	linux-mm@kvack.org, Michal Hocko <mhocko@suse.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	Andrew Morton <akpm@linux-foundation.org>,
	Christoph Hellwig <hch@lst.de>,
	linux-riscv@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	Oscar Salvador <osalvador@suse.de>
Subject: Re: [PATCH 2/2] mm/sparse: add common helper to mark all memblocks present
Date: Wed, 7 Nov 2018 13:36:34 -0700
Message-ID: <b1cc442e-7314-4a8e-3eec-9adc200d7582@deltatee.com> (raw)
Message-ID: <20181107203634.KWIyH6DSaXw6TnCDBKZMK5rmOXmPMtP4sGZczdikmQM@z> (raw)
In-Reply-To: <alpine.DEB.2.21.1811072125280.1666@nanos.tec.linutronix.de>



On 2018-11-07 1:26 p.m., Thomas Gleixner wrote:
> Logan,
> 
> On Wed, 7 Nov 2018, Logan Gunthorpe wrote:
>> On 2018-11-07 1:12 p.m., Andrew Morton wrote:
>>>> +void __init memblocks_present(void)
>>>> +{
>>>> +	struct memblock_region *reg;
>>>> +
>>>> +	for_each_memblock(memory, reg) {
>>>> +		memory_present(memblock_get_region_node(reg),
>>>> +			       memblock_region_memory_base_pfn(reg),
>>>> +			       memblock_region_memory_end_pfn(reg));
>>>> +	}
>>>> +}
>>>> +
>>>
>>> I don't like the name much.  To me, memblocks_present means "are
>>> memblocks present" whereas this actually means "memblocks are present".
>>> But whatever.  A little covering comment which describes what this
>>> does and why it does it would be nice.
>>
>> The same argument can be made about the existing memory_present()
>> function and I think it's worth keeping the naming consistent. I'll add
>> a comment and resend shortly.
> 
> Actually if both names suck, then there also is the option to rename both
> instead of adding a comment to explain the suckage.

Ok, well, I wasn't expecting to take on a big rename like that as it
would create a patch touching a bunch of arches and mm files... But if
we can come to some agreement on a better name and someone is willing to
take that patch without significant delay then I'd be happy to create
the patch and add it to the start of my series.

Some ideas for new names:

mark_memory_present() / mark_memblocks_present()
set_memory_present() / set_memblocks_present()
memory_register() / memblocks_register()
register_memory() / register_memblocks()

Logan

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

  parent reply index

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-07 17:38 [PATCH 0/2] Introduce common code for risc-v sparsemem support logang
2018-11-07 17:38 ` Logan Gunthorpe
2018-11-07 17:38 ` [PATCH 1/2] mm: Introduce common STRUCT_PAGE_MAX_SHIFT define logang
2018-11-07 17:38   ` Logan Gunthorpe
2018-11-07 20:11   ` akpm
2018-11-07 20:11     ` Andrew Morton
2018-11-07 17:38 ` [PATCH 2/2] mm/sparse: add common helper to mark all memblocks present logang
2018-11-07 17:38   ` Logan Gunthorpe
2018-11-07 20:12   ` akpm
2018-11-07 20:12     ` Andrew Morton
2018-11-07 20:19     ` logang
2018-11-07 20:19       ` Logan Gunthorpe
2018-11-07 20:26       ` tglx
2018-11-07 20:26         ` Thomas Gleixner
2018-11-07 20:36         ` logang [this message]
2018-11-07 20:36           ` Logan Gunthorpe
2018-11-07 20:38           ` akpm
2018-11-07 20:38             ` Andrew Morton
2018-11-07 20:56             ` tglx
2018-11-07 20:56               ` Thomas Gleixner
2018-12-06 17:40     ` Logan Gunthorpe
2018-12-07 19:56       ` Andrew Morton

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b1cc442e-7314-4a8e-3eec-9adc200d7582@deltatee.com \
    --to=logang@deltatee.com \
    --cc=linux-riscv@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-RISC-V Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-riscv/0 linux-riscv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-riscv linux-riscv/ https://lore.kernel.org/linux-riscv \
		linux-riscv@lists.infradead.org infradead-linux-riscv@archiver.kernel.org
	public-inbox-index linux-riscv


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-riscv


AGPL code for this site: git clone https://public-inbox.org/ public-inbox