| Message ID | 20201008153048.19369-2-rohit.raj@nxp.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Thomas Monjalon |
| Headers | show |
| Series | [v4,1/5] eal: add API for bus close | expand |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
On Thu, Oct 8, 2020 at 5:31 PM <rohit.raj@nxp.com> wrote: > > From: Rohit Raj <rohit.raj@nxp.com> > > Certain bus objects may need to be closed and re-acquired > while terminating and rerunning the client application. > Hence a signal handler is required to catch the termination > of the App and hence closing the bus objects. > > This patch adds the missing signal handler in the client > app and closes the Bus objects in both client and server > applications when the signal Handler is called. > > Signed-off-by: Rohit Raj <rohit.raj@nxp.com> > --- > .../multi_process/client_server_mp/mp_client/client.c | 11 +++++++++++ > .../multi_process/client_server_mp/mp_server/main.c | 4 +++- > 2 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/examples/multi_process/client_server_mp/mp_client/client.c b/examples/multi_process/client_server_mp/mp_client/client.c > index 361d90b54..c37516b4c 100644 > --- a/examples/multi_process/client_server_mp/mp_client/client.c > +++ b/examples/multi_process/client_server_mp/mp_client/client.c > @@ -11,6 +11,7 @@ > #include <stdlib.h> > #include <getopt.h> > #include <string.h> > +#include <signal.h> > > #include <rte_common.h> > #include <rte_malloc.h> > @@ -196,6 +197,14 @@ handle_packet(struct rte_mbuf *buf) > > } > > +static void > +signal_handler(int signal) > +{ > + if (signal == SIGINT) > + rte_eal_cleanup(); > + exit(0); > +} Calling rte_eal_cleanup from a signal handler is a bad idea. In most cases, you are racing with other threads still using DPDK resources. https://git.dpdk.org/dpdk/commit?id=2c434431f4 https://git.dpdk.org/dpdk/commit?id=613ce6691c This might not be a problem in this multi_process example, but let's keep a consistent way across all examples. Thanks.
在 2020/10/18 17:25, David Marchand 写道: > On Thu, Oct 8, 2020 at 5:31 PM <rohit.raj@nxp.com> wrote: >> >> From: Rohit Raj <rohit.raj@nxp.com> >> >> Certain bus objects may need to be closed and re-acquired >> while terminating and rerunning the client application. >> Hence a signal handler is required to catch the termination >> of the App and hence closing the bus objects. >> >> This patch adds the missing signal handler in the client >> app and closes the Bus objects in both client and server >> applications when the signal Handler is called. >> >> Signed-off-by: Rohit Raj <rohit.raj@nxp.com> >> --- >> .../multi_process/client_server_mp/mp_client/client.c | 11 +++++++++++ >> .../multi_process/client_server_mp/mp_server/main.c | 4 +++- >> 2 files changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/examples/multi_process/client_server_mp/mp_client/client.c b/examples/multi_process/client_server_mp/mp_client/client.c >> index 361d90b54..c37516b4c 100644 >> --- a/examples/multi_process/client_server_mp/mp_client/client.c >> +++ b/examples/multi_process/client_server_mp/mp_client/client.c >> @@ -11,6 +11,7 @@ >> #include <stdlib.h> >> #include <getopt.h> >> #include <string.h> >> +#include <signal.h> >> >> #include <rte_common.h> >> #include <rte_malloc.h> >> @@ -196,6 +197,14 @@ handle_packet(struct rte_mbuf *buf) >> >> } >> >> +static void >> +signal_handler(int signal) >> +{ >> + if (signal == SIGINT) >> + rte_eal_cleanup(); >> + exit(0); >> +} > > Calling rte_eal_cleanup from a signal handler is a bad idea. > In most cases, you are racing with other threads still using DPDK resources. > https://git.dpdk.org/dpdk/commit?id=2c434431f4 > https://git.dpdk.org/dpdk/commit?id=613ce6691c > > This might not be a problem in this multi_process example, but let's > keep a consistent way across all examples. > Thanks. Hi， I want to know why you don't think he's a problem. recently, we use the patch https://patchwork.dpdk.org/patch/86988/ startup with multiprocess. Use the tester to send traffic and kill the slave process repeatedly. The coredump exception occurs. According to my analysis, the cause is that resources are not released after the slave process is killed. Thanks Lijun Ou >
On Mon, Jan 25, 2021 at 12:07 PM oulijun <oulijun@huawei.com> wrote: > >> +static void > >> +signal_handler(int signal) > >> +{ > >> + if (signal == SIGINT) > >> + rte_eal_cleanup(); > >> + exit(0); > >> +} > > > > Calling rte_eal_cleanup from a signal handler is a bad idea. > > In most cases, you are racing with other threads still using DPDK resources. > > https://git.dpdk.org/dpdk/commit?id=2c434431f4 > > https://git.dpdk.org/dpdk/commit?id=613ce6691c > > > > This might not be a problem in this multi_process example, but let's > > keep a consistent way across all examples. > > Thanks. > Hi， > I want to know why you don't think he's a problem. recently, we use > the patch > https://patchwork.dpdk.org/patch/86988/ > startup with multiprocess. Use the tester to send traffic and kill the > slave process repeatedly. > The coredump exception occurs. According to my analysis, the cause is > that resources are not released after the slave process is killed. > Not sure I get your question and I don't see the relation with the testpmd patch. I did not say we must not release resources. Just to rephrase my previous mail: Generally speaking, calling rte_eal_cleanup() from a signal handler is wrong since it creates races with other threads. I recommend putting in place a synchronisation mechanism so that all worker threads break out of their processing loop and the main thread synchronizes/joins with them before calling rte_eal_cleanup() in its exit path. Now, for this patch, in examples/multi_process/client_server_mp/mp_client/client.c, this secondary process code seems to only have one thread (but I might be wrong). If this is true, then no race in theory => my comment "This might not be a problem in this multi_process example".
diff --git a/examples/multi_process/client_server_mp/mp_client/client.c b/examples/multi_process/client_server_mp/mp_client/client.c index 361d90b54..c37516b4c 100644 --- a/examples/multi_process/client_server_mp/mp_client/client.c +++ b/examples/multi_process/client_server_mp/mp_client/client.c @@ -11,6 +11,7 @@ #include <stdlib.h> #include <getopt.h> #include <string.h> +#include <signal.h> #include <rte_common.h> #include <rte_malloc.h> @@ -196,6 +197,14 @@ handle_packet(struct rte_mbuf *buf) } +static void +signal_handler(int signal) +{ + if (signal == SIGINT) + rte_eal_cleanup(); + exit(0); +} + /* * Application main function - loops through * receiving and processing packets. Never returns @@ -217,6 +226,8 @@ main(int argc, char *argv[]) argc -= retval; argv += retval; + signal(SIGINT, signal_handler); + if (parse_app_args(argc, argv) < 0) rte_exit(EXIT_FAILURE, "Invalid command-line arguments\n"); diff --git a/examples/multi_process/client_server_mp/mp_server/main.c b/examples/multi_process/client_server_mp/mp_server/main.c index 280dab867..b0241cc20 100644 --- a/examples/multi_process/client_server_mp/mp_server/main.c +++ b/examples/multi_process/client_server_mp/mp_server/main.c @@ -275,11 +275,13 @@ signal_handler(int signal) { uint16_t port_id; - if (signal == SIGINT) + if (signal == SIGINT) { RTE_ETH_FOREACH_DEV(port_id) { rte_eth_dev_stop(port_id); rte_eth_dev_close(port_id); } + rte_eal_cleanup(); + } exit(0); }