[LTP] [PATCH] cpuset_hotplug_test.sh: Fix a race condition

[LTP] [PATCH] cpuset_hotplug_test.sh: Fix a race condition

[Qais Yousef]

Cpuset performs hotplug updates asynchronously in the kernel. This would
lead to a race window where userspace could read a stale value before
the asynchronous update is performed (via a delayed work).

An attempt to fix the issue in the kernel failed.

https://lore.kernel.org/lkml/20200211141554.24181-1-qais.yousef@arm.com/T/#u

There was a patch to make the update synchronous, but it hit a wall and
was dropped

https://lore.kernel.org/lkml/F0388D99-84D7-453B-9B6B-EEFF0E7BE4CC@lca.pw/

The sleep is not ideal, but the maintainer has pushed back for a fix in
the kernel so far.

Fixes #693.
Signed-off-by: Qais Yousef <qais.yousef@arm.com>
---
 .../cpuset/cpuset_hotplug_test/cpuset_hotplug_test.sh  | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/testcases/kernel/controllers/cpuset/cpuset_hotplug_test/cpuset_hotplug_test.sh b/testcases/kernel/controllers/cpuset/cpuset_hotplug_test/cpuset_hotplug_test.sh
index e973de7b9..1337e0cd6 100755
--- a/testcases/kernel/controllers/cpuset/cpuset_hotplug_test/cpuset_hotplug_test.sh
+++ b/testcases/kernel/controllers/cpuset/cpuset_hotplug_test/cpuset_hotplug_test.sh
@@ -86,6 +86,11 @@ root_cpu_hotplug_test()
 		return 1
 	fi
 
+	# cpuset hotplug is asynchronous operation, we could end up reading a
+	# stale value here. sleep is aweful, but we can't do better.
+	# See https://github.com/linux-test-project/ltp/issues/693
+	sleep 1
+
 	root_cpus="`cat $CPUSET/cpuset.cpus`"
 
 	task_cpus="`cat /proc/$tst_pid/status | grep Cpus_allowed_list`"
@@ -155,6 +160,11 @@ general_cpu_hotplug_test()
 		return 1
 	fi
 
+	# cpuset hotplug is asynchronous operation, we could end up reading a
+	# stale value here. sleep is aweful, but we can't do better.
+	# See https://github.com/linux-test-project/ltp/issues/693
+	sleep 1
+
 	cpus="`cat $path/cpuset.cpus`"
 
 	task_cpus="`cat /proc/$tst_pid/status | grep Cpus_allowed_list`"
-- 
2.17.1

[LTP] [PATCH] cpuset_hotplug_test.sh: Fix a race condition

[Petr Vorel]

Hi Qais,

thanks for your patch.
BTW this has already been reviewed and tested by Huacai Chen [1].
LGTM, although I'd prefer to detect with with polling, isn't it possible? [1].

Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/pull/706#issuecomment-658492329
[2] https://people.kernel.org/metan/why-sleep-is-almost-never-acceptable-in-tests

[LTP] [PATCH] cpuset_hotplug_test.sh: Fix a race condition

[Qais Yousef]

Hi Petr


On 07/16/20 08:07, Petr Vorel wrote:
> Hi Qais,
> 
> thanks for your patch.

Thanks for having a look.

> BTW this has already been reviewed and tested by Huacai Chen [1].
> LGTM, although I'd prefer to detect with with polling, isn't it possible? [1].

FWIW I did try to avoid the sleep [1].

Were you thinking of something like that (pseudo code)?

	for i in $(seq 3)
	do
		sleep 1
		verify()
		if [ sucess ]; then
			break;
		fi
	done

Or you had something more sophisticated in mind?

Thanks

--
Qais Yousef

[1] https://lore.kernel.org/lkml/20200407133520.GI162390@mtj.duckdns.org/

[LTP] [PATCH] cpuset_hotplug_test.sh: Fix a race condition

[Petr Vorel]

Hi Qais,

> > BTW this has already been reviewed and tested by Huacai Chen [1].
> > LGTM, although I'd prefer to detect with with polling, isn't it possible? [1].

> FWIW I did try to avoid the sleep [1].
Yes, I know, but that was in kernel code (great you tried to fix the problem in
the kernel). Here I mean avoid blind sleep in the test. Reporting problem and
taking "sleep 1" fix would be most probably just enough. Below are suggestions to
consider before taking your posted fix as is.

> Were you thinking of something like that (pseudo code)?

> 	for i in $(seq 3)
> 	do
> 		sleep 1
> 		verify()
> 		if [ sucess ]; then
> 			break;
> 		fi
> 	done

> Or you had something more sophisticated in mind?
No, certainly not more sophisticated :). You can also use TST_RETRY_FUNC helper
instead of creating loop manually. It sleeps in 1 sec.

NOTE: TST_RETRY_FUNC is a wrapper for TST_RETRY_FN_EXP_BACKOFF, using it you can
define sleep time. Unfortunately current code does not allow to loop over less
than 1s, maybe it'd be worth for some cases, where the event is really fast.

@Metan, @Li: would it be worth to change TST_RETRY_FUNC (in both C and shell) to
use ms instead of s?

Also, we have tst_sleep helper, which supports also ms and us (but using
TST_RETRY_FUNC is IMHO better).

I'd have to look more deeply into the test to figure out the verifier.

Kind regards,
Petr

[LTP] [PATCH] cpuset_hotplug_test.sh: Fix a race condition

[Qais Yousef]

On 07/16/20 11:57, Petr Vorel wrote:
> Hi Qais,
> 
> > > BTW this has already been reviewed and tested by Huacai Chen [1].
> > > LGTM, although I'd prefer to detect with with polling, isn't it possible? [1].
> 
> > FWIW I did try to avoid the sleep [1].
> Yes, I know, but that was in kernel code (great you tried to fix the problem in
> the kernel). Here I mean avoid blind sleep in the test. Reporting problem and
> taking "sleep 1" fix would be most probably just enough. Below are suggestions to
> consider before taking your posted fix as is.

We're probably talking about the same thing. But to clarify further, my
original fix in kernel had no sleep and would force a wait by flushing the
workqueue when the userspace does the read

https://lore.kernel.org/lkml/20200211141554.24181-1-qais.yousef@arm.com/

But the maintainer was suggesting to do the sleep in the test instead. Which I
didn't like and I didn't understand why my fix isn't good.

Anyways. Looking forward.. :)

> 
> > Were you thinking of something like that (pseudo code)?
> 
> > 	for i in $(seq 3)
> > 	do
> > 		sleep 1
> > 		verify()
> > 		if [ sucess ]; then
> > 			break;
> > 		fi
> > 	done
> 
> > Or you had something more sophisticated in mind?
> No, certainly not more sophisticated :). You can also use TST_RETRY_FUNC helper
> instead of creating loop manually. It sleeps in 1 sec.
> 
> NOTE: TST_RETRY_FUNC is a wrapper for TST_RETRY_FN_EXP_BACKOFF, using it you can
> define sleep time. Unfortunately current code does not allow to loop over less
> than 1s, maybe it'd be worth for some cases, where the event is really fast.
> 
> @Metan, @Li: would it be worth to change TST_RETRY_FUNC (in both C and shell) to
> use ms instead of s?
> 
> Also, we have tst_sleep helper, which supports also ms and us (but using
> TST_RETRY_FUNC is IMHO better).
> 
> I'd have to look more deeply into the test to figure out the verifier.

I'd be happy to consider this as long as it won't steal my day. I'm just
a by-passer developer and my knowledge about the code base is minimal :)

Let me know what would be the preferred approach.

Thanks!

--
Qais Yousef

[LTP] [PATCH] cpuset_hotplug_test.sh: Fix a race condition

[Petr Vorel]

Hi Qais,

> > > > BTW this has already been reviewed and tested by Huacai Chen [1].
> > > > LGTM, although I'd prefer to detect with with polling, isn't it possible? [1].

> > > FWIW I did try to avoid the sleep [1].
> > Yes, I know, but that was in kernel code (great you tried to fix the problem in
> > the kernel). Here I mean avoid blind sleep in the test. Reporting problem and
> > taking "sleep 1" fix would be most probably just enough. Below are suggestions to
> > consider before taking your posted fix as is.

> We're probably talking about the same thing. But to clarify further, my
> original fix in kernel had no sleep and would force a wait by flushing the
> workqueue when the userspace does the read

> https://lore.kernel.org/lkml/20200211141554.24181-1-qais.yousef@arm.com/

> But the maintainer was suggesting to do the sleep in the test instead. Which I
> didn't like and I didn't understand why my fix isn't good.
Maybe ask to reconsider your patch again? But cpuset_wait_for_hotplug() might be
needed on more places, what a shame that turning code into synchronous didn't
work :(.

> Anyways. Looking forward.. :)


> > > Were you thinking of something like that (pseudo code)?

> > > 	for i in $(seq 3)
> > > 	do
> > > 		sleep 1
> > > 		verify()
> > > 		if [ sucess ]; then
> > > 			break;
> > > 		fi
> > > 	done

> > > Or you had something more sophisticated in mind?
> > No, certainly not more sophisticated :). You can also use TST_RETRY_FUNC helper
> > instead of creating loop manually. It sleeps in 1 sec.

> > NOTE: TST_RETRY_FUNC is a wrapper for TST_RETRY_FN_EXP_BACKOFF, using it you can
> > define sleep time. Unfortunately current code does not allow to loop over less
> > than 1s, maybe it'd be worth for some cases, where the event is really fast.

> > @Metan, @Li: would it be worth to change TST_RETRY_FUNC (in both C and shell) to
> > use ms instead of s?

> > Also, we have tst_sleep helper, which supports also ms and us (but using
> > TST_RETRY_FUNC is IMHO better).

> > I'd have to look more deeply into the test to figure out the verifier.

> I'd be happy to consider this as long as it won't steal my day. I'm just
> a by-passer developer and my knowledge about the code base is minimal :)
Understand. I'm not myself familiar with LTP cpuset tests, nor with the kernel code.

> Let me know what would be the preferred approach.
Just please try to send a patch using TST_RETRY_FUNC (thus you need to figure
out the verifier), or let us know and we either figure that or just simply use
your original patch.

Kind regards,
Petr

[LTP] [PATCH] cpuset_hotplug_test.sh: Fix a race condition

[Qais Yousef]

Hi Petr

On 07/16/20 14:08, Petr Vorel wrote:
> Hi Qais,
> 
> > > > > BTW this has already been reviewed and tested by Huacai Chen [1].
> > > > > LGTM, although I'd prefer to detect with with polling, isn't it possible? [1].
> 
> > > > FWIW I did try to avoid the sleep [1].
> > > Yes, I know, but that was in kernel code (great you tried to fix the problem in
> > > the kernel). Here I mean avoid blind sleep in the test. Reporting problem and
> > > taking "sleep 1" fix would be most probably just enough. Below are suggestions to
> > > consider before taking your posted fix as is.
> 
> > We're probably talking about the same thing. But to clarify further, my
> > original fix in kernel had no sleep and would force a wait by flushing the
> > workqueue when the userspace does the read
> 
> > https://lore.kernel.org/lkml/20200211141554.24181-1-qais.yousef@arm.com/
> 
> > But the maintainer was suggesting to do the sleep in the test instead. Which I
> > didn't like and I didn't understand why my fix isn't good.
> Maybe ask to reconsider your patch again? But cpuset_wait_for_hotplug() might be
> needed on more places, what a shame that turning code into synchronous didn't
> work :(.

Yeah it was a shame. The locking depdency seemed a tricky one too.

> 
> > Anyways. Looking forward.. :)
> 
> 
> > > > Were you thinking of something like that (pseudo code)?
> 
> > > > 	for i in $(seq 3)
> > > > 	do
> > > > 		sleep 1
> > > > 		verify()
> > > > 		if [ sucess ]; then
> > > > 			break;
> > > > 		fi
> > > > 	done
> 
> > > > Or you had something more sophisticated in mind?
> > > No, certainly not more sophisticated :). You can also use TST_RETRY_FUNC helper
> > > instead of creating loop manually. It sleeps in 1 sec.
> 
> > > NOTE: TST_RETRY_FUNC is a wrapper for TST_RETRY_FN_EXP_BACKOFF, using it you can
> > > define sleep time. Unfortunately current code does not allow to loop over less
> > > than 1s, maybe it'd be worth for some cases, where the event is really fast.
> 
> > > @Metan, @Li: would it be worth to change TST_RETRY_FUNC (in both C and shell) to
> > > use ms instead of s?
> 
> > > Also, we have tst_sleep helper, which supports also ms and us (but using
> > > TST_RETRY_FUNC is IMHO better).
> 
> > > I'd have to look more deeply into the test to figure out the verifier.
> 
> > I'd be happy to consider this as long as it won't steal my day. I'm just
> > a by-passer developer and my knowledge about the code base is minimal :)
> Understand. I'm not myself familiar with LTP cpuset tests, nor with the kernel code.
> 
> > Let me know what would be the preferred approach.
> Just please try to send a patch using TST_RETRY_FUNC (thus you need to figure
> out the verifier), or let us know and we either figure that or just simply use
> your original patch.

So I have cooked something quick but I either get

	/root/arm64-ltp/testcases/bin/cpuset_hotplug_test.sh: line 159: TST_RETRY_FUNC: command not found

or

	cpuset_hotplug_test 1 TBROK: Test /root/arm64-ltp/testcases/bin/cpuset_hotplug_test.sh must call tst_run!

Depending whether I impost test.sh or tst_test.sh.

It seems there's a dependency on the overall test construction depending on
what file is imported. And one can import one or the other. It seems
tst_test.sh is the legacy approach?

My WIP patch is attached in case there's somethign obvious to be done here that
I missed.

It looks like the current patch is the simplest thing to do otherwise? :-/

Thanks

--
Qais Yousef
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-fixup.patch
Type: text/x-diff
Size: 6204 bytes
Desc: not available
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200716/ab280809/attachment.patch>

[LTP] [PATCH] cpuset_hotplug_test.sh: Fix a race condition

[Petr Vorel]

Hi Qais,

> > > Let me know what would be the preferred approach.
> > Just please try to send a patch using TST_RETRY_FUNC (thus you need to figure
> > out the verifier), or let us know and we either figure that or just simply use
> > your original patch.

> So I have cooked something quick but I either get

> 	/root/arm64-ltp/testcases/bin/cpuset_hotplug_test.sh: line 159: TST_RETRY_FUNC: command not found
I'm sorry, I have forgotten that the tests haven't been ported to the new API
yet (tst_test.sh). Thus, I'm ok to take the original patch, unless anybody
objects.

Why? Rewriting is going to be bigger task, as shell scripts has interdependencies,
thus turning cpuset_funcs.sh to use new API requires turning the other tests
which use it (it's not acceptable to load both legacy and new API in a test).
Besides new API needs to rename some functions (s/tst_resm/tst_res/, ... that's
easy task), but also works differently, so it's more work that renaming these
functions. Also tests need heavy cleanup (style is not optimal, using full path
like /bin/kill is ugly).

Although this should be done, the bigger problem is that these tests test just
cgroup v1 (and only early implementations, see notes about 2.6.29 in the code),
has other bugs than just one you spotted. There might be similar approach we
took with NUMA tests: shell tests are in maintenance mode and new tests are
written from the scratch in C.

BTW there is some support for cgroup in lib/tst_cgroup.c.

> or

> 	cpuset_hotplug_test 1 TBROK: Test /root/arm64-ltp/testcases/bin/cpuset_hotplug_test.sh must call tst_run!

> Depending whether I impost test.sh or tst_test.sh.

> It seems there's a dependency on the overall test construction depending on
> what file is imported. And one can import one or the other. It seems
> tst_test.sh is the legacy approach?
No, test.sh is the legacy one.
There is a docs for new API (C and shell), if you're interested.
https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines

> My WIP patch is attached in case there's somethign obvious to be done here that
> I missed.

> It looks like the current patch is the simplest thing to do otherwise? :-/
yes :).

Kind regards,
Petr

[LTP] [PATCH] cpuset_hotplug_test.sh: Fix a race condition

[Qais Yousef]

On 07/16/20 16:21, Petr Vorel wrote:
> Hi Qais,
> 
> > > > Let me know what would be the preferred approach.
> > > Just please try to send a patch using TST_RETRY_FUNC (thus you need to figure
> > > out the verifier), or let us know and we either figure that or just simply use
> > > your original patch.
> 
> > So I have cooked something quick but I either get
> 
> > 	/root/arm64-ltp/testcases/bin/cpuset_hotplug_test.sh: line 159: TST_RETRY_FUNC: command not found
> I'm sorry, I have forgotten that the tests haven't been ported to the new API
> yet (tst_test.sh). Thus, I'm ok to take the original patch, unless anybody
> objects.
> 
> Why? Rewriting is going to be bigger task, as shell scripts has interdependencies,
> thus turning cpuset_funcs.sh to use new API requires turning the other tests
> which use it (it's not acceptable to load both legacy and new API in a test).
> Besides new API needs to rename some functions (s/tst_resm/tst_res/, ... that's
> easy task), but also works differently, so it's more work that renaming these
> functions. Also tests need heavy cleanup (style is not optimal, using full path
> like /bin/kill is ugly).
> 
> Although this should be done, the bigger problem is that these tests test just
> cgroup v1 (and only early implementations, see notes about 2.6.29 in the code),
> has other bugs than just one you spotted. There might be similar approach we
> took with NUMA tests: shell tests are in maintenance mode and new tests are
> written from the scratch in C.
> 
> BTW there is some support for cgroup in lib/tst_cgroup.c.

Yeah that's what I thought if the test will need porting to a different
infrastructure. It won't be just this test that will need changing..

> 
> > or
> 
> > 	cpuset_hotplug_test 1 TBROK: Test /root/arm64-ltp/testcases/bin/cpuset_hotplug_test.sh must call tst_run!
> 
> > Depending whether I impost test.sh or tst_test.sh.
> 
> > It seems there's a dependency on the overall test construction depending on
> > what file is imported. And one can import one or the other. It seems
> > tst_test.sh is the legacy approach?
> No, test.sh is the legacy one.
> There is a docs for new API (C and shell), if you're interested.
> https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines

Ah cool thanks.

> 
> > My WIP patch is attached in case there's somethign obvious to be done here that
> > I missed.
> 
> > It looks like the current patch is the simplest thing to do otherwise? :-/
> yes :).

Thanks for your review!

Cheers

--
Qais Yousef

[LTP] [PATCH] cpuset_hotplug_test.sh: Fix a race condition

[Petr Vorel]

Hi Qais,

merged your original patch.
Thanks for your time.

Kind regards,
Petr

[LTP] [PATCH] cpuset_hotplug_test.sh: Fix a race condition

[Qais Yousef]

On 07/16/20 22:56, Petr Vorel wrote:
> Hi Qais,
> 
> merged your original patch.
> Thanks for your time.

Thanks Petr for your responsiveness and helpful remarks. And Huacai for his
reviewed-and-tested-by.

Thanks!

--
Qais Yousef

[LTP] [PATCH] cpuset_hotplug_test.sh: Fix a race condition

[Petr Vorel]

Hi,

> On 07/16/20 22:56, Petr Vorel wrote:
> > Hi Qais,

> > merged your original patch.
> > Thanks for your time.

> Thanks Petr for your responsiveness and helpful remarks. And Huacai for his
> reviewed-and-tested-by.
I'm sorry, Huacai, I've forgotten to include your tags.

Kind regards,
Petr