On 7/11/19 11: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?
>> 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.
My bad I think I jumped here, I saw what you are referring to here.
I will fix these kind of things.
>> 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
