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

[Dave Hansen]

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
}