Re: [RFC][Patch v11 1/2] mm: page_hinting: core infrastructure

From: Dave Hansen
Date: Thu Jul 11 2019 - 12:22:52 EST

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

>> 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
// cleanup