Follow-up commit will replace blocking bthread_stop with non-blocking bthread_cancel. Prepare for this by dropping exit codes. This is not much of a loss, because most users of bthreads will only call bthread_stop at cleanup time. bthread command is an exception, so have it take manual care of passing around exit codes. As we touch the bthread_stop prototype anyway, rename it to __bthread_stop. This will make it clearer in the future, that it's not meant for driver use. Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- commands/bthread.c | 48 ++++++++++++++++++----------- common/bthread.c | 20 ++++++------ drivers/usb/gadget/f_mass_storage.c | 12 +++----- include/bthread.h | 5 +-- 4 files changed, 47 insertions(+), 38 deletions(-) diff --git a/commands/bthread.c b/commands/bthread.c index 0c9c221884e8..446fe9c37ac3 100644 --- a/commands/bthread.c +++ b/commands/bthread.c @@ -33,12 +33,10 @@ static int bthread_time(void) return i; } -static int bthread_infinite(void *data) +static void bthread_infinite(void *data) { while (!bthread_should_stop()) ; - - return 0; } static int bthread_isolated_time(void) @@ -62,14 +60,20 @@ static int bthread_isolated_time(void) i += 2; } - bthread_stop(bthread); + __bthread_stop(bthread); bthread_free(bthread); return i; } -static int bthread_printer(void *arg) +struct arg { + unsigned long in; + long out; +}; + +static void bthread_printer(void *_arg) { + struct arg *arg = _arg; volatile u64 start; volatile unsigned long i = 0; start = get_time_ns(); @@ -78,28 +82,30 @@ static int bthread_printer(void *arg) if (!is_timeout_non_interruptible(start, 225 * MSECOND)) continue; - if ((unsigned long)arg == i++) + if (arg->in == i++) printf("%s yield #%lu\n", bthread_name(current), i); start = get_time_ns(); } - return i; + arg->out = i; } static int yields; -static int bthread_spawner(void *arg) +static void bthread_spawner(void *_spawner_arg) { + struct arg *arg, *spawner_arg = _spawner_arg; struct bthread *bthread[4]; volatile u64 start; volatile unsigned long i = 0; int ret = 0; - int ecode; start = get_time_ns(); for (i = 0; i < ARRAY_SIZE(bthread); i++) { - bthread[i] = bthread_run(bthread_printer, (void *)(long)i, + arg = malloc(sizeof(*arg)); + arg->in = i; + bthread[i] = bthread_run(bthread_printer, arg, "%s-bthread%u", bthread_name(current), i+1); if (!bthread[i]) { ret = -ENOMEM; @@ -112,14 +118,16 @@ static int bthread_spawner(void *arg) cleanup: while (i--) { - ecode = bthread_stop(bthread[i]); + arg = bthread_data(bthread[i]); + __bthread_stop(bthread[i]); bthread_free(bthread[i]); - if (!ret && (ecode != 4 || yields < ecode)) + if (!ret && (arg->out != 4 || yields < arg->out)) ret = -EIO; + free(arg); } - return ret; + spawner_arg->out = ret; } struct spawn { @@ -132,8 +140,9 @@ static int do_bthread(int argc, char *argv[]) LIST_HEAD(spawners); struct spawn *spawner, *tmp; int ret = 0; - int ecode, opt, i = 0; + int opt, i = 0; bool time = false; + struct arg *arg; while ((opt = getopt(argc, argv, "itcv")) > 0) { switch (opt) { @@ -146,7 +155,8 @@ static int do_bthread(int argc, char *argv[]) break; case 'v': spawner = xzalloc(sizeof(*spawner)); - spawner->bthread = bthread_run(bthread_spawner, NULL, + arg = malloc(sizeof(*arg)); + spawner->bthread = bthread_run(bthread_spawner, arg, "spawner%u", ++i); if (!spawner->bthread) { free(spawner); @@ -172,10 +182,12 @@ static int do_bthread(int argc, char *argv[]) cleanup: list_for_each_entry_safe(spawner, tmp, &spawners, list) { - ecode = bthread_stop(spawner->bthread); + arg = bthread_data(spawner->bthread); + __bthread_stop(spawner->bthread); bthread_free(spawner->bthread); - if (!ret && ecode) - ret = ecode; + if (!ret && arg->out) + ret = arg->out; + free(arg); free(spawner); } diff --git a/common/bthread.c b/common/bthread.c index 80651344da64..48248dfad41a 100644 --- a/common/bthread.c +++ b/common/bthread.c @@ -20,11 +20,8 @@ #endif static struct bthread { - int (*threadfn)(void *); - union { - void *data; - int ret; - }; + void (*threadfn)(void *); + void *data; char *name; jmp_buf jmp_buf; void *stack; @@ -54,7 +51,7 @@ static void __noreturn bthread_trampoline(void) finish_switch_fiber(current); bthread_reschedule(); - current->ret = current->threadfn(current->data); + current->threadfn(current->data); bthread_suspend(current); current->has_stopped = true; @@ -81,7 +78,7 @@ const char *bthread_name(struct bthread *bthread) return bthread->name; } -struct bthread *bthread_create(int (*threadfn)(void *), void *data, +struct bthread *bthread_create(void (*threadfn)(void *), void *data, const char *namefmt, ...) { struct bthread *bthread; @@ -117,6 +114,11 @@ err: return NULL; } +void *bthread_data(struct bthread *bthread) +{ + return bthread->data; +} + void bthread_wake(struct bthread *bthread) { if (bthread->awake) @@ -133,14 +135,12 @@ void bthread_suspend(struct bthread *bthread) list_del(&bthread->list); } -int bthread_stop(struct bthread *bthread) +void __bthread_stop(struct bthread *bthread) { bthread->should_stop = true; while (!bthread->has_stopped) bthread_reschedule(); - - return bthread->ret; } int bthread_should_stop(void) diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c index 4894f91fa6ef..0033a95f68f3 100644 --- a/drivers/usb/gadget/f_mass_storage.c +++ b/drivers/usb/gadget/f_mass_storage.c @@ -2335,7 +2335,7 @@ static void handle_exception(struct fsg_common *common) /*-------------------------------------------------------------------------*/ -static int fsg_main_thread(void *common_) +static void fsg_main_thread(void *common_) { struct fsg_common *common = common_; int ret = 0; @@ -2374,12 +2374,8 @@ static int fsg_main_thread(void *common_) common->state = FSG_STATE_IDLE; } - if (ret && ret != -ERESTARTSYS) { + if (ret && ret != -ERESTARTSYS) pr_warn("%s: error %pe\n", __func__, ERR_PTR(ret)); - return ret; - } - - return 0; } static void fsg_common_release(struct fsg_common *common); @@ -2541,7 +2537,7 @@ static void fsg_common_release(struct fsg_common *common) /* If the thread isn't already dead, tell it to exit now */ if (common->state != FSG_STATE_TERMINATED) { raise_exception(common, FSG_STATE_EXIT); - bthread_stop(thread_task); + __bthread_stop(thread_task); bthread_free(thread_task); } @@ -2568,7 +2564,7 @@ static void fsg_unbind(struct usb_configuration *c, struct usb_function *f) fsg->common->new_fsg = NULL; raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE); - bthread_stop(thread_task); + __bthread_stop(thread_task); while (common->fsg == fsg) bthread_reschedule(); } diff --git a/include/bthread.h b/include/bthread.h index e3871fb11555..407aa830a835 100644 --- a/include/bthread.h +++ b/include/bthread.h @@ -12,14 +12,15 @@ struct bthread; extern struct bthread *current; -struct bthread *bthread_create(int (*threadfn)(void *), void *data, const char *namefmt, ...); +struct bthread *bthread_create(void (*threadfn)(void *), void *data, const char *namefmt, ...); void bthread_free(struct bthread *bthread); void bthread_schedule(struct bthread *); void bthread_wake(struct bthread *bthread); void bthread_suspend(struct bthread *bthread); int bthread_should_stop(void); -int bthread_stop(struct bthread *bthread); +void __bthread_stop(struct bthread *bthread); +void *bthread_data(struct bthread *bthread); void bthread_info(void); const char *bthread_name(struct bthread *bthread); bool bthread_is_main(struct bthread *bthread); -- 2.30.2 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
When bthread were first merged, they could be scheduled in any context and bthread_stop could just keep rescheduling until the bthread in question exits after which it would return the exit code. Now that bthreads are only scheduled in command context, bthread_stop also can only be scheduled in command context, making it much less useful and easier to shoot yourself in the foot with. Avoid this by introducing a bthread_cancel function instead that will asynchronously terminate the thread. For most purposes that should be fine, because bthread_stop is used to synchronize cleanup and we can move the cleanup into the thread instead. The only exception is the bthread command, which relies on being able to wait on bthreads to complete. For these __bthread_stop remains available, but should not be used in new code. This fixes a hang that is encountered when the usb mass storage gadget unbind is called from a poller leading barebox to wait indefinitely. Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- commands/bthread.c | 3 -- common/bthread.c | 44 ++++++++++++++++++------- drivers/usb/gadget/f_mass_storage.c | 51 +++++++++++++---------------- include/bthread.h | 2 +- 4 files changed, 56 insertions(+), 44 deletions(-) diff --git a/commands/bthread.c b/commands/bthread.c index 446fe9c37ac3..ce3db2d3d292 100644 --- a/commands/bthread.c +++ b/commands/bthread.c @@ -61,7 +61,6 @@ static int bthread_isolated_time(void) } __bthread_stop(bthread); - bthread_free(bthread); return i; } @@ -120,7 +119,6 @@ cleanup: while (i--) { arg = bthread_data(bthread[i]); __bthread_stop(bthread[i]); - bthread_free(bthread[i]); if (!ret && (arg->out != 4 || yields < arg->out)) ret = -EIO; @@ -184,7 +182,6 @@ cleanup: list_for_each_entry_safe(spawner, tmp, &spawners, list) { arg = bthread_data(spawner->bthread); __bthread_stop(spawner->bthread); - bthread_free(spawner->bthread); if (!ret && arg->out) ret = arg->out; free(arg); diff --git a/common/bthread.c b/common/bthread.c index 48248dfad41a..46e6987149bb 100644 --- a/common/bthread.c +++ b/common/bthread.c @@ -32,10 +32,12 @@ static struct bthread { #endif u8 awake :1; u8 should_stop :1; + u8 should_clean :1; u8 has_stopped :1; } main_thread = { .list = LIST_HEAD_INIT(main_thread.list), .name = "main", + .awake = true, }; struct bthread *current = &main_thread; @@ -66,7 +68,7 @@ bool bthread_is_main(struct bthread *bthread) return bthread == &main_thread; } -void bthread_free(struct bthread *bthread) +static void bthread_free(struct bthread *bthread) { free(bthread->stack); free(bthread->name); @@ -104,6 +106,8 @@ struct bthread *bthread_create(void (*threadfn)(void *), void *data, if (len < 0) goto err; + list_add_tail(&bthread->list, ¤t->list); + /* set up bthread context with the new stack */ initjmp(bthread->jmp_buf, bthread_trampoline, bthread->stack + CONFIG_STACK_SIZE); @@ -121,26 +125,31 @@ void *bthread_data(struct bthread *bthread) void bthread_wake(struct bthread *bthread) { - if (bthread->awake) - return; - list_add_tail(&bthread->list, ¤t->list); bthread->awake = true; } void bthread_suspend(struct bthread *bthread) { - if (!bthread->awake) - return; bthread->awake = false; - list_del(&bthread->list); +} + +void bthread_cancel(struct bthread *bthread) +{ + bthread->should_stop = true; + bthread->should_clean = true; } void __bthread_stop(struct bthread *bthread) { bthread->should_stop = true; + pr_debug("waiting for %s to stop\n", bthread->name); + while (!bthread->has_stopped) bthread_reschedule(); + + list_del(&bthread->list); + bthread_free(bthread); } int bthread_should_stop(void) @@ -163,10 +172,23 @@ void bthread_info(void) void bthread_reschedule(void) { - struct bthread *next = list_next_entry(current, list); - if (current != next) - pr_debug("switch %s -> %s\n", current->name, next->name); - bthread_schedule(next); + struct bthread *next, *tmp; + + if (current == list_next_entry(current, list)) + return; + + list_for_each_entry_safe(next, tmp, ¤t->list, list) { + if (next->awake) { + pr_debug("switch %s -> %s\n", current->name, next->name); + bthread_schedule(next); + return; + } + if (next->has_stopped && next->should_clean) { + pr_debug("deleting %s\n", next->name); + list_del(&next->list); + bthread_free(next); + } + } } void bthread_schedule(struct bthread *to) diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c index 0033a95f68f3..753042125d14 100644 --- a/drivers/usb/gadget/f_mass_storage.c +++ b/drivers/usb/gadget/f_mass_storage.c @@ -2335,9 +2335,12 @@ static void handle_exception(struct fsg_common *common) /*-------------------------------------------------------------------------*/ -static void fsg_main_thread(void *common_) +static void fsg_main_thread(void *fsg_) { - struct fsg_common *common = common_; + struct fsg_dev *fsg = fsg_; + struct fsg_common *common = fsg->common; + struct fsg_buffhd *bh; + unsigned i; int ret = 0; /* The main loop */ @@ -2376,6 +2379,21 @@ static void fsg_main_thread(void *common_) if (ret && ret != -ERESTARTSYS) pr_warn("%s: error %pe\n", __func__, ERR_PTR(ret)); + + usb_free_all_descriptors(&fsg->function); + + for (i = 0; i < ums_count; i++) + close(ums[i].fd); + + bh = common->buffhds; + i = FSG_NUM_BUFFERS; + + do { + dma_free(bh->buf); + } while (++bh, --i); + + ums_count = 0; + ums_files = NULL; } static void fsg_common_release(struct fsg_common *common); @@ -2409,7 +2427,7 @@ static int fsg_common_init(struct fsg_common *common, common->ep0 = gadget->ep0; common->ep0req = cdev->req; - thread_task = bthread_run(fsg_main_thread, common, "mass-storage-gadget"); + thread_task = bthread_run(fsg_main_thread, common->fsg, "mass-storage-gadget"); if (IS_ERR(thread_task)) return PTR_ERR(thread_task); @@ -2531,52 +2549,27 @@ close: static void fsg_common_release(struct fsg_common *common) { - struct fsg_buffhd *bh; - unsigned i; - /* If the thread isn't already dead, tell it to exit now */ if (common->state != FSG_STATE_TERMINATED) { raise_exception(common, FSG_STATE_EXIT); - __bthread_stop(thread_task); - bthread_free(thread_task); } - bh = common->buffhds; - i = FSG_NUM_BUFFERS; - - do { - dma_free(bh->buf); - } while (++bh, --i); - - ums_count = 0; + bthread_cancel(thread_task); } static void fsg_unbind(struct usb_configuration *c, struct usb_function *f) { struct fsg_dev *fsg = fsg_from_func(f); - struct fsg_common *common = fsg->common; - int i; DBG(fsg, "unbind\n"); if (fsg->common->fsg == fsg) { fsg->common->new_fsg = NULL; raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE); - - __bthread_stop(thread_task); - while (common->fsg == fsg) - bthread_reschedule(); } - usb_free_all_descriptors(&fsg->function); - - for (i = 0; i < ums_count; i++) - close(ums[i].fd); - fsg_common_release(fsg->common); - - ums_files = NULL; } static int fsg_bind(struct usb_configuration *c, struct usb_function *f) diff --git a/include/bthread.h b/include/bthread.h index 407aa830a835..4441b53696fb 100644 --- a/include/bthread.h +++ b/include/bthread.h @@ -13,7 +13,7 @@ struct bthread; extern struct bthread *current; struct bthread *bthread_create(void (*threadfn)(void *), void *data, const char *namefmt, ...); -void bthread_free(struct bthread *bthread); +void bthread_cancel(struct bthread *bthread); void bthread_schedule(struct bthread *); void bthread_wake(struct bthread *bthread); -- 2.30.2 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
With few bthread users currently, it can be useful to spin up some threads to test stuff. Add a new option to add one more thread into the mix and another to remove one of these dummy threads. Unlike the existing -v option that can also be stacked, these threads remain active after command termination unless explicitly removed. Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- commands/bthread.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/commands/bthread.c b/commands/bthread.c index ce3db2d3d292..aaade46e9206 100644 --- a/commands/bthread.c +++ b/commands/bthread.c @@ -135,6 +135,8 @@ struct spawn { static int do_bthread(int argc, char *argv[]) { + static int dummynr; + static LIST_HEAD(dummies); LIST_HEAD(spawners); struct spawn *spawner, *tmp; int ret = 0; @@ -142,8 +144,29 @@ static int do_bthread(int argc, char *argv[]) bool time = false; struct arg *arg; - while ((opt = getopt(argc, argv, "itcv")) > 0) { + while ((opt = getopt(argc, argv, "aritcv")) > 0) { switch (opt) { + case 'a': + spawner = xzalloc(sizeof(*spawner)); + spawner->bthread = bthread_run(bthread_infinite, NULL, + "dummy%u", dummynr++); + if (!spawner->bthread) { + free(spawner); + ret = -ENOMEM; + goto cleanup; + } + + list_add(&spawner->list, &dummies); + break; + case 'r': + if (dummynr == 0) + return -EINVAL; + spawner = list_first_entry(&dummies, struct spawn, list); + bthread_cancel(spawner->bthread); + list_del(&spawner->list); + free(spawner); + dummynr--; + break; case 'i': bthread_info(); break; @@ -198,6 +221,8 @@ BAREBOX_CMD_HELP_START(bthread) 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 ("-a", "add a dummy bthread") + BAREBOX_CMD_HELP_OPT ("-r", "remove a dummy bthread") BAREBOX_CMD_HELP_OPT ("-v", "verify correct bthread operation") BAREBOX_CMD_HELP_END -- 2.30.2 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
On Mon, Jun 28, 2021 at 09:07:30AM +0200, Ahmad Fatoum wrote: > Follow-up commit will replace blocking bthread_stop with non-blocking > bthread_cancel. Prepare for this by dropping exit codes. This is not > much of a loss, because most users of bthreads will only call > bthread_stop at cleanup time. bthread command is an exception, so have > it take manual care of passing around exit codes. > > As we touch the bthread_stop prototype anyway, rename it to > __bthread_stop. This will make it clearer in the future, that it's not > meant for driver use. > > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > --- > commands/bthread.c | 48 ++++++++++++++++++----------- > common/bthread.c | 20 ++++++------ > drivers/usb/gadget/f_mass_storage.c | 12 +++----- > include/bthread.h | 5 +-- > 4 files changed, 47 insertions(+), 38 deletions(-) Applied, thanks 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