[dpdk-dev] [PATCH] Remove printf from signal handler.

[dpdk-dev] [PATCH] Remove printf from signal handler.

[Prateek Agarwal]

printf is not async-signal safe. Using printf in signal handlers may lead to deadlock. Removed printf from signal handlers present in several applications.

Signed-off-by: Prateek Agarwal <prateekag@cse.iitb.ac.in>
---
 app/pdump/main.c             | 2 --
 app/test-eventdev/evt_main.c | 4 ----
 app/test-flow-perf/main.c    | 3 ---
 app/test-pmd/testpmd.c       | 2 --
 app/test/test_pmd_perf.c     | 1 -
 5 files changed, 12 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index b34bf3353..380f0ea0f 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -573,8 +573,6 @@ static void
 signal_handler(int sig_num)
 {
 	if (sig_num == SIGINT) {
-		printf("\n\nSignal %d received, preparing to exit...\n",
-				sig_num);
 		quit_signal = 1;
 	}
 }
diff --git a/app/test-eventdev/evt_main.c b/app/test-eventdev/evt_main.c
index a8d304bab..51d5897f8 100644
--- a/app/test-eventdev/evt_main.c
+++ b/app/test-eventdev/evt_main.c
@@ -22,12 +22,8 @@ signal_handler(int signum)
 {
 	int i;
 	static uint8_t once;
-
 	if ((signum == SIGINT || signum == SIGTERM) && !once) {
 		once = true;
-		printf("\nSignal %d received, preparing to exit...\n",
-				signum);
-
 		if (test != NULL) {
 			/* request all lcores to exit from the main loop */
 			*(int *)test->test_priv = true;
diff --git a/app/test-flow-perf/main.c b/app/test-flow-perf/main.c
index 03d01a8b5..aeb0ef3b0 100644
--- a/app/test-flow-perf/main.c
+++ b/app/test-flow-perf/main.c
@@ -1001,9 +1001,6 @@ static void
 signal_handler(int signum)
 {
 	if (signum == SIGINT || signum == SIGTERM) {
-		printf("\n\nSignal %d received, preparing to exit...\n",
-					signum);
-		printf("Error: Stats are wrong due to sudden signal!\n\n");
 		force_quit = true;
 	}
 }
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 33fc0fddf..7ec87e7fd 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3794,8 +3794,6 @@ static void
 signal_handler(int signum)
 {
 	if (signum == SIGINT || signum == SIGTERM) {
-		printf("\nSignal %d received, preparing to exit...\n",
-				signum);
 #ifdef RTE_LIB_PDUMP
 		/* uninitialize packet capture framework */
 		rte_pdump_uninit();
diff --git a/app/test/test_pmd_perf.c b/app/test/test_pmd_perf.c
index 4db816a36..58cb84401 100644
--- a/app/test/test_pmd_perf.c
+++ b/app/test/test_pmd_perf.c
@@ -319,7 +319,6 @@ signal_handler(int signum)
 {
 	/*  USR1 signal, stop testing */
 	if (signum == SIGUSR1) {
-		printf("Force Stop!\n");
 		stop = 1;
 	}
 
-- 
2.25.1

Re: [dpdk-dev] [PATCH] Remove printf from signal handler.

[Li, Xiaoyun]

Hi
I don't object with all the removing of printf.
Just one concern, I don't think you actually solved the problem in this patch.

Take testpmd as an example, the signal_handler includes many complicated actions after that very first printf like force_quit() which includes stop port, close port, hotplug... and of course a lot of printf in it.
So only removing the first printf doesn't actually solve the issue you mentioned.

And many examples do similar things as testpmd, they have the same issues too.

BRs
Xiaoyun

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Prateek Agarwal
> Sent: Saturday, December 5, 2020 01:52
> To: dev@dpdk.org
> Cc: thomas@monjalon.net; Prateek Agarwal <pratekag@gmail.com>; Prateek
> Agarwal <prateekag@cse.iitb.ac.in>
> Subject: [dpdk-dev] [PATCH] Remove printf from signal handler.
> 
> printf is not async-signal safe. Using printf in signal handlers may lead to
> deadlock. Removed printf from signal handlers present in several applications.
> 
> Signed-off-by: Prateek Agarwal <prateekag@cse.iitb.ac.in>
> ---
>  app/pdump/main.c             | 2 --
>  app/test-eventdev/evt_main.c | 4 ----
>  app/test-flow-perf/main.c    | 3 ---
>  app/test-pmd/testpmd.c       | 2 --
>  app/test/test_pmd_perf.c     | 1 -
>  5 files changed, 12 deletions(-)
> 
> diff --git a/app/pdump/main.c b/app/pdump/main.c index
> b34bf3353..380f0ea0f 100644
> --- a/app/pdump/main.c
> +++ b/app/pdump/main.c
> @@ -573,8 +573,6 @@ static void
>  signal_handler(int sig_num)
>  {
>  	if (sig_num == SIGINT) {
> -		printf("\n\nSignal %d received, preparing to exit...\n",
> -				sig_num);
>  		quit_signal = 1;
>  	}
>  }
> diff --git a/app/test-eventdev/evt_main.c b/app/test-eventdev/evt_main.c
> index a8d304bab..51d5897f8 100644
> --- a/app/test-eventdev/evt_main.c
> +++ b/app/test-eventdev/evt_main.c
> @@ -22,12 +22,8 @@ signal_handler(int signum)  {
>  	int i;
>  	static uint8_t once;
> -
>  	if ((signum == SIGINT || signum == SIGTERM) && !once) {
>  		once = true;
> -		printf("\nSignal %d received, preparing to exit...\n",
> -				signum);
> -
>  		if (test != NULL) {
>  			/* request all lcores to exit from the main loop */
>  			*(int *)test->test_priv = true;
> diff --git a/app/test-flow-perf/main.c b/app/test-flow-perf/main.c index
> 03d01a8b5..aeb0ef3b0 100644
> --- a/app/test-flow-perf/main.c
> +++ b/app/test-flow-perf/main.c
> @@ -1001,9 +1001,6 @@ static void
>  signal_handler(int signum)
>  {
>  	if (signum == SIGINT || signum == SIGTERM) {
> -		printf("\n\nSignal %d received, preparing to exit...\n",
> -					signum);
> -		printf("Error: Stats are wrong due to sudden signal!\n\n");
>  		force_quit = true;
>  	}
>  }
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 33fc0fddf..7ec87e7fd 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3794,8 +3794,6 @@ static void
>  signal_handler(int signum)
>  {
>  	if (signum == SIGINT || signum == SIGTERM) {
> -		printf("\nSignal %d received, preparing to exit...\n",
> -				signum);
>  #ifdef RTE_LIB_PDUMP
>  		/* uninitialize packet capture framework */
>  		rte_pdump_uninit();
> diff --git a/app/test/test_pmd_perf.c b/app/test/test_pmd_perf.c index
> 4db816a36..58cb84401 100644
> --- a/app/test/test_pmd_perf.c
> +++ b/app/test/test_pmd_perf.c
> @@ -319,7 +319,6 @@ signal_handler(int signum)  {
>  	/*  USR1 signal, stop testing */
>  	if (signum == SIGUSR1) {
> -		printf("Force Stop!\n");
>  		stop = 1;
>  	}
> 
> --
> 2.25.1

Re: [dpdk-dev] [PATCH] Remove printf from signal handler.

[prateekag]

Hi
Agree. What is the way out? The printfs you mentioned look like important 
messages and may have been called outside signal handler routines.
Shall they be removed as well or converted to "write" function call? Or we 
live with the possiblity of deadlock howsoever unlikely.
Regards
Prateek Agarwal


On 2020-12-08 08:28, Li, Xiaoyun wrote:
> Hi
> I don't object with all the removing of printf.
> Just one concern, I don't think you actually solved the problem in this 
> patch.
> 
> Take testpmd as an example, the signal_handler includes many complicated
> actions after that very first printf like force_quit() which includes stop
> port, close port, hotplug... and of course a lot of printf in it.
> So only removing the first printf doesn't actually solve the issue you 
> mentioned.
> 
> And many examples do similar things as testpmd, they have the same issues 
> too.
> 
> BRs
> Xiaoyun
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Prateek Agarwal
>> Sent: Saturday, December 5, 2020 01:52
>> To: dev@dpdk.org
>> Cc: thomas@monjalon.net; Prateek Agarwal <pratekag@gmail.com>; Prateek
>> Agarwal <prateekag@cse.iitb.ac.in>
>> Subject: [dpdk-dev] [PATCH] Remove printf from signal handler.
>> 
>> printf is not async-signal safe. Using printf in signal handlers may lead 
>> to
>> deadlock. Removed printf from signal handlers present in several 
>> applications.
>> 
>> Signed-off-by: Prateek Agarwal <prateekag@cse.iitb.ac.in>
>> ---
>>  app/pdump/main.c             | 2 --
>>  app/test-eventdev/evt_main.c | 4 ----
>>  app/test-flow-perf/main.c    | 3 ---
>>  app/test-pmd/testpmd.c       | 2 --
>>  app/test/test_pmd_perf.c     | 1 -
>>  5 files changed, 12 deletions(-)
>> 
>> diff --git a/app/pdump/main.c b/app/pdump/main.c index
>> b34bf3353..380f0ea0f 100644
>> --- a/app/pdump/main.c
>> +++ b/app/pdump/main.c
>> @@ -573,8 +573,6 @@ static void
>>  signal_handler(int sig_num)
>>  {
>>  	if (sig_num == SIGINT) {
>> -		printf("\n\nSignal %d received, preparing to exit...\n",
>> -				sig_num);
>>  		quit_signal = 1;
>>  	}
>>  }
>> diff --git a/app/test-eventdev/evt_main.c b/app/test-eventdev/evt_main.c
>> index a8d304bab..51d5897f8 100644
>> --- a/app/test-eventdev/evt_main.c
>> +++ b/app/test-eventdev/evt_main.c
>> @@ -22,12 +22,8 @@ signal_handler(int signum)  {
>>  	int i;
>>  	static uint8_t once;
>> -
>>  	if ((signum == SIGINT || signum == SIGTERM) && !once) {
>>  		once = true;
>> -		printf("\nSignal %d received, preparing to exit...\n",
>> -				signum);
>> -
>>  		if (test != NULL) {
>>  			/* request all lcores to exit from the main loop */
>>  			*(int *)test->test_priv = true;
>> diff --git a/app/test-flow-perf/main.c b/app/test-flow-perf/main.c index
>> 03d01a8b5..aeb0ef3b0 100644
>> --- a/app/test-flow-perf/main.c
>> +++ b/app/test-flow-perf/main.c
>> @@ -1001,9 +1001,6 @@ static void
>>  signal_handler(int signum)
>>  {
>>  	if (signum == SIGINT || signum == SIGTERM) {
>> -		printf("\n\nSignal %d received, preparing to exit...\n",
>> -					signum);
>> -		printf("Error: Stats are wrong due to sudden signal!\n\n");
>>  		force_quit = true;
>>  	}
>>  }
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>> 33fc0fddf..7ec87e7fd 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -3794,8 +3794,6 @@ static void
>>  signal_handler(int signum)
>>  {
>>  	if (signum == SIGINT || signum == SIGTERM) {
>> -		printf("\nSignal %d received, preparing to exit...\n",
>> -				signum);
>>  #ifdef RTE_LIB_PDUMP
>>  		/* uninitialize packet capture framework */
>>  		rte_pdump_uninit();
>> diff --git a/app/test/test_pmd_perf.c b/app/test/test_pmd_perf.c index
>> 4db816a36..58cb84401 100644
>> --- a/app/test/test_pmd_perf.c
>> +++ b/app/test/test_pmd_perf.c
>> @@ -319,7 +319,6 @@ signal_handler(int signum)  {
>>  	/*  USR1 signal, stop testing */
>>  	if (signum == SIGUSR1) {
>> -		printf("Force Stop!\n");
>>  		stop = 1;
>>  	}
>> 
>> --
>> 2.25.1

Re: [dpdk-dev] [PATCH] Remove printf from signal handler.

[Li, Xiaoyun]

Hi
I don't have a good solution here.
Removing doesn't seem a good idea because there are many important alerts or reminders in all of those function calls and they may be called in other places too.
I think replacing with "write" can't solve the problem too. Because behavior like stopping port will call driver functions. It's not very reasonable to me that replacing all possible logs. 
Use a flag and check the flag for each possible thread? Most examples already do this way. But for complicated app, it will be tricky and may cause other issues.

Maybe other people have better ideas?

BRs
Xiaoyun

> -----Original Message-----
> From: prateekag <prateekag@cse.iitb.ac.in>
> Sent: Thursday, December 10, 2020 08:28
> To: Li, Xiaoyun <xiaoyun.li@intel.com>
> Cc: Prateek Agarwal <pratekag@gmail.com>; dev@dpdk.org;
> thomas@monjalon.net
> Subject: Re: [dpdk-dev] [PATCH] Remove printf from signal handler.
> 
> Hi
> Agree. What is the way out? The printfs you mentioned look like important
> messages and may have been called outside signal handler routines.
> Shall they be removed as well or converted to "write" function call? Or we live
> with the possiblity of deadlock howsoever unlikely.
> Regards
> Prateek Agarwal
> 
> 
> On 2020-12-08 08:28, Li, Xiaoyun wrote:
> > Hi
> > I don't object with all the removing of printf.
> > Just one concern, I don't think you actually solved the problem in
> > this patch.
> >
> > Take testpmd as an example, the signal_handler includes many
> > complicated actions after that very first printf like force_quit()
> > which includes stop port, close port, hotplug... and of course a lot of printf in it.
> > So only removing the first printf doesn't actually solve the issue you
> > mentioned.
> >
> > And many examples do similar things as testpmd, they have the same
> > issues too.
> >
> > BRs
> > Xiaoyun
> >
> >> -----Original Message-----
> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Prateek Agarwal
> >> Sent: Saturday, December 5, 2020 01:52
> >> To: dev@dpdk.org
> >> Cc: thomas@monjalon.net; Prateek Agarwal <pratekag@gmail.com>;
> >> Prateek Agarwal <prateekag@cse.iitb.ac.in>
> >> Subject: [dpdk-dev] [PATCH] Remove printf from signal handler.
> >>
> >> printf is not async-signal safe. Using printf in signal handlers may
> >> lead to deadlock. Removed printf from signal handlers present in
> >> several applications.
> >>
> >> Signed-off-by: Prateek Agarwal <prateekag@cse.iitb.ac.in>
> >> ---
> >>  app/pdump/main.c             | 2 --
> >>  app/test-eventdev/evt_main.c | 4 ----
> >>  app/test-flow-perf/main.c    | 3 ---
> >>  app/test-pmd/testpmd.c       | 2 --
> >>  app/test/test_pmd_perf.c     | 1 -
> >>  5 files changed, 12 deletions(-)
> >>
> >> diff --git a/app/pdump/main.c b/app/pdump/main.c index
> >> b34bf3353..380f0ea0f 100644
> >> --- a/app/pdump/main.c
> >> +++ b/app/pdump/main.c
> >> @@ -573,8 +573,6 @@ static void
> >>  signal_handler(int sig_num)
> >>  {
> >>  	if (sig_num == SIGINT) {
> >> -		printf("\n\nSignal %d received, preparing to exit...\n",
> >> -				sig_num);
> >>  		quit_signal = 1;
> >>  	}
> >>  }
> >> diff --git a/app/test-eventdev/evt_main.c
> >> b/app/test-eventdev/evt_main.c index a8d304bab..51d5897f8 100644
> >> --- a/app/test-eventdev/evt_main.c
> >> +++ b/app/test-eventdev/evt_main.c
> >> @@ -22,12 +22,8 @@ signal_handler(int signum)  {
> >>  	int i;
> >>  	static uint8_t once;
> >> -
> >>  	if ((signum == SIGINT || signum == SIGTERM) && !once) {
> >>  		once = true;
> >> -		printf("\nSignal %d received, preparing to exit...\n",
> >> -				signum);
> >> -
> >>  		if (test != NULL) {
> >>  			/* request all lcores to exit from the main loop */
> >>  			*(int *)test->test_priv = true;
> >> diff --git a/app/test-flow-perf/main.c b/app/test-flow-perf/main.c
> >> index
> >> 03d01a8b5..aeb0ef3b0 100644
> >> --- a/app/test-flow-perf/main.c
> >> +++ b/app/test-flow-perf/main.c
> >> @@ -1001,9 +1001,6 @@ static void
> >>  signal_handler(int signum)
> >>  {
> >>  	if (signum == SIGINT || signum == SIGTERM) {
> >> -		printf("\n\nSignal %d received, preparing to exit...\n",
> >> -					signum);
> >> -		printf("Error: Stats are wrong due to sudden signal!\n\n");
> >>  		force_quit = true;
> >>  	}
> >>  }
> >> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> >> 33fc0fddf..7ec87e7fd 100644
> >> --- a/app/test-pmd/testpmd.c
> >> +++ b/app/test-pmd/testpmd.c
> >> @@ -3794,8 +3794,6 @@ static void
> >>  signal_handler(int signum)
> >>  {
> >>  	if (signum == SIGINT || signum == SIGTERM) {
> >> -		printf("\nSignal %d received, preparing to exit...\n",
> >> -				signum);
> >>  #ifdef RTE_LIB_PDUMP
> >>  		/* uninitialize packet capture framework */
> >>  		rte_pdump_uninit();
> >> diff --git a/app/test/test_pmd_perf.c b/app/test/test_pmd_perf.c
> >> index
> >> 4db816a36..58cb84401 100644
> >> --- a/app/test/test_pmd_perf.c
> >> +++ b/app/test/test_pmd_perf.c
> >> @@ -319,7 +319,6 @@ signal_handler(int signum)  {
> >>  	/*  USR1 signal, stop testing */
> >>  	if (signum == SIGUSR1) {
> >> -		printf("Force Stop!\n");
> >>  		stop = 1;
> >>  	}
> >>
> >> --
> >> 2.25.1

Re: [dpdk-dev] [PATCH] Remove printf from signal handler.

[Ananyev, Konstantin]

> Hi
> I don't have a good solution here.
> Removing doesn't seem a good idea because there are many important alerts or reminders in all of those function calls and they may be
> called in other places too.
> I think replacing with "write" can't solve the problem too. Because behavior like stopping port will call driver functions. It's not very
> reasonable to me that replacing all possible logs.
> Use a flag and check the flag for each possible thread? Most examples already do this way. But for complicated app, it will be tricky and
> may cause other issues.

I wonder do we really have to try to 'fix' that?
I don't remember anyone complained about such problem in 10 years DPDK exists.
Konstantin

> 
> Maybe other people have better ideas?
> 
> BRs
> Xiaoyun
> 
> > -----Original Message-----
> > From: prateekag <prateekag@cse.iitb.ac.in>
> > Sent: Thursday, December 10, 2020 08:28
> > To: Li, Xiaoyun <xiaoyun.li@intel.com>
> > Cc: Prateek Agarwal <pratekag@gmail.com>; dev@dpdk.org;
> > thomas@monjalon.net
> > Subject: Re: [dpdk-dev] [PATCH] Remove printf from signal handler.
> >
> > Hi
> > Agree. What is the way out? The printfs you mentioned look like important
> > messages and may have been called outside signal handler routines.
> > Shall they be removed as well or converted to "write" function call? Or we live
> > with the possiblity of deadlock howsoever unlikely.
> > Regards
> > Prateek Agarwal
> >
> >
> > On 2020-12-08 08:28, Li, Xiaoyun wrote:
> > > Hi
> > > I don't object with all the removing of printf.
> > > Just one concern, I don't think you actually solved the problem in
> > > this patch.
> > >
> > > Take testpmd as an example, the signal_handler includes many
> > > complicated actions after that very first printf like force_quit()
> > > which includes stop port, close port, hotplug... and of course a lot of printf in it.
> > > So only removing the first printf doesn't actually solve the issue you
> > > mentioned.
> > >
> > > And many examples do similar things as testpmd, they have the same
> > > issues too.
> > >
> > > BRs
> > > Xiaoyun
> > >
> > >> -----Original Message-----
> > >> From: dev <dev-bounces@dpdk.org> On Behalf Of Prateek Agarwal
> > >> Sent: Saturday, December 5, 2020 01:52
> > >> To: dev@dpdk.org
> > >> Cc: thomas@monjalon.net; Prateek Agarwal <pratekag@gmail.com>;
> > >> Prateek Agarwal <prateekag@cse.iitb.ac.in>
> > >> Subject: [dpdk-dev] [PATCH] Remove printf from signal handler.
> > >>
> > >> printf is not async-signal safe. Using printf in signal handlers may
> > >> lead to deadlock. Removed printf from signal handlers present in
> > >> several applications.
> > >>
> > >> Signed-off-by: Prateek Agarwal <prateekag@cse.iitb.ac.in>
> > >> ---
> > >>  app/pdump/main.c             | 2 --
> > >>  app/test-eventdev/evt_main.c | 4 ----
> > >>  app/test-flow-perf/main.c    | 3 ---
> > >>  app/test-pmd/testpmd.c       | 2 --
> > >>  app/test/test_pmd_perf.c     | 1 -
> > >>  5 files changed, 12 deletions(-)
> > >>
> > >> diff --git a/app/pdump/main.c b/app/pdump/main.c index
> > >> b34bf3353..380f0ea0f 100644
> > >> --- a/app/pdump/main.c
> > >> +++ b/app/pdump/main.c
> > >> @@ -573,8 +573,6 @@ static void
> > >>  signal_handler(int sig_num)
> > >>  {
> > >>  	if (sig_num == SIGINT) {
> > >> -		printf("\n\nSignal %d received, preparing to exit...\n",
> > >> -				sig_num);
> > >>  		quit_signal = 1;
> > >>  	}
> > >>  }
> > >> diff --git a/app/test-eventdev/evt_main.c
> > >> b/app/test-eventdev/evt_main.c index a8d304bab..51d5897f8 100644
> > >> --- a/app/test-eventdev/evt_main.c
> > >> +++ b/app/test-eventdev/evt_main.c
> > >> @@ -22,12 +22,8 @@ signal_handler(int signum)  {
> > >>  	int i;
> > >>  	static uint8_t once;
> > >> -
> > >>  	if ((signum == SIGINT || signum == SIGTERM) && !once) {
> > >>  		once = true;
> > >> -		printf("\nSignal %d received, preparing to exit...\n",
> > >> -				signum);
> > >> -
> > >>  		if (test != NULL) {
> > >>  			/* request all lcores to exit from the main loop */
> > >>  			*(int *)test->test_priv = true;
> > >> diff --git a/app/test-flow-perf/main.c b/app/test-flow-perf/main.c
> > >> index
> > >> 03d01a8b5..aeb0ef3b0 100644
> > >> --- a/app/test-flow-perf/main.c
> > >> +++ b/app/test-flow-perf/main.c
> > >> @@ -1001,9 +1001,6 @@ static void
> > >>  signal_handler(int signum)
> > >>  {
> > >>  	if (signum == SIGINT || signum == SIGTERM) {
> > >> -		printf("\n\nSignal %d received, preparing to exit...\n",
> > >> -					signum);
> > >> -		printf("Error: Stats are wrong due to sudden signal!\n\n");
> > >>  		force_quit = true;
> > >>  	}
> > >>  }
> > >> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > >> 33fc0fddf..7ec87e7fd 100644
> > >> --- a/app/test-pmd/testpmd.c
> > >> +++ b/app/test-pmd/testpmd.c
> > >> @@ -3794,8 +3794,6 @@ static void
> > >>  signal_handler(int signum)
> > >>  {
> > >>  	if (signum == SIGINT || signum == SIGTERM) {
> > >> -		printf("\nSignal %d received, preparing to exit...\n",
> > >> -				signum);
> > >>  #ifdef RTE_LIB_PDUMP
> > >>  		/* uninitialize packet capture framework */
> > >>  		rte_pdump_uninit();
> > >> diff --git a/app/test/test_pmd_perf.c b/app/test/test_pmd_perf.c
> > >> index
> > >> 4db816a36..58cb84401 100644
> > >> --- a/app/test/test_pmd_perf.c
> > >> +++ b/app/test/test_pmd_perf.c
> > >> @@ -319,7 +319,6 @@ signal_handler(int signum)  {
> > >>  	/*  USR1 signal, stop testing */
> > >>  	if (signum == SIGUSR1) {
> > >> -		printf("Force Stop!\n");
> > >>  		stop = 1;
> > >>  	}
> > >>
> > >> --
> > >> 2.25.1

Re: [dpdk-dev] [PATCH] Remove printf from signal handler.

[prateekag]

Hi,

We reused some code snippets of signal handlers in one of our projects and 
faced this problem of deadlock when the user sent an interrupt signal 
(ctrl-C) in quick succession.

This is a non-deterministic bug and highly unlikely to be observed 
frequently. The main idea behind pointing out was to look at the possible 
solutions suggested by the community. Another reason was to make everyone 
aware of this bug.

Anyways, leaving a comment in the source or mentioning this in documentation 
may not be a bad idea.

Regards
Prateek Agarwal


On 2020-12-10 14:54, Ananyev, Konstantin wrote:
>> Hi
>> I don't have a good solution here.
>> Removing doesn't seem a good idea because there are many important alerts 
>> or reminders in all of those function calls and they may be
>> called in other places too.
>> I think replacing with "write" can't solve the problem too. Because 
>> behavior like stopping port will call driver functions. It's not very
>> reasonable to me that replacing all possible logs.
>> Use a flag and check the flag for each possible thread? Most examples 
>> already do this way. But for complicated app, it will be tricky and
>> may cause other issues.
> 
> I wonder do we really have to try to 'fix' that?
> I don't remember anyone complained about such problem in 10 years DPDK 
> exists.
> Konstantin
> 
>> 
>> Maybe other people have better ideas?
>> 
>> BRs
>> Xiaoyun
>> 
>> > -----Original Message-----
>> > From: prateekag <prateekag@cse.iitb.ac.in>
>> > Sent: Thursday, December 10, 2020 08:28
>> > To: Li, Xiaoyun <xiaoyun.li@intel.com>
>> > Cc: Prateek Agarwal <pratekag@gmail.com>; dev@dpdk.org;
>> > thomas@monjalon.net
>> > Subject: Re: [dpdk-dev] [PATCH] Remove printf from signal handler.
>> >
>> > Hi
>> > Agree. What is the way out? The printfs you mentioned look like important
>> > messages and may have been called outside signal handler routines.
>> > Shall they be removed as well or converted to "write" function call? Or we live
>> > with the possiblity of deadlock howsoever unlikely.
>> > Regards
>> > Prateek Agarwal
>> >
>> >
>> > On 2020-12-08 08:28, Li, Xiaoyun wrote:
>> > > Hi
>> > > I don't object with all the removing of printf.
>> > > Just one concern, I don't think you actually solved the problem in
>> > > this patch.
>> > >
>> > > Take testpmd as an example, the signal_handler includes many
>> > > complicated actions after that very first printf like force_quit()
>> > > which includes stop port, close port, hotplug... and of course a lot of printf in it.
>> > > So only removing the first printf doesn't actually solve the issue you
>> > > mentioned.
>> > >
>> > > And many examples do similar things as testpmd, they have the same
>> > > issues too.
>> > >
>> > > BRs
>> > > Xiaoyun
>> > >
>> > >> -----Original Message-----
>> > >> From: dev <dev-bounces@dpdk.org> On Behalf Of Prateek Agarwal
>> > >> Sent: Saturday, December 5, 2020 01:52
>> > >> To: dev@dpdk.org
>> > >> Cc: thomas@monjalon.net; Prateek Agarwal <pratekag@gmail.com>;
>> > >> Prateek Agarwal <prateekag@cse.iitb.ac.in>
>> > >> Subject: [dpdk-dev] [PATCH] Remove printf from signal handler.
>> > >>
>> > >> printf is not async-signal safe. Using printf in signal handlers may
>> > >> lead to deadlock. Removed printf from signal handlers present in
>> > >> several applications.
>> > >>
>> > >> Signed-off-by: Prateek Agarwal <prateekag@cse.iitb.ac.in>
>> > >> ---
>> > >>  app/pdump/main.c             | 2 --
>> > >>  app/test-eventdev/evt_main.c | 4 ----
>> > >>  app/test-flow-perf/main.c    | 3 ---
>> > >>  app/test-pmd/testpmd.c       | 2 --
>> > >>  app/test/test_pmd_perf.c     | 1 -
>> > >>  5 files changed, 12 deletions(-)
>> > >>
>> > >> diff --git a/app/pdump/main.c b/app/pdump/main.c index
>> > >> b34bf3353..380f0ea0f 100644
>> > >> --- a/app/pdump/main.c
>> > >> +++ b/app/pdump/main.c
>> > >> @@ -573,8 +573,6 @@ static void
>> > >>  signal_handler(int sig_num)
>> > >>  {
>> > >>  	if (sig_num == SIGINT) {
>> > >> -		printf("\n\nSignal %d received, preparing to exit...\n",
>> > >> -				sig_num);
>> > >>  		quit_signal = 1;
>> > >>  	}
>> > >>  }
>> > >> diff --git a/app/test-eventdev/evt_main.c
>> > >> b/app/test-eventdev/evt_main.c index a8d304bab..51d5897f8 100644
>> > >> --- a/app/test-eventdev/evt_main.c
>> > >> +++ b/app/test-eventdev/evt_main.c
>> > >> @@ -22,12 +22,8 @@ signal_handler(int signum)  {
>> > >>  	int i;
>> > >>  	static uint8_t once;
>> > >> -
>> > >>  	if ((signum == SIGINT || signum == SIGTERM) && !once) {
>> > >>  		once = true;
>> > >> -		printf("\nSignal %d received, preparing to exit...\n",
>> > >> -				signum);
>> > >> -
>> > >>  		if (test != NULL) {
>> > >>  			/* request all lcores to exit from the main loop */
>> > >>  			*(int *)test->test_priv = true;
>> > >> diff --git a/app/test-flow-perf/main.c b/app/test-flow-perf/main.c
>> > >> index
>> > >> 03d01a8b5..aeb0ef3b0 100644
>> > >> --- a/app/test-flow-perf/main.c
>> > >> +++ b/app/test-flow-perf/main.c
>> > >> @@ -1001,9 +1001,6 @@ static void
>> > >>  signal_handler(int signum)
>> > >>  {
>> > >>  	if (signum == SIGINT || signum == SIGTERM) {
>> > >> -		printf("\n\nSignal %d received, preparing to exit...\n",
>> > >> -					signum);
>> > >> -		printf("Error: Stats are wrong due to sudden signal!\n\n");
>> > >>  		force_quit = true;
>> > >>  	}
>> > >>  }
>> > >> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>> > >> 33fc0fddf..7ec87e7fd 100644
>> > >> --- a/app/test-pmd/testpmd.c
>> > >> +++ b/app/test-pmd/testpmd.c
>> > >> @@ -3794,8 +3794,6 @@ static void
>> > >>  signal_handler(int signum)
>> > >>  {
>> > >>  	if (signum == SIGINT || signum == SIGTERM) {
>> > >> -		printf("\nSignal %d received, preparing to exit...\n",
>> > >> -				signum);
>> > >>  #ifdef RTE_LIB_PDUMP
>> > >>  		/* uninitialize packet capture framework */
>> > >>  		rte_pdump_uninit();
>> > >> diff --git a/app/test/test_pmd_perf.c b/app/test/test_pmd_perf.c
>> > >> index
>> > >> 4db816a36..58cb84401 100644
>> > >> --- a/app/test/test_pmd_perf.c
>> > >> +++ b/app/test/test_pmd_perf.c
>> > >> @@ -319,7 +319,6 @@ signal_handler(int signum)  {
>> > >>  	/*  USR1 signal, stop testing */
>> > >>  	if (signum == SIGUSR1) {
>> > >> -		printf("Force Stop!\n");
>> > >>  		stop = 1;
>> > >>  	}
>> > >>
>> > >> --
>> > >> 2.25.1

[PATCH] app: do not call printf in signal handlers

[Stephen Hemminger]

Using printf is not async-signal safe and worst case may lead to deadlock.
Remove printf from signal handlers present in several applications.

Testpmd was already fixed by
commit 0fd1386c30c3 ("app/testpmd: cleanup cleanly from signal")

Signed-off-by: Prateek Agarwal <prateekag@cse.iitb.ac.in>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/pdump/main.c             | 2 --
 app/test-eventdev/evt_main.c | 3 ---
 app/test-flow-perf/main.c    | 3 ---
 app/test/test_pmd_perf.c     | 1 -
 4 files changed, 9 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index c6cf9d9c8769..c94606275b28 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -571,8 +571,6 @@ static void
 signal_handler(int sig_num)
 {
 	if (sig_num == SIGINT) {
-		printf("\n\nSignal %d received, preparing to exit...\n",
-				sig_num);
 		quit_signal = 1;
 	}
 }
diff --git a/app/test-eventdev/evt_main.c b/app/test-eventdev/evt_main.c
index 5c7ac2cce9ab..13a8500ef730 100644
--- a/app/test-eventdev/evt_main.c
+++ b/app/test-eventdev/evt_main.c
@@ -21,9 +21,6 @@ static void
 signal_handler(int signum)
 {
 	if (signum == SIGINT || signum == SIGTERM) {
-		printf("\nSignal %d received, preparing to exit...\n",
-				signum);
-
 		if (test != NULL) {
 			/* request all lcores to exit from the main loop */
 			*(int *)test->test_priv = true;
diff --git a/app/test-flow-perf/main.c b/app/test-flow-perf/main.c
index e0ef78a84013..e224ef67983d 100644
--- a/app/test-flow-perf/main.c
+++ b/app/test-flow-perf/main.c
@@ -1708,9 +1708,6 @@ static void
 signal_handler(int signum)
 {
 	if (signum == SIGINT || signum == SIGTERM) {
-		printf("\n\nSignal %d received, preparing to exit...\n",
-					signum);
-		printf("Error: Stats are wrong due to sudden signal!\n\n");
 		force_quit = true;
 	}
 }
diff --git a/app/test/test_pmd_perf.c b/app/test/test_pmd_perf.c
index ff84d251ff5f..3ef590cb517d 100644
--- a/app/test/test_pmd_perf.c
+++ b/app/test/test_pmd_perf.c
@@ -318,7 +318,6 @@ signal_handler(int signum)
 {
 	/*  USR1 signal, stop testing */
 	if (signum == SIGUSR1) {
-		printf("Force Stop!\n");
 		stop = 1;
 	}
 
-- 
2.39.2

Re: [PATCH] app: do not call printf in signal handlers

[Tyler Retzlaff]

On Mon, Jun 12, 2023 at 05:11:50PM -0700, Stephen Hemminger wrote:
> Using printf is not async-signal safe and worst case may lead to deadlock.
> Remove printf from signal handlers present in several applications.
> 
> Testpmd was already fixed by
> commit 0fd1386c30c3 ("app/testpmd: cleanup cleanly from signal")
> 
> Signed-off-by: Prateek Agarwal <prateekag@cse.iitb.ac.in>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---

Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Re: [PATCH] app: do not call printf in signal handlers

[Thomas Monjalon]

15/06/2023 22:14, Tyler Retzlaff:
> On Mon, Jun 12, 2023 at 05:11:50PM -0700, Stephen Hemminger wrote:
> > Using printf is not async-signal safe and worst case may lead to deadlock.
> > Remove printf from signal handlers present in several applications.
> > 
> > Testpmd was already fixed by
> > commit 0fd1386c30c3 ("app/testpmd: cleanup cleanly from signal")
> > 
> > Signed-off-by: Prateek Agarwal <prateekag@cse.iitb.ac.in>
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> 
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Applied, thanks for reviving this old patch.