Re: [PATCH] Add mmap_assert_locked() annotations to find_vma*()

[Andrew Morton]

On Mon, 2 Aug 2021 02:16:14 +0200 Luigi Rizzo <lrizzo@xxxxxxxxxx> wrote:

> > 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).

Cool, thanks, that's convincing.