From: Johannes Weiner <hannes@cmpxchg.org>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Shaohua Li <shli@fb.com>,
Siddhesh Poyarekar <siddhesh.poyarekar@gmail.com>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org,
kernel-team@fb.com
Subject: Re: [PATCH] proc: revert /proc/<pid>/maps [stack:TID] annotation
Date: Tue, 19 Jan 2016 18:38:22 -0500 [thread overview]
Message-ID: <20160119233822.GA10788@cmpxchg.org> (raw)
In-Reply-To: <20160119141430.8ff9c464.akpm@linux-foundation.org>
On Tue, Jan 19, 2016 at 02:14:30PM -0800, Andrew Morton wrote:
> On Tue, 19 Jan 2016 13:02:39 -0500 Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> > b764375 ("procfs: mark thread stack correctly in proc/<pid>/maps")
> > added [stack:TID] annotation to /proc/<pid>/maps. Finding the task of
> > a stack VMA requires walking the entire thread list, turning this into
> > quadratic behavior: a thousand threads means a thousand stacks, so the
> > rendering of /proc/<pid>/maps needs to look at a million threads. The
> > cost is not in proportion to the usefulness as described in the patch.
> >
> > Drop the [stack:TID] annotation to make /proc/<pid>/maps (and
> > /proc/<pid>/numa_maps) usable again for higher thread counts.
> >
> > The [stack] annotation inside /proc/<pid>/task/<tid>/maps is retained,
> > as identifying the stack VMA there is an O(1) operation.
>
> Four years ago, ouch.
>
> Any thoughts on the obvious back-compatibility concerns? ie, why did
> Siddhesh implement this in the first place? My bad for not ensuring
> that the changelog told us this.
I thought about storing the TID of the thread using the VMA as the
stack directly inside vm_area_struct; maybe using vm_private_data?
However, that's a bit of work and ugliness that I wouldn't want to
commit to until we know that people ended up using this in practice.
> I note that this patch is a partial revert - the smaps and numa_maps
> parts of b764375 remain in place. What's up with that?
I left the stack annotations in the thread-specific files because that
sounds useful and is cheap enough - we only have to test the vma range
against that thread's stack pointer. The last changelog paragraph says
that for maps, I'll update it to include smaps and numa_maps.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
WARNING: multiple messages have this Message-ID (diff)
From: Johannes Weiner <hannes@cmpxchg.org>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Shaohua Li <shli@fb.com>,
Siddhesh Poyarekar <siddhesh.poyarekar@gmail.com>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org,
kernel-team@fb.com
Subject: Re: [PATCH] proc: revert /proc/<pid>/maps [stack:TID] annotation
Date: Tue, 19 Jan 2016 18:38:22 -0500 [thread overview]
Message-ID: <20160119233822.GA10788@cmpxchg.org> (raw)
In-Reply-To: <20160119141430.8ff9c464.akpm@linux-foundation.org>
On Tue, Jan 19, 2016 at 02:14:30PM -0800, Andrew Morton wrote:
> On Tue, 19 Jan 2016 13:02:39 -0500 Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> > b764375 ("procfs: mark thread stack correctly in proc/<pid>/maps")
> > added [stack:TID] annotation to /proc/<pid>/maps. Finding the task of
> > a stack VMA requires walking the entire thread list, turning this into
> > quadratic behavior: a thousand threads means a thousand stacks, so the
> > rendering of /proc/<pid>/maps needs to look at a million threads. The
> > cost is not in proportion to the usefulness as described in the patch.
> >
> > Drop the [stack:TID] annotation to make /proc/<pid>/maps (and
> > /proc/<pid>/numa_maps) usable again for higher thread counts.
> >
> > The [stack] annotation inside /proc/<pid>/task/<tid>/maps is retained,
> > as identifying the stack VMA there is an O(1) operation.
>
> Four years ago, ouch.
>
> Any thoughts on the obvious back-compatibility concerns? ie, why did
> Siddhesh implement this in the first place? My bad for not ensuring
> that the changelog told us this.
I thought about storing the TID of the thread using the VMA as the
stack directly inside vm_area_struct; maybe using vm_private_data?
However, that's a bit of work and ugliness that I wouldn't want to
commit to until we know that people ended up using this in practice.
> I note that this patch is a partial revert - the smaps and numa_maps
> parts of b764375 remain in place. What's up with that?
I left the stack annotations in the thread-specific files because that
sounds useful and is cheap enough - we only have to test the vma range
against that thread's stack pointer. The last changelog paragraph says
that for maps, I'll update it to include smaps and numa_maps.
next prev parent reply other threads:[~2016-01-19 23:38 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-19 18:02 [PATCH] proc: revert /proc/<pid>/maps [stack:TID] annotation Johannes Weiner
2016-01-19 18:02 ` Johannes Weiner
2016-01-19 22:14 ` Andrew Morton
2016-01-19 22:14 ` Andrew Morton
2016-01-19 23:30 ` Kirill A. Shutemov
2016-01-19 23:30 ` Kirill A. Shutemov
2016-01-20 3:21 ` Siddhesh Poyarekar
2016-01-20 3:21 ` Siddhesh Poyarekar
2016-01-19 23:38 ` Johannes Weiner [this message]
2016-01-19 23:38 ` Johannes Weiner
2016-01-20 3:17 ` Siddhesh Poyarekar
2016-01-20 3:17 ` Siddhesh Poyarekar
2016-01-20 5:27 ` Andrew Morton
2016-01-20 5:27 ` Andrew Morton
2016-01-21 10:49 ` [PATCH] proc: add missing 'mm' variable in nommu is_stack() Arnd Bergmann
2016-01-21 10:49 ` Arnd Bergmann
-- strict thread matches above, loose matches on Subject: below --
2016-01-25 21:30 [PATCH] proc: revert /proc/<pid>/maps [stack:TID] annotation Colin Cross
2016-01-25 21:30 ` Colin Cross
2016-01-25 23:14 ` Kirill A. Shutemov
2016-01-25 23:14 ` Kirill A. Shutemov
2016-01-25 23:53 ` Colin Cross
2016-01-25 23:53 ` Colin Cross
2016-01-28 10:25 ` Kirill A. Shutemov
2016-01-28 10:25 ` Kirill A. Shutemov
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=20160119233822.GA10788@cmpxchg.org \
--to=hannes@cmpxchg.org \
--cc=akpm@linux-foundation.org \
--cc=kernel-team@fb.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=shli@fb.com \
--cc=siddhesh.poyarekar@gmail.com \
/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: link
Be 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.