From: Miles Chen <miles.chen@mediatek.com> To: Mike Rapoport <rppt@kernel.org> Cc: Andrew Morton <akpm@linux-foundation.org>, Mark Rutland <mark.rutland@arm.com>, <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>, <linux-mediatek@lists.infradead.org>, <wsd_upstream@mediatek.com> Subject: Re: [PATCH] mm/sparse: clarify pgdat_to_phys Date: Mon, 5 Jul 2021 17:24:41 +0800 [thread overview] Message-ID: <1625477081.856.5.camel@mtkswgap22> (raw) In-Reply-To: <YN8QaoJuDP9Nr744@kernel.org> On Fri, 2021-07-02 at 16:11 +0300, Mike Rapoport wrote: > On Wed, Jun 30, 2021 at 05:23:09PM +0800, Miles Chen wrote: > > clarify pgdat_to_phys() by wrapping pgdat_to_phys > > with CONFIG_NUMA. (the same config as contig_page_data) > > > > No functional change intended. > > > > Comment from Mark [1]: > > " > > ... and I reckon it'd be clearer and more robust to define > > pgdat_to_phys() in the same ifdefs as contig_page_data so > > that these, stay in-sync. e.g. have: > > > > | #ifdef CONFIG_NUMA > > | #define pgdat_to_phys(x) virt_to_phys(x) > > | #else /* CONFIG_NUMA */ > > | > > | extern struct pglist_data contig_page_data; > > | ... > > | #define pgdat_to_phys(x) __pa_symbol(&contig_page_data) > > | > > | #endif /* CONIFIG_NUMA */ > > " > > > > [1] https://lore.kernel.org/linux-arm-kernel/20210615131902.GB47121@C02TD0UTHF1T.local/ > > > > Cc: Mark Rutland <mark.rutland@arm.com> > > Signed-off-by: Miles Chen <miles.chen@mediatek.com> > > --- > > mm/sparse.c | 17 +++++++++-------- > > 1 file changed, 9 insertions(+), 8 deletions(-) > > > > diff --git a/mm/sparse.c b/mm/sparse.c > > index 7272f7a1449d..62c21ec28e33 100644 > > --- a/mm/sparse.c > > +++ b/mm/sparse.c > > @@ -344,14 +344,15 @@ size_t mem_section_usage_size(void) > > return sizeof(struct mem_section_usage) + usemap_size(); > > } > > > > -static inline phys_addr_t pgdat_to_phys(struct pglist_data *pgdat) > > -{ > > -#ifndef CONFIG_NUMA > > - return __pa_symbol(pgdat); > > -#else > > - return __pa(pgdat); > > -#endif > > -} > > +#ifdef CONFIG_NUMA > > +#define pgdat_to_phys(pgdat) __pa(pgdat) > > +#else /* CONFIG_NUMA */ > > +/* > > + * When !CONFIG_NUMA, we only expect pgdat == &contig_page_data, > > + * and use __pa_symbol(). > > + */ > > +#define pgdat_to_phys(pgdat) __pa_symbol(pgdat) > > +#endif /* CONFIG_NUMA */ > > I'm not sure a macro is better than a static inline. > > Maybe we'd want to warn if pgdat passed to pgtat_to_phys() is not > &contig_page_data, e.g something like > > static inline phys_addr_t pgdat_to_phys(struct pglist_data *pgdat) > { > if (!IS_ENABLED(CONFIG_NUMA)) { > if (pgdat == &contig_page_data) > return __pa_symbol(&contig_page_data); > else > pr_warn("Unexpected pglist_data pointer!\n"); > } > > return __pa(pgdat); > } Sorry for my late response. Thanks for the guide. It looks better this way.I will submit patch v2. > > > #ifdef CONFIG_MEMORY_HOTREMOVE > > static struct mem_section_usage * __init > > -- > > 2.18.0 > > >
WARNING: multiple messages have this Message-ID (diff)
From: Miles Chen <miles.chen@mediatek.com> To: Mike Rapoport <rppt@kernel.org> Cc: Andrew Morton <akpm@linux-foundation.org>, Mark Rutland <mark.rutland@arm.com>, <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>, <linux-mediatek@lists.infradead.org>, <wsd_upstream@mediatek.com> Subject: Re: [PATCH] mm/sparse: clarify pgdat_to_phys Date: Mon, 5 Jul 2021 17:24:41 +0800 [thread overview] Message-ID: <1625477081.856.5.camel@mtkswgap22> (raw) In-Reply-To: <YN8QaoJuDP9Nr744@kernel.org> On Fri, 2021-07-02 at 16:11 +0300, Mike Rapoport wrote: > On Wed, Jun 30, 2021 at 05:23:09PM +0800, Miles Chen wrote: > > clarify pgdat_to_phys() by wrapping pgdat_to_phys > > with CONFIG_NUMA. (the same config as contig_page_data) > > > > No functional change intended. > > > > Comment from Mark [1]: > > " > > ... and I reckon it'd be clearer and more robust to define > > pgdat_to_phys() in the same ifdefs as contig_page_data so > > that these, stay in-sync. e.g. have: > > > > | #ifdef CONFIG_NUMA > > | #define pgdat_to_phys(x) virt_to_phys(x) > > | #else /* CONFIG_NUMA */ > > | > > | extern struct pglist_data contig_page_data; > > | ... > > | #define pgdat_to_phys(x) __pa_symbol(&contig_page_data) > > | > > | #endif /* CONIFIG_NUMA */ > > " > > > > [1] https://lore.kernel.org/linux-arm-kernel/20210615131902.GB47121@C02TD0UTHF1T.local/ > > > > Cc: Mark Rutland <mark.rutland@arm.com> > > Signed-off-by: Miles Chen <miles.chen@mediatek.com> > > --- > > mm/sparse.c | 17 +++++++++-------- > > 1 file changed, 9 insertions(+), 8 deletions(-) > > > > diff --git a/mm/sparse.c b/mm/sparse.c > > index 7272f7a1449d..62c21ec28e33 100644 > > --- a/mm/sparse.c > > +++ b/mm/sparse.c > > @@ -344,14 +344,15 @@ size_t mem_section_usage_size(void) > > return sizeof(struct mem_section_usage) + usemap_size(); > > } > > > > -static inline phys_addr_t pgdat_to_phys(struct pglist_data *pgdat) > > -{ > > -#ifndef CONFIG_NUMA > > - return __pa_symbol(pgdat); > > -#else > > - return __pa(pgdat); > > -#endif > > -} > > +#ifdef CONFIG_NUMA > > +#define pgdat_to_phys(pgdat) __pa(pgdat) > > +#else /* CONFIG_NUMA */ > > +/* > > + * When !CONFIG_NUMA, we only expect pgdat == &contig_page_data, > > + * and use __pa_symbol(). > > + */ > > +#define pgdat_to_phys(pgdat) __pa_symbol(pgdat) > > +#endif /* CONFIG_NUMA */ > > I'm not sure a macro is better than a static inline. > > Maybe we'd want to warn if pgdat passed to pgtat_to_phys() is not > &contig_page_data, e.g something like > > static inline phys_addr_t pgdat_to_phys(struct pglist_data *pgdat) > { > if (!IS_ENABLED(CONFIG_NUMA)) { > if (pgdat == &contig_page_data) > return __pa_symbol(&contig_page_data); > else > pr_warn("Unexpected pglist_data pointer!\n"); > } > > return __pa(pgdat); > } Sorry for my late response. Thanks for the guide. It looks better this way.I will submit patch v2. > > > #ifdef CONFIG_MEMORY_HOTREMOVE > > static struct mem_section_usage * __init > > -- > > 2.18.0 > > > _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek
next prev parent reply other threads:[~2021-07-05 9:24 UTC|newest] Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-30 9:23 [PATCH] mm/sparse: clarify pgdat_to_phys Miles Chen 2021-06-30 9:23 ` Miles Chen 2021-07-02 13:11 ` Mike Rapoport 2021-07-02 13:11 ` Mike Rapoport 2021-07-05 9:24 ` Miles Chen [this message] 2021-07-05 9:24 ` Miles Chen
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=1625477081.856.5.camel@mtkswgap22 \ --to=miles.chen@mediatek.com \ --cc=akpm@linux-foundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mediatek@lists.infradead.org \ --cc=linux-mm@kvack.org \ --cc=mark.rutland@arm.com \ --cc=rppt@kernel.org \ --cc=wsd_upstream@mediatek.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.