From: David Hildenbrand <david@redhat.com>
To: "Verma, Vishal L" <vishal.l.verma@intel.com>,
"Williams, Dan J" <dan.j.williams@intel.com>,
"Jiang, Dave" <dave.jiang@intel.com>,
"osalvador@suse.de" <osalvador@suse.de>,
"akpm@linux-foundation.org" <akpm@linux-foundation.org>
Cc: "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
"Huang, Ying" <ying.huang@intel.com>,
"linux-mm@kvack.org" <linux-mm@kvack.org>,
"aneesh.kumar@linux.ibm.com" <aneesh.kumar@linux.ibm.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>,
"Hocko, Michal" <mhocko@suse.com>,
"nvdimm@lists.linux.dev" <nvdimm@lists.linux.dev>,
"jmoyer@redhat.com" <jmoyer@redhat.com>,
"Jonathan.Cameron@Huawei.com" <Jonathan.Cameron@Huawei.com>
Subject: Re: [PATCH v5 1/2] mm/memory_hotplug: split memmap_on_memory requests across memblocks
Date: Mon, 9 Oct 2023 17:15:39 +0200 [thread overview]
Message-ID: <0bc29e93-7efe-4bce-ee3f-1fdf76104843@redhat.com> (raw)
In-Reply-To: <45cfd268da63eeddb741e9c9c3026b0e15eade4b.camel@intel.com>
On 07.10.23 00:01, Verma, Vishal L wrote:
> On Fri, 2023-10-06 at 14:52 +0200, David Hildenbrand wrote:
>> On 05.10.23 20:31, Vishal Verma wrote:
>>>
> <..>
>>> @@ -2167,47 +2221,28 @@ static int __ref try_remove_memory(u64 start, u64 size)
>>> if (rc)
>>> return rc;
>>>
>>> + mem_hotplug_begin();
>>> +
>>> /*
>>> - * We only support removing memory added with MHP_MEMMAP_ON_MEMORY in
>>> - * the same granularity it was added - a single memory block.
>>> + * For memmap_on_memory, the altmaps could have been added on
>>> + * a per-memblock basis. Loop through the entire range if so,
>>> + * and remove each memblock and its altmap.
>>> */
>>> if (mhp_memmap_on_memory()) {
>>> - rc = walk_memory_blocks(start, size, &mem, test_has_altmap_cb);
>>> - if (rc) {
>>> - if (size != memory_block_size_bytes()) {
>>> - pr_warn("Refuse to remove %#llx - %#llx,"
>>> - "wrong granularity\n",
>>> - start, start + size);
>>> - return -EINVAL;
>>> - }
>>> - altmap = mem->altmap;
>>> - /*
>>> - * Mark altmap NULL so that we can add a debug
>>> - * check on memblock free.
>>> - */
>>> - mem->altmap = NULL;
>>> - }
>>> + unsigned long memblock_size = memory_block_size_bytes();
>>> + u64 cur_start;
>>> +
>>> + for (cur_start = start; cur_start < start + size;
>>> + cur_start += memblock_size)
>>> + remove_memory_block_and_altmap(nid, cur_start,
>>> + memblock_size);
>>> + } else {
>>> + remove_memory_block_and_altmap(nid, start, size);
>>
>> Better call remove_memory_block_devices() and arch_remove_memory(start,
>> size, altmap) here explicitly instead of using
>> remove_memory_block_and_altmap() that really can only handle a single
>> memory block with any inputs.
>>
> I'm not sure I follow. Even in the non memmap_on_memory case, we'd have
> to walk_memory_blocks() to get to the memory_block->altmap, right?
See my other reply to, at least with mhp_memmap_on_memory()==false, we
don't have to worry about the altmap.
>
> Or is there a more direct way? If we have to walk_memory_blocks, what's
> the advantage of calling those directly instead of calling the helper
> created above?
I think we have two cases to handle
1) All have an altmap. Remove them block-by-block. Probably we should
call a function remove_memory_blocks(altmap=true) [or alternatively
remove_memory_blocks_and_altmaps()] and just handle iterating internally.
2) All don't have an altmap. We can remove them in one go. Probably we
should call that remove_memory_blocks(altmap=false) [or alternatively
remove_memory_blocks_no_altmaps()].
I guess it's best to do a walk upfront to make sure either all have an
altmap or none has one. Then we can branch off to the right function
knowing whether we have to process altmaps or not.
The existing
if (mhp_memmap_on_memory()) {
...
}
Can be extended for that case.
Please let me know if I failed to express what I mean, then I can
briefly prototype it on top of your changes.
--
Cheers,
David / dhildenb
next prev parent reply other threads:[~2023-10-09 15:15 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-05 18:31 [PATCH v5 0/2] mm: use memmap_on_memory semantics for dax/kmem Vishal Verma
2023-10-05 18:31 ` [PATCH v5 1/2] mm/memory_hotplug: split memmap_on_memory requests across memblocks Vishal Verma
2023-10-05 21:20 ` Dan Williams
2023-10-06 16:46 ` Verma, Vishal L
2023-10-06 12:52 ` David Hildenbrand
2023-10-06 22:01 ` Verma, Vishal L
2023-10-09 15:15 ` David Hildenbrand [this message]
2023-10-07 8:55 ` Huang, Ying
2023-10-09 15:04 ` David Hildenbrand
2023-10-12 5:53 ` Verma, Vishal L
2023-10-12 8:40 ` David Hildenbrand
2023-10-16 18:19 ` Verma, Vishal L
2023-10-05 18:31 ` [PATCH v5 2/2] dax/kmem: allow kmem to add memory with memmap_on_memory Vishal Verma
2023-10-05 21:16 ` Dan Williams
2023-10-17 0:31 ` Verma, Vishal L
2023-10-17 5:18 ` Huang, Ying
2023-10-17 5:44 ` Verma, Vishal L
2023-10-17 5:54 ` Huang, Ying
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=0bc29e93-7efe-4bce-ee3f-1fdf76104843@redhat.com \
--to=david@redhat.com \
--cc=Jonathan.Cameron@Huawei.com \
--cc=akpm@linux-foundation.org \
--cc=aneesh.kumar@linux.ibm.com \
--cc=dan.j.williams@intel.com \
--cc=dave.hansen@linux.intel.com \
--cc=dave.jiang@intel.com \
--cc=jmoyer@redhat.com \
--cc=linux-cxl@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@suse.com \
--cc=nvdimm@lists.linux.dev \
--cc=osalvador@suse.de \
--cc=vishal.l.verma@intel.com \
--cc=ying.huang@intel.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 read-only IMAP folder(s) and NNTP newsgroup(s).