Re: [RFC PATCH 82/86] treewide: mtd: remove cond_resched()

[Miquel Raynal]

Hi Ankur,

ankur.a.arora@xxxxxxxxxx wrote on Tue,  7 Nov 2023 15:08:18 -0800:

> There are broadly three sets of uses of cond_resched():
> 
> 1.  Calls to cond_resched() out of the goodness of our heart,
>     otherwise known as avoiding lockup splats.
> 
> 2.  Open coded variants of cond_resched_lock() which call
>     cond_resched().
> 
> 3.  Retry or error handling loops, where cond_resched() is used as a
>     quick alternative to spinning in a tight-loop.
> 
> When running under a full preemption model, the cond_resched() reduces
> to a NOP (not even a barrier) so removing it obviously cannot matter.
> 
> But considering only voluntary preemption models (for say code that
> has been mostly tested under those), for set-1 and set-2 the
> scheduler can now preempt kernel tasks running beyond their time
> quanta anywhere they are preemptible() [1]. Which removes any need
> for these explicitly placed scheduling points.
> 
> The cond_resched() calls in set-3 are a little more difficult.
> To start with, given it's NOP character under full preemption, it
> never actually saved us from a tight loop.
> With voluntary preemption, it's not a NOP, but it might as well be --
> for most workloads the scheduler does not have an interminable supply
> of runnable tasks on the runqueue.
> 
> So, cond_resched() is useful to not get softlockup splats, but not
> terribly good for error handling. Ideally, these should be replaced
> with some kind of timed or event wait.
> For now we use cond_resched_stall(), which tries to schedule if
> possible, and executes a cpu_relax() if not.
> 
> Most of the uses here are in set-1 (some right after we give up a lock
> or enable bottom-halves, causing an explicit preemption check.)
> 
> There are a few cases from set-3. Replace them with
> cond_resched_stall(). Some of those places, however, have wait-times
> milliseconds, so maybe we should just have an msleep() there?

Yeah, I believe this should work.

> 
> [1] https://lore.kernel.org/lkml/20231107215742.363031-1-ankur.a.arora@xxxxxxxxxx/
> 
> Cc: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> Cc: Richard Weinberger <richard@xxxxxx>
> Cc: Vignesh Raghavendra <vigneshr@xxxxxx>
> Cc: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> Cc: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>
> Cc: Pratyush Yadav <pratyush@xxxxxxxxxx>
> Signed-off-by: Ankur Arora <ankur.a.arora@xxxxxxxxxx>
> ---

[...]

> --- a/drivers/mtd/nand/raw/nand_legacy.c
> +++ b/drivers/mtd/nand/raw/nand_legacy.c
> @@ -203,7 +203,13 @@ void nand_wait_ready(struct nand_chip *chip)
>  	do {
>  		if (chip->legacy.dev_ready(chip))
>  			return;
> -		cond_resched();
> +		/*
> +		 * Use a cond_resched_stall() to avoid spinning in
> +		 * a tight loop.
> +		 * Though, given that the timeout is in milliseconds,
> +		 * maybe this should timeout or event wait?

Event waiting is precisely what we do here, with the hardware access
which are available in this case. So I believe this part of the comment
(in general) is not relevant. Now regarding the timeout I believe it is
closer to the second than the millisecond, so timeout-ing is not
relevant either in most cases (talking about mtd/ in general).

> +		 */
> +		cond_resched_stall();
>  	} while (time_before(jiffies, timeo));

Thanks,
Miquèl