On Mon, 17 Jun 2013 22:15:28 -0400 Mathieu Desnoyers <[email protected]> wrote:
> * Andrew Morton ([email protected]) wrote: > > On Mon, 17 Jun 2013 17:39:36 -0400 Rapha__l Beamonte > > <[email protected]> wrote: > > > > > 2013/6/17 Andrew Morton <[email protected]> > > > > > > > That change wasn't terribly efficient - if there are any unpopulated > > > > pages in the range (which is quite likely), fadvise() will now always > > > > call invalidate_mapping_pages() a second time. > > > > > > > > Perhaps this is fixable. Say, make lru_add_drain_all() return a > > > > success code, or even teach lru_add_drain_all() to return a code > > > > indicating that one of the spilled pages was (or might have been) on a > > > > particular mapping. > > > > > > > > > > Following our tests results, that was the call to lru_add_drain_all() that > > > causes the problem. The second call to invalidate_mapping_pages() isn't > > > really important. We tried to compile a kernel with the commit introducing > > > this change but with the "lru_add_drain_all()" line removed, and the > > > problem disappeared, even if we called two times > > > invalidate_mapping_pages() > > > (as the rest of the commit was still here). > > > > Ah, OK, schedule_on_each_cpu() could certainly do that - it has to wait > > for every CPU to context switch and schedule the worker function. > > > > There's a lot we could do here. Such as not doing the schedule_work() > > at all for a cpu which has an empty lru_add_pvec. > > First approach proposed, submitted as RFC. Compile-tested only. > > ... > > Second approach, submitted as RFC with some questions left unanswered > in the code. Compile-tested only. > > --- > include/linux/swap.h | 1 > mm/fadvise.c | 2 - > mm/swap.c | 62 > +++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 64 insertions(+), 1 deletion(-) > > Index: linux/include/linux/swap.h > =================================================================== > --- linux.orig/include/linux/swap.h > +++ linux/include/linux/swap.h > @@ -242,6 +242,7 @@ extern void mark_page_accessed(struct pa > extern void lru_add_drain(void); > extern void lru_add_drain_cpu(int cpu); > extern int lru_add_drain_all(void); > +extern int lru_add_drain_mapping(struct address_space *mapping); > extern void rotate_reclaimable_page(struct page *page); > extern void deactivate_page(struct page *page); > extern void swap_setup(void); > Index: linux/mm/swap.c > =================================================================== > --- linux.orig/mm/swap.c > +++ linux/mm/swap.c > @@ -689,6 +689,68 @@ int lru_add_drain_all(void) > } > > /* > + * Returns 0 for success > + */ > +int lru_add_drain_mapping(struct address_space *mapping) > +{ > + int cpu; > + struct work_struct __percpu *works; > + > + works = alloc_percpu(struct work_struct); > + if (!works) > + return -ENOMEM; > + > + get_online_cpus(); > + > + for_each_online_cpu(cpu) { > + struct pagevec *pvecs = per_cpu(lru_add_pvecs, cpu); > + struct work_struct *work = per_cpu_ptr(works, cpu); > + struct pagevec *pvec; > + int lru; > + > + INIT_WORK(work, lru_add_drain_per_cpu); > + for_each_lru(lru) { > + int i; > + > + pvec = &pvecs[lru - LRU_BASE]; > + /* > + * Race failure mode is to flush unnecessarily. > + * We use PAGEVEC_SIZE rather than pvec->nr to > + * stay on the safe-side wrt pvec resize. > + */ > + for (i = 0; i < PAGEVEC_SIZE; i++) { > + struct page *page; > + > + /* > + * Racy access to page. TODO: is it OK > + * to access it from the remote CPU's > + * lru without any kind of ownership or > + * synchronization ? > + */ Almost. The only problem I can think of is that the other CPU flushes its pagevec then a page gets freed then a memory hot-unplug occurs and the memory hotplug handler frees and then unmaps the memory which was used to hold the page's `struct page' (I don't think the current mem-hotplug code even does this) and now this cpu's page->mapping access goes oops. IOW, not worth worrying for a prototype "hey lets test this and see if it fixes things" patch. > + page = ACCESS_ONCE(pvec->pages[i]); > + if (!pvec->pages[i]) > + continue; > + if (page->mapping == mapping) { > + schedule_work_on(cpu, work); > + goto next_cpu; > + } > + } > + } > + next_cpu: ; /* TODO: coding ugliness */ > + } > + > + for_each_online_cpu(cpu) { > + struct work_struct *work = per_cpu_ptr(works, cpu); > + if (work_pending(work)) > + flush_work(work); > + } > + > + put_online_cpus(); > + free_percpu(works); > + return 0; > +} > > ... > _______________________________________________ lttng-dev mailing list [email protected] http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
