[PATCH] fixup! common: introduce bthreads, co-operative barebox threads

[PATCH] fixup! common: introduce bthreads, co-operative barebox threads

[Ahmad Fatoum]

Specifying __aligned for a struct member only ensures a relative
alignment to the start of the struct. To get an absolute alignment,
we must ensure the struct itself is aligned suitably as well. Do so.

This fixes an issue where printf("%llu" printed bogus values when
run from a bthread, because gcc va_arg on RISC-V requires 16-bit
stack alignment.

Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
 common/bthread.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/bthread.c b/common/bthread.c
index 80b486c99af7..ece02e091da9 100644
--- a/common/bthread.c
+++ b/common/bthread.c
@@ -88,7 +88,7 @@ struct bthread *bthread_create(int (*threadfn)(void *), void *data,
 	va_list ap;
 	int len;
 
-	bthread = malloc(struct_size(bthread, stack_space, CONFIG_STACK_SIZE));
+	bthread = memalign(16, struct_size(bthread, stack_space, CONFIG_STACK_SIZE));
 	if (!bthread)
 		goto err;
 
-- 
2.30.0


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

Re: [PATCH] fixup! common: introduce bthreads, co-operative barebox threads

[Sascha Hauer]

On Sun, Mar 21, 2021 at 04:00:50PM +0100, Ahmad Fatoum wrote:
> Specifying __aligned for a struct member only ensures a relative
> alignment to the start of the struct. To get an absolute alignment,
> we must ensure the struct itself is aligned suitably as well. Do so.
> 
> This fixes an issue where printf("%llu" printed bogus values when
> run from a bthread, because gcc va_arg on RISC-V requires 16-bit
> stack alignment.
> 
> Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
> ---
>  common/bthread.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/bthread.c b/common/bthread.c
> index 80b486c99af7..ece02e091da9 100644
> --- a/common/bthread.c
> +++ b/common/bthread.c
> @@ -88,7 +88,7 @@ struct bthread *bthread_create(int (*threadfn)(void *), void *data,
>  	va_list ap;
>  	int len;
>  
> -	bthread = malloc(struct_size(bthread, stack_space, CONFIG_STACK_SIZE));
> +	bthread = memalign(16, struct_size(bthread, stack_space, CONFIG_STACK_SIZE));

Wouldn't it be clearer to make the stack an extra allocation instead of
attaching it to struct bthread?

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

Re: [PATCH] fixup! common: introduce bthreads, co-operative barebox threads

[Ahmad Fatoum]

On 04.03.21 09:49, Sascha Hauer wrote:
> Could you separate the option parsing from the functionality? The way it
> currently is is rather hard to extend.

Sure thing.

> 
> 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

Re: [PATCH] fixup! common: introduce bthreads, co-operative barebox threads

[Sascha Hauer]

On Wed, Mar 03, 2021 at 11:20:48AM +0100, Ahmad Fatoum wrote:
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  commands/bthread.c | 142 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 142 insertions(+)
>  create mode 100644 commands/bthread.c
> 
> diff --git a/commands/bthread.c b/commands/bthread.c
> new file mode 100644
> index 000000000000..1fd782f03f43
> --- /dev/null
> +++ b/commands/bthread.c
> @@ -0,0 +1,142 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2021 Ahmad Fatoum, Pengutronix
> + */
> +
> +#include <bthread.h>
> +#include <errno.h>
> +#include <malloc.h>
> +#include <stdio.h>
> +#include <command.h>
> +#include <getopt.h>
> +#include <clock.h>
> +
> +static int bthread_time(void)
> +{
> +	uint64_t start = get_time_ns();
> +	int i = 0;
> +
> +	/*
> +	 * How many background tasks can we have in one second?
> +	 *
> +	 * A low number here may point to problems with bthreads taking too
> +	 * much time.
> +	 */
> +	while (!is_timeout(start, SECOND))
> +		i++;
> +
> +	return i;
> +}
> +
> +static int bthread_infinite(void *data)
> +{
> +	while (!bthread_should_stop())
> +		;
> +
> +	return 0;
> +}
> +
> +static int bthread_isolated_time(void)
> +{
> +	uint64_t start = get_time_ns();
> +	struct bthread *bthread;
> +	int i = 0;
> +
> +	bthread = bthread_create(bthread_infinite, NULL, "infinite");
> +	if (!bthread)
> +		return -ENOMEM;
> +
> +	bthread_wake(bthread);
> +
> +	/*
> +	 * How many context switches can we do in one second?
> +	 *
> +	 * A low number here may point to problems with bthreads taking too
> +	 * much time.
> +	 */
> +	while (!is_timeout_non_interruptible(start, SECOND)) {
> +		bthread_schedule(bthread);
> +		i += 2;
> +	}
> +
> +	bthread_stop(bthread);
> +	bthread_free(bthread);
> +
> +	return i;
> +}
> +
> +static int bthread_printer(void *arg)
> +{
> +	volatile u64 start;
> +	volatile int i = 0;
> +	start = get_time_ns();
> +
> +	while (!bthread_should_stop()) {
> +		if (!is_timeout_non_interruptible(start, 225 * MSECOND))
> +			continue;
> +
> +		printf("%s yield #%d\n", __func__, ++i);
> +		start = get_time_ns();
> +	}
> +
> +	return i;
> +}
> +
> +BAREBOX_CMD_HELP_START(bthread)
> +	BAREBOX_CMD_HELP_TEXT("print info about registered barebox threads")
> +	BAREBOX_CMD_HELP_TEXT("")
> +	BAREBOX_CMD_HELP_TEXT("Options:")
> +	BAREBOX_CMD_HELP_OPT ("-i", "Print information about registered bthreads")
> +	BAREBOX_CMD_HELP_OPT ("-t", "measure how many bthreads we currently run in 1s")
> +	BAREBOX_CMD_HELP_OPT ("-c", "count maximum context switches in 1s")
> +	BAREBOX_CMD_HELP_OPT ("-v", "verify correct bthread operation")
> +	BAREBOX_CMD_HELP_END
> +
> +static int do_bthread(int argc, char *argv[])
> +{
> +	struct bthread *bthread = NULL;
> +	int ret, opt;
> +	int yields;
> +
> +	while ((opt = getopt(argc, argv, "itcv")) > 0) {
> +		switch (opt) {
> +		case 'i':
> +			bthread_info();
> +			return 0;
> +		case 'c':
> +			yields = bthread_isolated_time();
> +			printf("%d bthread context switches possible in 1s\n", yields);
> +			break;
> +		case 'v':
> +			bthread = bthread_create(bthread_printer, NULL, "bthread");
> +			if (!bthread)
> +				return -ENOMEM;
> +
> +			bthread_wake(bthread);
> +
> +			/* fallthrough */
> +		case 't':
> +			yields = bthread_time();
> +			printf("%d bthread yield calls in 1s\n", yields);
> +		}
> +
> +		if (bthread) {
> +			ret = bthread_stop(bthread);
> +			bthread_free(bthread);
> +
> +			if (ret != 4 || yields < ret)
> +				return COMMAND_ERROR;
> +		}
> +
> +		return 0;
> +	}

Could you separate the option parsing from the functionality? The way it
currently is is rather hard to extend.

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

[PATCH] fixup! common: introduce bthreads, co-operative barebox threads

[Ahmad Fatoum]

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 commands/bthread.c | 142 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 142 insertions(+)
 create mode 100644 commands/bthread.c

diff --git a/commands/bthread.c b/commands/bthread.c
new file mode 100644
index 000000000000..1fd782f03f43
--- /dev/null
+++ b/commands/bthread.c
@@ -0,0 +1,142 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2021 Ahmad Fatoum, Pengutronix
+ */
+
+#include <bthread.h>
+#include <errno.h>
+#include <malloc.h>
+#include <stdio.h>
+#include <command.h>
+#include <getopt.h>
+#include <clock.h>
+
+static int bthread_time(void)
+{
+	uint64_t start = get_time_ns();
+	int i = 0;
+
+	/*
+	 * How many background tasks can we have in one second?
+	 *
+	 * A low number here may point to problems with bthreads taking too
+	 * much time.
+	 */
+	while (!is_timeout(start, SECOND))
+		i++;
+
+	return i;
+}
+
+static int bthread_infinite(void *data)
+{
+	while (!bthread_should_stop())
+		;
+
+	return 0;
+}
+
+static int bthread_isolated_time(void)
+{
+	uint64_t start = get_time_ns();
+	struct bthread *bthread;
+	int i = 0;
+
+	bthread = bthread_create(bthread_infinite, NULL, "infinite");
+	if (!bthread)
+		return -ENOMEM;
+
+	bthread_wake(bthread);
+
+	/*
+	 * How many context switches can we do in one second?
+	 *
+	 * A low number here may point to problems with bthreads taking too
+	 * much time.
+	 */
+	while (!is_timeout_non_interruptible(start, SECOND)) {
+		bthread_schedule(bthread);
+		i += 2;
+	}
+
+	bthread_stop(bthread);
+	bthread_free(bthread);
+
+	return i;
+}
+
+static int bthread_printer(void *arg)
+{
+	volatile u64 start;
+	volatile int i = 0;
+	start = get_time_ns();
+
+	while (!bthread_should_stop()) {
+		if (!is_timeout_non_interruptible(start, 225 * MSECOND))
+			continue;
+
+		printf("%s yield #%d\n", __func__, ++i);
+		start = get_time_ns();
+	}
+
+	return i;
+}
+
+BAREBOX_CMD_HELP_START(bthread)
+	BAREBOX_CMD_HELP_TEXT("print info about registered barebox threads")
+	BAREBOX_CMD_HELP_TEXT("")
+	BAREBOX_CMD_HELP_TEXT("Options:")
+	BAREBOX_CMD_HELP_OPT ("-i", "Print information about registered bthreads")
+	BAREBOX_CMD_HELP_OPT ("-t", "measure how many bthreads we currently run in 1s")
+	BAREBOX_CMD_HELP_OPT ("-c", "count maximum context switches in 1s")
+	BAREBOX_CMD_HELP_OPT ("-v", "verify correct bthread operation")
+	BAREBOX_CMD_HELP_END
+
+static int do_bthread(int argc, char *argv[])
+{
+	struct bthread *bthread = NULL;
+	int ret, opt;
+	int yields;
+
+	while ((opt = getopt(argc, argv, "itcv")) > 0) {
+		switch (opt) {
+		case 'i':
+			bthread_info();
+			return 0;
+		case 'c':
+			yields = bthread_isolated_time();
+			printf("%d bthread context switches possible in 1s\n", yields);
+			break;
+		case 'v':
+			bthread = bthread_create(bthread_printer, NULL, "bthread");
+			if (!bthread)
+				return -ENOMEM;
+
+			bthread_wake(bthread);
+
+			/* fallthrough */
+		case 't':
+			yields = bthread_time();
+			printf("%d bthread yield calls in 1s\n", yields);
+		}
+
+		if (bthread) {
+			ret = bthread_stop(bthread);
+			bthread_free(bthread);
+
+			if (ret != 4 || yields < ret)
+				return COMMAND_ERROR;
+		}
+
+		return 0;
+	}
+
+	return COMMAND_ERROR_USAGE;
+}
+
+BAREBOX_CMD_START(bthread)
+	.cmd = do_bthread,
+	BAREBOX_CMD_DESC("print info about registered bthreads")
+	BAREBOX_CMD_GROUP(CMD_GRP_MISC)
+	BAREBOX_CMD_HELP(cmd_bthread_help)
+BAREBOX_CMD_END
-- 
2.29.2


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

[PATCH] fixup! common: introduce bthreads, co-operative barebox threads

[Ahmad Fatoum]

Fix copy-paste left-overs and an instance of Denglish.

Suggested-by: Peter Korsgaard <peter@korsgaard.com>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 Documentation/devel/background-execution.rst | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/devel/background-execution.rst b/Documentation/devel/background-execution.rst
index 2fd70934d0b2..b9daaa135ccb 100644
--- a/Documentation/devel/background-execution.rst
+++ b/Documentation/devel/background-execution.rst
@@ -88,15 +88,15 @@ bthreads.
 Unlike pollers, which bthreads are replacing, bthreads are allowed
 access to virtual filesystem. The macro ``assert_command_context()`` is added
 to entry points of the VFS to have the thread yield until it may execute in
-in the correct context. The poller interface is declared in
+in the correct context. The bthread interface is declared in
 ``include/bthread.h``.  ``bthread_create()`` is used to allocate a bthread
-control block along with its stack. ``bthread_wake()`` can be used to hang
+control block along with its stack. ``bthread_wake()`` can be used to add
 it into the run queue. From this moment on and until the thread terminates,
 the thread will be switched to regularly as long as someone calls
 ``is_timeout()``. bthreads are allowed to call ``is_timeout()``, which will
 arrange for other threads to execute.
 
-barebox threads replace previous the previous pollers and workqueues. Poller
+barebox threads replace the previous infrastructure, pollers and workqueues. Poller
 like behavior can be easily achieved by looping and yielding on every
 iteration. There's ``bthread_should_stop()``, which can be used as condition
 for continuing the loop. Workqueues can be replaced along the same line. On
-- 
2.29.2


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