diff mbox series

[RFC,v2,4/7] mm: Optimise SPARSEMEM implementation of for_each_valid_pfn()

Message ID 20250404155959.3442111-4-dwmw2@infradead.org (mailing list archive)
State New
Headers show
Series [RFC,v2,1/7] mm: Introduce for_each_valid_pfn() and use it from reserve_bootmem_region() | expand

Commit Message

David Woodhouse April 4, 2025, 3:59 p.m. UTC 
From: David Woodhouse <dwmw@amazon.co.uk>

There's no point in checking the section and subsection bitmap for *every*
PFN in the same section; they're either all valid or they aren't.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 include/linux/mmzone.h | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

Comments

Mike Rapoport April 7, 2025, 7:07 a.m. UTC | #1 
On Fri, Apr 04, 2025 at 04:59:56PM +0100, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> There's no point in checking the section and subsection bitmap for *every*
> PFN in the same section; they're either all valid or they aren't.

Don't you want to merge this with the previous commit?
 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  include/linux/mmzone.h | 39 +++++++++++++++++++++++++++++----------
>  1 file changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 67cdf675a4b9..0da1b0ba5d9f 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -2154,21 +2154,20 @@ static inline int pfn_valid(unsigned long pfn)
>  	return ret;
>  }
>  
> -static inline bool first_valid_pfn(unsigned long *p_pfn)
> +/* Returns -1 (an invalid PFN) if no valid PFN remaining */
> +static inline unsigned long first_valid_pfn(unsigned long pfn, unsigned long end_pfn)
>  {
> -	unsigned long pfn = *p_pfn;
>  	unsigned long nr = pfn_to_section_nr(pfn);
>  
>  	rcu_read_lock_sched();
>  
> -	while (nr <= __highest_present_section_nr) {
> +	while (nr <= __highest_present_section_nr && pfn < end_pfn) {
>  		struct mem_section *ms = __pfn_to_section(pfn);
>  
>  		if (valid_section(ms) &&
>  		    (early_section(ms) || pfn_section_first_valid(ms, &pfn))) {
> -			*p_pfn = pfn;
>  			rcu_read_unlock_sched();
> -			return true;
> +			return pfn;
>  		}
>  
>  		/* Nothing left in this section? Skip to next section */
> @@ -2177,14 +2176,34 @@ static inline bool first_valid_pfn(unsigned long *p_pfn)
>  	}
>  
>  	rcu_read_unlock_sched();
> +	return (unsigned long)-1;
> +}
>  
> -	return false;
> +static inline unsigned long next_valid_pfn(unsigned long pfn, unsigned long end_pfn)
> +{
> +	pfn++;
> +
> +	if (pfn >= end_pfn)
> +		return (unsigned long)-1;
> +
> +	/*
> +	 * Either every PFN within the section (or subsection for VMEMMAP) is
> +	 * valid, or none of them are. So there's no point repeating the check
> +	 * for every PFN; only call first_valid_pfn() the first time, and when
> +	 * crossing a (sub)section boundary (i.e. !(pfn & ~PFN_VALID_MASK)).
> +	 */
> +	if (pfn & (IS_ENABLED(CONFIG_SPARSEMEM_VMEMMAP) ?
> +		   PAGE_SUBSECTION_MASK : PAGE_SECTION_MASK))
> +		return pfn;
> +
> +	return first_valid_pfn(pfn, end_pfn);
>  }
>  
> -#define for_each_valid_pfn(_pfn, _start_pfn, _end_pfn)	       \
> -	for ((_pfn) = (_start_pfn);			       \
> -	     first_valid_pfn(&(_pfn)) && (_pfn) < (_end_pfn);  \
> -	     (_pfn)++)
> +
> +#define for_each_valid_pfn(_pfn, _start_pfn, _end_pfn)			\
> +	for ((_pfn) = first_valid_pfn((_start_pfn), (_end_pfn));	\
> +	     (_pfn) != (unsigned long)-1;				\
> +	     (_pfn) = next_valid_pfn((_pfn), (_end_pfn)))
>  
>  #endif
>  
> -- 
> 2.49.0
>
David Woodhouse April 7, 2025, 8:01 a.m. UTC | #2 
On Mon, 2025-04-07 at 10:07 +0300, Mike Rapoport wrote:
> On Fri, Apr 04, 2025 at 04:59:56PM +0100, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > There's no point in checking the section and subsection bitmap for *every*
> > PFN in the same section; they're either all valid or they aren't.
> 
> Don't you want to merge this with the previous commit?

Maybe. Or at least the previous commit should be using the 'return -1'
model to minimise the differences.

To start with though, I wanted it to be reviewable as an incremental
patch to what we'd already been discussing. (And I figured there was at
least a non-zero chance of you not liking it just because it's too
complex, so the whole thing is easy to drop this way).

Even after review, keeping it as a separate patch means it's easily
revertible if we find we want to go back to the simpler version.
diff mbox series

Patch

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 67cdf675a4b9..0da1b0ba5d9f 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -2154,21 +2154,20 @@  static inline int pfn_valid(unsigned long pfn)
 	return ret;
 }
 
-static inline bool first_valid_pfn(unsigned long *p_pfn)
+/* Returns -1 (an invalid PFN) if no valid PFN remaining */
+static inline unsigned long first_valid_pfn(unsigned long pfn, unsigned long end_pfn)
 {
-	unsigned long pfn = *p_pfn;
 	unsigned long nr = pfn_to_section_nr(pfn);
 
 	rcu_read_lock_sched();
 
-	while (nr <= __highest_present_section_nr) {
+	while (nr <= __highest_present_section_nr && pfn < end_pfn) {
 		struct mem_section *ms = __pfn_to_section(pfn);
 
 		if (valid_section(ms) &&
 		    (early_section(ms) || pfn_section_first_valid(ms, &pfn))) {
-			*p_pfn = pfn;
 			rcu_read_unlock_sched();
-			return true;
+			return pfn;
 		}
 
 		/* Nothing left in this section? Skip to next section */
@@ -2177,14 +2176,34 @@  static inline bool first_valid_pfn(unsigned long *p_pfn)
 	}
 
 	rcu_read_unlock_sched();
+	return (unsigned long)-1;
+}
 
-	return false;
+static inline unsigned long next_valid_pfn(unsigned long pfn, unsigned long end_pfn)
+{
+	pfn++;
+
+	if (pfn >= end_pfn)
+		return (unsigned long)-1;
+
+	/*
+	 * Either every PFN within the section (or subsection for VMEMMAP) is
+	 * valid, or none of them are. So there's no point repeating the check
+	 * for every PFN; only call first_valid_pfn() the first time, and when
+	 * crossing a (sub)section boundary (i.e. !(pfn & ~PFN_VALID_MASK)).
+	 */
+	if (pfn & (IS_ENABLED(CONFIG_SPARSEMEM_VMEMMAP) ?
+		   PAGE_SUBSECTION_MASK : PAGE_SECTION_MASK))
+		return pfn;
+
+	return first_valid_pfn(pfn, end_pfn);
 }
 
-#define for_each_valid_pfn(_pfn, _start_pfn, _end_pfn)	       \
-	for ((_pfn) = (_start_pfn);			       \
-	     first_valid_pfn(&(_pfn)) && (_pfn) < (_end_pfn);  \
-	     (_pfn)++)
+
+#define for_each_valid_pfn(_pfn, _start_pfn, _end_pfn)			\
+	for ((_pfn) = first_valid_pfn((_start_pfn), (_end_pfn));	\
+	     (_pfn) != (unsigned long)-1;				\
+	     (_pfn) = next_valid_pfn((_pfn), (_end_pfn)))
 
 #endif