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: <0b871cf1-e54f-f072-1eaf-511a03c2907f@redhat.com>
Date:   Thu, 11 Jul 2019 11:25:47 -0400
From:   Nitesh Narayan Lal <nitesh@...hat.com>
To:     Dave Hansen <dave.hansen@...el.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 4:45 PM, Dave Hansen wrote:
> 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'?
Will it be acceptable to add fields in struct zone, when they will only
be used by page hinting?
>   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
> 	}
> }
I usually run checkpatch to detect such indentation issues. For the
patches, I shared it didn't show me any issues.
>
> 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.
>
-- 
Thanks
Nitesh