[LTP] [PATCH v1] connectors/cn_pec: improve reliability

[LTP] [PATCH v1] connectors/cn_pec: improve reliability

[Bogdan Lezhepekov]

Sometimes pec_listener has not enough time to handle all
the generated events.

This patch only returns back the delay that was removed
in a recent patch.

Signed-off-by: Bogdan Lezhepekov <bogdan.lezhepekov@suse.com>
---
 testcases/kernel/connectors/pec/cn_pec.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/testcases/kernel/connectors/pec/cn_pec.sh b/testcases/kernel/connectors/pec/cn_pec.sh
index 9b85a5c81..24b1db761 100755
--- a/testcases/kernel/connectors/pec/cn_pec.sh
+++ b/testcases/kernel/connectors/pec/cn_pec.sh
@@ -77,6 +77,8 @@ test()
 	tst_sleep 100ms
 
 	ROD event_generator -n $num_events -e $event \>gen_$event.log 2\>gen_$event.err
+    # sleep until pec_listener has seen and handled all of the generated events
+    tst_sleep 100ms
 
 	kill -s INT $pid 2> /dev/null
 	wait $pid
-- 
2.32.0

[LTP] [PATCH v1] connectors/cn_pec: improve reliability

[Joerg Vehlow]

Hi,

On 8/5/2021 4:08 PM, Bogdan Lezhepekov via ltp wrote:
> Sometimes pec_listener has not enough time to handle all
> the generated events.
>
> This patch only returns back the delay that was removed
> in a recent patch.
I think I thought about keeping or removing this delay and finally 
decided to remove it for some reason, that I do not remember right now.
Can you tell me the environment where this failed?

Joerg

[LTP] [PATCH v1] connectors/cn_pec: improve reliability

[Petr Vorel]

Hi Bogdan, Joerg,
> Sometimes pec_listener has not enough time to handle all
> the generated events.

> This patch only returns back the delay that was removed
> in a recent patch.

> Signed-off-by: Bogdan Lezhepekov <bogdan.lezhepekov@suse.com>
> ---
>  testcases/kernel/connectors/pec/cn_pec.sh | 2 ++
>  1 file changed, 2 insertions(+)

> diff --git a/testcases/kernel/connectors/pec/cn_pec.sh b/testcases/kernel/connectors/pec/cn_pec.sh
> index 9b85a5c81..24b1db761 100755
> --- a/testcases/kernel/connectors/pec/cn_pec.sh
> +++ b/testcases/kernel/connectors/pec/cn_pec.sh
> @@ -77,6 +77,8 @@ test()
>  	tst_sleep 100ms

>  	ROD event_generator -n $num_events -e $event \>gen_$event.log 2\>gen_$event.err
> +    # sleep until pec_listener has seen and handled all of the generated events
> +    tst_sleep 100ms
nit: mixing tabs and spaces (script uses tabs), can be fixed during merge.

If I understand the code correctly, pec_listener keeps receiving msg from PEC
until got signaled by kill. We generally try to avoid sleep in tests [1].

@Joerg: I'm not sure if it's reasonable to add another signal handler in which
pec_listener would print number of already handled requests (with write() as
printf() is not reentrant). Then we could loop for certain time before sending INT.

Kind regards,
Petr

[1] https://people.kernel.org/metan/why-sleep-is-almost-never-acceptable-in-tests

>  	kill -s INT $pid 2> /dev/null
>  	wait $pid

[LTP] [PATCH v1] connectors/cn_pec: improve reliability

[Joerg Vehlow]

Hi Petr,

On 8/6/2021 9:48 AM, Petr Vorel wrote:
>
> If I understand the code correctly, pec_listener keeps receiving msg from PEC
> until got signaled by kill. We generally try to avoid sleep in tests [1].
>
> @Joerg: I'm not sure if it's reasonable to add another signal handler in which
> pec_listener would print number of already handled requests (with write() as
> printf() is not reentrant). Then we could loop for certain time before sending INT.
>
The problem is, that it is hard to know, if all expected events were 
received. pec_listener receives events from all processes.
The only way to reliably handle the issue without sleeping is tracking 
exit-events in pec_listener and to tell it to die, once it received an 
exit event from event_generator.
Only then we can be sure, that all events generated by the generator 
(and hopefully all it's children) were received.

We could also get rid of the other sleep (after starting the listener 
using some kind of synchronization (I guess right after 
PROC_CN_MCAST_LISTEN should work). I think checkpoints are not part of 
the new lib...

Joerg

[LTP] [PATCH v1] connectors/cn_pec: improve reliability

[Bogdan Lezhepekov]

Hi Joerg, Petr!

Thanks for reviewing and commenting! I don't have a strong opinion on how the test flow should look like, but I can say this sleep definitely makes the test much more reliable.

The results, we get on known to both of you SUSE Carwos, are pretty consistent. My feeling is that 8/10 runs end up with some sporadic failures. The fact that extra sleep solves the problem obviously indicates a lack of synchronization.

@Joerg: if you need more info about the environment, please reach out to me.

-Bogdan
________________________________
From: Joerg Vehlow <lkml@jv-coder.de>
Sent: Friday, August 6, 2021 11:14
To: Petr Vorel <pvorel@suse.cz>; Bogdan Lezhepekov <bogdan.lezhepekov@suse.com>
Cc: ltp@lists.linux.it <ltp@lists.linux.it>
Subject: Re: [LTP] [PATCH v1] connectors/cn_pec: improve reliability

Hi Petr,

On 8/6/2021 9:48 AM, Petr Vorel wrote:
>
> If I understand the code correctly, pec_listener keeps receiving msg from PEC
> until got signaled by kill. We generally try to avoid sleep in tests [1].
>
> @Joerg: I'm not sure if it's reasonable to add another signal handler in which
> pec_listener would print number of already handled requests (with write() as
> printf() is not reentrant). Then we could loop for certain time before sending INT.
>
The problem is, that it is hard to know, if all expected events were
received. pec_listener receives events from all processes.
The only way to reliably handle the issue without sleeping is tracking
exit-events in pec_listener and to tell it to die, once it received an
exit event from event_generator.
Only then we can be sure, that all events generated by the generator
(and hopefully all it's children) were received.

We could also get rid of the other sleep (after starting the listener
using some kind of synchronization (I guess right after
PROC_CN_MCAST_LISTEN should work). I think checkpoints are not part of
the new lib...

Joerg

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210806/8c6a05fb/attachment-0001.htm>

[LTP] [PATCH v1] connectors/cn_pec: improve reliability

[Joerg Vehlow]

Hi Bogdan,

On 8/6/2021 11:06 AM, Bogdan Lezhepekov wrote:
> Hi Joerg, Petr!
>
> Thanks for reviewing and commenting! I don't have a strong opinion on 
> how the test flow should look like, but I can say this sleep 
> definitely makes the test much more reliable.
>
> The results, we get on known to both of you SUSE Carwos, are pretty 
> consistent. My feeling is that 8/10 runs end up with some sporadic 
> failures. The fact that extra sleep solves the problem obviously 
> indicates a lack of synchronization.
>
> @Joerg: if you need more info about the environment, please reach out 
> to me.
Strange, my test environment was probably something derived from carwos 
(not exactly sure anymore).
I will try to reproduce and implement a correct synchronization as 
described in my earlier mail next week.
But maybe you can tell me more details about the actual used environment 
(If it is related to carwos customer project, you can send it to my 
corporate email address: joerg.vehlow@aox-tech.de).

Joerg