From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936502AbcHJS4c (ORCPT ); Wed, 10 Aug 2016 14:56:32 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:43437 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753668AbcHJS4X (ORCPT ); Wed, 10 Aug 2016 14:56:23 -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> 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 10:16:45 -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: <20160809223004.GA7099@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-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?