Re: [dpdk-dev] [PATCH v2 1/6] eal: introduce oops handling API

[Jerin Jacob <jerinjacobk@gmail.com>]

On Wed, Aug 18, 2021 at 10:16 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Wed, 18 Aug 2021 15:07:25 +0530
> Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> > On Tue, Aug 17, 2021 at 9:22 PM Stephen Hemminger
> > <stephen@networkplumber.org> wrote:
> > >
> > > On Tue, 17 Aug 2021 20:57:50 +0530
> > > Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > >
> > > > On Tue, Aug 17, 2021 at 8:39 PM Stephen Hemminger
> > > > <stephen@networkplumber.org> wrote:
> > > > >
> > > > > On Tue, 17 Aug 2021 13:08:46 +0530
> > > > > Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > >
> > > > > > On Tue, Aug 17, 2021 at 9:23 AM Stephen Hemminger
> > > > > > <stephen@networkplumber.org> wrote:
> > > > > > >
> > > > > > > On Tue, 17 Aug 2021 08:57:18 +0530
> > > > > > > <jerinj@marvell.com> wrote:
> > > > > > >
> > > > > > > > From: Jerin Jacob <jerinj@marvell.com>
> > > > > > > >
> > > > > > > > Introducing oops handling API with following specification
> > > > > > > > and enable stub implementation for Linux and FreeBSD.
> > > > > > > >
> > > > > > > > On rte_eal_init() invocation, the EAL library installs the
> > > > > > > > oops handler for the essential signals.
> > > > > > > > The rte_oops_signals_enabled() API provides the list
> > > > > > > > of signals the library installed by the EAL.
> > > > > > >
> > > > > > > This is a big change, and many applications already handle these
> > > > > > > signals themselves. Therefore adding this needs to be opt-in
> > > > > > > and not enabled by default.
> > > > > >
> > > > > > In order to avoid every application explicitly register this
> > > > > > sighandler and to cater to the
> > > > > > co-existing application-specific signal-hander usage.
> > > > > > The following design has been chosen. (It is mentioned in the commit log,
> > > > > > I will describe here for more clarity)
> > > > > >
> > > > > > Case 1:
> > > > > > a) The application installs the signal handler prior to rte_eal_init().
> > > > > > b) Implementation stores the application-specific signal and replace a
> > > > > > signal handler as oops eal handler
> > > > > > c) when application/DPDK get the segfault, the default EAL oops
> > > > > > handler gets invoked
> > > > > > d) Then it dumps the EAL specific message, it calls the
> > > > > > application-specific signal handler
> > > > > > installed in step 1 by application. This avoids breaking any contract
> > > > > > with the application.
> > > > > > i.e Behavior is the same current EAL now.
> > > > > > That is the reason for not using SA_RESETHAND(which call SIG_DFL after
> > > > > > eal oops handler instead
> > > > > > application-specific handler)
> > > > > >
> > > > > > Case 2:
> > > > > > a) The application install the signal handler after rte_eal_init(),
> > > > > > b) EAL hander get replaced with application handle then the application can call
> > > > > > rte_oops_decode() to decode.
> > > > > >
> > > > > > In order to cater the above use case, rte_oops_signals_enabled() and
> > > > > > rte_oops_decode()
> > > > > > provided.
> > > > > >
> > > > > > Here we are not breaking any contract with the application.
> > > > > > Do you have concerns about this design?
> > > > >
> > > > > In our application as a service it is important not to do any backtrace
> > > > > in production. We rely on other infrastructure to process coredumps.
> > > >
> > > > Other infrastructure will work. For example, If we are using standard coredump
> > > > using linux infra. In Current implementation,
> > > > - EAL handler dump the DPDK OOPS like kernel on stderr
> > > > - Implementation calls SIG_DFL in eal oops handler
> > > > - The above step creates the coredump or re-directs any other
> > > > infrastructure you are using for coredump.
> > > >
> > > > >
> > > > > This should be controlled enabled by a command line argument.
> > > >
> > > > If we allow other infrastructure coredump to work as-is, why
> > > > enable/disable required from eal?
> > >
> > > The addition of DPDK OOPS adds additional steps which make all
> > > faults be identified as the oops code.
> >
> > Since we are using SA_ONSTACK it is not losing the original segfault
> > info.
> >
> > I verified like this, Please find below the steps.
> >
> > 0) Enable coredump infra in Linux using coredumpctl or so
> > 1) Apply this series
> > 2) Apply for the following patch to create a segfault from the library.
> > This will test, segfault caught by eal and forward to default Linux singal
> > handler.
> >
> > [main]dell[dpdk.org] $ git diff
> > diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
> > index 3438a96b75..b935c32c98 100644
> > --- a/lib/eal/linux/eal.c
> > +++ b/lib/eal/linux/eal.c
> > @@ -1338,6 +1338,8 @@ rte_eal_init(int argc, char **argv)
> >
> >         eal_mcfg_complete();
> >
> > +       /* Generate a segfault */
> > +       *(volatile int *)0x05 = 0;
> >         return fctret;
> >
> >  }
> > 3)Build
> > meson --buildtype debug build
> > ninja -C build
> >
> > 4) Run
> > $ ./build/app/test/dpdk-test --no-huge  -c 0x2
> >
> > Please find oops dump[1] and gdb core dump backtrace[2].
> > Gdb core dump trace preserves the original segfault cause and trace.
> >
> > Any other concerns?
>
> Your new oops handling duplicates existing code in our application
> (and I know others that do this as well). The problem is that an
> application may do this before calling rte_eal_init and your new
> code will break that.

Not sure what it breaks, Could you elaborate on this? Your app signal
handler will be called with the original signal
the info it is registered before rte_eal_init().

We can have an additional API to disable the oops prints if you
insist. (Though I don't the know use case
where someone needs this other than someone don't want to see/log this
print). If that is rational,
I can add API to disable oops print it. I prefer to install it by
default as it won't break anything and it helps
to not add oops API in existing apps i.e without calling any
additional features in all existing applications.

>
> Therefore my recommendation is that the new oops handling needs
> to be not a built in feature of EAL.
>
>
>