LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Luigi Rizzo <lrizzo@google.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel <linux-kernel@vger.kernel.org>,
	David Rientjes <rientjes@google.com>,
	linux-mm@kvack.org
Subject: Re: [PATCH] Add mmap_assert_locked() annotations to find_vma*()
Date: Mon, 2 Aug 2021 02:16:14 +0200	[thread overview]
Message-ID: <CAMOZA0+mAC-tHDehzqMVP4rd7wggY_DPofRdH=GMouZA9DRC1w@mail.gmail.com> (raw)
In-Reply-To: <20210801123335.6a7f8e1ee1e52ea64db80323@linux-foundation.org>

(repost in plain text)

On Sun, Aug 1, 2021 at 9:33 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Sat, 31 Jul 2021 10:53:41 -0700 Luigi Rizzo <lrizzo@google.com> wrote:
>
> > find_vma() and variants need protection when used.
> > This patch adds mmap_assert_lock() calls in the functions.
> >
> > To make sure the invariant is satisfied, we also need to add a
> > mmap_read_loc() around the get_user_pages_remote() call in
> > get_arg_page(). The lock is not strictly necessary because the mm
> > has been newly created, but the extra cost is limited because
> > the same mutex was also acquired shortly before in __bprm_mm_init(),
> > so it is hot and uncontended.
> >
>
> Well, it isn't cost-free.  find_vma() is called a lot and a surprising
> number of systems apparently run with CONFIG_DEBUG_VM.  Why do you
> think this cost is justified?

I assume you are concerned with the cost of mmap_assert_locked() ?

I'd say the justification is the same as for all asserts:
at some point some code change may miss the required lock, and the
asserts are there to catch elusive race conditions,

There are in fact already instances of mmap_locked_assert()
right before find_vma() in walk_page_range(), and a couple before
calls to __get_user_pages().

As for the cost, I'd think that if CONFIG_DEBUG_VM is set,
one does it on purpose to catch errors and is prepared to pay
the cost (in this case the atomic_read(counter) in rwsem_is_locked(),
the counter should be hot).

FWIW I have instrumented find_vma() on a fast machine using kstats

   https://github.com/luigirizzo/lr-cstats

(load the module then enable the trace with
  echo "trace pcpu:find_vma bits 3" > /sys/kernel/debug/kstats/_control
and monitor the time with
  watch "grep CPUS /sys/kernel/debug/kstats/find_vma"

I didn't run anything especially intensive except some network
benchmarks, but I have collected ~2M samples with the following
distribution of find_vma() time in nanoseconds in 3 configs:

CONFIGURATION         p10   p50   p90   p95   p98

no-debug               89   109   214   332    605
debug                 331   369   603   862   1338
debug+this patch      337   369   603   863   1339

As you can see, just compiling a debug kernel, even without this patch,
makes the function 3x more expensive. The effect of this patch is
not measurable (the differences are below measurement error).

cheers
luigi

  reply	other threads:[~2021-08-02  0:16 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-31 17:53 Luigi Rizzo
2021-08-01 19:33 ` Andrew Morton
2021-08-02  0:16   ` Luigi Rizzo [this message]
2021-08-02 21:11     ` Andrew Morton
2021-08-03 16:08 ` Jason Gunthorpe
2021-08-03 21:48   ` Luigi Rizzo
2021-08-03 23:07     ` Liam Howlett
2021-08-03 23:35       ` Jason Gunthorpe
2021-08-03 23:57         ` Luigi Rizzo
2021-08-04  5:12           ` Liam Howlett
2021-08-04 14:42       ` Jann Horn
2021-08-04 15:21         ` Jason Gunthorpe
2021-08-04 21:22           ` Jann Horn
2021-08-04 17:32         ` Liam Howlett

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='CAMOZA0+mAC-tHDehzqMVP4rd7wggY_DPofRdH=GMouZA9DRC1w@mail.gmail.com' \
    --to=lrizzo@google.com \
    --cc=akpm@linux-foundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=rientjes@google.com \
    --subject='Re: [PATCH] Add mmap_assert_locked() annotations to find_vma*()' \
    /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

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox