<div dir="ltr"><div dir="ltr">Hi,<div><br></div><div>I think you understood what I meant. The issue I have is for this 4 test cases in ./regression/tools/tracker/test_event_tracker</div><div>- test_event_vpid_tracker ust 0 "${EVENT_NAME}"<br>- test_event_vpid_track_untrack ust 0 "${EVENT_NAME}"<br>- test_event_pid_tracker ust 0 "${EVENT_NAME}"<br>- test_event_pid_track_untrack ust 0 "${EVENT_NAME}"<br><br></div><div>I have 2 patches ready, but I want to run some more tests first before posting them</div><div> <br></div><div><div><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature">Anders Wallin</div></div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Apr 1, 2021 at 3:45 PM Jonathan Rajotte-Julien <<a href="mailto:jonathan.rajotte-julien@efficios.com">jonathan.rajotte-julien@efficios.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu, Apr 01, 2021 at 03:21:10AM +0200, Anders Wallin wrote:<br>
> Hi Jonathan :)<br>
> <br>
> It's very unlikely that the race could occur, BUT it can happen.<br>
> <br>
> OK run<br>
> 1. test_event_tracker starts gen-ust-events<br>
> 2. test_event_tracker waits for gen-ust-events to create AFTER_FIRST_PATH<br>
> 3. gen-ust-event create first event and create AFTER_FIRST_PATH<br>
> 4. gen-ust-event continue and create 99 events<br>
> 5. gen-ust-event wait for  BEFORE_LAST_PATH<br>
> 6. test_event_track start collecting events (lttng start .....)<br>
<br>
> 7. test_event_tracker calls "lttng track ... -pid "Faulty PID"<br>
> 8. test_event_tracker touches BEFORE_LAST_PATH<br>
> 9. gen-ust-events creates the last event<br>
> 10. test_event finishes and since it tracks the faulty pid the result will<br>
> be 0 events == OK<br>
<br>
The sequence of event for the `test_event_tracker` function is (as of master):<br>
<br>
create<br>
enable event<br>
start<br>
track<br>
<br>
launch app<br>
wait for app return<br>
stop<br>
destroy<br>
<br>
<br>
The sequence you are describing here is:<br>
<br>
lauch app<br>
start<br>
track<br>
...<br>
<br>
I'm pretty sure we are not talking about the same things. Please specify the<br>
test case and all functions involved and make sure to use the proper name for<br>
each of them.<br>
<br>
I suspect you are talking about test_event_pid_tracker. Still let's make sure of<br>
it. If it is, I do agree that it seems to have a window where we can gather<br>
event for.<br>
<br>
You might want to look if there is a real reason for this sequence instead of<br>
mimicking test_event_tracker<br>
<br>
Current code:<br>
 enable_"$domain"_lttng_event_ok $SESSION_NAME "$wildcard" "$channel"<br>
 prepare_"$domain"_app<br>
 start_lttng_tracing_ok<br>
<br>
 if [ "$expect_event" -eq 1 ]; then<br>
         lttng_track_"$domain"_ok "--vpid ${CHILD_PID}"<br>
 else<br>
         lttng_track_"$domain"_ok "--vpid $((CHILD_PID+1))"<br>
 fi<br>
 trace_"$domain"_app<br>
 stop_lttng_tracing_ok<br>
 destroy_lttng_session_ok $SESSION_NAME<br>
<br>
<br>
We might simply want to move the track command before the start considering have<br>
all the information to do so.<br>
<br>
 enable_"$domain"_lttng_event_ok $SESSION_NAME "$wildcard" "$channel"<br>
<br>
 prepare_"$domain"_app<br>
<br>
 if [ "$expect_event" -eq 1 ]; then<br>
         lttng_track_"$domain"_ok "--vpid ${CHILD_PID}"<br>
 else<br>
         lttng_track_"$domain"_ok "--vpid $((CHILD_PID+1))"<br>
 fi<br>
<br>
 start_lttng_tracing_ok<br>
 trace_"$domain"_app<br>
<br>
 stop_lttng_tracing_ok<br>
 destroy_lttng_session_ok $SESSION_NAME<br>
<br>
After testing this, seems like the validate_trace_empty does not handle the<br>
case were simply stream allocation did not take place since no application was<br>
'valid' at the moment of the trace start.<br>
<br>
Well we got a good one here. We will wait for your updated patch and go from<br>
there.<br>
<br>
Cheers<br>
<br>
> <br>
> Faulty run<br>
> 1. test_event_tracker starts gen-ust-events<br>
> 2. test_event_tracker waits for gen-ust-events to create AFTER_FIRST_PATH<br>
> 3. gen-ust-event create first event and create AFTER_FIRST_PATH<br>
> 4. gen-ust-event gets rescheduled before it has created 99 events, e.g<br>
> after 9 events<br>
> 5. test_event_track start collecting events (lttng start .....)<br>
> 6. gen-ust-event is rescheduled and starts generating the remaining events.<br>
> 90 events<br>
> 7. lttng collects these 90 events since we have not setup "tracking" of PID<br>
> yet<br>
> 8. test_event_tracker calls "lttng track ... -pid "Faulty PID"<br>
> 9. test_event_tracker touches BEFORE_LAST_PATH<br>
> 10. gen-ust-events creates the last event<br>
> 11. test_event finishes and since it tracks the faulty pid the result<br>
> should  be 0 events, but we got 90 == FAULTY<br>
> <br>
> We can solve this by;<br>
> A: using NR_ITER=2<br>
> or<br>
> B: by adding a flag to gen-ust-events to wait before sending the first event<br>
> 1. test_event_tracker starts gen-ust-events<br>
> 2. test_event_tracker waits for gen-ust-events waits for BEFORE_FIRST_PATH<br>
> 3. test_event_track start collecting events (lttng start .....)<br>
> 4. test_event_tracker calls "lttng track ... -pid "Faulty PID"<br>
> 5. test_event_track creates BEFORE_FIRST_PATH<br>
> 6. gen_ust_event creates 100 events<br>
> 7. test_event_tracker waits for gen_event_ust to end<br>
> 8. test_event finishes and since it tracked the faulty pid the result will<br>
> be 0 events == OK<br>
> <br>
> This is in principle how gen-kernel-test-events works (but with different<br>
> arguments)<br>
> I would suggest to use B since that will be bulletproof<br>
> <br>
> Anders Wallin<br>
> <br>
> <br>
> On Wed, Mar 31, 2021 at 11:33 PM Jonathan Rajotte-Julien <<br>
> <a href="mailto:jonathan.rajotte-julien@efficios.com" target="_blank">jonathan.rajotte-julien@efficios.com</a>> wrote:<br>
> <br>
> > Hi,<br>
> ><br>
> > On Wed, Mar 31, 2021 at 11:09:42PM +0200, Anders Wallin wrote:<br>
> > > Hi Julian,<br>
> ><br>
> > You can use Jonathan. ;)<br>
> ><br>
> > ><br>
> > > Neither mine "sleep 0.1" or your version with "while [! -f ............"<br>
> > > are race condition free.<br>
> ><br>
> > I might be missing something here but as far as I understand the race you<br>
> > are<br>
> > trying to mitigate is that the test script execute/continue before the<br>
> > `backgrounded`<br>
> > command (background test app) had time to execute, right?<br>
> ><br>
> > If so at least waiting for the app to create a file is necessary. Now<br>
> > gen_kernel_test_events does not have this functionality. The<br>
> > PATH_WAIT_FILE is<br>
> > used to control when the testapp can continue. Hence the script still<br>
> > cannot<br>
> > know if the app have been scheduled.<br>
> ><br>
> > Now based on the test case you might need more synchronization for the test<br>
> > cases.<br>
> ><br>
> > Note that in the ust cases, the trace_ust_app uses `touch<br>
> > "$BEFORE_LAST_PATH"`<br>
> > that effectively unblock the app and allows it to perform the last<br>
> > tracepoint<br>
> > hit and the we `wait` on the background process.<br>
> ><br>
> > Note: some tests case are a bit clever and uses "trace_"$domain"_app"<br>
> > instead of<br>
> > calling trace_ust_app directly.<br>
> ><br>
> > For these tests case it seems that we are only expecting at least a single<br>
> > event<br>
> > matching the event name under test. Here the last tracepoint hit should<br>
> > satisfy<br>
> > this criteria.<br>
> ><br>
> > Am I missing a race?<br>
> ><br>
> > Cheers<br>
> ><br>
> ><br>
> > > I suggest that we add an option to gen-ust-events to wait before the<br>
> > first<br>
> > > event is generated.<br>
> > > gen_kernel_test_events already have this functionality to wait before the<br>
> > > first event.<br>
> > ><br>
> > > Something like this<br>
> > > static struct option long_options[] =<br>
> > > {<br>
> > > /* These options set a flag. */<br>
> > > {"iter", required_argument, 0, 'i'},<br>
> > > {"wait", required_argument, 0, 'w'},<br>
> > > {"sync-after-first-event", required_argument, 0, 'a'},<br>
> > > {"sync-before-last-event", required_argument, 0, 'b'},<br>
> > > {"sync-before-last-event-touch", required_argument, 0, 'c'},<br>
> > > {"sync-before-exit", required_argument, 0, 'd'},<br>
> > > {"sync-before-exit-touch", required_argument, 0, 'e'},<br>
> > > *+ {"sync-before-first-event", required_argument, 0, 'f'},*<br>
> > ><br>
> > > {0, 0, 0, 0}<br>
> > > };<br>
> > > ....<br>
> > ><br>
> > > I will create one or more patches for this tomorrow<br>
> > ><br>
> > > Anders Wallin<br>
> > ><br>
> > ><br>
> > > On Wed, Mar 31, 2021 at 9:25 PM Jonathan Rajotte-Julien <<br>
> > > <a href="mailto:jonathan.rajotte-julien@efficios.com" target="_blank">jonathan.rajotte-julien@efficios.com</a>> wrote:<br>
> > ><br>
> > > > > #<br>
> > > > > # SPDX-License-Identifier: GPL-2.0-only<br>
> > > > ><br>
> > > > > -TEST_DESC="LTTng - Event traker test"<br>
> > > > > +TEST_DESC="LTTng - Event tracker test"<br>
> > > > ><br>
> > > > > CURDIR=$(dirname "$0")/<br>
> > > > > TESTDIR="$CURDIR/../../.."<br>
> > > > > @@ -42,6 +42,8 @@ function prepare_ust_app<br>
> > > > ><br>
> > > > >       $TESTAPP_BIN -i $NR_ITER -w $NR_USEC_WAIT -a<br>
> > "$AFTER_FIRST_PATH" -b<br>
> > > > >       "$BEFORE_LAST_PATH" &<br>
> > > > >       CHILD_PID=$!<br>
> > > > > +     # voluntary context switch to start $TESTAPP_BIN<br>
> > > > > +     sleep 0.1<br>
> > > ><br>
> > > > A wait on the $AFTER_FIRST_PATH file would be probably more<br>
> > deterministic<br>
> > > > than a sleep here.<br>
> > > ><br>
> > > >   while [ ! -f "${AFTER_FIRST_PATH}" ]; do<br>
> > > >           sleep 0.1<br>
> > > >   done<br>
> > > ><br>
> > > > I would also expect something similar for the `prepare_kernel_app`<br>
> > > > function considering the same race is most probably present and simply<br>
> > not<br>
> > > > triggered by a chance of luck.<br>
> > > > Seems like gen-kernel-test-events does not expose the same sync<br>
> > > > capabilities here, please use gen-ust-events as an example of how it is<br>
> > > > done.<br>
> > > ><br>
> > > > Let us know how your testing goes.<br>
> > > ><br>
> > > > Thanks<br>
> > > ><br>
> ><br>
> > --<br>
> > Jonathan Rajotte-Julien<br>
> > EfficiOS<br>
> ><br>
<br>
-- <br>
Jonathan Rajotte-Julien<br>
EfficiOS<br>
</blockquote></div></div>