On Fri, 8 Aug 2025 03:23:37 +0000
"Varghese, Vipin" <vipin.vargh...@amd.com> wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> Hi Khadem,
>
> Thank you for sharing but few thoughts here
>
> > -----Original Message-----
> > From: Khadem Ullah <14pwcse1...@uetpeshawar.edu.pk>
> > Sent: Monday, August 4, 2025 5:03 PM
> > To: step...@networkplumber.org; tho...@monjalon.net; Yigit, Ferruh
> > <ferruh.yi...@amd.com>; andrew.rybche...@oktetlabs.ru
> > Cc: dev@dpdk.org; sta...@dpdk.org; Khadem Ullah
> > <14pwcse1...@uetpeshawar.edu.pk>
> > Subject: [PATCH v7] app/testpmd: monitor state of primary process when using
> > secondary
> >
> > Caution: This message originated from an External Source. Use proper caution
> > when opening attachments, clicking links, or responding.
> >
> >
> > In secondary processes, accessing device after primary has exited will cause
> > crash.
> >
> > This patch adds a mechanism in testpmd to monitor the primary process from
> > the
> > secondary process.
> > When primary process exits it forces secondary to exit avoiding issues from
> > cleanup logic.
> >
> > Fixes: a550baf24af9 ('app/testpmd: support multi-process')
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: Khadem Ullah <14pwcse1...@uetpeshawar.edu.pk>
> > Signed-off-by: Stephen Hemminger <step...@networkplumber.org>
> > ---
> > app/test-pmd/testpmd.c | 47
> > +++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 46 insertions(+), 1 deletion(-)
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > bb88555328..b7affa6da9 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -101,13 +101,15 @@
> > uint16_t verbose_level = 0; /**< Silent by default. */ int
> > testpmd_logtype; /**< Log
> > type for testpmd logs */
> >
> > +/* Maximum delay for exiting after primary process. */ #define
> > +MONITOR_INTERVAL (500 * 1000)
> > +
> > /* use main core for command line ? */
> > uint8_t interactive = 0;
> > uint8_t auto_start = 0;
> > uint8_t tx_first;
> > char cmdline_filename[PATH_MAX] = {0};
> > bool echo_cmdline_file;
> > -
> > /*
> > * NUMA support configuration.
> > * When set, the NUMA support attempts to dispatch the allocation of the
> > @@ -
> > 4332,6 +4334,38 @@ signal_handler(int signum __rte_unused)
> > prompt_exit();
> > }
> >
> > +#ifndef RTE_EXEC_ENV_WINDOWS
> > +/* Alarm signal handler, used to check that primary process */ static
> > +void monitor_primary(void *arg __rte_unused) {
> > + if (rte_eal_primary_proc_alive(NULL)) {
> > + rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
> > + } else {
> > + /*
> > + * If primary process exits, then all the device information
> > + * is no longer valid. Calling any cleanup code is going to
> > + * run into use after free.
> > + */
> > + fprintf(stderr, "\nPrimary process is no longer active,
> > exiting...\n");
> > + exit(EXIT_FAILURE);
>
> Indeed, the idea for monitoring and finding if primary is still alive.
> When we exit, should not we need graceful exit? If yes, we can not simply use
> `exit`, but need to use `eal_cleanup`.
>
> Hence I request for rework with cleanup.
>
> NACK: Vipin Varghese <vipin.vargh...@amd.com>
Eal_cleanup will reference data that has already been freed by the primary.
That is the problem.
