| Date | Wed, 8 Nov 2023 17:28:27 +0100 | From | Miquel Raynal <> | Subject | Re: [RFC PATCH 82/86] treewide: mtd: remove cond_resched() |
| |
Hi Ankur,
ankur.a.arora@oracle.com 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@oracle.com/ > > Cc: Miquel Raynal <miquel.raynal@bootlin.com> > Cc: Richard Weinberger <richard@nod.at> > Cc: Vignesh Raghavendra <vigneshr@ti.com> > Cc: Kyungmin Park <kyungmin.park@samsung.com> > Cc: Tudor Ambarus <tudor.ambarus@linaro.org> > Cc: Pratyush Yadav <pratyush@kernel.org> > Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com> > ---
[...]
> --- 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
|