On 12.11.20 16:18, Vlastimil Babka wrote:
On 11/11/20 6:58 PM, David Hildenbrand wrote:
On 11.11.20 10:28, Vlastimil Babka wrote:
- /*
- * per-cpu pages are drained after start_isolate_page_range, but
- * if there are still pages that are not free, make sure that we
- * drain again, because when we isolated range we might have
- * raced with another thread that was adding pages to pcp list.
- *
- * Forward progress should be still guaranteed because
- * pages on the pcp list can only belong to MOVABLE_ZONE
- * because has_unmovable_pages explicitly checks for
- * PageBuddy on freed pages on other zones.
- */
ret = test_pages_isolated(start_pfn, end_pfn, MEMORY_OFFLINE);
- if (ret)
- drain_all_pages(zone);
+
Why two empty lines before the "} while (ret);" ? (unless I'm confused
while looking at this diff)
No there's just a single emply line after "ret = test_pages_isolated..."
Before there was none, which looked ok with the extra identation of the
now-removed "drain_all_pages(zone);"
+void __zone_set_pageset_high_and_batch(struct zone *zone, unsigned long high,
+ unsigned long batch)
+{
+ struct per_cpu_pageset *p;
+ int cpu;
+
+ for_each_possible_cpu(cpu) {
+ p = per_cpu_ptr(zone->pageset, cpu);
+ pageset_update(&p->pcp, high, batch);
+ }
+}
+
/*
* Calculate and set new high and batch values for all per-cpu pagesets of a
* zone, based on the zone's size and the percpu_pagelist_fraction sysctl.
@@ -6315,8 +6338,6 @@ static void pageset_init(struct per_cpu_pageset *p)
static void zone_set_pageset_high_and_batch(struct zone *zone)
{
unsigned long new_high, new_batch;
- struct per_cpu_pageset *p;
- int cpu;
if (percpu_pagelist_fraction) {
new_high = zone_managed_pages(zone) / percpu_pagelist_fraction;
@@ -6336,10 +6357,7 @@ static void zone_set_pageset_high_and_batch(struct zone
*zone)
zone->pageset_high = new_high;
zone->pageset_batch = new_batch;
- for_each_possible_cpu(cpu) {
- p = per_cpu_ptr(zone->pageset, cpu);
- pageset_update(&p->pcp, new_high, new_batch);
- }
+ __zone_set_pageset_high_and_batch(zone, new_high, new_batch);
}
These two hunks look like an unrelated cleanup, or am I missing something?
It's extracting part of functionality to __zone_set_pageset_high_and_batch()
that's now called also from zone_pcp_enable() and zone_pcp_disable() - to only
adjust the per-cpu zone->pageset values without the usual calculation.
Thanks for looking into this!
Thanks for review. Here's updated version that adds more detailed comment about
force_all_cpus parameter to __drain_all_pages() header, hopefully that clarifies
your concerns.
LGTM!
Reviewed-by: David Hildenbrand <da...@redhat.com>
--
Thanks,
David / dhildenb
