[PATCH] eal: fix segment fault when exit trace

[PATCH] eal: fix segment fault when exit trace

[Chengwen Feng]

Bug scenario:
1. start testpmd:
	dpdk-testpmd -l 4-6 -a 0000:7d:00.0 --trace=.* -- -i
2. quit testpmd and then observed segment fault:
	Bye...
	Segmentation fault (core dumped)

The root cause is that rte_trace_save() and eal_trace_fini() access
the huge pages which were cleanup by rte_eal_memory_detach().

This patch moves rte_trace_save() and eal_trace_fini() before
rte_eal_memory_detach() to fix the bug.

Fixes: dfbc61a2f9a6 ("mem: detach memsegs on cleanup")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/eal/linux/eal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
index 1ef263434a..c6f2056197 100644
--- a/lib/eal/linux/eal.c
+++ b/lib/eal/linux/eal.c
@@ -1266,13 +1266,13 @@ rte_eal_cleanup(void)
 	vfio_mp_sync_cleanup();
 #endif
 	rte_mp_channel_cleanup();
+	rte_trace_save();
+	eal_trace_fini();
 	/* after this point, any DPDK pointers will become dangling */
 	rte_eal_memory_detach();
 	eal_mp_dev_hotplug_cleanup();
 	rte_eal_malloc_heap_cleanup();
 	rte_eal_alarm_cleanup();
-	rte_trace_save();
-	eal_trace_fini();
 	eal_cleanup_config(internal_conf);
 	rte_eal_log_cleanup();
 	return 0;
-- 
2.33.0

Re: [PATCH] eal: fix segment fault when exit trace

[fengchengwen]

Hi Bruce,

  Could you please test freebsd platform? I think it also have
the same problem, but I hasn't freebsd enviorment.

Thanks

On 2022/6/7 20:00, Chengwen Feng wrote:
> Bug scenario:
> 1. start testpmd:
> 	dpdk-testpmd -l 4-6 -a 0000:7d:00.0 --trace=.* -- -i
> 2. quit testpmd and then observed segment fault:
> 	Bye...
> 	Segmentation fault (core dumped)
> 
> The root cause is that rte_trace_save() and eal_trace_fini() access
> the huge pages which were cleanup by rte_eal_memory_detach().
> 
> This patch moves rte_trace_save() and eal_trace_fini() before
> rte_eal_memory_detach() to fix the bug.
> 
> Fixes: dfbc61a2f9a6 ("mem: detach memsegs on cleanup")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  lib/eal/linux/eal.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
> index 1ef263434a..c6f2056197 100644
> --- a/lib/eal/linux/eal.c
> +++ b/lib/eal/linux/eal.c
> @@ -1266,13 +1266,13 @@ rte_eal_cleanup(void)
>  	vfio_mp_sync_cleanup();
>  #endif
>  	rte_mp_channel_cleanup();
> +	rte_trace_save();
> +	eal_trace_fini();
>  	/* after this point, any DPDK pointers will become dangling */
>  	rte_eal_memory_detach();
>  	eal_mp_dev_hotplug_cleanup();
>  	rte_eal_malloc_heap_cleanup();
>  	rte_eal_alarm_cleanup();
> -	rte_trace_save();
> -	eal_trace_fini();
>  	eal_cleanup_config(internal_conf);
>  	rte_eal_log_cleanup();
>  	return 0;
> 

Re: [PATCH] eal: fix segment fault when exit trace

[David Marchand]

On Tue, Jun 7, 2022 at 2:26 PM fengchengwen <fengchengwen@huawei.com> wrote:
> Hi Bruce,
>
>   Could you please test freebsd platform? I think it also have
> the same problem, but I hasn't freebsd enviorment.

I can reproduce a crash with FreeBSD 13.

Moving rte_trace_save() and co earlier (as is done for Linux in this
proposed patch) avoids the crash, so I can't tell the traces file is
valid.

dpdk@freebsd:~/dpdk $ git diff
diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c
index a6b20960f2..26fbc91b26 100644
--- a/lib/eal/freebsd/eal.c
+++ b/lib/eal/freebsd/eal.c
@@ -893,11 +893,11 @@ rte_eal_cleanup(void)
                eal_get_internal_configuration();
        rte_service_finalize();
        rte_mp_channel_cleanup();
+       rte_trace_save();
+       eal_trace_fini();
        /* after this point, any DPDK pointers will become dangling */
        rte_eal_memory_detach();
        rte_eal_alarm_cleanup();
-       rte_trace_save();
-       eal_trace_fini();
        eal_cleanup_config(internal_conf);
        return 0;
 }


-- 
David Marchand

Re: [PATCH] eal: fix segment fault when exit trace

[fengchengwen]

Thansk for your testing, David.


On 2022/6/13 22:15, David Marchand wrote:
> On Tue, Jun 7, 2022 at 2:26 PM fengchengwen <fengchengwen@huawei.com> wrote:
>> Hi Bruce,
>>
>>   Could you please test freebsd platform? I think it also have
>> the same problem, but I hasn't freebsd enviorment.
> 
> I can reproduce a crash with FreeBSD 13.
> 
> Moving rte_trace_save() and co earlier (as is done for Linux in this
> proposed patch) avoids the crash, so I can't tell the traces file is
> valid.
> 
> dpdk@freebsd:~/dpdk $ git diff
> diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c
> index a6b20960f2..26fbc91b26 100644
> --- a/lib/eal/freebsd/eal.c
> +++ b/lib/eal/freebsd/eal.c
> @@ -893,11 +893,11 @@ rte_eal_cleanup(void)
>                 eal_get_internal_configuration();
>         rte_service_finalize();
>         rte_mp_channel_cleanup();
> +       rte_trace_save();
> +       eal_trace_fini();
>         /* after this point, any DPDK pointers will become dangling */
>         rte_eal_memory_detach();
>         rte_eal_alarm_cleanup();
> -       rte_trace_save();
> -       eal_trace_fini();
>         eal_cleanup_config(internal_conf);
>         return 0;
>  }
> 
> 

Re: [PATCH] eal: fix segment fault when exit trace

[David Marchand]

On Tue, Jun 7, 2022 at 2:06 PM Chengwen Feng <fengchengwen@huawei.com> wrote:
>
> Bug scenario:
> 1. start testpmd:
>         dpdk-testpmd -l 4-6 -a 0000:7d:00.0 --trace=.* -- -i
> 2. quit testpmd and then observed segment fault:
>         Bye...
>         Segmentation fault (core dumped)
>
> The root cause is that rte_trace_save() and eal_trace_fini() access
> the huge pages which were cleanup by rte_eal_memory_detach().
>
> This patch moves rte_trace_save() and eal_trace_fini() before
> rte_eal_memory_detach() to fix the bug.
>
> Fixes: dfbc61a2f9a6 ("mem: detach memsegs on cleanup")
> Cc: stable@dpdk.org
>
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>

Good catch.
It was introduced in 21.05.
It's a bit concerning that it got unnoticed so far.

-- 
David Marchand

Re: [PATCH] eal: fix segment fault when exit trace

[Jerin Jacob]

On Tue, Jun 7, 2022 at 5:36 PM Chengwen Feng <fengchengwen@huawei.com> wrote:
>
> Bug scenario:
> 1. start testpmd:
>         dpdk-testpmd -l 4-6 -a 0000:7d:00.0 --trace=.* -- -i
> 2. quit testpmd and then observed segment fault:
>         Bye...
>         Segmentation fault (core dumped)
>
> The root cause is that rte_trace_save() and eal_trace_fini() access
> the huge pages which were cleanup by rte_eal_memory_detach().
>
> This patch moves rte_trace_save() and eal_trace_fini() before
> rte_eal_memory_detach() to fix the bug.
>
> Fixes: dfbc61a2f9a6 ("mem: detach memsegs on cleanup")
> Cc: stable@dpdk.org
>
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>


Tested-by: Jerin Jacob <jerinj@marvell.com>


> ---
>  lib/eal/linux/eal.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
> index 1ef263434a..c6f2056197 100644
> --- a/lib/eal/linux/eal.c
> +++ b/lib/eal/linux/eal.c
> @@ -1266,13 +1266,13 @@ rte_eal_cleanup(void)
>         vfio_mp_sync_cleanup();
>  #endif
>         rte_mp_channel_cleanup();
> +       rte_trace_save();
> +       eal_trace_fini();
>         /* after this point, any DPDK pointers will become dangling */
>         rte_eal_memory_detach();
>         eal_mp_dev_hotplug_cleanup();
>         rte_eal_malloc_heap_cleanup();
>         rte_eal_alarm_cleanup();
> -       rte_trace_save();
> -       eal_trace_fini();
>         eal_cleanup_config(internal_conf);
>         rte_eal_log_cleanup();
>         return 0;
> --
> 2.33.0
>

[PATCH v2 0/3] bugfix for trace

[Chengwen Feng]

This patch contains two bugfix for trace and one test patch.

Chengwen Feng (3):
  eal: fix segment fault when exit trace
  eal: fix trace init fail with long file-prefix
  test: support trace-autotest when enable trace

---
v2:
* add test-by from Jerin Jacob for the 1/3 patch.
* add freebsd change file because David Marchand verify it.
* add more bugfix patch when try to enhance testcase.

 app/test/meson.build                    | 11 +++++++++++
 lib/eal/common/eal_common_trace_utils.c |  2 +-
 lib/eal/freebsd/eal.c                   |  4 ++--
 lib/eal/linux/eal.c                     |  4 ++--
 4 files changed, 16 insertions(+), 5 deletions(-)

-- 
2.33.0

[PATCH v2 1/3] eal: fix segment fault when exit trace

[Chengwen Feng]

Bug scenario:
1. start testpmd:
	dpdk-testpmd -l 4-6 -a 0000:7d:00.0 --trace=.* -- -i
2. quit testpmd and then observed segment fault:
	Bye...
	Segmentation fault (core dumped)

The root cause is that rte_trace_save() and eal_trace_fini() access
the huge pages which were cleanup by rte_eal_memory_detach().

This patch moves rte_trace_save() and eal_trace_fini() before
rte_eal_memory_detach() to fix the bug.

Fixes: dfbc61a2f9a6 ("mem: detach memsegs on cleanup")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Tested-by: Jerin Jacob <jerinj@marvell.com>
---
 lib/eal/freebsd/eal.c | 4 ++--
 lib/eal/linux/eal.c   | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c
index a6b20960f2..26fbc91b26 100644
--- a/lib/eal/freebsd/eal.c
+++ b/lib/eal/freebsd/eal.c
@@ -893,11 +893,11 @@ rte_eal_cleanup(void)
 		eal_get_internal_configuration();
 	rte_service_finalize();
 	rte_mp_channel_cleanup();
+	rte_trace_save();
+	eal_trace_fini();
 	/* after this point, any DPDK pointers will become dangling */
 	rte_eal_memory_detach();
 	rte_eal_alarm_cleanup();
-	rte_trace_save();
-	eal_trace_fini();
 	eal_cleanup_config(internal_conf);
 	return 0;
 }
diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
index 1ef263434a..c6f2056197 100644
--- a/lib/eal/linux/eal.c
+++ b/lib/eal/linux/eal.c
@@ -1266,13 +1266,13 @@ rte_eal_cleanup(void)
 	vfio_mp_sync_cleanup();
 #endif
 	rte_mp_channel_cleanup();
+	rte_trace_save();
+	eal_trace_fini();
 	/* after this point, any DPDK pointers will become dangling */
 	rte_eal_memory_detach();
 	eal_mp_dev_hotplug_cleanup();
 	rte_eal_malloc_heap_cleanup();
 	rte_eal_alarm_cleanup();
-	rte_trace_save();
-	eal_trace_fini();
 	eal_cleanup_config(internal_conf);
 	rte_eal_log_cleanup();
 	return 0;
-- 
2.33.0

Re: [PATCH v2 1/3] eal: fix segment fault when exit trace

[David Marchand]

On Tue, Jun 14, 2022 at 8:06 AM Chengwen Feng <fengchengwen@huawei.com> wrote:
>
> Bug scenario:
> 1. start testpmd:
>         dpdk-testpmd -l 4-6 -a 0000:7d:00.0 --trace=.* -- -i
> 2. quit testpmd and then observed segment fault:
>         Bye...
>         Segmentation fault (core dumped)
>
> The root cause is that rte_trace_save() and eal_trace_fini() access
> the huge pages which were cleanup by rte_eal_memory_detach().
>
> This patch moves rte_trace_save() and eal_trace_fini() before
> rte_eal_memory_detach() to fix the bug.
>
> Fixes: dfbc61a2f9a6 ("mem: detach memsegs on cleanup")
> Cc: stable@dpdk.org
>
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Tested-by: Jerin Jacob <jerinj@marvell.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

[PATCH v2 2/3] eal: fix trace init fail with long file-prefix

[Chengwen Feng]

Bug scenario:
1. start testpmd:
dpdk-testpmd -l 4-6 -a 0000:7d:00.0 --trace=.* \
-file-prefix=trace_autotest -- -i
2. then observed:
EAL: eal_trace_init():94 failed to initialize trace [File exists]
EAL: FATAL: Cannot init trace
EAL: Cannot init trace
EAL: Error - exiting with code: 1

The root cause it that the offset set wrong with long file-prefix and
then lead the strftime return failed.

Fixes: 321dd5f8fa62 ("trace: add internal init and fini interface")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/eal/common/eal_common_trace_utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/eal/common/eal_common_trace_utils.c b/lib/eal/common/eal_common_trace_utils.c
index 64f58fb66a..0822678a60 100644
--- a/lib/eal/common/eal_common_trace_utils.c
+++ b/lib/eal/common/eal_common_trace_utils.c
@@ -104,7 +104,7 @@ trace_session_name_generate(char *trace_dir)
 	rc = rte_strscpy(trace_dir, eal_get_hugefile_prefix(),
 			TRACE_PREFIX_LEN);
 	if (rc == -E2BIG)
-		rc = TRACE_PREFIX_LEN;
+		rc = TRACE_PREFIX_LEN - 1;
 	trace_dir[rc++] = '-';
 
 	rc = strftime(trace_dir + rc, TRACE_DIR_STR_LEN - rc,
-- 
2.33.0

Re: [PATCH v2 2/3] eal: fix trace init fail with long file-prefix

[David Marchand]

On Tue, Jun 14, 2022 at 8:06 AM Chengwen Feng <fengchengwen@huawei.com> wrote:
>
> Bug scenario:
> 1. start testpmd:
> dpdk-testpmd -l 4-6 -a 0000:7d:00.0 --trace=.* \
> -file-prefix=trace_autotest -- -i
> 2. then observed:
> EAL: eal_trace_init():94 failed to initialize trace [File exists]

This is not directly related to the issue being fixed, but this error
log is really obscure..
Can we enhance this, maybe in a followup patch?


> EAL: FATAL: Cannot init trace
> EAL: Cannot init trace
> EAL: Error - exiting with code: 1
>
> The root cause it that the offset set wrong with long file-prefix and
> then lead the strftime return failed.
>
> Fixes: 321dd5f8fa62 ("trace: add internal init and fini interface")
> Cc: stable@dpdk.org
>
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>

Reviewed-by: David Marchand <david.marchand@redhat.com>


And another topic, I see no reason for the limitation/truncation on
the file prefix.
Testing with the last patch of the series, I can see odd directory, like
EAL: Trace dir:
/home/dmarchan/builds/main/build-gcc-shared/app/test/trace_autot-2022-06-15-PM-03-18-39
I'll send a cleanup series for this, later.


-- 
David Marchand

Re: [PATCH v2 2/3] eal: fix trace init fail with long file-prefix

[Jerin Jacob]

On Wed, Jun 15, 2022 at 7:32 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Tue, Jun 14, 2022 at 8:06 AM Chengwen Feng <fengchengwen@huawei.com> wrote:
> >
> > Bug scenario:
> > 1. start testpmd:
> > dpdk-testpmd -l 4-6 -a 0000:7d:00.0 --trace=.* \
> > -file-prefix=trace_autotest -- -i
> > 2. then observed:
> > EAL: eal_trace_init():94 failed to initialize trace [File exists]
>
> This is not directly related to the issue being fixed, but this error
> log is really obscure..
> Can we enhance this, maybe in a followup patch?
>
>
> > EAL: FATAL: Cannot init trace
> > EAL: Cannot init trace
> > EAL: Error - exiting with code: 1
> >
> > The root cause it that the offset set wrong with long file-prefix and
> > then lead the strftime return failed.
> >
> > Fixes: 321dd5f8fa62 ("trace: add internal init and fini interface")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>
> Reviewed-by: David Marchand <david.marchand@redhat.com>

Acked-by: Jerin Jacob <jerinj@marvell.com>

[PATCH v2 3/3] test: support trace-autotest when enable trace

[Chengwen Feng]

There are a bug[1] when exit application while enable tracing, this
bug has not been discovered for a long time, to quickly detect such
bugs, this patch was introduced.

This patch adds a testcase with trace enabling, it also depends on
patch[2] because it has a long file-prefix.

[1] eal: fix segment fault when exit trace
[2] eal: fix trace init fail with long file-prefix

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test/meson.build | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/app/test/meson.build b/app/test/meson.build
index 7fe261cae8..eb37aa632a 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -509,6 +509,17 @@ foreach arg : fast_tests
                 is_parallel : false,
                 suite : 'fast-tests')
     endif
+
+    if run_test and arg[0] == 'trace_autotest' and (not is_windows)
+        test_args += ['--trace=.*']
+        test_args += ['--trace-dir=@0@'.format(meson.current_build_dir())]
+        test(arg[0], dpdk_test,
+                env : ['DPDK_TEST=' + arg[0]],
+                args : test_args,
+                timeout : timeout_seconds_fast,
+                is_parallel : false,
+                suite : 'fast-tests')
+    endif
 endforeach
 
 foreach arg : perf_test_names
-- 
2.33.0

Re: [PATCH v2 3/3] test: support trace-autotest when enable trace

[David Marchand]

On Tue, Jun 14, 2022 at 8:06 AM Chengwen Feng <fengchengwen@huawei.com> wrote:
>
> There are a bug[1] when exit application while enable tracing, this
> bug has not been discovered for a long time, to quickly detect such
> bugs, this patch was introduced.
>
> This patch adds a testcase with trace enabling, it also depends on
> patch[2] because it has a long file-prefix.
>
> [1] eal: fix segment fault when exit trace
> [2] eal: fix trace init fail with long file-prefix

This commitlog feels more like a cover letter.

Please describe what the impact of the patch is, like mention that the
trace_autotest unit test is being called twice, once with traces
disabled, and once with traces enabled.
And that the traces file is being written in a directory part of the
build directory.


>
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  app/test/meson.build | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/app/test/meson.build b/app/test/meson.build
> index 7fe261cae8..eb37aa632a 100644
> --- a/app/test/meson.build
> +++ b/app/test/meson.build
> @@ -509,6 +509,17 @@ foreach arg : fast_tests
>                  is_parallel : false,
>                  suite : 'fast-tests')
>      endif
> +
> +    if run_test and arg[0] == 'trace_autotest' and (not is_windows)

Calling this test on Windows should not be an issue.
Is it not the case?


> +        test_args += ['--trace=.*']
> +        test_args += ['--trace-dir=@0@'.format(meson.current_build_dir())]
> +        test(arg[0], dpdk_test,

58/68 DPDK:fast-tests / trace_autotest                   OK              0.16s
59/68 DPDK:fast-tests / trace_autotest                   OK              0.16s

By using the same test name, it's hard to tell what the difference is
between those two lines.
And practically, as a developer reproducing/troubleshooting a test
failure, you can't call only one of the test case.

I am not sure what could be done to enhance this.., how about using a
dedicated test name?
Like below snippet, replacing the whole proposed patch:

@@ -508,6 +508,16 @@ foreach arg : fast_tests
                 timeout : timeout_seconds_fast,
                 is_parallel : false,
                 suite : 'fast-tests')
+        if arg[0] == 'trace_autotest'
+            test_args += ['--trace=.*']
+            test_args += ['--trace-dir=@0@'.format(meson.current_build_dir())]
+            test(arg[0] + '_with_traces', dpdk_test,
+                    env : ['DPDK_TEST=' + arg[0]],
+                    args : test_args,
+                    timeout : timeout_seconds_fast,
+                    is_parallel : false,
+                    suite : 'fast-tests')
+        endif
     endif
 endforeach


-- 
David Marchand

Re: [PATCH v2 3/3] test: support trace-autotest when enable trace

[fengchengwen]

Hi David, thanks for your review.

On 2022/6/15 22:00, David Marchand wrote:
> On Tue, Jun 14, 2022 at 8:06 AM Chengwen Feng <fengchengwen@huawei.com> wrote:
>>
>> There are a bug[1] when exit application while enable tracing, this
>> bug has not been discovered for a long time, to quickly detect such
>> bugs, this patch was introduced.
>>
>> This patch adds a testcase with trace enabling, it also depends on
>> patch[2] because it has a long file-prefix.
>>
>> [1] eal: fix segment fault when exit trace
>> [2] eal: fix trace init fail with long file-prefix
> 
> This commitlog feels more like a cover letter.
> 
> Please describe what the impact of the patch is, like mention that the
> trace_autotest unit test is being called twice, once with traces
> disabled, and once with traces enabled.
> And that the traces file is being written in a directory part of the
> build directory.
> 
> 
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> ---
>>  app/test/meson.build | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/app/test/meson.build b/app/test/meson.build
>> index 7fe261cae8..eb37aa632a 100644
>> --- a/app/test/meson.build
>> +++ b/app/test/meson.build
>> @@ -509,6 +509,17 @@ foreach arg : fast_tests
>>                  is_parallel : false,
>>                  suite : 'fast-tests')
>>      endif
>> +
>> +    if run_test and arg[0] == 'trace_autotest' and (not is_windows)
> 
> Calling this test on Windows should not be an issue.
> Is it not the case?

I notice that eal_parse_common_option still keep windows macro for trace parameter parse:

#ifndef RTE_EXEC_ENV_WINDOWS
	case OPT_TRACE_NUM: {
		...
	case OPT_TRACE_MODE_NUM: {
		if (eal_trace_mode_args_save(optarg) < 0) {
			RTE_LOG(ERR, EAL, "invalid parameters for --"
				OPT_TRACE_MODE "\n");
			return -1;
		}
		break;
	}
#endif /* !RTE_EXEC_ENV_WINDOWS */

Maybe trace already support in windows, but I havn't the windows enviorment to verify it.

So I think we should keep '(not is_windows)' currently.

> 
> 
>> +        test_args += ['--trace=.*']
>> +        test_args += ['--trace-dir=@0@'.format(meson.current_build_dir())]
>> +        test(arg[0], dpdk_test,
> 
> 58/68 DPDK:fast-tests / trace_autotest                   OK              0.16s
> 59/68 DPDK:fast-tests / trace_autotest                   OK              0.16s
> 
> By using the same test name, it's hard to tell what the difference is
> between those two lines.
> And practically, as a developer reproducing/troubleshooting a test
> failure, you can't call only one of the test case.
> 
> I am not sure what could be done to enhance this.., how about using a
> dedicated test name?
> Like below snippet, replacing the whole proposed patch:

Good idea, already fix in v3

> 
> @@ -508,6 +508,16 @@ foreach arg : fast_tests
>                  timeout : timeout_seconds_fast,
>                  is_parallel : false,
>                  suite : 'fast-tests')
> +        if arg[0] == 'trace_autotest'
> +            test_args += ['--trace=.*']
> +            test_args += ['--trace-dir=@0@'.format(meson.current_build_dir())]
> +            test(arg[0] + '_with_traces', dpdk_test,
> +                    env : ['DPDK_TEST=' + arg[0]],
> +                    args : test_args,
> +                    timeout : timeout_seconds_fast,
> +                    is_parallel : false,
> +                    suite : 'fast-tests')
> +        endif
>      endif
>  endforeach
> 
> 

Re: [PATCH v2 3/3] test: support trace-autotest when enable trace

[David Marchand]

On Fri, Jun 17, 2022 at 4:43 AM fengchengwen <fengchengwen@huawei.com> wrote:
>
> Hi David, thanks for your review.
>
> On 2022/6/15 22:00, David Marchand wrote:
> > On Tue, Jun 14, 2022 at 8:06 AM Chengwen Feng <fengchengwen@huawei.com> wrote:
> >>
> >> There are a bug[1] when exit application while enable tracing, this
> >> bug has not been discovered for a long time, to quickly detect such
> >> bugs, this patch was introduced.
> >>
> >> This patch adds a testcase with trace enabling, it also depends on
> >> patch[2] because it has a long file-prefix.
> >>
> >> [1] eal: fix segment fault when exit trace
> >> [2] eal: fix trace init fail with long file-prefix
> >
> > This commitlog feels more like a cover letter.
> >
> > Please describe what the impact of the patch is, like mention that the
> > trace_autotest unit test is being called twice, once with traces
> > disabled, and once with traces enabled.
> > And that the traces file is being written in a directory part of the
> > build directory.
> >
> >
> >>
> >> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> >> ---
> >>  app/test/meson.build | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/app/test/meson.build b/app/test/meson.build
> >> index 7fe261cae8..eb37aa632a 100644
> >> --- a/app/test/meson.build
> >> +++ b/app/test/meson.build
> >> @@ -509,6 +509,17 @@ foreach arg : fast_tests
> >>                  is_parallel : false,
> >>                  suite : 'fast-tests')
> >>      endif
> >> +
> >> +    if run_test and arg[0] == 'trace_autotest' and (not is_windows)
> >
> > Calling this test on Windows should not be an issue.
> > Is it not the case?
>
> I notice that eal_parse_common_option still keep windows macro for trace parameter parse:
>
> #ifndef RTE_EXEC_ENV_WINDOWS
>         case OPT_TRACE_NUM: {
>                 ...
>         case OPT_TRACE_MODE_NUM: {
>                 if (eal_trace_mode_args_save(optarg) < 0) {
>                         RTE_LOG(ERR, EAL, "invalid parameters for --"
>                                 OPT_TRACE_MODE "\n");
>                         return -1;
>                 }
>                 break;
>         }
> #endif /* !RTE_EXEC_ENV_WINDOWS */
>
> Maybe trace already support in windows, but I havn't the windows enviorment to verify it.
>
> So I think we should keep '(not is_windows)' currently.

Indeed.


-- 
David Marchand

[PATCH v3 0/4] bugfix for trace

[Chengwen Feng]

This patch contains three bugfix for trace and one test patch.

---
v3:
* address David Marchand's comments.
* add xxx-by from David Marchand and Jerin Jacob.
v2:
* add test-by from Jerin Jacob for the 1/3 patch.
* add freebsd change file because David Marchand verify it.
* add more bugfix patch when try to enhance testcase.

Chengwen Feng (4):
  eal: fix segment fault when exit trace
  eal: fix errno not set if strftime return zero
  eal: fix trace init fail with long file-prefix
  test: introduce trace-autotest with traces enabled

 app/test/meson.build                    | 10 ++++++++++
 lib/eal/common/eal_common_trace_utils.c |  6 ++++--
 lib/eal/freebsd/eal.c                   |  4 ++--
 lib/eal/linux/eal.c                     |  4 ++--
 4 files changed, 18 insertions(+), 6 deletions(-)

-- 
2.33.0

[PATCH v3 1/4] eal: fix segment fault when exit trace

[Chengwen Feng]

Bug scenario:
1. start testpmd:
	dpdk-testpmd -l 4-6 -a 0000:7d:00.0 --trace=.* -- -i
2. quit testpmd and then observed segment fault:
	Bye...
	Segmentation fault (core dumped)

The root cause is that rte_trace_save() and eal_trace_fini() access
the huge pages which were cleanup by rte_eal_memory_detach().

This patch moves rte_trace_save() and eal_trace_fini() before
rte_eal_memory_detach() to fix the bug.

Fixes: dfbc61a2f9a6 ("mem: detach memsegs on cleanup")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Tested-by: Jerin Jacob <jerinj@marvell.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/freebsd/eal.c | 4 ++--
 lib/eal/linux/eal.c   | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c
index a6b20960f2..26fbc91b26 100644
--- a/lib/eal/freebsd/eal.c
+++ b/lib/eal/freebsd/eal.c
@@ -893,11 +893,11 @@ rte_eal_cleanup(void)
 		eal_get_internal_configuration();
 	rte_service_finalize();
 	rte_mp_channel_cleanup();
+	rte_trace_save();
+	eal_trace_fini();
 	/* after this point, any DPDK pointers will become dangling */
 	rte_eal_memory_detach();
 	rte_eal_alarm_cleanup();
-	rte_trace_save();
-	eal_trace_fini();
 	eal_cleanup_config(internal_conf);
 	return 0;
 }
diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
index 1ef263434a..c6f2056197 100644
--- a/lib/eal/linux/eal.c
+++ b/lib/eal/linux/eal.c
@@ -1266,13 +1266,13 @@ rte_eal_cleanup(void)
 	vfio_mp_sync_cleanup();
 #endif
 	rte_mp_channel_cleanup();
+	rte_trace_save();
+	eal_trace_fini();
 	/* after this point, any DPDK pointers will become dangling */
 	rte_eal_memory_detach();
 	eal_mp_dev_hotplug_cleanup();
 	rte_eal_malloc_heap_cleanup();
 	rte_eal_alarm_cleanup();
-	rte_trace_save();
-	eal_trace_fini();
 	eal_cleanup_config(internal_conf);
 	rte_eal_log_cleanup();
 	return 0;
-- 
2.33.0

[PATCH v3 2/4] eal: fix errno not set if strftime return zero

[Chengwen Feng]

The trace_session_name_generate() takes errno as the return value, but
the errno was not set if strftime return zero, the previously set errno
is returned in this case, this will result in inaccurate prompting.

This patch sets errno to ENOSPC if strftime return zero to fix it.

Fixes: 321dd5f8fa62 ("trace: add internal init and fini interface")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/eal/common/eal_common_trace_utils.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/eal/common/eal_common_trace_utils.c b/lib/eal/common/eal_common_trace_utils.c
index 64f58fb66a..09f97d3c34 100644
--- a/lib/eal/common/eal_common_trace_utils.c
+++ b/lib/eal/common/eal_common_trace_utils.c
@@ -109,8 +109,10 @@ trace_session_name_generate(char *trace_dir)
 
 	rc = strftime(trace_dir + rc, TRACE_DIR_STR_LEN - rc,
 			"%Y-%m-%d-%p-%I-%M-%S", tm_result);
-	if (rc == 0)
+	if (rc == 0) {
+		errno = ENOSPC;
 		goto fail;
+	}
 
 	return rc;
 fail:
-- 
2.33.0

Re: [PATCH v3 2/4] eal: fix errno not set if strftime return zero

[David Marchand]

On Fri, Jun 17, 2022 at 4:35 AM Chengwen Feng <fengchengwen@huawei.com> wrote:
>
> The trace_session_name_generate() takes errno as the return value, but
> the errno was not set if strftime return zero, the previously set errno
> is returned in this case, this will result in inaccurate prompting.
>
> This patch sets errno to ENOSPC if strftime return zero to fix it.
>
> Fixes: 321dd5f8fa62 ("trace: add internal init and fini interface")
> Cc: stable@dpdk.org
>
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>

Good catch.

With the next fix, the error condition (on trace_dir being too short
to accomodate with formatted date) cannot be reached anymore.
I squashed those two patches together.
I also updated the commitlog to give some details why we were getting
a EEXIST (or ENOENT) errno.

-- 
David Marchand

[PATCH v3 3/4] eal: fix trace init fail with long file-prefix

[Chengwen Feng]

Bug scenario:
1. start testpmd:
	dpdk-testpmd -l 4-6 -a 0000:7d:00.0 --trace=.* \
	--file-prefix=trace_autotest -- -i
2. then observed:
	EAL: eal_trace_init():93 failed to initialize trace \
		[No space left on device]
	EAL: FATAL: Cannot init trace
	EAL: Cannot init trace
	EAL: Error - exiting with code: 1

The root cause it that the offset set wrong with long file-prefix and
then lead the strftime return failed.

Fixes: 321dd5f8fa62 ("trace: add internal init and fini interface")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>
Acked-by: Jerin Jacob <jerinj@marvell.com>
---
 lib/eal/common/eal_common_trace_utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/eal/common/eal_common_trace_utils.c b/lib/eal/common/eal_common_trace_utils.c
index 09f97d3c34..2b55dbec65 100644
--- a/lib/eal/common/eal_common_trace_utils.c
+++ b/lib/eal/common/eal_common_trace_utils.c
@@ -104,7 +104,7 @@ trace_session_name_generate(char *trace_dir)
 	rc = rte_strscpy(trace_dir, eal_get_hugefile_prefix(),
 			TRACE_PREFIX_LEN);
 	if (rc == -E2BIG)
-		rc = TRACE_PREFIX_LEN;
+		rc = TRACE_PREFIX_LEN - 1;
 	trace_dir[rc++] = '-';
 
 	rc = strftime(trace_dir + rc, TRACE_DIR_STR_LEN - rc,
-- 
2.33.0

[PATCH v3 4/4] test: introduce trace-autotest with traces enabled

[Chengwen Feng]

Currently trace_autotest unit test is executed with traces disabled.
This patch introduces trace_autotest unit test with traces enabled,
and the traces file is written to the directory where dpdk-test is
located.

Note: this patch depends on following:
[1] eal: fix segment fault when exit trace
[2] eal: fix trace init fail with long file-prefix

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test/meson.build | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/app/test/meson.build b/app/test/meson.build
index 7fe261cae8..e56fb997bd 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -508,6 +508,16 @@ foreach arg : fast_tests
                 timeout : timeout_seconds_fast,
                 is_parallel : false,
                 suite : 'fast-tests')
+        if arg[0] == 'trace_autotest' and (not is_windows)
+            test_args += ['--trace=.*']
+            test_args += ['--trace-dir=@0@'.format(meson.current_build_dir())]
+            test(arg[0] + '_with_traces', dpdk_test,
+                    env : ['DPDK_TEST=' + arg[0]],
+                    args : test_args,
+                    timeout : timeout_seconds_fast,
+                    is_parallel : false,
+                    suite : 'fast-tests')
+        endif
     endif
 endforeach
 
-- 
2.33.0

Re: [PATCH v3 4/4] test: introduce trace-autotest with traces enabled

[David Marchand]

On Fri, Jun 17, 2022 at 4:35 AM Chengwen Feng <fengchengwen@huawei.com> wrote:
>
> Currently trace_autotest unit test is executed with traces disabled.
> This patch introduces trace_autotest unit test with traces enabled,
> and the traces file is written to the directory where dpdk-test is
> located.
>
>
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>

> ---
>  app/test/meson.build | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/app/test/meson.build b/app/test/meson.build
> index 7fe261cae8..e56fb997bd 100644
> --- a/app/test/meson.build
> +++ b/app/test/meson.build
> @@ -508,6 +508,16 @@ foreach arg : fast_tests
>                  timeout : timeout_seconds_fast,
>                  is_parallel : false,
>                  suite : 'fast-tests')
> +        if arg[0] == 'trace_autotest' and (not is_windows)

Parenthesis are unneeded here, I updated this when applying.


-- 
David Marchand

Re: [PATCH v3 0/4] bugfix for trace

[David Marchand]

On Fri, Jun 17, 2022 at 4:35 AM Chengwen Feng <fengchengwen@huawei.com> wrote:
>
> This patch contains three bugfix for trace and one test patch.
>
> ---
> v3:
> * address David Marchand's comments.
> * add xxx-by from David Marchand and Jerin Jacob.
> v2:
> * add test-by from Jerin Jacob for the 1/3 patch.
> * add freebsd change file because David Marchand verify it.
> * add more bugfix patch when try to enhance testcase.
>
> Chengwen Feng (4):
>   eal: fix segment fault when exit trace
>   eal: fix errno not set if strftime return zero
>   eal: fix trace init fail with long file-prefix
>   test: introduce trace-autotest with traces enabled
>
>  app/test/meson.build                    | 10 ++++++++++
>  lib/eal/common/eal_common_trace_utils.c |  6 ++++--
>  lib/eal/freebsd/eal.c                   |  4 ++--
>  lib/eal/linux/eal.c                     |  4 ++--
>  4 files changed, 18 insertions(+), 6 deletions(-)

Series applied, thanks.

-- 
David Marchand