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