> 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 <pratee...@cse.iitb.ac.in>
> > Sent: Thursday, December 10, 2020 08:28
> > To: Li, Xiaoyun <xiaoyun...@intel.com>
> > Cc: Prateek Agarwal <prate...@gmail.com>; dev@dpdk.org;
> > tho...@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-boun...@dpdk.org> On Behalf Of Prateek Agarwal
> > >> Sent: Saturday, December 5, 2020 01:52
> > >> To: dev@dpdk.org
> > >> Cc: tho...@monjalon.net; Prateek Agarwal <prate...@gmail.com>;
> > >> Prateek Agarwal <pratee...@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 <pratee...@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
