Re: [PATCH v1 2/2] vmalloc: Optimize vfree

[David Laight <david.laight.linux@gmail.com>]

On Tue, 6 Jan 2026 04:36:23 +0000
Matthew Wilcox <willy@infradead.org> wrote:

> On Mon, Jan 05, 2026 at 04:17:38PM +0000, Ryan Roberts wrote:
> > +	if (vm->nr_pages) {
> > +		start_pfn = page_to_pfn(vm->pages[0]);
> > +		nr = 1;
> > +		for (i = 1; i < vm->nr_pages; i++) {
> > +			unsigned long pfn = page_to_pfn(vm->pages[i]);
> > +
> > +			if (start_pfn + nr != pfn) {
> > +				__free_contig_range(start_pfn, nr);
> > +				start_pfn = pfn;
> > +				nr = 1;
> > +				cond_resched();
> > +			} else {
> > +				nr++;
> > +			}  
> 
> It kind of feels like __free_contig_range() and this routine do the same
> thing -- iterate over each page and make sure that it's compatible with
> being freed.  What if we did ...
> 
	nr = 0;
> +	for (i = 0; i < vm->nr_pages; i++) {
> +		struct page *page = vm->pages[i];
> +
> +		if (!put_page_testzero(page)) {

Do you need an if (nr) here?
If nothing else something might complain about start_page being unset.
Is this a common/expected path?
If not I think you can just 'continue' and it all still look ok.
There is also a cond_reshed() below, if a common path should
there be one here?

> +			__free_frozen_contig_pages(start_page, nr);
> +			nr = 0;
> +			continue;
> +		}
> +
> +		if (!nr) {
> +			start_page = page;
> +			nr = 1;
> +			continue;
> +		}
> +
> +		if (start_page + nr != page) {
> +			__free_frozen_contig_pages(start_page, nr);
> +			start_page = page;
> +			nr = 1;
> +			cond_resched();
> +		} else {
> +			nr++;
> +		}
> +	}
> +
> +	__free_frozen_contig_pages(start_page, nr);
> 
> That way we don't need to mess around with returning the number of pages
> not freed.
> 

I think this shorter form is equivalent.

	nr = 0;
	start_page = NULL;
	for (i = 0; i < vm->nr_pages; i++) {
		struct page *page = vm->pages[i];

		if (!put_page_testzero(page))
			continue;

		if (start_page + nr != page) {
			if (nr) {
				__free_frozen_contig_pages(start_page, nr);
				cond_resched();
			}
			start_page = page;
			nr = 1;
		} else {
			nr++;
		}
	}

	if (nr)
		__free_frozen_contig_pages(start_page, nr);

    David