Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755134AbbGPJy4 (ORCPT ); Thu, 16 Jul 2015 05:54:56 -0400 Received: from mx2.parallels.com ([199.115.105.18]:44113 "EHLO mx2.parallels.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753692AbbGPJyQ (ORCPT ); Thu, 16 Jul 2015 05:54:16 -0400 Date: Thu, 16 Jul 2015 12:53:56 +0300 From: Vladimir Davydov To: Andres Lagar-Cavilla CC: Andrew Morton , Minchan Kim , Raghavendra K T , Johannes Weiner , Michal Hocko , Greg Thelen , Michel Lespinasse , David Rientjes , Pavel Emelyanov , Cyrill Gorcunov , Jonathan Corbet , , , , , Subject: Re: [PATCH -mm v8 6/7] proc: add kpageidle file Message-ID: <20150716095356.GB2001@esperanza> References: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: X-Originating-IP: [10.30.4.177] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2984 Lines: 94 On Wed, Jul 15, 2015 at 12:42:28PM -0700, Andres Lagar-Cavilla wrote: > On Wed, Jul 15, 2015 at 6:54 AM, Vladimir Davydov > wrote: [...] > > +static void kpageidle_clear_pte_refs(struct page *page) > > +{ > > + struct rmap_walk_control rwc = { > > + .rmap_one = kpageidle_clear_pte_refs_one, > > + .anon_lock = page_lock_anon_vma_read, > > + }; > > + bool need_lock; > > + > > + if (!page_mapped(page) || > > Question: what about mlocked pages? Is there any point in calculating > their idleness? Those can be filtered out with the aid of /proc/kpageflags (this is what the script attached to patch #0 of the series actually does). We have to read the latter anyway in order to get information about THP. That said, I prefer not to introduce any artificial checks for locked memory. Who knows, may be one day somebody will use this API to track access pattern to an mlocked area. > > > + !page_rmapping(page)) > > Not sure, does this skip SwapCache pages? Is there any point in > calculating their idleness? A SwapCache page may be mapped, and if it is we should not skip it. If it is unmapped, we have nothing to do. Regarding idleness of SwapCache pages, I think we shouldn't differentiate them from other user pages here, because a shmem/anon page can migrate to-and-fro the swap cache occasionally during a memory-active workload, and we don't want to lose its idle status then. > > > + return; > > + > > + need_lock = !PageAnon(page) || PageKsm(page); > > + if (need_lock && !trylock_page(page)) > > + return; > > + > > + rmap_walk(page, &rwc); > > + > > + if (need_lock) > > + unlock_page(page); > > +} [...] > > @@ -1754,6 +1754,11 @@ static void __split_huge_page_refcount(struct page *page, > > /* clear PageTail before overwriting first_page */ > > smp_wmb(); > > > > + if (page_is_young(page)) > > + set_page_young(page_tail); > > + if (page_is_idle(page)) > > + set_page_idle(page_tail); > > + > > Why not in the block above? > > page_tail->flags |= (page->flags & > ... > #ifdef CONFIG_WHATEVER_IT_WAS > 1 << PG_idle > 1 << PG_young > #endif Too many ifdef's :-/ Note, the flags can be in page_ext, which mean we would have to add something like this #if defined(CONFIG_WHATEVER_IT_WAS) && defined(CONFIG_64BIT) 1 << PG_idle 1 << PG_young #endif <...> #ifndef CONFIG_64BIT if (page_is_young(page)) set_page_young(page_tail); if (page_is_idle(page)) set_page_idle(page_tail); #endif which IMO looks less readable than what we have now. Thanks, Vladimir -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/