linux-kernel - Re: [RFC][Patch v11 1/2] mm: page

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: <3f9a7e7b-c026-3530-e985-804fc7f1ec31@intel.com>
Date:   Wed, 10 Jul 2019 13:45:19 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Nitesh Narayan Lal <nitesh@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        pbonzini@...hat.com, lcapitulino@...hat.com, pagupta@...hat.com,
        wei.w.wang@...el.com, yang.zhang.wz@...il.com, riel@...riel.com,
        david@...hat.com, mst@...hat.com, dodgen@...gle.com,
        konrad.wilk@...cle.com, dhildenb@...hat.com, aarcange@...hat.com,
        alexander.duyck@...il.com, john.starks@...rosoft.com,
        mhocko@...e.com
Subject: Re: [RFC][Patch v11 1/2] mm: page_hinting: core infrastructure

On 7/10/19 12:51 PM, Nitesh Narayan Lal wrote:
> +struct zone_free_area {
> +	unsigned long *bitmap;
> +	unsigned long base_pfn;
> +	unsigned long end_pfn;
> +	atomic_t free_pages;
> +	unsigned long nbits;
> +} free_area[MAX_NR_ZONES];

Why do we need an extra data structure.  What's wrong with putting
per-zone data in ... 'struct zone'?  The cover letter claims that it
doesn't touch core-mm infrastructure, but if it depends on mechanisms
like this, I think that's a very bad thing.

To be honest, I'm not sure this series is worth reviewing at this point.
 It's horribly lightly commented and full of kernel antipatterns lik

void func()
{
	if () {
		... indent entire logic
		... of function
	}
}

It has big "TODO"s.  It's virtually comment-free.  I'm shocked it's at
the 11th version and still looking like this.

> +
> +		for (zone_idx = 0; zone_idx < MAX_NR_ZONES; zone_idx++) {
> +			unsigned long pages = free_area[zone_idx].end_pfn -
> +					free_area[zone_idx].base_pfn;
> +			bitmap_size = (pages >> PAGE_HINTING_MIN_ORDER) + 1;
> +			if (!bitmap_size)
> +				continue;
> +			free_area[zone_idx].bitmap = bitmap_zalloc(bitmap_size,
> +								   GFP_KERNEL);

This doesn't support sparse zones.  We can have zones with massive
spanned page sizes, but very few present pages.  On those zones, this
will exhaust memory for no good reason.

Comparing this to Alex's patch set, it's of much lower quality and at a
much earlier stage of development.  The two sets are not really even
comparable right now.  This certainly doesn't sell me on (or even really
enumerate the deltas in) this approach vs. Alex's.