Re: [PATCH] mm/mmu_notifier: Remove non_block_start/end() from notifier invocation

["David Hildenbrand (Arm)" <david@kernel.org>]

On 8/11/26 10:58, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> This effectively reverts commit ba170f76b69d ("mm, notifier: Catch
> sleeping/blocking for !blockable") for the mmu_notifier call sites.
> 
> The non_block_start/end() annotation causes the scheduler to complain
> about *any* voluntary sleep in a non-blockable notifier. But that was
> never the actual constraint. As Michal Hocko put it when the
> annotation was first proposed (quoted in commit 312364f3534c
> ("kernel.h: Add non_block_start/end()")), the OOM reaper "shouldn't
> depend on any locks or sleepable conditionals" and checking for
> sleepable context was "the best thing we could come up with that would
> describe these demands at least partially". The real requirement is that the reaper
> must not block on anything which may itself depend on memory
> allocation (or on the dying mm) to make progress — which is why
> spinning locks were always considered fine.

I think it's conceptually more than that: "we mostly do care about it to make a
forward progress". So yes, memory allocations are the obvious problem, but we
also wouldn't want to wait on any lock that will be hard/impossible to get while
reaping.

Just take a look at what some mmu_notifier_range_blockable() users end up doing:
they skip taking locks.

[...]

> 
> diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c
> index 245b74f39f91..cd5d15cd646a 100644
> --- a/mm/mmu_notifier.c
> +++ b/mm/mmu_notifier.c
> @@ -520,11 +520,7 @@ static int mn_hlist_invalidate_range_start(
>  		if (ops->invalidate_range_start) {
>  			int _ret;
>  
> -			if (!mmu_notifier_range_blockable(range))
> -				non_block_start();
>  			_ret = ops->invalidate_range_start(subscription, range);
> -			if (!mmu_notifier_range_blockable(range))
> -				non_block_end();
>  			if (_ret) {
>  				pr_info("%pS callback failed with %d in %sblockable context.\n",
>  					ops->invalidate_range_start, _ret,
> @@ -591,14 +587,9 @@ mn_hlist_invalidate_end(struct mmu_notifier_subscriptions *subscriptions,
>  	id = srcu_read_lock(&srcu);
>  	hlist_for_each_entry_srcu(subscription, &subscriptions->list, hlist,
>  				 srcu_read_lock_held(&srcu)) {
> -		if (subscription->ops->invalidate_range_end) {
> -			if (!mmu_notifier_range_blockable(range))
> -				non_block_start();
> +		if (subscription->ops->invalidate_range_end)
>  			subscription->ops->invalidate_range_end(subscription,
>  								range);
> -			if (!mmu_notifier_range_blockable(range))
> -				non_block_end();
> -		}
>  	}
>  	srcu_read_unlock(&srcu, id);
>  }

It's a bit odd. We have infrastructure to disallow blocking, and do so on
multiple paths (just check for mmu_notifier_range_blockable() users where we
skip taking mutexes, not performing memory allocations!), but now essentially
allow blocking on some paths.

That's just inconsistent. If we want different semantics, I think the whole
thing should be re-thought: if blocking is suddenly allowed.

-- 
Cheers,

David