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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> 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