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
next prev parent 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 [PATCH] Add mmap_assert_locked() annotations to find_vma*() 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 \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).