Should the trylock succeed (and thus blocking was avoided), the routine
wants to ensure blocking was still legal to do. However, the method
used ends up calling __cond_resched injecting a voluntary preemption
point with the freshly acquired lock.

One can hack around it using __might_sleep instead of mere might_sleep,
but since threads keep going off CPU here, I figured it is better to
accomodate it.

Drop the trylock, do the read lock which does the job prior to lock
acquire.

Found by checking off-CPU time during kernel build (like so:
"offcputime-bpfcc -Ku"), sample backtrace:
    finish_task_switch.isra.0
    __schedule
    __cond_resched
    lock_mm_and_find_vma
    do_user_addr_fault
    exc_page_fault
    asm_exc_page_fault
    -                sh (4502)
        10

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---
 mm/memory.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 1ec1ef3418bf..f31d5243272b 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5257,12 +5257,6 @@ EXPORT_SYMBOL_GPL(handle_mm_fault);
 
 static inline bool get_mmap_lock_carefully(struct mm_struct *mm, struct pt_regs *regs)
 {
-	/* Even if this succeeds, make it clear we *might* have slept */
-	if (likely(mmap_read_trylock(mm))) {
-		might_sleep();
-		return true;
-	}
-
 	if (regs && !user_mode(regs)) {
 		unsigned long ip = instruction_pointer(regs);
 		if (!search_exception_tables(ip))
-- 
2.39.2

On Sun, Aug 20, 2023 at 12:43:03PM +0200, Mateusz Guzik wrote:
> Should the trylock succeed (and thus blocking was avoided), the routine
> wants to ensure blocking was still legal to do. However, the method
> used ends up calling __cond_resched injecting a voluntary preemption
> point with the freshly acquired lock.
> 
> One can hack around it using __might_sleep instead of mere might_sleep,
> but since threads keep going off CPU here, I figured it is better to
> accomodate it.

Except now we search the exception tables every time we call it.
The now-deleted comment (c2508ec5a58d) suggests this is slow:

-       /*
-        * Kernel-mode access to the user address space should only occur
-        * on well-defined single instructions listed in the exception
-        * tables.  But, an erroneous kernel fault occurring outside one of
-        * those areas which also holds mmap_lock might deadlock attempting
-        * to validate the fault against the address space.
-        *
-        * Only do the expensive exception table search when we might be at
-        * risk of a deadlock.  This happens if we
-        * 1. Failed to acquire mmap_lock, and
-        * 2. The access did not originate in userspace.
-        */

Now, this doesn't mean we're doing it on every page fault.  We skip
all of this if we're able to handle the fault under the VMA lock,
so the effect is probably much smaller than it was.  But I'm surprised
not to see you send any data quantifying the effect of this change!

On 8/20/23, Matthew Wilcox <willy@infradead.org> wrote:
> On Sun, Aug 20, 2023 at 12:43:03PM +0200, Mateusz Guzik wrote:
>> Should the trylock succeed (and thus blocking was avoided), the routine
>> wants to ensure blocking was still legal to do. However, the method
>> used ends up calling __cond_resched injecting a voluntary preemption
>> point with the freshly acquired lock.
>>
>> One can hack around it using __might_sleep instead of mere might_sleep,
>> but since threads keep going off CPU here, I figured it is better to
>> accomodate it.
>
> Except now we search the exception tables every time we call it.
> The now-deleted comment (c2508ec5a58d) suggests this is slow:
>

I completely agree it a rather unfortunate side-effect.

> -       /*
> -        * Kernel-mode access to the user address space should only occur
> -        * on well-defined single instructions listed in the exception
> -        * tables.  But, an erroneous kernel fault occurring outside one of
> -        * those areas which also holds mmap_lock might deadlock attempting
> -        * to validate the fault against the address space.
> -        *
> -        * Only do the expensive exception table search when we might be at
> -        * risk of a deadlock.  This happens if we
> -        * 1. Failed to acquire mmap_lock, and
> -        * 2. The access did not originate in userspace.
> -        */
>
> Now, this doesn't mean we're doing it on every page fault.  We skip
> all of this if we're able to handle the fault under the VMA lock,
> so the effect is probably much smaller than it was.  But I'm surprised
> not to see you send any data quantifying the effect of this change!
>

Going off CPU *after* taking the lock sounds like an obviously bad
thing to happen and as such I did not think it warrants any
measurements.

My first patch looked like this:
diff --git a/mm/memory.c b/mm/memory.c
index 1ec1ef3418bf..8662fd69eae8 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5259,7 +5259,9 @@ static inline bool
get_mmap_lock_carefully(struct mm_struct *mm, struct pt_regs
 {
        /* Even if this succeeds, make it clear we *might* have slept */
        if (likely(mmap_read_trylock(mm))) {
-               might_sleep();
+#if defined(CONFIG_DEBUG_ATOMIC_SLEEP)
+               __might_sleep(__FILE__, __LINE__);
+#endif
                return true;
        }

This keeps assertions while dodging __cond_resched.

But then I figured someone may complain about scheduling latency which
was not there prior to the patch.

Between the 2 not so great choices I rolled with what I considered the
safer one.

However, now that I said it, I wonder if perhaps the search could be
circumvented on x86-64? The idea would be to check if SMAP got
disabled (and assuming the CPU supports it) -- if so and the faulting
address belongs to userspace, assume it's all good. This is less
precise in that SMAP can be disabled over the intended users access
but would be fine as far as I'm concerned if the search is such a big
deal.

-- 
Mateusz Guzik <mjguzik gmail.com>

On Sun, 20 Aug 2023 at 14:41, Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> On 8/20/23, Matthew Wilcox <willy@infradead.org> wrote:
> >
> > Except now we search the exception tables every time we call it.
> > The now-deleted comment (c2508ec5a58d) suggests this is slow:

Yeah, that was the intent.

But I agree that we should basically avoid trying to sleep just as we
got the lock.

> My first patch looked like this:

Well, that's disgusting and strange.

> -               might_sleep();
> +#if defined(CONFIG_DEBUG_ATOMIC_SLEEP)
> +               __might_sleep(__FILE__, __LINE__);
> +#endif

Why would you have that strange #ifdef? __might_sleep() just goes away
without that debug option anyway.

But without that odd ifdef, I think it's fine.

            Linus

On Sun, 20 Aug 2023 at 14:47, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> But without that odd ifdef, I think it's fine.

Another option might be to just move the might_sleep() to the top, and
do it unconditionally. If the trylock fails, the overhead of possibly
doing a cond_resched() is kind of moot.

IOW, the main problem here is not that it causes a scheduling point
(if the kernel isn't preemptable), it seems to be just that we
unnecessarily schedule in a place with the mm lock is held, so it
unnecessarily causes possible lock contention for writers.

With the per-vma locking catching most cases, does any of this even matter?

Mateusz - on that note: I'm wondering what made you see this as a
problem. The case you quote doesn't actually seem to be threaded, so
the vm lock contention shouldn't actually matter there.

Does it schedule away? Sure. But only if "needs_resched" is set, so it
doesn't seem to be a *bad* thing per se.

It might just be that this particular scheduling point ends up being a
common one on that load, and with those kernel config options (ie
PREEMPT_VOLUNTARY)?

              Linus

On Sun, Aug 20, 2023 at 02:59:07PM +0200, Linus Torvalds wrote:
> On Sun, 20 Aug 2023 at 14:47, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > But without that odd ifdef, I think it's fine.
> 
> Another option might be to just move the might_sleep() to the top, and
> do it unconditionally. If the trylock fails, the overhead of possibly
> doing a cond_resched() is kind of moot.
> 

I wanted to do it, but then I found this comment:

 * For example, if we have a kernel bug that causes a page
 * fault, we don't want to just use mmap_read_lock() to get
 * the mm lock, because that would deadlock if the bug were
 * to happen while we're holding the mm lock for writing.

I figured scheduling away while on the way to OOPS/similar is not the
best thing to happen.

> IOW, the main problem here is not that it causes a scheduling point
> (if the kernel isn't preemptable), it seems to be just that we
> unnecessarily schedule in a place with the mm lock is held, so it
> unnecessarily causes possible lock contention for writers.
> 
> With the per-vma locking catching most cases, does any of this even matter?
> 
> Mateusz - on that note: I'm wondering what made you see this as a
> problem. The case you quote doesn't actually seem to be threaded, so
> the vm lock contention shouldn't actually matter there.
> 
> Does it schedule away? Sure. But only if "needs_resched" is set, so it
> doesn't seem to be a *bad* thing per se.
> 
> It might just be that this particular scheduling point ends up being a
> common one on that load, and with those kernel config options (ie
> PREEMPT_VOLUNTARY)?
> 

I did not cause a slowdown for me and I did not say it did.

I am saying down_read + going off CPU, should it happen in a
multithreaded program, is going to delay any down_write issued by other
threads. And that going off CPU here was clearly not intended.

As I noted in another e-mail this is just a side thing I spotted while
looking at other stuff. I don't find it important enough to discuss it
any further, so as far as I am concerned you are most welcome to take
any of the 2 patches, write your own or or leave the code be.

[I am going to post other stuff later which *I am* going to argue to
push for ;>]

On Sun, Aug 20, 2023 at 02:47:41PM +0200, Linus Torvalds wrote:
> On Sun, 20 Aug 2023 at 14:41, Mateusz Guzik <mjguzik@gmail.com> wrote:
> > My first patch looked like this:
> 
> Well, that's disgusting and strange.
> 
> > -               might_sleep();
> > +#if defined(CONFIG_DEBUG_ATOMIC_SLEEP)
> > +               __might_sleep(__FILE__, __LINE__);
> > +#endif
> 
> Why would you have that strange #ifdef? __might_sleep() just goes away
> without that debug option anyway.
> 
> But without that odd ifdef, I think it's fine.
> 

Heh, I wrote the patch last night and I could swear it failed to compile
without the ifdef.

That said I think it looks more than disgusting and I'm happy to confirm
it does build both ways.

That said:

mm: remove unintentional voluntary preemption in get_mmap_lock_carefully

Should the trylock succeed (and thus blocking was avoided), the routine
wants to ensure blocking was still legal to do. However, might_sleep()
ends up calling __cond_resched() injecting a voluntary preemption point
with the freshly acquired lock.

__might_sleep() instead to only get the asserts.

Found while checking off-CPU time during kernel build (like so:
"offcputime-bpfcc -Ku"), sample backtrace:
    finish_task_switch.isra.0
    __schedule
    __cond_resched
    lock_mm_and_find_vma
    do_user_addr_fault
    exc_page_fault
    asm_exc_page_fault
    -                sh (4502)
        10

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---
 mm/memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memory.c b/mm/memory.c
index 1ec1ef3418bf..d82316a8a48b 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5259,7 +5259,7 @@ static inline bool get_mmap_lock_carefully(struct mm_struct *mm, struct pt_regs
 {
 	/* Even if this succeeds, make it clear we *might* have slept */
 	if (likely(mmap_read_trylock(mm))) {
-		might_sleep();
+		__might_sleep(__FILE__, __LINE__);
 		return true;
 	}
 
-- 
2.39.2

On 8/20/23, Mateusz Guzik <mjguzik@gmail.com> wrote:
> On 8/20/23, Matthew Wilcox <willy@infradead.org> wrote:
>> On Sun, Aug 20, 2023 at 12:43:03PM +0200, Mateusz Guzik wrote:
>>> Should the trylock succeed (and thus blocking was avoided), the routine
>>> wants to ensure blocking was still legal to do. However, the method
>>> used ends up calling __cond_resched injecting a voluntary preemption
>>> point with the freshly acquired lock.
>>>
>>> One can hack around it using __might_sleep instead of mere might_sleep,
>>> but since threads keep going off CPU here, I figured it is better to
>>> accomodate it.
>>
>> Except now we search the exception tables every time we call it.
>> The now-deleted comment (c2508ec5a58d) suggests this is slow:
>>
>
> I completely agree it a rather unfortunate side-effect.
>
>> -       /*
>> -        * Kernel-mode access to the user address space should only occur
>> -        * on well-defined single instructions listed in the exception
>> -        * tables.  But, an erroneous kernel fault occurring outside one
>> of
>> -        * those areas which also holds mmap_lock might deadlock
>> attempting
>> -        * to validate the fault against the address space.
>> -        *
>> -        * Only do the expensive exception table search when we might be
>> at
>> -        * risk of a deadlock.  This happens if we
>> -        * 1. Failed to acquire mmap_lock, and
>> -        * 2. The access did not originate in userspace.
>> -        */
>>
>> Now, this doesn't mean we're doing it on every page fault.  We skip
>> all of this if we're able to handle the fault under the VMA lock,
>> so the effect is probably much smaller than it was.  But I'm surprised
>> not to see you send any data quantifying the effect of this change!
>>
>
> Going off CPU *after* taking the lock sounds like an obviously bad
> thing to happen and as such I did not think it warrants any
> measurements.
>
> My first patch looked like this:
> diff --git a/mm/memory.c b/mm/memory.c
> index 1ec1ef3418bf..8662fd69eae8 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -5259,7 +5259,9 @@ static inline bool
> get_mmap_lock_carefully(struct mm_struct *mm, struct pt_regs
>  {
>         /* Even if this succeeds, make it clear we *might* have slept */
>         if (likely(mmap_read_trylock(mm))) {
> -               might_sleep();
> +#if defined(CONFIG_DEBUG_ATOMIC_SLEEP)
> +               __might_sleep(__FILE__, __LINE__);
> +#endif
>                 return true;
>         }
>
> This keeps assertions while dodging __cond_resched.
>
> But then I figured someone may complain about scheduling latency which
> was not there prior to the patch.
>
> Between the 2 not so great choices I rolled with what I considered the
> safer one.
>
> However, now that I said it, I wonder if perhaps the search could be
> circumvented on x86-64? The idea would be to check if SMAP got
> disabled (and assuming the CPU supports it) -- if so and the faulting
> address belongs to userspace, assume it's all good. This is less
> precise in that SMAP can be disabled over the intended users access
> but would be fine as far as I'm concerned if the search is such a big
> deal.
>

Oof, hit send too fast.

This is less precise in that SMAP can be disabled over A LARGER AREA
THAN the intended users access but would be fine as far as I'm
concerned if the search is such a big.

there :)

Anyhow I don't feel strongly about any of this, I was mostly
interested in what happens with VFS on the off-CPU front and this one
is just a random thing I needed to check.

Now that I elaborated on my $0,03 I'm happy to respin with the
__might_sleep variant. If someone wants a different fix altogether
they are welcome to ignore these patches.

I do claim the current state *is* a problem though -- it can block
down_write for no good reason.

-- 
Mateusz Guzik <mjguzik gmail.com>