From: Oleg Nesterov <oleg@redhat.com> To: robert.foss@collabora.com Cc: corbet@lwn.net, akpm@linux-foundation.org, vbabka@suse.cz, hughd@google.com, mhocko@suse.com, koct9i@gmail.com, n-horiguchi@ah.jp.nec.com, kirill.shutemov@linux.intel.com, john.stultz@linaro.org, minchan@kernel.org, ross.zwisler@linux.intel.com, jmarchan@redhat.com, hannes@cmpxchg.org, keescook@chromium.org, viro@zeniv.linux.org.uk, mguzik@redhat.com, jdanis@google.com, calvinowens@fb.com, adobriyan@gmail.com, ebiederm@xmission.com, sonnyrao@chromium.org, seth.forshee@canonical.com, tixxdz@gmail.com, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, Ben Zhang <benzh@chromium.org>, Bryan Freed <bfreed@chromium.org>, Filipe Brandenburger <filbranden@chromium.org>, Jann Horn <jann@thejh.net>, Michal Hocko <mhocko@kernel.org>, linux-api@vger.kernel.org, Jacek Anaszewski <j.anaszewski@samsung.com> Subject: Re: [PATCH v5 1/3] mm, proc: Implement /proc/<pid>/totmaps Date: Wed, 7 Sep 2016 14:58:07 +0200 [thread overview] Message-ID: <20160907125806.GA3849@redhat.com> (raw) In-Reply-To: <1473106449-12847-2-git-send-email-robert.foss@collabora.com> On 09/05, robert.foss@collabora.com wrote: > > @@ -2854,6 +2854,7 @@ static const struct pid_entry tgid_base_stuff[] = { > REG("clear_refs", S_IWUSR, proc_clear_refs_operations), > REG("smaps", S_IRUGO, proc_pid_smaps_operations), > REG("pagemap", S_IRUSR, proc_pagemap_operations), > + REG("totmaps", S_IRUGO, proc_totmaps_operations), I must have missed something, but I fail to understand why this patch is so complicated. Just use ONE("totmaps", S_IRUGO, proc_totmaps_operations) ? > +static int totmaps_proc_show(struct seq_file *m, void *data) > +{ > + struct proc_maps_private *priv = m->private; > + struct mm_struct *mm = priv->mm; > + struct vm_area_struct *vma; > + struct mem_size_stats mss_sum; > + > + memset(&mss_sum, 0, sizeof(mss_sum)); > + down_read(&mm->mmap_sem); > + hold_task_mempolicy(priv); ^^^^^^^^^^^^^^^^^^^^^^^^^ why? > + for (vma = mm->mmap; vma != priv->tail_vma; vma = vma->vm_next) { Hmm. the usage of ->tail_vma looks just wrong. I guess the code should work because it is NULL but still. > + 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); > + } > + } Why? I mean, why not walk_page_range() ? You do not need this for-each-vma loop at all? At least if you change this patch to use the ONE() helper, and everything else looks unneeded in this case. Oleg.
WARNING: multiple messages have this Message-ID (diff)
From: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> To: robert.foss-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org Cc: corbet-T1hC0tSOHrs@public.gmane.org, akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org, vbabka-AlSwsSmVLrQ@public.gmane.org, hughd-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org, mhocko-IBi9RG/b67k@public.gmane.org, koct9i-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, n-horiguchi-PaJj6Psr51x8UrSeD/g0lQ@public.gmane.org, kirill.shutemov-VuQAYsv1563Yd54FQh9/CA@public.gmane.org, john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, minchan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, ross.zwisler-VuQAYsv1563Yd54FQh9/CA@public.gmane.org, jmarchan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org, keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org, viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org, mguzik-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, jdanis-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org, calvinowens-b10kYP2dOMg@public.gmane.org, adobriyan-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org, sonnyrao-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org, seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org, tixxdz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Ben Zhang <benzh-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>, Bryan Freed <bfreed-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>, Filipe Brandenburger <filbranden-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>, Jann Horn <jann-XZ1E9jl8jIdeoWH0uzbU5w@public.gmane.org>, Michal Hocko <mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Jacek Subject: Re: [PATCH v5 1/3] mm, proc: Implement /proc/<pid>/totmaps Date: Wed, 7 Sep 2016 14:58:07 +0200 [thread overview] Message-ID: <20160907125806.GA3849@redhat.com> (raw) In-Reply-To: <1473106449-12847-2-git-send-email-robert.foss-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> On 09/05, robert.foss-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org wrote: > > @@ -2854,6 +2854,7 @@ static const struct pid_entry tgid_base_stuff[] = { > REG("clear_refs", S_IWUSR, proc_clear_refs_operations), > REG("smaps", S_IRUGO, proc_pid_smaps_operations), > REG("pagemap", S_IRUSR, proc_pagemap_operations), > + REG("totmaps", S_IRUGO, proc_totmaps_operations), I must have missed something, but I fail to understand why this patch is so complicated. Just use ONE("totmaps", S_IRUGO, proc_totmaps_operations) ? > +static int totmaps_proc_show(struct seq_file *m, void *data) > +{ > + struct proc_maps_private *priv = m->private; > + struct mm_struct *mm = priv->mm; > + struct vm_area_struct *vma; > + struct mem_size_stats mss_sum; > + > + memset(&mss_sum, 0, sizeof(mss_sum)); > + down_read(&mm->mmap_sem); > + hold_task_mempolicy(priv); ^^^^^^^^^^^^^^^^^^^^^^^^^ why? > + for (vma = mm->mmap; vma != priv->tail_vma; vma = vma->vm_next) { Hmm. the usage of ->tail_vma looks just wrong. I guess the code should work because it is NULL but still. > + 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); > + } > + } Why? I mean, why not walk_page_range() ? You do not need this for-each-vma loop at all? At least if you change this patch to use the ONE() helper, and everything else looks unneeded in this case. Oleg.
next prev parent reply other threads:[~2016-09-07 12:58 UTC|newest] Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-09-05 20:14 [PATCH v5 0/3] mm, proc: Implement /proc/<pid>/totmaps robert.foss 2016-09-05 20:14 ` robert.foss 2016-09-05 20:14 ` [PATCH v5 1/3] " robert.foss 2016-09-05 20:14 ` robert.foss 2016-09-07 12:58 ` Oleg Nesterov [this message] 2016-09-07 12:58 ` Oleg Nesterov 2016-09-12 22:12 ` Robert Foss 2016-09-12 22:12 ` Robert Foss 2016-09-05 20:14 ` [PATCH v5 2/3] Documentation/filesystems: Fixed typo robert.foss 2016-09-05 20:14 ` robert.foss-ZGY8ohtN/8qB+jHODAdFcQ 2016-09-07 23:22 ` Kees Cook 2016-09-07 23:22 ` Kees Cook 2016-09-08 0:22 ` Robert Foss 2016-09-08 0:22 ` Robert Foss 2016-09-08 6:23 ` Jonathan Corbet 2016-09-08 6:23 ` Jonathan Corbet 2016-09-05 20:14 ` [PATCH v5 3/3] Documentation/filesystems: Added /proc/PID/totmaps documentation robert.foss 2016-09-05 20:14 ` robert.foss 2016-09-12 12:02 ` [PATCH v5 0/3] mm, proc: Implement /proc/<pid>/totmaps Michal Hocko 2016-09-12 15:31 ` Sonny Rao 2016-09-12 15:31 ` Sonny Rao 2016-09-12 17:15 ` Michal Hocko 2016-09-12 17:15 ` Michal Hocko 2016-09-12 17:28 ` Sonny Rao 2016-09-12 17:28 ` Sonny Rao 2016-09-13 7:12 ` Michal Hocko 2016-09-13 7:12 ` Michal Hocko 2016-09-13 20:27 ` Sonny Rao 2016-09-13 20:27 ` Sonny Rao 2016-09-14 9:12 ` Michal Hocko 2016-09-14 9:12 ` Michal Hocko 2016-09-19 15:16 ` Robert Foss 2016-09-19 15:16 ` Robert Foss 2016-09-19 19:32 ` Michal Hocko 2016-09-19 19:32 ` Michal Hocko [not found] ` <20160919194001.GE2903@pc.thejh.net> [not found] ` <20160919195109.GB28639@dhcp22.suse.cz> 2016-09-19 19:56 ` Jann Horn 2016-09-19 19:56 ` Jann Horn 2016-09-19 20:15 ` Sonny Rao 2016-09-19 20:15 ` Sonny Rao 2016-09-20 0:27 ` Robert Foss 2016-09-20 0:27 ` Robert Foss 2016-09-20 0:29 ` Sonny Rao 2016-09-20 0:29 ` Sonny Rao
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20160907125806.GA3849@redhat.com \ --to=oleg@redhat.com \ --cc=adobriyan@gmail.com \ --cc=akpm@linux-foundation.org \ --cc=benzh@chromium.org \ --cc=bfreed@chromium.org \ --cc=calvinowens@fb.com \ --cc=corbet@lwn.net \ --cc=ebiederm@xmission.com \ --cc=filbranden@chromium.org \ --cc=hannes@cmpxchg.org \ --cc=hughd@google.com \ --cc=j.anaszewski@samsung.com \ --cc=jann@thejh.net \ --cc=jdanis@google.com \ --cc=jmarchan@redhat.com \ --cc=john.stultz@linaro.org \ --cc=keescook@chromium.org \ --cc=kirill.shutemov@linux.intel.com \ --cc=koct9i@gmail.com \ --cc=linux-api@vger.kernel.org \ --cc=linux-doc@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mguzik@redhat.com \ --cc=mhocko@kernel.org \ --cc=mhocko@suse.com \ --cc=minchan@kernel.org \ --cc=n-horiguchi@ah.jp.nec.com \ --cc=robert.foss@collabora.com \ --cc=ross.zwisler@linux.intel.com \ --cc=seth.forshee@canonical.com \ --cc=sonnyrao@chromium.org \ --cc=tixxdz@gmail.com \ --cc=vbabka@suse.cz \ --cc=viro@zeniv.linux.org.uk \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.