From: David Hildenbrand <david@redhat.com>
To: Oscar Salvador <osalvador@suse.de>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
Andrew Morton <akpm@linux-foundation.org>,
"Rafael J. Wysocki" <rafael@kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Michal Hocko <mhocko@suse.com>,
Rafael Parra <rparrazo@redhat.com>
Subject: Re: [PATCH v1 2/2] drivers/base/memory: determine and store zone for single-zone memory blocks
Date: Mon, 31 Jan 2022 12:42:49 +0100 [thread overview]
Message-ID: <10293d6c-c2d6-2277-0634-bdd237fc23de@redhat.com> (raw)
In-Reply-To: <20220131112909.GB18027@linux>
[...] having accidentally skipped two comments.
>
>> There are three scenarios to handle:
> ...
> ...
>
>> @@ -225,6 +226,9 @@ static int memory_block_offline(struct memory_block *mem)
>> unsigned long nr_vmemmap_pages = mem->nr_vmemmap_pages;
>> int ret;
>>
>> + if (!mem->zone)
>> + return -EBUSY;
>
> Should not we return -EINVAL? I mean, -EBUSY reads like this might be a
> temporary error which might get fixed later on, but that isn't the case.
We should, and I could have sworn I fixed that up last-minute.
>> +static struct zone *early_node_zone_for_memory_block(struct memory_block *mem,
>> + int nid)
>> +{
>> + const unsigned long start_pfn = section_nr_to_pfn(mem->start_section_nr);
>> + const unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block;
>> + struct zone *zone, *matching_zone = NULL;
>> + pg_data_t *pgdat = NODE_DATA(nid);
>
> I was about to complain because in init_memory_block() you call
> early_node_zone_for_memory_block() with nid == NUMA_NODE_NODE, but then
> I saw that NODE_DATA on !CONFIG_NUMA falls to contig_page_data.
> So, I guess we cannot really reach this on CONFIG_NUMA machines with nid
> being NUMA_NO_NODE, right? (do we want to add a check just in case?)
>
>> +#ifdef CONFIG_NUMA
>> +void memory_block_set_nid(struct memory_block *mem, int nid,
>> + enum meminit_context context)
>
> But we also set the zone? (Only for boot memory)
Yes, it's derived from the node internally, though, and not supplied
explicitly. Renaming it could be misleading IMHO.
--
Thanks,
David / dhildenb
prev parent reply other threads:[~2022-01-31 11:51 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-28 15:26 [PATCH v1 0/2] drivers/base/memory: determine and store zone for single-zone memory blocks David Hildenbrand
2022-01-28 15:26 ` [PATCH v1 1/2] drivers/base/node: rename link_mem_sections() to register_memory_block_under_node() David Hildenbrand
2022-01-31 10:38 ` Oscar Salvador
2022-01-28 15:26 ` [PATCH v1 2/2] drivers/base/memory: determine and store zone for single-zone memory blocks David Hildenbrand
2022-01-31 10:42 ` David Hildenbrand
2022-01-31 11:29 ` Oscar Salvador
2022-01-31 11:40 ` David Hildenbrand
2022-01-31 11:42 ` David Hildenbrand [this message]
Reply instructions:
You may reply publicly 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=10293d6c-c2d6-2277-0634-bdd237fc23de@redhat.com \
--to=david@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@suse.com \
--cc=osalvador@suse.de \
--cc=rafael@kernel.org \
--cc=rparrazo@redhat.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).