linux-kernel - Re: [PATCH v1 2/2] vmalloc: Optimize vfree

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20260106094727.5bea5ed6@pumpkin>
Date: Tue, 6 Jan 2026 09:47:27 +0000
From: David Laight <david.laight.linux@...il.com>
To: Matthew Wilcox <willy@...radead.org>
Cc: Ryan Roberts <ryan.roberts@....com>, Andrew Morton
 <akpm@...ux-foundation.org>, David Hildenbrand <david@...nel.org>, Lorenzo
 Stoakes <lorenzo.stoakes@...cle.com>, "Liam R. Howlett"
 <Liam.Howlett@...cle.com>, Vlastimil Babka <vbabka@...e.cz>, Mike Rapoport
 <rppt@...nel.org>, Suren Baghdasaryan <surenb@...gle.com>, Michal Hocko
 <mhocko@...e.com>, Brendan Jackman <jackmanb@...gle.com>, Johannes Weiner
 <hannes@...xchg.org>, Zi Yan <ziy@...dia.com>, Uladzislau Rezki
 <urezki@...il.com>, "Vishal Moola (Oracle)" <vishal.moola@...il.com>,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/2] vmalloc: Optimize vfree

On Tue, 6 Jan 2026 04:36:23 +0000
Matthew Wilcox <willy@...radead.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