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: <c41671f0-2080-b925-39e2-79e33a84088b@intel.com>
Date:   Thu, 11 Jul 2019 09:22:48 -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/11/19 8:25 AM, Nitesh Narayan Lal wrote:
> 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?

Wait a sec...  MAX_NR_ZONES the number of zone types not the maximum
number of *zones* in the system.

Did you test this on a NUMA system?

In any case, yes, you can put these in 'struct zone'.  It will waste
less space that way, on average, than what you have here (one you scale
it to MAX_NR_ZONE*MAX_NUM_NODES.

>>   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.

Just because checkpatch doesn't complain does not mean it is good form.
 We write the above as:

void func()
{
	if (!something)
		goto out;

	... logic of function here
out:
	// cleanup
}