At 09/20/2012 01:55 PM, Minchan Kim Wrote: > On Thu, Sep 20, 2012 at 01:32:59PM +0800, Wen Congyang wrote: >> At 09/20/2012 01:16 PM, Minchan Kim Wrote: >>> On Thu, Sep 20, 2012 at 11:12:40AM +0800, Wen Congyang wrote: >>>> At 09/20/2012 10:30 AM, Minchan Kim Wrote: >>>>> On Thu, Sep 20, 2012 at 09:17:43AM +0800, Wen Congyang wrote: >>>>>> At 09/20/2012 04:17 AM, Minchan Kim Wrote: >>>>>>> Hi KOSAKI, >>>>>>> >>>>>>> On Wed, Sep 19, 2012 at 02:05:20PM -0400, KOSAKI Motohiro wrote: >>>>>>>> On Wed, Sep 19, 2012 at 3:29 AM, Minchan Kim <minchan@xxxxxxxxxx> wrote: >>>>>>>>> During memory-hotplug stress test, I found NR_ISOLATED_[ANON|FILE] >>>>>>>>> are increasing so that kernel are hang out. >>>>>>>>> >>>>>>>>> The cause is that when we do memory-hotadd after memory-remove, >>>>>>>>> __zone_pcp_update clear out zone's ZONE_STAT_ITEMS in setup_pageset >>>>>>>>> without draining vm_stat_diff of all CPU. >>>>>>>>> >>>>>>>>> This patch fixes it. >>>>>>>> >>>>>>>> zone_pcp_update() is called from online pages path. but IMHO, >>>>>>>> the statistics should be drained offline path. isn't it? >>>>>>> >>>>>>> It isn't necessary because statistics is right until we reset it to zero >>>>>>> in online path. >>>>>>> Do you have something on your mind that we have to drain it in offline path? >>>>>> >>>>>> When a node is offlined and onlined again. We create node_data[i] in the >>>>> >>>>> I would like to clarify your word. >>>>> Create or recreate? >>>>> Why I have a question is as I look over the source code, hotadd_new_pgdat >>>>> seem to be called if we do hotadd *new* memory. It's not the case for >>>>> offline and online again you mentioned. If you're right, I should find >>>>> arch_free_nodedata to free pgdat when node is disappear but I can't find it. >>>>> Do I miss something? >>>> >>>> Hmm, when a memory is removed, we don't do cleanup now. We(Fujitsu) posted >>>> a patchset to do this: >>>> https://lkml.org/lkml/2012/9/5/201 >>>> >>>> We don't free pgdat in this patchset now. We have two choice: >>>> 1. free pgdat >>>> 2. don't free it, and reuse it when it is onlined again >>>> >>>> I'm not sure which choice is better. >>> >>> I have no idea because I don't know how you guys uses. >>> If there is use case that sometime you ues many node burstly but >>> ues a few node in most time, 1) would be good POV memory efficiency >>> although it makes code rather complicated. >>> >>> Anyway, it's another story with this patch because it's not merged yet. >>> >>>> >>>>> >>>>> >>>>>> function hotadd_new_pgdat(), and we will lost the statistics stored in >>>>>> zone->pageset. So we should drain it in offline path. >>>>> >>>>> Even we drain in offline patch, it still has a problem. >>>>> >>>>> 1. offline >>>>> 2. drain -> OKAY >>>>> 3. schedule >>>>> 4. Process A increase zone stat >>>>> 5. Process B increase zone stat >>>>> 6. online >>>>> 7. reset it -> we ends up lost zone stat counter which is modified between 2-6 >>>>> >>>> >>>> I understand why you drain it in online path now. But it still should drain it >>>> in offline path because if all pages in this zone are offlined, we will call >>>> zone_pcp_reset() to reset zone's pcp. We should also drop it in the function >>>> zone_pcp_reset(). >>> >>> Good point. >>> How about this? >>> >>> >From e92bf3e96720c89cb18ec32c5db095a27ad4133c Mon Sep 17 00:00:00 2001 >>> From: Minchan Kim <minchan@xxxxxxxxxx> >>> Date: Thu, 20 Sep 2012 14:11:49 +0900 >>> Subject: [PATCH v2] memory-hotplug: fix zone stat mismatch >>> >>> During memory-hotplug, I found NR_ISOLATED_[ANON|FILE] >>> are increasing so that kernel are hang out. >>> >>> The cause is that when we do memory-hotadd after memory-remove, >>> __zone_pcp_update clear out zone's ZONE_STAT_ITEMS in setup_pageset >>> although vm_stat_diff of all CPU still have value. >>> >>> In addtion, when we offline all pages of the zone, we reset them >>> in zone_pcp_reset without drain so that we lost zone stat item. >>> >>> This patch fixes it. >>> >>> * from v1 >>> * drain offline patch - KOSAKI, Wen >>> >>> Cc: Kamezawa Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx> >>> Cc: Yasuaki Ishimatsu <isimatu.yasuaki@xxxxxxxxxxxxxx> >>> Cc: Wen Congyang <wency@xxxxxxxxxxxxxx> >>> Cc: KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx> >>> Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx> >>> --- >>> include/linux/vmstat.h | 4 ++++ >>> mm/page_alloc.c | 7 +++++++ >>> mm/vmstat.c | 12 ++++++++++++ >>> 3 files changed, 23 insertions(+) >>> >>> diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h >>> index ad2cfd5..5d31876 100644 >>> --- a/include/linux/vmstat.h >>> +++ b/include/linux/vmstat.h >>> @@ -198,6 +198,8 @@ extern void __dec_zone_state(struct zone *, enum zone_stat_item); >>> void refresh_cpu_vm_stats(int); >>> void refresh_zone_stat_thresholds(void); >>> >>> +void drain_zonestat(struct zone *zone, struct per_cpu_pageset *); >>> + >>> int calculate_pressure_threshold(struct zone *zone); >>> int calculate_normal_threshold(struct zone *zone); >>> void set_pgdat_percpu_threshold(pg_data_t *pgdat, >>> @@ -251,6 +253,8 @@ static inline void __dec_zone_page_state(struct page *page, >>> static inline void refresh_cpu_vm_stats(int cpu) { } >>> static inline void refresh_zone_stat_thresholds(void) { } >>> >>> +static inline void drain_zonestat(struct zone *zone, >>> + struct per_cpu_pageset *pset) { } >>> #endif /* CONFIG_SMP */ >>> >>> extern const char * const vmstat_text[]; >>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >>> index ab58346..980f2e7 100644 >>> --- a/mm/page_alloc.c >>> +++ b/mm/page_alloc.c >>> @@ -5904,6 +5904,7 @@ static int __meminit __zone_pcp_update(void *data) >>> local_irq_save(flags); >>> if (pcp->count > 0) >>> free_pcppages_bulk(zone, pcp->count, pcp); >>> + drain_zonestat(zone, pset); >>> setup_pageset(pset, batch); >>> local_irq_restore(flags); >>> } >>> @@ -5920,10 +5921,16 @@ void __meminit zone_pcp_update(struct zone *zone) >>> void zone_pcp_reset(struct zone *zone) >>> { >>> unsigned long flags; >>> + int cpu; >>> + struct per_cpu_pageset *pset; >>> >>> /* avoid races with drain_pages() */ >>> local_irq_save(flags); >>> if (zone->pageset != &boot_pageset) { >>> + for_each_online_cpu(cpu) { >> >> A cpu can be offlined before the pages in the zone are offlined. So >> I think you should drain it on all possible cpu, not online cpu. >> > > I'm not CPU hotplug expert but IMHO, it should be handled > by vmstat_cpuup_callback when the CPU is down. Otherwise, > it should never drain until zone is empty by memory-hotplug. > > And if it is, it should be another patch. It has been done in page_alloc_cpu_notify(). Your patch is OK to me now. Thanks Wen Congyang -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>