From: Daniel Axtens <dja@axtens•net>
To: Jordan Niethe <jniethe5@gmail•com>, linuxppc-dev@lists•ozlabs.org
Cc: Jordan Niethe <jniethe5@gmail•com>,
npiggin@gmail•com, aneesh.kumar@linux•ibm.com
Subject: Re: [PATCH 1/4] powerpc/64s: Add DEBUG_PAGEALLOC for radix
Date: Fri, 18 Jun 2021 17:28:37 +1000 [thread overview]
Message-ID: <87im2bskbe.fsf@dja-thinkpad.axtens.net> (raw)
In-Reply-To: <20210517061658.194708-2-jniethe5@gmail.com>
Jordan Niethe <jniethe5@gmail•com> writes:
> There is support for DEBUG_PAGEALLOC on hash but not on radix.
> Add support on radix.
Somewhat off-topic but I wonder at what point we can drop the weird !PPC
condition in mm/Kconfig.debug:
config DEBUG_PAGEALLOC
bool "Debug page memory allocations"
depends on DEBUG_KERNEL
depends on !HIBERNATION || ARCH_SUPPORTS_DEBUG_PAGEALLOC && !PPC && !SPARC
I can't figure out from git history why it exists or if it still serves
any function. Given that HIBERNATION isn't much use on Book3S systems it
probably never matters, it just bugs me a bit.
Again, nothing that has to block this series, just maybe something to
follow up at some vague and undefined point in the future!
> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
> index a666d561b44d..b89482aed82a 100644
> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> @@ -812,6 +822,15 @@ static inline bool check_pte_access(unsigned long access, unsigned long ptev)
> * Generic functions with hash/radix callbacks
> */
>
> +#ifdef CONFIG_DEBUG_PAGEALLOC
> +static inline void __kernel_map_pages(struct page *page, int numpages, int enable)
> +{
> + if (radix_enabled())
> + radix__kernel_map_pages(page, numpages, enable);
> + hash__kernel_map_pages(page, numpages, enable);
Does this require an else? On radix we will call both radix__ and
hash__kernel_map_pages.
I notice we call both hash__ and radix__ in map_kernel_page under radix,
so maybe this makes sense?
I don't fully understand the mechanism by which memory removal works: it
looks like on radix you mark the page as 'absent', which I think is
enough? Then you fall through to the hash code here:
for (i = 0; i < numpages; i++, page++) {
vaddr = (unsigned long)page_address(page);
lmi = __pa(vaddr) >> PAGE_SHIFT;
if (lmi >= linear_map_hash_count)
continue;
I think linear_map_hash_count will be zero unless it gets inited to a
non-zero value in htab_initialize(). I am fairly sure htab_initialize()
doesn't get called for a radix MMU. In that case you'll just `continue;`
out of every iteration of the loop, which would explain why nothing
weird would happen on radix.
Have I missed something here?
The rest of the patch looks good to me.
Kind regards,
Daniel
next prev parent reply other threads:[~2021-06-18 7:29 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-17 6:16 [PATCH 0/4] powerpc/64s: Enable KFENCE Jordan Niethe
2021-05-17 6:16 ` [PATCH 1/4] powerpc/64s: Add DEBUG_PAGEALLOC for radix Jordan Niethe
2021-06-18 7:28 ` Daniel Axtens [this message]
2021-05-17 6:16 ` [PATCH 2/4] powerpc/64s: Remove unneeded #ifdef CONFIG_DEBUG_PAGEALLOC in hash_utils Jordan Niethe
2021-06-18 7:49 ` Daniel Axtens
2021-05-17 6:16 ` [PATCH 3/4] powerpc/64s: Allow double call of kernel_[un]map_linear_page() Jordan Niethe
2021-05-17 6:16 ` [PATCH 4/4] powerpc: Enable KFENCE on BOOK3S/64 Jordan Niethe
2021-06-18 8:00 ` Daniel Axtens
2021-06-18 8:02 ` Daniel Axtens
2021-06-22 8:57 ` Michael Ellerman
-- strict thread matches above, loose matches on Subject: below --
2022-09-19 1:44 [PATCH 1/4] powerpc/64s: Add DEBUG_PAGEALLOC for radix Nicholas Miehlbradt
2022-09-19 6:17 ` Christophe Leroy
2022-09-19 7:00 ` Michael Ellerman
2022-09-19 7:05 ` Christophe Leroy
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=87im2bskbe.fsf@dja-thinkpad.axtens.net \
--to=dja@axtens$(echo .)net \
--cc=aneesh.kumar@linux$(echo .)ibm.com \
--cc=jniethe5@gmail$(echo .)com \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=npiggin@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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