Re: [PATCH] mm: remove unintentional voluntary preemption in get_mmap_lock_carefully

[Mateusz Guzik]

On Sun, Aug 20, 2023 at 02:47:41PM +0200, Linus Torvalds wrote:
> On Sun, 20 Aug 2023 at 14:41, Mateusz Guzik <mjguzik@xxxxxxxxx> 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@xxxxxxxxx>
---
 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