Re: [RFC PATCH 3/3] mm: Implement for_each_valid_pfn() for CONFIG_SPARSEMEM

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 3546 bytes --]

On Thu, 2025-04-03 at 09:24 +0300, Mike Rapoport wrote:
> On Wed, Apr 02, 2025 at 09:18:41PM +0100, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > Introduce a pfn_first_valid() helper which takes a pointer to the
> > PFN and
> > updates it to point to the first valid PFN starting from that
> > point, and
> > returns true if a valid PFN was found.
> > 
> > This largely mirrors pfn_valid(), calling into a
> > pfn_section_first_valid()
> > helper which is trivial for the !CONFIG_SPARSEMEM_VMEMMAP case, and
> > in
> > the VMEMMAP case will skip to the next subsection as needed.
> > 
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> 
> Reviewed-by: Mike Rapoport (Microsoft) <rppt@kernel.org>

Thanks.

> with a small nit below
> 
> > +static inline bool first_valid_pfn(unsigned long *p_pfn)
> > +{
> > +	unsigned long pfn = *p_pfn;
> > +	unsigned long nr = pfn_to_section_nr(pfn);
> > +	struct mem_section *ms;
> > +	bool ret = false;
> > +
> > +	ms = __pfn_to_section(pfn);
> > +
> > +	rcu_read_lock_sched();
> > +
> > +	while (!ret && nr <= __highest_present_section_nr) {
> 
> This could be just for(;;), we anyway break when ret becomes true or we get
> past last present section.

True for the 'ret' part but not *nicely* for the last present section.
If the original pfn is higher than the last present section it could
trigger that check before entering the loop.

Yes, in that case 'ms' will be NULL, valid_section(NULL) is false and
you're right that it'll make it through to the check in the loop
without crashing. So it would currently be harmless, but I didn't like
it. It's relying on the loop not to do the wrong thing with an input
which is arguably invalid.

I'll see if I can make it neater. I may drop the 'ret' variable
completely and just turn the match clause into unlock-and-return-true.
I *like* having a single unlock site. But I think I like simpler loop
code more than that.

FWIW I think the check for (PHYS_PFN(PFN_PHYS(pfn)) != pfn) at the
start of pfn_valid() a few lines above is similarly redundant. Because
if the high bits are set in the PFN then pfn_to_section_nr(pfn) is
surely going to be higher than NR_MEM_SECTIONS and it'll get thrown out
at the very next check, won't it?

I care because I didn't bother to duplicate that 'redundant' check in
my first_valid_pfn(), so if there's a reason for it that I'm missing, I
should take a closer look.

I'm also missing the reason why the FLATMEM code in memory_model.h does
'unsigned long pfn_offset = ARCH_PFN_OFFSET' and then uses its local
pfn_offset variable, instead of just using ARCH_PFN_OFFSET directly as
I do in the FLATMEM for_each_valid_pfn() macro.

> > +		if (valid_section(ms) &&
> > +		    (early_section(ms) || pfn_section_first_valid(ms, &pfn))) {
> > +			ret = true;
> > +			break;
> > +		}
> > +
> > +		nr++;
> > +		if (nr > __highest_present_section_nr)
> > +			break;
> > +
> > +		pfn = section_nr_to_pfn(nr);
> > +		ms = __pfn_to_section(pfn);
> > +	}
> > +
> > +	rcu_read_unlock_sched();
> > +
> > +	*p_pfn = pfn;
> > +
> > +	return ret;
> > +}
> > +
> > +#define for_each_valid_pfn(_pfn, _start_pfn, _end_pfn)	       \
> > +	for ((_pfn) = (_start_pfn);			       \
> > +	     first_valid_pfn(&(_pfn)) && (_pfn) < (_end_pfn);  \
> > +	     (_pfn)++)
> > +
> >  #endif
> >  
> >  static inline int pfn_in_present_section(unsigned long pfn)
> > -- 
> > 2.49.0
> > 
> 


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]