(repost in plain text) On Sun, Aug 1, 2021 at 9:33 PM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote: > > On Sat, 31 Jul 2021 10:53:41 -0700 Luigi Rizzo <lrizzo@xxxxxxxxxx> 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
