[v2,07/14] cyclic: invoke uthread_schedule() from schedule()

Make the schedule() call from the CYCLIC framework a uthread scheduling
point too. This makes sense since schedule() is called from a lot of
places where uthread_schedule() needs to be called.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
---
 common/cyclic.c           | 3 +++
 include/u-boot/schedule.h | 3 +++
 2 files changed, 6 insertions(+)

Hi Jerome,

On 25.02.25 17:34, Jerome Forissier wrote:
> Make the schedule() call from the CYCLIC framework a uthread scheduling
> point too. This makes sense since schedule() is called from a lot of
> places where uthread_schedule() needs to be called.
> 
> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>

Frankly, at first I was wondering a bit, if and why another framework
for "multitasking" is needed in U-Boot, additionally to the cyclic
framework that I introduced a few years ago. Which was greatly enhanced
by Rasmus over the time. But looking at your "uthread" implementation
it makes sense to add such a probably more intuitive interface as well.

In general I'm really happy seeing activity in this "multitaking" area
in U-Boot. As it brings a lot of new possibilities and, as you've also
shown in your patchset, may greatly help reducing boot time in the
USB example. :)

One question though:
Do you have some means in your uthread framework, measuring and
perhaps limiting the time spent in these uthreads? If and how is a
preemption of a uthread possible? So that it does not consume too
much time resulting in e.g. things like dropping input chars on
the prompt? Sorry, I did not thoroughly go through all your code
to get the internals from there. It would be great if you could
elaborate a bit on this.

For this patch:

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   common/cyclic.c           | 3 +++
>   include/u-boot/schedule.h | 3 +++
>   2 files changed, 6 insertions(+)
> 
> diff --git a/common/cyclic.c b/common/cyclic.c
> index fad071a39c6..b695f092f52 100644
> --- a/common/cyclic.c
> +++ b/common/cyclic.c
> @@ -16,6 +16,7 @@
>   #include <linux/list.h>
>   #include <asm/global_data.h>
>   #include <u-boot/schedule.h>
> +#include <uthread.h>
>   
>   DECLARE_GLOBAL_DATA_PTR;
>   
> @@ -100,6 +101,8 @@ void schedule(void)
>   	 */
>   	if (gd)
>   		cyclic_run();
> +
> +	uthread_schedule();
>   }
>   
>   int cyclic_unregister_all(void)
> diff --git a/include/u-boot/schedule.h b/include/u-boot/schedule.h
> index 4fd34c41229..4605971fdcb 100644
> --- a/include/u-boot/schedule.h
> +++ b/include/u-boot/schedule.h
> @@ -3,6 +3,8 @@
>   #ifndef _U_BOOT_SCHEDULE_H
>   #define _U_BOOT_SCHEDULE_H
>   
> +#include <uthread.h>
> +
>   #if CONFIG_IS_ENABLED(CYCLIC)
>   /**
>    * schedule() - Schedule all potentially waiting tasks
> @@ -17,6 +19,7 @@ void schedule(void);
>   
>   static inline void schedule(void)
>   {
> +	uthread_schedule();
>   }
>   
>   #endif

Viele Grüße,
Stefan Roese

Hi Stefan,

On 2/27/25 13:30, Stefan Roese wrote:
> Hi Jerome,
> 
> On 25.02.25 17:34, Jerome Forissier wrote:
>> Make the schedule() call from the CYCLIC framework a uthread scheduling
>> point too. This makes sense since schedule() is called from a lot of
>> places where uthread_schedule() needs to be called.
>>
>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
> 
> Frankly, at first I was wondering a bit, if and why another framework
> for "multitasking" is needed in U-Boot, additionally to the cyclic
> framework that I introduced a few years ago. Which was greatly enhanced
> by Rasmus over the time. But looking at your "uthread" implementation
> it makes sense to add such a probably more intuitive interface as well.

cyclic is clean and simple and certainly well suited when introducing new
code. But when reworking older code I find it somewhat difficult to use
due to the need to keep a context and pass it everywhere. This can lead
to lots of changes when call stacks are deep.

> In general I'm really happy seeing activity in this "multitaking" area
> in U-Boot. As it brings a lot of new possibilities and, as you've also
> shown in your patchset, may greatly help reducing boot time in the
> USB example. :)

Thank you.
 
> One question though:
> Do you have some means in your uthread framework, measuring and
> perhaps limiting the time spent in these uthreads? If and how is a
> preemption of a uthread possible? So that it does not consume too
> much time resulting in e.g. things like dropping input chars on
> the prompt? Sorry, I did not thoroughly go through all your code
> to get the internals from there. It would be great if you could
> elaborate a bit on this.

That's a very valid point. The short answer is no, there is no control
over how long a thread keeps the CPU busy. uthread is similar to cyclic
in that respect. That said, I occasionally noted the issue you mentioned
about the console dropping characters, and yes it is annoying. I believe
there is a simple solution though. If we can somehow make sure we're
always scheduling the main thread every other time, we're much less
likely to starve it from the CPU, especially when many threads are
active. That is, assume we have 2 threads in addition to the main thread.
The thread list is main -> thread1 -> thread2 and uthread_schedule() will
iterate in that order. So back to main only after thread1 *and* thread2
have run and called uthread_schedule(). The idea is to schedule in a
different order: main -> thread1 -> main -> thread2 -> main etc.,
effectively giving a higher priority to the main thread (which would be
the console parsing thread).

I feel that introducing preemption would be opening a can of worms...
Because in this case we would likely need locking everywhere. Without
preemption, we still do need locking in theory, it's just that I have
not yet identified critical sections where locks would be mandatory in
the code that I have "parallelized". BTW I believe a uthread lock would
be trivial to implement like so:

struct uthread_lock {
	bool locked;
};

void uthread_lock(struct uthread_lock *l)
{
	while (l->locked)
		uthread_schedule();
	l->locked = true;
}
void uthread_unlock(struct uthread_lock *l)
{
	l->locked = false;
}

> 
> For this patch:
> 
> Reviewed-by: Stefan Roese <sr@denx.de>
> 
> Thanks,
> Stefan

Thanks,

On Tue, 25 Feb 2025 at 18:35, Jerome Forissier
<jerome.forissier@linaro.org> wrote:
>
> Make the schedule() call from the CYCLIC framework a uthread scheduling
> point too. This makes sense since schedule() is called from a lot of
> places where uthread_schedule() needs to be called.
>
> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>

[...]

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Hi Jerome,

On 27.02.25 18:05, Jerome Forissier wrote:
> Hi Stefan,
> 
> On 2/27/25 13:30, Stefan Roese wrote:
>> Hi Jerome,
>>
>> On 25.02.25 17:34, Jerome Forissier wrote:
>>> Make the schedule() call from the CYCLIC framework a uthread scheduling
>>> point too. This makes sense since schedule() is called from a lot of
>>> places where uthread_schedule() needs to be called.
>>>
>>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
>>
>> Frankly, at first I was wondering a bit, if and why another framework
>> for "multitasking" is needed in U-Boot, additionally to the cyclic
>> framework that I introduced a few years ago. Which was greatly enhanced
>> by Rasmus over the time. But looking at your "uthread" implementation
>> it makes sense to add such a probably more intuitive interface as well.
> 
> cyclic is clean and simple and certainly well suited when introducing new
> code. But when reworking older code I find it somewhat difficult to use
> due to the need to keep a context and pass it everywhere. This can lead
> to lots of changes when call stacks are deep.
> 
>> In general I'm really happy seeing activity in this "multitaking" area
>> in U-Boot. As it brings a lot of new possibilities and, as you've also
>> shown in your patchset, may greatly help reducing boot time in the
>> USB example. :)
> 
> Thank you.
>   
>> One question though:
>> Do you have some means in your uthread framework, measuring and
>> perhaps limiting the time spent in these uthreads? If and how is a
>> preemption of a uthread possible? So that it does not consume too
>> much time resulting in e.g. things like dropping input chars on
>> the prompt? Sorry, I did not thoroughly go through all your code
>> to get the internals from there. It would be great if you could
>> elaborate a bit on this.
> 
> That's a very valid point. The short answer is no, there is no control
> over how long a thread keeps the CPU busy. uthread is similar to cyclic
> in that respect. That said, I occasionally noted the issue you mentioned
> about the console dropping characters, and yes it is annoying. I believe
> there is a simple solution though. If we can somehow make sure we're
> always scheduling the main thread every other time, we're much less
> likely to starve it from the CPU, especially when many threads are
> active. That is, assume we have 2 threads in addition to the main thread.
> The thread list is main -> thread1 -> thread2 and uthread_schedule() will
> iterate in that order. So back to main only after thread1 *and* thread2
> have run and called uthread_schedule(). The idea is to schedule in a
> different order: main -> thread1 -> main -> thread2 -> main etc.,
> effectively giving a higher priority to the main thread (which would be
> the console parsing thread).

Sounds like a good idea / improvement to me.

Thanks,
Stefan

> I feel that introducing preemption would be opening a can of worms...
> Because in this case we would likely need locking everywhere. Without
> preemption, we still do need locking in theory, it's just that I have
> not yet identified critical sections where locks would be mandatory in
> the code that I have "parallelized". BTW I believe a uthread lock would
> be trivial to implement like so:
> 
> struct uthread_lock {
> 	bool locked;
> };
> 
> void uthread_lock(struct uthread_lock *l)
> {
> 	while (l->locked)
> 		uthread_schedule();
> 	l->locked = true;
> }
> void uthread_unlock(struct uthread_lock *l)
> {
> 	l->locked = false;
> }
> 
>>
>> For this patch:
>>
>> Reviewed-by: Stefan Roese <sr@denx.de>
>>
>> Thanks,
>> Stefan
> 
> Thanks,