From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936507AbcHJTqY (ORCPT ); Wed, 10 Aug 2016 15:46:24 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:43430 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S938685AbcHJSq1 (ORCPT ); Wed, 10 Aug 2016 14:46:27 -0400 Subject: Re: [PACTH v1] mm, proc: Implement /proc//totmaps To: Jann Horn References: <1470758743-17685-1-git-send-email-robert.foss@collabora.com> <20160809192414.GA19573@pc.thejh.net> <8ac1b493-e051-ea0e-3a71-c4476054bdb2@collabora.com> <20160809223004.GA7099@pc.thejh.net> <20160810150221.GA23703@pc.thejh.net> Cc: Sonny Rao , akpm@linux-foundation.org, keescook@chromium.org, viro@zeniv.linux.org.uk, gorcunov@openvz.org, john.stultz@linaro.org, plaguedbypenguins@gmail.com, mguzik@redhat.com, adobriyan@gmail.com, jdanis@google.com, calvinowens@fb.com, mhocko@suse.com, koct9i@gmail.com, vbabka@suse.cz, n-horiguchi@ah.jp.nec.com, kirill.shutemov@linux.intel.com, ldufour@linux.vnet.ibm.com, hannes@cmpxchg.org, linux-kernel@vger.kernel.org, Ben Zhang , Bryan Freed , Filipe Brandenburger From: Robert Foss Message-ID: Date: Wed, 10 Aug 2016 12:24:01 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20160810150221.GA23703@pc.thejh.net> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2016-08-10 11:02 AM, Jann Horn wrote: > On Wed, Aug 10, 2016 at 10:16:45AM -0400, Robert Foss wrote: >> >> >> On 2016-08-09 06:30 PM, Jann Horn wrote: >>> On Tue, Aug 09, 2016 at 05:01:44PM -0400, Robert Foss wrote: >>>> On 2016-08-09 03:24 PM, Jann Horn wrote: >>>>> On Tue, Aug 09, 2016 at 12:05:43PM -0400, robert.foss@collabora.com wrote: >>>>>> + down_read(&mm->mmap_sem); >>>>>> + hold_task_mempolicy(priv); >>>>>> + >>>>>> + for (vma = mm->mmap; vma != priv->tail_vma; vma = vma->vm_next) { >>>>>> + struct mem_size_stats mss; >>>>>> + struct mm_walk smaps_walk = { >>>>>> + .pmd_entry = smaps_pte_range, >>>>>> + .mm = vma->vm_mm, >>>>>> + .private = &mss, >>>>>> + }; >>>>>> + >>>>>> + if (vma->vm_mm && !is_vm_hugetlb_page(vma)) { >>>>>> + memset(&mss, 0, sizeof(mss)); >>>>>> + walk_page_vma(vma, &smaps_walk); >>>>>> + add_smaps_sum(&mss, mss_sum); >>>>>> + } >>>>>> + } >>>>> >>>>> Errrr... what? You accumulate values from mem_size_stats items into a >>>>> struct mss_sum that is associated with the struct file? So when you >>>>> read the file the second time, you get the old values plus the new ones? >>>>> And when you read the file in parallel, you get inconsistent values? >>>>> >>>>> For most files in procfs, the behavior is that you can just call >>>>> pread(fd, buf, sizeof(buf), 0) on the same fd again and again, giving >>>>> you the current values every time, without mutating state. I strongly >>>>> recommend that you get rid of priv->mss and just accumulate the state >>>>> in a local variable (maybe one on the stack). >>>> >>>> So a simple "static struct mem_size_stats" in totmaps_proc_show() would be a >>>> better solution? >>> >>> Er, why "static"? Are you trying to create shared state between different >>> readers for some reason? >>> >> >> I think I'm a bit confused now, how are you suggesting that I replace >> priv->mss? > > Like this: > > static int totmaps_proc_show(struct seq_file *m, void *data) > { > struct proc_maps_private *priv = m->private; > struct mm_struct *mm; > struct vm_area_struct *vma; > struct mem_size_stats mss_sum; > > memset(&mss_sum, 0, sizeof(mss_sum)); > > [...] > > for (vma = mm->mmap; vma != priv->tail_vma; vma = vma->vm_next) { > struct mem_size_stats mss; > struct mm_walk smaps_walk = { > .pmd_entry = smaps_pte_range, > .mm = vma->vm_mm, > .private = &mss, > }; > > if (vma->vm_mm && !is_vm_hugetlb_page(vma)) { > memset(&mss, 0, sizeof(mss)); > walk_page_vma(vma, &smaps_walk); > add_smaps_sum(&mss, &mss_sum); > } > } > seq_printf(m, > "Rss: %8lu kB\n" > "Pss: %8lu kB\n" > "Shared_Clean: %8lu kB\n" > [...], > mss_sum.resident >> 10, > (unsigned long)(mss_sum.pss >> (10 + PSS_SHIFT)), > mss_sum.shared_clean >> 10, > [...]); > [...] > } > Thanks Jann for being really clear about this stuff. It is much appreciated!