DPDK patches and discussions
 help / color / mirror / Atom feed
From: Aaron Conole <aconole@redhat.com>
To: "Burakov\, Anatoly" <anatoly.burakov@intel.com>
Cc: David Marchand <david.marchand@redhat.com>,
	Harry van Haaren <harry.van.haaren@intel.com>, "Ananyev\,
	Konstantin" <konstantin.ananyev@intel.com>, dev <dev@dpdk.org>,
	dpdk stable <stable@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v2] eal/service: fix exit by resetting service lcores
Date: Tue, 14 Apr 2020 09:22:26 -0400	[thread overview]
Message-ID: <f7two6i5hil.fsf@dhcp-25.97.bos.redhat.com> (raw)
In-Reply-To: <c6819060-183a-108f-258c-832f1eb120d6@intel.com> (Anatoly Burakov's message of "Mon, 6 Apr 2020 11:30:46 +0100")

"Burakov, Anatoly" <anatoly.burakov@intel.com> writes:

> On 13-Mar-20 10:04 AM, David Marchand wrote:
>> On Wed, Mar 11, 2020 at 3:39 PM Harry van Haaren
>> <harry.van.haaren@intel.com> wrote:
>>>
>>> This commit releases all service cores from their role,
>>> returning them to ROLE_RTE on rte_service_finalize().
>>>
>>> This may fix an issue relating to the service cores causing
>>
>> s/may fix/fixes/
>>
>>> a race-condition on eal_cleanup(), where the service core
>>> could still be executing while the main thread has already
>>> free-d the service memory, leading to a segfault.
>>>
>>> Fixes: 21698354c832 ("service: introduce service cores concept")
>>
>> Replaced with:
>> Fixes: da23f0aa87d8 ("service: fix memory leak with new function")
>>
>>> Cc: stable@dpdk.org
>>>
>>> Reported-by: David Marchand <david.marchand@redhat.com>
>>> Reported-by: Aaron Conole <aconole@redhat.com>
>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
>>> Acked-by: Aaron Conole <aconole@redhat.com>
>>
>> Applied, thanks.
>>
>>
>
> This patch breaks a couple of apps (or rather the apps were broken to
> begin with, but the brokenness has been exposed with this patch).
>
> A "good" way to handle a SIGINT is to catch it, set some kind of
> global exit flag, and exit the signal handler, so that all of the
> threads see the exit flag, stop spinning, and exit the main loop and
> proceed to gracefully shutdown. That's what majority of our apps do.
>
> A bad way to handle SIGINT is to call rte_exit() inside the signal
> handler, without setting any global exit flags. Since rte_exit() now
> waits for all of the threads to stop, the exit will never actually
> happen because threads can't stop without an exit signal, and no exit
> signal was provided by the signal handler.

Yes, I don't consider it 'breaking' anything - exit in signal handlers
is always a bad idea.  I guess we should correct the examples to show
this.

> Affected apps:
>
> * l3fwd-power (i'm preparing a patch)
> * ip_reassembly (see main.c:988) - +Konstantin
>
> There are also a bunch of apps that simply call exit(0) and do unclean
> shutdown without DPDK cleanup, and also apps i have no idea what
> they're doing (call kill() on themselves in the SIGINT handler?
> l3fwd-cat does that, so do a bunch of others), but this is probably a
> bigger problem that should be addressed separately.

I think one way to mitigate this is to register an at_exit() function
that will check if eal is currently initialized and do the needed
cleanup call.  I don't know if there are any side-effects that we need
to consider for it, though.


      reply	other threads:[~2020-04-14 13:22 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-10 13:33 [dpdk-dev] [PATCH] " Harry van Haaren
2020-03-10 16:31 ` David Marchand
2020-03-10 16:38   ` Van Haaren, Harry
2020-03-10 17:44     ` Aaron Conole
2020-03-10 19:14       ` Aaron Conole
2020-03-11  9:09     ` David Marchand
2020-03-11 14:39 ` [dpdk-dev] [PATCH v2] " Harry van Haaren
2020-03-11 16:15   ` David Marchand
2020-03-11 16:21     ` Van Haaren, Harry
2020-03-12  8:59       ` David Marchand
2020-03-11 17:08     ` Aaron Conole
2020-03-12  9:03       ` David Marchand
2020-03-13 10:04   ` David Marchand
2020-04-06 10:30     ` Burakov, Anatoly
2020-04-14 13:22       ` Aaron Conole [this message]

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=f7two6i5hil.fsf@dhcp-25.97.bos.redhat.com \
    --to=aconole@redhat.com \
    --cc=anatoly.burakov@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=harry.van.haaren@intel.com \
    --cc=konstantin.ananyev@intel.com \
    --cc=stable@dpdk.org \
    /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).