From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936358AbcHJSjE (ORCPT ); Wed, 10 Aug 2016 14:39:04 -0400 Received: from thejh.net ([37.221.195.125]:41816 "EHLO thejh.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934132AbcHJSjC (ORCPT ); Wed, 10 Aug 2016 14:39:02 -0400 Date: Wed, 10 Aug 2016 17:02:21 +0200 From: Jann Horn To: Robert Foss 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 Subject: Re: [PACTH v1] mm, proc: Implement /proc//totmaps Message-ID: <20160810150221.GA23703@pc.thejh.net> 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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="GvXjxJ+pjyke8COw" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --GvXjxJ+pjyke8COw Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Aug 10, 2016 at 10:16:45AM -0400, Robert Foss wrote: >=20 >=20 > 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 wr= ote: > >>>>+ down_read(&mm->mmap_sem); > >>>>+ hold_task_mempolicy(priv); > >>>>+ > >>>>+ for (vma =3D mm->mmap; vma !=3D priv->tail_vma; vma =3D vma->vm_nex= t) { > >>>>+ struct mem_size_stats mss; > >>>>+ struct mm_walk smaps_walk =3D { > >>>>+ .pmd_entry =3D smaps_pte_range, > >>>>+ .mm =3D vma->vm_mm, > >>>>+ .private =3D &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 one= s? > >>>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? > > >=20 > 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 =3D 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 =3D mm->mmap; vma !=3D priv->tail_vma; vma =3D vma->vm_nex= t) { struct mem_size_stats mss; struct mm_walk smaps_walk =3D { .pmd_entry =3D smaps_pte_range, .mm =3D vma->vm_mm, .private =3D &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, [...]); [...] } --GvXjxJ+pjyke8COw Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXq0H9AAoJED4KNFJOeCOoZEwP/0Wc2E27zBpgwETwSywISNYa /3dhB7MTNRDDQYQ4uFod8s85xQzvlOCWoL289FxGXLd9N62qaFRZ7Xv9BBRW2wbj 90z7TxWMYtZAxhfgWdY9ngfHE7C/MaQSv9+boAvbQ0QP5/zjhPXVGsVc5/87JxTM hwzkW0dHPTpPxYqiCD3RUdufouI7sti61R2zJXvSnTgiNy/wMcGmkx9DywjtkkOT VdC3sQtHQ/7ReZzyJeMsFeDShcy/VX62JP84qdyCS4cR9JGMDxB8/Y7k8bLagKDv Bc4V1OFfHMVr4UEj2duvaLMck3ffQfwyB8RNoaCJgi/XDMCqRm3zONt0dOE9XXSc mkJBHVvsl9u+2Vo8ngiTzpItD37pUy9OOctWNjCZi4AxcjWrhIne6UxT/w2flVmu VQk4eKlQhtBW0+k2g2CPBJGa6cNxrENv95x/CPPdAY+g6EFLumpRywCqkXLavw+6 iy9uzYQy1SyT4eQhIS0dwA7M6uOjNnpoPHzGw1AqrBq3eQ4I2PGvnklOlHGy5VtY OLPDOmUI76sjD+hz+5hsvdxGLjwTtcRx33urn55eR7p7Alki8O6+8rLzYrLeR8g8 ijhoXMwfq2lBxAplT4XRGyxi/DgFvnb7lYa8YPxEteAlN56Avc7IcVCceeoyp2cq NUXBPkf7sRNfx2mPCw6n =6Nzu -----END PGP SIGNATURE----- --GvXjxJ+pjyke8COw--