On Thu, May 28, 2020 at 5:24 PM Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 28 May 2020 16:52:38 -0700
> Axel Rasmussen <axelrasmussen@google.com> wrote:
>
> Hi Axel,
>
> First, your patch threading is messed up. All the patches should be a
> reply to this cover page, and not individual emails which get lost
> among other patches.
>

Sorry - I'll fix this in the future. (I mistakenly ran git send-email on
each patch individually, rather than globbing them into a single command.)


>
> Next, we already have histogram logic with trace events. Why not build
> off of that. Or perhaps update lockdep which can record contention with
> all locks. Why create yet another histogram infrastructure that is used
> for just a specific purpose?
>

The use case we have in mind for this is to enable this instrumentation
widely in Google's production fleet. Internally, we have a userspace thing
which scrapes these metrics and publishes them such that we can look at
aggregate metrics across our fleet. The thinking is that mechanisms like
lockdep or getting histograms with e.g. BPF attached to the tracepoint
introduces too much overhead for this to be viable. (Although, granted, I
don't have benchmarks to prove this - if there's skepticism, I can produce
such a thing - or prove myself wrong and rethink my approach. :) )


>
> -- Steve
>
>
> > The overall goal of this patchset is to add a latency histogram which
> measures
> > `mmap_lock` acquisition time. This is useful to measure the impact of
> ongoing
> > work like maple trees and range locks (https://lwn.net/Articles/787629/),
> and
> > it is also useful to debug userspace processes which experience long
> waits due
> > to lock contention.
> >
> > This patchset is built upon walken@google.com's new `mmap_lock` API
> > (https://lkml.org/lkml/2020/4/21/1307). In its current form, it should
> apply
> > cleanly to a 5.7-rc7 tree to which Michel's patchset has already been
> applied.
> >
> > To summarize the changes being made at a high level:
> >
> > - Add a histogram library: a `struct histogram` is effectively an array
> of
> >   thresholds (i.e., buckets), and an array of per-cpu `u64` counts of the
> >   number of samples in each bucket.
> >
> > - Modify Michel's mmap_lock API to record samples in a histogram, owned
> by the
> >  `mm_struct`, on each lock acquisition. For contended lock acquisitions,
> we
> >  compute the amount of time spent waiting, which determines the bucket.
> >
> > - For uncontended cases, we still record a sample, but with "0" latency.
> The
> >   reasoning for this is, a) we don't want to incur the overhead of
> actually
> >   measuring the time, but b) we still want to end up with an accurate
> count of
> >   acquisition attempts, as this lets us compute latency percentiles
> (e.g., "x%
> >   of lock acquisitions completed in <= y ns").
> >
> > Changes since v1 (sent to a few folks within Google for initial review):
> >
> > - Added a tracepoint to the contended case.
> > - Modified `mmap_write_lock_nested` to split the {un,}contended cases.
> > - Removed support for having more than one histogram in `mm_struct`.
> > - Removed any histogram code not explicitly used in this patchset.
> > - Whitespace cleanups.
> >
> > Axel Rasmussen (7):
> >   histogram: add struct histogram
> >   histogram: add helper function to expose histograms to userspace
> >   mmap_lock: add a histogram structure to struct mm_struct
> >   mmap_lock: allocate histogram (if enabled) in mm_init
> >   mmap_lock: add /proc/<pid>/mmap_lock_contention interface
> >   mmap_lock: increment histogram whenever mmap_lock is acquired
> >   mmap_lock: add a tracepoint to contended acquisitions
> >
> >  fs/proc/base.c                   |  25 +++
> >  include/linux/histogram.h        | 293 +++++++++++++++++++++++++++++++
> >  include/linux/mm_types.h         |  11 ++
> >  include/linux/mmap_lock.h        |  92 +++++++++-
> >  include/trace/events/mmap_lock.h |  34 ++++
> >  kernel/fork.c                    |  55 ++++++
> >  kernel/locking/rwsem.c           |   4 +-
> >  lib/Kconfig                      |   3 +
> >  lib/Makefile                     |   2 +
> >  lib/histogram.c                  | 212 ++++++++++++++++++++++
> >  mm/Kconfig                       |  13 ++
> >  mm/Makefile                      |   1 +
> >  mm/mmap_lock.c                   |  46 +++++
> >  13 files changed, 782 insertions(+), 9 deletions(-)
> >  create mode 100644 include/linux/histogram.h
> >  create mode 100644 include/trace/events/mmap_lock.h
> >  create mode 100644 lib/histogram.c
> >  create mode 100644 mm/mmap_lock.c
> >
> > --
> > 2.27.0.rc0.183.gde8f92d652-goog
>
>