Two simple patches here resulting from using greybus on gbsim and developing support for async. I found a bug in the user-space tool and while doing that decided to update the kernel thread to be better behaved when waiting for test completions.
Bryan O'Donoghue (2): staging: greybus: loopback: use gb_loopback_async_wait_all don't spin staging: greybus: loopback_test: Fix race preventing test completion
drivers/staging/greybus/loopback.c | 12 ++++++++++++ drivers/staging/greybus/tools/loopback_test.c | 10 +--------- 2 files changed, 13 insertions(+), 9 deletions(-)
Currently the greybus-loopback thread logic spins around waiting for send_count == iteration_max which on real hardware doesn't make a difference to us but in simulation is excruciatingly slow, anti-social and bad manners. Use the existing gb_loopback_async_wait_all() function to gate continuing when the send_count == iteration_max and go to sleep until there's something worthwhile to-do.
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie --- drivers/staging/greybus/loopback.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c index 7882306..d6302ef 100644 --- a/drivers/staging/greybus/loopback.c +++ b/drivers/staging/greybus/loopback.c @@ -1008,11 +1008,23 @@ static int gb_loopback_fn(void *data)
/* Optionally terminate */ if (gb->send_count == gb->iteration_max) { + mutex_unlock(&gb->mutex); + + /* Wait for synchronous and asynchronus completion */ + gb_loopback_async_wait_all(gb); + + /* Mark complete unless user-space has poked us */ + mutex_lock(&gb->mutex); if (gb->iteration_count == gb->iteration_max) { gb->type = 0; gb->send_count = 0; sysfs_notify(&gb->dev->kobj, NULL, "iteration_count"); + dev_dbg(&gb->connection->bundle->dev, + "load test complete\n"); + } else { + dev_dbg(&gb->connection->bundle->dev, + "continuing on with new test set\n"); } mutex_unlock(&gb->mutex); continue;
On Tue, Dec 20, 2016 at 01:12:08AM +0000, Bryan O'Donoghue wrote:
Currently the greybus-loopback thread logic spins around waiting for send_count == iteration_max which on real hardware doesn't make a difference to us but in simulation is excruciatingly slow, anti-social and bad manners. Use the existing gb_loopback_async_wait_all() function to gate continuing when the send_count == iteration_max and go to sleep until there's something worthwhile to-do.
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie
You forgot to CC me and Alex.
drivers/staging/greybus/loopback.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c index 7882306..d6302ef 100644 --- a/drivers/staging/greybus/loopback.c +++ b/drivers/staging/greybus/loopback.c @@ -1008,11 +1008,23 @@ static int gb_loopback_fn(void *data) /* Optionally terminate */ if (gb->send_count == gb->iteration_max) {
mutex_unlock(&gb->mutex);/* Wait for synchronous and asynchronus completion */gb_loopback_async_wait_all(gb);
This introduces a non-interruptible wait here, which should be ok given that all outstanding operations will be cancelled by core as part of connection disable before stopping the kthread during disconnect.
But this driver is full of such subtleties and really needs a clean up (or rewrite).
/* Mark complete unless user-space has poked us */mutex_lock(&gb->mutex); if (gb->iteration_count == gb->iteration_max) { gb->type = 0; gb->send_count = 0; sysfs_notify(&gb->dev->kobj, NULL, "iteration_count");dev_dbg(&gb->connection->bundle->dev,
You have a pointer to the bundle you should use here and below.
"load test complete\n");} else {dev_dbg(&gb->connection->bundle->dev,"continuing on with new test set\n");
The fact that user-space can reset test counters and change parameters while a test is running is really another bug.
} mutex_unlock(&gb->mutex); continue;
Johan
On 20/12/16 10:28, Johan Hovold wrote:
On Tue, Dec 20, 2016 at 01:12:08AM +0000, Bryan O'Donoghue wrote:
Currently the greybus-loopback thread logic spins around waiting for send_count == iteration_max which on real hardware doesn't make a difference to us but in simulation is excruciatingly slow, anti-social and bad manners. Use the existing gb_loopback_async_wait_all() function to gate continuing when the send_count == iteration_max and go to sleep until there's something worthwhile to-do.
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie
You forgot to CC me and Alex.
drivers/staging/greybus/loopback.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c index 7882306..d6302ef 100644 --- a/drivers/staging/greybus/loopback.c +++ b/drivers/staging/greybus/loopback.c @@ -1008,11 +1008,23 @@ static int gb_loopback_fn(void *data) /* Optionally terminate */ if (gb->send_count == gb->iteration_max) {
mutex_unlock(&gb->mutex);/* Wait for synchronous and asynchronus completion */gb_loopback_async_wait_all(gb);This introduces a non-interruptible wait here, which should be ok given that all outstanding operations will be cancelled by core as part of connection disable before stopping the kthread during disconnect.
But this driver is full of such subtleties and really needs a clean up (or rewrite).
A rewrite is on the bucket list at some point.
/* Mark complete unless user-space has poked us */mutex_lock(&gb->mutex); if (gb->iteration_count == gb->iteration_max) { gb->type = 0; gb->send_count = 0; sysfs_notify(&gb->dev->kobj, NULL, "iteration_count");dev_dbg(&gb->connection->bundle->dev,You have a pointer to the bundle you should use here and below.
Will do.
"load test complete\n");} else {dev_dbg(&gb->connection->bundle->dev,"continuing on with new test set\n");The fact that user-space can reset test counters and change parameters while a test is running is really another bug.
Agreed will be done on the rewrite.
--- bod
commit 9250c0ee2626 ("greybus: Loopback_test: use poll instead of inotify") changes the flow of determining when to break out of a loop polling for loopback test completion.
The clause is_complete() which determines if all tests are complete - as used is subject to a race condition where one of the tests has completed but at least one other test has not. On real hardware this typically doesn't present itself however in gbsim - which is a lot slower due in-part to a panopoly of printouts - we see that running a loopback test to more than one Interface in gbsim will fail in all instances printing out "Iteration count did not finish".
We don't even need to have the poll() loop timeout tests in since the kernel threads themselves should eventually complete - and if not it's a kernel bug. A user of the tool can still terminate tests by doing a ctrl-c.
This patch fixes the race by ensuring the poll() loop waits for all active Interfaces to complete - as opposed to the first active Interface before breaking the loop.
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie --- drivers/staging/greybus/tools/loopback_test.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/staging/greybus/tools/loopback_test.c b/drivers/staging/greybus/tools/loopback_test.c index f7f4cd6..f053d4fe 100644 --- a/drivers/staging/greybus/tools/loopback_test.c +++ b/drivers/staging/greybus/tools/loopback_test.c @@ -747,7 +747,7 @@ static int wait_for_complete(struct loopback_test *t) if (t->poll_timeout.tv_sec != 0) ts = &t->poll_timeout;
- while (1) { + while (!is_complete(t)) {
ret = ppoll(t->fds, t->poll_count, ts, &mask_old); if (ret <= 0) { @@ -763,14 +763,6 @@ static int wait_for_complete(struct loopback_test *t) number_of_events++; } } - - if (number_of_events == t->poll_count) - break; - } - - if (!is_complete(t)) { - fprintf(stderr, "Iteration count did not finish!\n"); - return -1; }
return 0;
-
Bryan O'Donoghue -
Johan Hovold