DPDK patches and discussions
 help / color / Atom feed
From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
To: David Marchand <david.marchand@redhat.com>
Cc: dev <dev@dpdk.org>, Bruce Richardson <bruce.richardson@intel.com>,
	Stephen Hemminger <stephen@networkplumber.org>,
	dpdk stable <stable@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v4] eal: fix proc type auto detection
Date: Thu, 24 Oct 2019 17:07:00 +0100
Message-ID: <27c0cf41-2dea-9e0e-7c25-df76796ca2e6@intel.com> (raw)
In-Reply-To: <CAJFAV8w1i_6v7oA01ETJgWcwSWh2m=+8jj+M7cn_BE36GXKbOg@mail.gmail.com>

On 12-Aug-19 11:03 AM, David Marchand wrote:
> On Wed, Jul 24, 2019 at 6:08 PM Anatoly Burakov
> <anatoly.burakov@intel.com> wrote:
>>
>> Currently, primary process holds an exclusive lock on the config
>> file, thereby preventing other primaries from spinning up. However,
>> when the primary dies, the lock is no longer being held, even though
>> there might be other secondary processes still running.
>>
>> The fix is two-fold. First of all, downgrade the primary process's
>> exclusive lock to a shared lock once we have it. Second of all,
>> also take out shared locks on the config from the secondaries. We
>> are using fcntl() locks, which get dropped when the file handle is
>> closed, so also remove the closure of config file handle.
>>
>> Fixes: af75078fece3 ("first public release")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>>

Hi David,

I've been investigating how to improve this patch, and i've hit a dead end.

The problems here are two-fold. Using fcntl() and flock() locks together 
is not advisable, so both primary-secondary detection and 
rte_eal_primary_proc_alive() (as per Harry's point) have to use the same 
method for checking locks.

Using flock() would work for this purpose. Unfortunately, on FreeBSD, 
converting exclusive lock into a shared lock involves unlocking first 
[1] (creating a race). On Linux it doesn't specifically say that it does 
that, but it does mention that it is not guaranteed to be atomic [2]. 
So, we can't use flock() here.

It seems that fcntl() lock conversions are atomic, however fcntl() locks 
on both Linux and FreeBSD are implemented in a very stupid way in that 
/any/ closure of a file descriptor drops /all/ locks on that file. 
Meaning, the moment secondary does the check in primary_proc_alive() and 
closes the config file fd, the process-wide lock drops. Mind you, 
primary_proc_alive() is implemented using lockf() rather than fcntl(), 
which is an issue in itself, but on Linux, lockf() is implemented on top 
of fcntl() locks and thus suffers from the same issue.

So, unless you have better ideas, i think this patch can be marked as 
rejected.

[1] https://www.freebsd.org/cgi/man.cgi?query=flock&sektion=2
[2] https://linux.die.net/man/2/flock

-- 
Thanks,
Anatoly

  parent reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-23 13:19 [dpdk-dev] [PATCH] " Anatoly Burakov
2019-07-23 18:38 ` Stephen Hemminger
2019-07-24 10:04 ` [dpdk-dev] [PATCH v2] " Anatoly Burakov
2019-07-24 16:04   ` [dpdk-dev] [PATCH v3] " Anatoly Burakov
2019-07-24 16:07     ` [dpdk-dev] [PATCH v4] " Anatoly Burakov
2019-07-30  8:13       ` Thomas Monjalon
2019-07-30  9:19         ` Burakov, Anatoly
2019-08-12 10:03       ` David Marchand
2019-08-12 10:21         ` Van Haaren, Harry
2019-08-12 13:30           ` Burakov, Anatoly
2019-08-12 13:31         ` Burakov, Anatoly
2019-10-24 16:07         ` Burakov, Anatoly [this message]
2019-10-27 19:41           ` David Marchand

Reply instructions:

You may reply publically 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=27c0cf41-2dea-9e0e-7c25-df76796ca2e6@intel.com \
    --to=anatoly.burakov@intel.com \
    --cc=bruce.richardson@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=stable@dpdk.org \
    --cc=stephen@networkplumber.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

DPDK patches and discussions

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox