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 <minc...@kernel.org> > >>>>>> 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 <minc...@kernel.org> > > 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.hir...@jp.fujitsu.com> > > Cc: Yasuaki Ishimatsu <isimatu.yasu...@jp.fujitsu.com> > > Cc: Wen Congyang <we...@cn.fujitsu.com> > > Cc: KOSAKI Motohiro <kosaki.motoh...@jp.fujitsu.com> > > Signed-off-by: Minchan Kim <minc...@kernel.org> > > --- > > 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. -- Kind regards, Minchan Kim -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
