From: Heiko Carstens <heiko.carstens@de.ibm.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: Michal Hocko <mhocko@kernel.org>,
Sebastian Ott <sebott@linux.vnet.ibm.com>,
Linux MM <linux-mm@kvack.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Andrew Morton <akpm@linux-foundation.org>,
"Rafael J. Wysocki" <rjw@rjwysocki.net>,
Vladimir Davydov <vdavydov.dev@gmail.com>,
Ben Hutchings <ben@decadent.org.uk>
Subject: Re: [PATCH] mm, add_memory_resource: hold device_hotplug lock over mem_hotplug_{begin, done}
Date: Wed, 1 Mar 2017 18:04:29 +0100 [thread overview]
Message-ID: <20170301170429.GB5208@osiris> (raw)
In-Reply-To: <CAPcyv4ghK3GWUD0qBNigfQvPM6qUWLMwmfgT5THcDcjuYrjSSQ@mail.gmail.com>
On Wed, Mar 01, 2017 at 07:52:18AM -0800, Dan Williams wrote:
> On Wed, Mar 1, 2017 at 4:51 AM, Heiko Carstens
> <heiko.carstens@de.ibm.com> wrote:
> > Since it is anything but obvious why Dan wrote in changelog of b5d24fda9c3d
> > ("mm, devm_memremap_pages: hold device_hotplug lock over
> > mem_hotplug_{begin, done}") that write accesses to
> > mem_hotplug.active_writer are coordinated via lock_device_hotplug() I'd
> > rather propose a new private memory_add_remove_lock which has similar
> > semantics like the cpu_add_remove_lock for cpu hotplug (see patch below).
> >
> > However instead of sprinkling locking/unlocking of that new lock around all
> > calls of mem_hotplug_begin() and mem_hotplug_end() simply include locking
> > and unlocking into these two functions.
> >
> > This still allows get_online_mems() and put_online_mems() to work, while at
> > the same time preventing mem_hotplug.active_writer corruption.
> >
> > Any opinions?
>
> Sorry, yes, I didn't make it clear that I derived that locking
> requirement from store_mem_state() and its usage of
> lock_device_hotplug_sysfs().
>
> That routine is trying very hard not trip the soft-lockup detector. It
> seems like that wants to be an interruptible wait.
If you look at commit 5e33bc4165f3 ("driver core / ACPI: Avoid device hot
remove locking issues") then lock_device_hotplug_sysfs() was introduced to
avoid a different subtle deadlock, but it also sleeps uninterruptible, but
not for more than 5ms ;)
However I'm not sure if the device hotplug lock should also be used to fix
an unrelated bug that was introduced with the get_online_mems() /
put_online_mems() interface. Should it?
If so, we need to sprinkle around a couple of lock_device_hotplug() calls
near mem_hotplug_begin() calls, like Sebastian already started, and give it
additional semantics (protecting mem_hotplug.active_writer), and hope it
doesn't lead to deadlocks anywhere.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
WARNING: multiple messages have this Message-ID (diff)
From: Heiko Carstens <heiko.carstens@de.ibm.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: Michal Hocko <mhocko@kernel.org>,
Sebastian Ott <sebott@linux.vnet.ibm.com>,
Linux MM <linux-mm@kvack.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Andrew Morton <akpm@linux-foundation.org>,
"Rafael J. Wysocki" <rjw@rjwysocki.net>,
Vladimir Davydov <vdavydov.dev@gmail.com>,
Ben Hutchings <ben@decadent.org.uk>
Subject: Re: [PATCH] mm, add_memory_resource: hold device_hotplug lock over mem_hotplug_{begin, done}
Date: Wed, 1 Mar 2017 18:04:29 +0100 [thread overview]
Message-ID: <20170301170429.GB5208@osiris> (raw)
In-Reply-To: <CAPcyv4ghK3GWUD0qBNigfQvPM6qUWLMwmfgT5THcDcjuYrjSSQ@mail.gmail.com>
On Wed, Mar 01, 2017 at 07:52:18AM -0800, Dan Williams wrote:
> On Wed, Mar 1, 2017 at 4:51 AM, Heiko Carstens
> <heiko.carstens@de.ibm.com> wrote:
> > Since it is anything but obvious why Dan wrote in changelog of b5d24fda9c3d
> > ("mm, devm_memremap_pages: hold device_hotplug lock over
> > mem_hotplug_{begin, done}") that write accesses to
> > mem_hotplug.active_writer are coordinated via lock_device_hotplug() I'd
> > rather propose a new private memory_add_remove_lock which has similar
> > semantics like the cpu_add_remove_lock for cpu hotplug (see patch below).
> >
> > However instead of sprinkling locking/unlocking of that new lock around all
> > calls of mem_hotplug_begin() and mem_hotplug_end() simply include locking
> > and unlocking into these two functions.
> >
> > This still allows get_online_mems() and put_online_mems() to work, while at
> > the same time preventing mem_hotplug.active_writer corruption.
> >
> > Any opinions?
>
> Sorry, yes, I didn't make it clear that I derived that locking
> requirement from store_mem_state() and its usage of
> lock_device_hotplug_sysfs().
>
> That routine is trying very hard not trip the soft-lockup detector. It
> seems like that wants to be an interruptible wait.
If you look at commit 5e33bc4165f3 ("driver core / ACPI: Avoid device hot
remove locking issues") then lock_device_hotplug_sysfs() was introduced to
avoid a different subtle deadlock, but it also sleeps uninterruptible, but
not for more than 5ms ;)
However I'm not sure if the device hotplug lock should also be used to fix
an unrelated bug that was introduced with the get_online_mems() /
put_online_mems() interface. Should it?
If so, we need to sprinkle around a couple of lock_device_hotplug() calls
near mem_hotplug_begin() calls, like Sebastian already started, and give it
additional semantics (protecting mem_hotplug.active_writer), and hope it
doesn't lead to deadlocks anywhere.
next prev parent reply other threads:[~2017-03-01 17:04 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-02-26 11:42 [PATCH] mm, add_memory_resource: hold device_hotplug lock over mem_hotplug_{begin, done} Sebastian Ott
2017-02-26 11:42 ` Sebastian Ott
2017-02-27 16:20 ` Michal Hocko
2017-02-27 16:20 ` Michal Hocko
2017-02-28 11:57 ` Heiko Carstens
2017-02-28 11:57 ` Heiko Carstens
2017-03-01 12:51 ` Heiko Carstens
2017-03-01 12:51 ` Heiko Carstens
2017-03-01 15:52 ` Dan Williams
2017-03-01 15:52 ` Dan Williams
2017-03-01 17:04 ` Heiko Carstens [this message]
2017-03-01 17:04 ` Heiko Carstens
2017-03-01 22:55 ` Dan Williams
2017-03-01 22:55 ` Dan Williams
2017-03-06 8:22 ` Heiko Carstens
2017-03-06 8:22 ` Heiko Carstens
2017-03-09 6:26 ` Dan Williams
2017-03-09 6:26 ` Dan Williams
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=20170301170429.GB5208@osiris \
--to=heiko.carstens@de.ibm.com \
--cc=akpm@linux-foundation.org \
--cc=ben@decadent.org.uk \
--cc=dan.j.williams@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@kernel.org \
--cc=rjw@rjwysocki.net \
--cc=sebott@linux.vnet.ibm.com \
--cc=vdavydov.dev@gmail.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.