Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934121AbbGUXfl (ORCPT ); Tue, 21 Jul 2015 19:35:41 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:44146 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934044AbbGUXex (ORCPT ); Tue, 21 Jul 2015 19:34:53 -0400 Date: Tue, 21 Jul 2015 16:34:52 -0700 From: Andrew Morton To: Vladimir Davydov Cc: Andres Lagar-Cavilla , 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 v9 6/8] proc: add kpageidle file Message-Id: <20150721163452.c1e4075a2b193bcd325fad56@linux-foundation.org> In-Reply-To: References: X-Mailer: Sylpheed 3.4.1 (GTK+ 2.24.23; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5645 Lines: 192 On Sun, 19 Jul 2015 15:31:15 +0300 Vladimir Davydov wrote: > Knowing the portion of memory that is not used by a certain application > or memory cgroup (idle memory) can be useful for partitioning the system > efficiently, e.g. by setting memory cgroup limits appropriately. > Currently, the only means to estimate the amount of idle memory provided > by the kernel is /proc/PID/{clear_refs,smaps}: the user can clear the > access bit for all pages mapped to a particular process by writing 1 to > clear_refs, wait for some time, and then count smaps:Referenced. > However, this method has two serious shortcomings: > > - it does not count unmapped file pages > - it affects the reclaimer logic > > To overcome these drawbacks, this patch introduces two new page flags, > Idle and Young, and a new proc file, /proc/kpageidle. A page's Idle flag > can only be set from userspace by setting bit in /proc/kpageidle at the > offset corresponding to the page, and it is cleared whenever the page is > accessed either through page tables (it is cleared in page_referenced() > in this case) or using the read(2) system call (mark_page_accessed()). > Thus by setting the Idle flag for pages of a particular workload, which > can be found e.g. by reading /proc/PID/pagemap, waiting for some time to > let the workload access its working set, and then reading the kpageidle > file, one can estimate the amount of pages that are not used by the > workload. > > The Young page flag is used to avoid interference with the memory > reclaimer. A page's Young flag is set whenever the Access bit of a page > table entry pointing to the page is cleared by writing to kpageidle. If > page_referenced() is called on a Young page, it will add 1 to its return > value, therefore concealing the fact that the Access bit was cleared. > > Note, since there is no room for extra page flags on 32 bit, this > feature uses extended page flags when compiled on 32 bit. > > ... > > > ... > > +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, > + }; I think this can be static const, since `arg' is unused? That would save some cycles and stack. > + bool need_lock; > + > + if (!page_mapped(page) || > + !page_rmapping(page)) > + return; > + > + need_lock = !PageAnon(page) || PageKsm(page); > + if (need_lock && !trylock_page(page)) Oh. So the feature is a bit unreliable. I'm not immediately seeing anything which would prevent us from using plain old lock_page() here. What's going on? > + return; > + > + rmap_walk(page, &rwc); > + > + if (need_lock) > + unlock_page(page); > +} > + > +static ssize_t kpageidle_read(struct file *file, char __user *buf, > + size_t count, loff_t *ppos) > +{ > + u64 __user *out = (u64 __user *)buf; > + struct page *page; > + unsigned long pfn, end_pfn; > + ssize_t ret = 0; > + u64 idle_bitmap = 0; > + int bit; > + > + if (*ppos & KPMMASK || count & KPMMASK) > + return -EINVAL; Interface requires 8-byte aligned offset and size. > + pfn = *ppos * BITS_PER_BYTE; > + if (pfn >= max_pfn) > + return 0; > + > + end_pfn = pfn + count * BITS_PER_BYTE; > + if (end_pfn > max_pfn) > + end_pfn = ALIGN(max_pfn, KPMBITS); So we lose up to 63 pages. Presumably max_pfn is well enough aligned for this to not matter, dunno. > + for (; pfn < end_pfn; pfn++) { > + bit = pfn % KPMBITS; > + page = kpageidle_get_page(pfn); > + if (page) { > + if (page_is_idle(page)) { > + /* > + * The page might have been referenced via a > + * pte, in which case it is not idle. Clear > + * refs and recheck. > + */ > + kpageidle_clear_pte_refs(page); > + if (page_is_idle(page)) > + idle_bitmap |= 1ULL << bit; I don't understand what's going on here. More details, please? > + } > + put_page(page); > + } > + if (bit == KPMBITS - 1) { > + if (put_user(idle_bitmap, out)) { > + ret = -EFAULT; > + break; > + } > + idle_bitmap = 0; > + out++; > + } > + } > + > + *ppos += (char __user *)out - buf; > + if (!ret) > + ret = (char __user *)out - buf; > + return ret; > +} > + > +static ssize_t kpageidle_write(struct file *file, const char __user *buf, > + size_t count, loff_t *ppos) > +{ > + const u64 __user *in = (const u64 __user *)buf; > + struct page *page; > + unsigned long pfn, end_pfn; > + ssize_t ret = 0; > + u64 idle_bitmap = 0; > + int bit; > + > + if (*ppos & KPMMASK || count & KPMMASK) > + return -EINVAL; > + > + pfn = *ppos * BITS_PER_BYTE; > + if (pfn >= max_pfn) > + return -ENXIO; > + > + end_pfn = pfn + count * BITS_PER_BYTE; > + if (end_pfn > max_pfn) > + end_pfn = ALIGN(max_pfn, KPMBITS); > + > + for (; pfn < end_pfn; pfn++) { > + bit = pfn % KPMBITS; > + if (bit == 0) { > + if (get_user(idle_bitmap, in)) { > + ret = -EFAULT; > + break; > + } > + in++; > + } > + if (idle_bitmap >> bit & 1) { Hate it when I have to go look up a C precedence table. This is if ((idle_bitmap >> bit) & 1) { > + page = kpageidle_get_page(pfn); > + if (page) { > + kpageidle_clear_pte_refs(page); > + set_page_idle(page); > + put_page(page); > + } > + } > + } > + > + *ppos += (const char __user *)in - buf; > + if (!ret) > + ret = (const char __user *)in - buf; > + return ret; > +} > + > > ... > -- 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/