From: Jerin Jacob <jerinjacobk@gmail.com>
To: Stephen Hemminger <stephen@networkplumber.org>
Cc: Jerin Jacob <jerinj@marvell.com>, dpdk-dev <dev@dpdk.org>,
Thomas Monjalon <thomas@monjalon.net>,
David Marchand <david.marchand@redhat.com>,
"Richardson, Bruce" <bruce.richardson@intel.com>,
Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>,
Narcisa Ana Maria Vasile <navasile@linux.microsoft.com>,
"Dmitry Malloy (MESHCHANINOV)" <dmitrym@microsoft.com>,
Pallavi Kadam <pallavi.kadam@intel.com>,
"Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
"Ruifeng Wang (Arm Technology China)" <ruifeng.wang@arm.com>,
Jan Viktorin <viktorin@rehivetech.com>,
David Christensen <drc@linux.vnet.ibm.com>
Subject: Re: [dpdk-dev] [PATCH v2 2/6] eal: oops handling API implementation
Date: Tue, 17 Aug 2021 15:54:15 +0530 [thread overview]
Message-ID: <CALBAE1Mkfnurc69badJNoA7E0G+qSMLAvBaPfum3kLsjqTaZTQ@mail.gmail.com> (raw)
In-Reply-To: <20210816205235.3dd48286@hermes.local>
On Tue, Aug 17, 2021 at 9:22 AM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Tue, 17 Aug 2021 08:57:19 +0530
> <jerinj@marvell.com> wrote:
>
> > +#define oops_print(...) rte_log(RTE_LOG_ERR, RTE_LOGTYPE_EAL, __VA_ARGS__)
>
> It is problematic to call rte_log from a signal handler.
> The malloc pool maybe corrupted and rte_log can call functions that
> use malloc.
OK. What to use instead, fprint(stderr, ...)?
>
> Even rte_dump_stack() is unsafe from these signals.
OK
>
> > +
> > +static int oops_signals[] = {SIGSEGV, SIGBUS, SIGILL, SIGABRT, SIGFPE, SIGSYS};
>
> Should be constant.
Ack
>
> > +
> > +struct oops_signal {
> > + int sig;
>
> Redundant, you defined the oops_signals above.
Ack.
>
> > + bool enabled;
>
> Redundant, you can just compare with action.
Anyway, we need to database to hold the sigactions. This makes clean
to implement rte_oops_signals_enabled().
Also != SIG_DFL is not enabled.
>
> > + struct sigaction sa;
> > +};
> > +
> > +static struct oops_signal signals_db[RTE_DIM(oops_signals)];
> > +
> > +static void
> > +back_trace_dump(ucontext_t *context)
> > +{
> > + RTE_SET_USED(context);
> > +
> > + rte_dump_stack();
> > +}
>
> rte_dump_stack() is not safe in signal handler:
>
> Recommend backtrace_symbols_fd ??
>
> Better yet use libunwind
libunwind is an optional dependency. You can see in the next patch,
back_trace_dump() will be implemented with libunwind based stack unwind,
if the dependency is met.
>
> > +static void
> > +siginfo_dump(int sig, siginfo_t *info)
> > +{
> > + oops_print("PID: %" PRIdMAX "\n", (intmax_t)getpid());
> > +
> > + if (info == NULL)
> > + return;
> > + if (sig != info->si_signo)
> > + oops_print("Invalid signal info\n");
> > +
> > + oops_print("Signal number: %d\n", info->si_signo);
> > + oops_print("Fault address: %p\n", info->si_addr);
> > +}
> > +
> > +static void
> > +mem32_dump(void *ptr)
>
> Should be const
Ack.
>
> > +{
> > + uint32_t *p = ptr;
> > + int i;
> > +
> > + for (i = 0; i < 16; i++)
> > + oops_print("%p: 0x%x\n", p + i, rte_be_to_cpu_32(p[i]));
> > +}
>
> Why reinvent hexdump?
Make sense. I can change to hexdump, But, it will use rte_log. Shouldn't we use
fprint(stderr,..) variant.
>
> > +
> > +static void
> > +stack_dump_header(void)
> > +{
> > + oops_print("Stack dump:\n");
> > + oops_print("----------\n");
> > +}
> > +
> > +static void
> > +code_dump_header(void)
> > +{
> > + oops_print("Code dump:\n");
> > + oops_print("----------\n");
> > +}
> > +
> > +static void
> > +stack_code_dump(void *stack, void *code)
> > +{
> > + if (stack == NULL || code == NULL)
> > + return;
> > +
> > + oops_print("\n");
> > + stack_dump_header();
> > + mem32_dump(stack);
> > + oops_print("\n");
> > +
> > + code_dump_header();
> > + mem32_dump(code);
> > + oops_print("\n");
> > +}
> > +static void
> > +archinfo_dump(ucontext_t *uc)
> > {
> > - RTE_SET_USED(sig);
> > - RTE_SET_USED(info);
> > RTE_SET_USED(uc);
> >
> > + stack_code_dump(NULL, NULL);
> > +}
> > +
> > +static void
> > +default_signal_handler_invoke(int sig)
> > +{
> > + unsigned int idx;
> > +
> > + for (idx = 0; idx < RTE_DIM(oops_signals); idx++) {
> > + /* Skip disabled signals */
> > + if (signals_db[idx].sig != sig)
> > + continue;
> > + if (!signals_db[idx].enabled)
> > + continue;
> > + /* Replace with stored handler */
> > + sigaction(sig, &signals_db[idx].sa, NULL);
> > + kill(getpid(), sig);
>
> If you use SA_RESETHAND, you don't need this stuff.
As mentioned in other 1/6 email reply, This is NOT the case where
SIG_DFL handler
called from eal oops handler, instead, it will be calling the signal
handler which
is registered prior to rte_eal_init() which is stored local database.
>
> > + }
> > +}
> > +
> > +void
> > +rte_oops_decode(int sig, siginfo_t *info, ucontext_t *uc)
> > +{
> > + oops_print("Signal info:\n");
> > + oops_print("------------\n");
> > + siginfo_dump(sig, info);
> > + oops_print("\n");
> > +
> > + oops_print("Backtrace:\n");
> > + oops_print("----------\n");
> > + back_trace_dump(uc);
> > + oops_print("\n");
> > +
> > + oops_print("Arch info:\n");
> > + oops_print("----------\n");
> > + if (uc)
> > + archinfo_dump(uc);
> > +}
> > +
> > +static void
> > +eal_oops_handler(int sig, siginfo_t *info, void *ctx)
> > +{
> > + ucontext_t *uc = ctx;
> > +
> > + rte_oops_decode(sig, info, uc);
> > + default_signal_handler_invoke(sig);
>
> If you use SA_RESETHAND, then just doing raise(sig) here.
> > }
> >
> > int
> > rte_oops_signals_enabled(int *signals)
>
> Why is this necessary and exported?
Explained in 1/6 email reply.
next prev parent reply other threads:[~2021-08-17 10:24 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-30 8:49 [dpdk-dev] 0/6] support oops handling jerinj
2021-07-30 8:49 ` [dpdk-dev] 1/6] eal: introduce oops handling API jerinj
2021-08-17 3:27 ` [dpdk-dev] [PATCH v2 0/6] support oops handling jerinj
2021-08-17 3:27 ` [dpdk-dev] [PATCH v2 1/6] eal: introduce oops handling API jerinj
2021-08-17 3:53 ` Stephen Hemminger
2021-08-17 7:38 ` Jerin Jacob
2021-08-17 15:09 ` Stephen Hemminger
2021-08-17 15:27 ` Jerin Jacob
2021-08-17 15:52 ` Stephen Hemminger
2021-08-18 9:37 ` Jerin Jacob
2021-08-18 16:46 ` Stephen Hemminger
2021-08-18 18:04 ` Jerin Jacob
2021-08-17 3:27 ` [dpdk-dev] [PATCH v2 2/6] eal: oops handling API implementation jerinj
2021-08-17 3:52 ` Stephen Hemminger
2021-08-17 10:24 ` Jerin Jacob [this message]
2021-08-17 3:27 ` [dpdk-dev] [PATCH v2 3/6] eal: support libunwind based backtrace jerinj
2021-08-17 3:27 ` [dpdk-dev] [PATCH v2 4/6] eal/x86: support register dump for oops jerinj
2021-08-17 3:27 ` [dpdk-dev] [PATCH v2 5/6] eal/arm64: " jerinj
2021-08-17 3:27 ` [dpdk-dev] [PATCH v2 6/6] test/oops: support unit test case for oops handling APIs jerinj
2021-09-06 4:17 ` [dpdk-dev] [PATCH v3 0/6] support oops handling jerinj
2021-09-06 4:17 ` [dpdk-dev] [PATCH v3 1/6] eal: introduce oops handling API jerinj
2021-09-06 4:17 ` [dpdk-dev] [PATCH v3 2/6] eal: oops handling API implementation jerinj
2021-09-06 4:17 ` [dpdk-dev] [PATCH v3 3/6] eal: support libunwind based backtrace jerinj
2022-01-27 20:47 ` Stephen Hemminger
2022-01-28 4:33 ` Jerin Jacob
2022-01-28 8:41 ` Thomas Monjalon
2022-01-28 14:27 ` Jerin Jacob
2022-01-28 17:05 ` Stephen Hemminger
2021-09-06 4:17 ` [dpdk-dev] [PATCH v3 4/6] eal/x86: support register dump for oops jerinj
2021-09-06 4:17 ` [dpdk-dev] [PATCH v3 5/6] eal/arm64: " jerinj
2021-09-06 4:17 ` [dpdk-dev] [PATCH v3 6/6] test/oops: support unit test case for oops handling APIs jerinj
2021-09-21 17:30 ` [dpdk-dev] [PATCH v3 0/6] support oops handling Thomas Monjalon
2021-09-21 17:54 ` Jerin Jacob
2021-09-22 7:34 ` Thomas Monjalon
2021-09-22 8:03 ` Jerin Jacob
2021-09-22 8:33 ` Thomas Monjalon
2021-09-22 8:49 ` Jerin Jacob
2021-07-30 8:49 ` [dpdk-dev] 2/6] eal: oops handling API implementation jerinj
2021-08-02 22:46 ` David Christensen
2021-07-30 8:49 ` [dpdk-dev] 3/6] eal: support libunwind based backtrace jerinj
2021-07-30 8:49 ` [dpdk-dev] 4/6] eal/x86: support register dump for oops jerinj
2021-07-30 8:49 ` [dpdk-dev] 5/6] eal/arm64: " jerinj
2021-08-02 22:49 ` David Christensen
2021-08-16 16:24 ` Jerin Jacob
2021-07-30 8:49 ` [dpdk-dev] 6/6] test/oops: support unit test case for oops handling APIs jerinj
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CALBAE1Mkfnurc69badJNoA7E0G+qSMLAvBaPfum3kLsjqTaZTQ@mail.gmail.com \
--to=jerinjacobk@gmail.com \
--cc=bruce.richardson@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=dmitry.kozliuk@gmail.com \
--cc=dmitrym@microsoft.com \
--cc=drc@linux.vnet.ibm.com \
--cc=jerinj@marvell.com \
--cc=konstantin.ananyev@intel.com \
--cc=navasile@linux.microsoft.com \
--cc=pallavi.kadam@intel.com \
--cc=ruifeng.wang@arm.com \
--cc=stephen@networkplumber.org \
--cc=thomas@monjalon.net \
--cc=viktorin@rehivetech.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).