Re: [PATCH v2 04/10] Introduce idle slice

[Sascha Hauer <s.hauer@pengutronix.de>]

On Thu, May 14, 2020 at 08:21:52PM +0200, Daniel Glöckner wrote:
> Some code can't foresee which resources will be used by its poller. This
> is the case especially in pollers that will execute arbitrary commands
> input by the user. With this commit a special slice is introduced that is
> released only when Barebox is waiting for console input and only when it
> is doing that outside of any command. Code that wants to depend on the
> idle slice must select CONFIG_IDLE_SLICE to make it available.
> 
> Signed-off-by: Daniel Glöckner <dg@emlix.com>
> ---
>  common/Kconfig             |  4 ++++
>  common/binfmt.c            |  3 +++
>  common/command.c           |  3 +++
>  common/console_countdown.c |  3 +++
>  common/poller.c            |  2 ++
>  common/slice.c             | 24 ++++++++++++++++++++++++
>  include/slice.h            | 10 ++++++++++
>  lib/readline.c             |  8 +++++++-
>  8 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/common/Kconfig b/common/Kconfig
> index 400c0553c..2fa9140de 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -913,6 +913,10 @@ config BAREBOXCRC32_TARGET
>  config POLLER
>  	bool "generic polling infrastructure"
>  
> +config IDLE_SLICE
> +	bool
> +	default n

We discussed config SLICE away, is there a reason to introduce config
IDLE_SLICE?

> +
>  config STATE
>  	bool "generic state infrastructure"
>  	select CRC32
> diff --git a/common/binfmt.c b/common/binfmt.c
> index f2ff62458..899f270d4 100644
> --- a/common/binfmt.c
> +++ b/common/binfmt.c
> @@ -10,6 +10,7 @@
>  #include <malloc.h>
>  #include <command.h>
>  #include <errno.h>
> +#include <slice.h>
>  
>  static LIST_HEAD(binfmt_hooks);
>  
> @@ -23,7 +24,9 @@ static int binfmt_run(char *file, int argc, char **argv)
>  		if (b->type != type)
>  			continue;
>  
> +		idle_slice_acquire();
>  		ret = b->hook(b, file, argc, argv);
> +		idle_slice_release();
>  		if (ret != -ERESTARTNOHAND)
>  			return ret;
>  	}
> diff --git a/common/command.c b/common/command.c
> index 49845938a..2b10248f5 100644
> --- a/common/command.c
> +++ b/common/command.c
> @@ -30,6 +30,7 @@
>  #include <init.h>
>  #include <complete.h>
>  #include <getopt.h>
> +#include <slice.h>
>  
>  LIST_HEAD(command_list);
>  EXPORT_SYMBOL(command_list);
> @@ -72,7 +73,9 @@ int execute_command(int argc, char **argv)
>  	/* Look up command in command table */
>  	if ((cmdtp = find_cmd(argv[0]))) {
>  		/* OK - call function to do the command */
> +		idle_slice_acquire();
>  		ret = cmdtp->cmd(argc, argv);
> +		idle_slice_release();

The command could be a shell in which case we would never be able to
release the idle slice as long this shell is running.

> @@ -200,11 +201,16 @@ int readline(const char *prompt, char *buf, int len)
>  	puts (prompt);
>  
>  	while (1) {
> +		idle_slice_release();
>  		while (!tstc()) {
>  			poller_call();
> -			if (IS_ENABLED(CONFIG_CONSOLE_RATP))
> +			if (IS_ENABLED(CONFIG_CONSOLE_RATP)) {
> +				idle_slice_acquire();
>  				barebox_ratp_command_run();
> +				idle_slice_release();
> +			}
>  		}
> +		idle_slice_acquire();

I am not sure this is the right place to release the idle slice, I would
rather do it in the caller. readline() may be called from different
places, not all of them seem to be appropriate for releasing the idle
slice.

I think the idle slice should be released right before the shell calls
into readline and acquired again right afterwards.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox