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
