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'? 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
> }
> }
>
> It has big "TODO"s. It's virtually comment-free. I'm shocked it's at
> the 11th version and still looking like this.
One of the reasons for being on v11 was that the entire design has
changed a few times.
But that's no excuse, I understand what you are saying and I will work
on it and improve 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.
Thanks, I will look into this.
>
> 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
