linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: David Hildenbrand <david@redhat.com>
To: Wei Yang <richard.weiyang@gmail.com>
Cc: Michal Hocko <mhocko@suse.com>,
	osalvador@suse.de, akpm@linux-foundation.org, linux-mm@kvack.org
Subject: Re: [PATCH v2] mm, hotplug: move init_currently_empty_zone() under zone_span_lock protection
Date: Mon, 26 Nov 2018 10:24:26 +0100	[thread overview]
Message-ID: <424dd9eb-7295-5cf4-98b2-7a1bfd53b32e@redhat.com> (raw)
In-Reply-To: <20181126014451.grrdei2luujiqwrh@master>

On 26.11.18 02:44, Wei Yang wrote:
> On Fri, Nov 23, 2018 at 09:46:52AM +0100, David Hildenbrand wrote:
>> On 23.11.18 09:42, Michal Hocko wrote:
>>> On Thu 22-11-18 16:26:40, David Hildenbrand wrote:
>>>> On 22.11.18 11:12, Wei Yang wrote:
>>>>> During online_pages phase, pgdat->nr_zones will be updated in case this
>>>>> zone is empty.
>>>>>
>>>>> Currently the online_pages phase is protected by the global lock
>>>>> mem_hotplug_begin(), which ensures there is no contention during the
>>>>> update of nr_zones. But this global lock introduces scalability issues.
>>>>>
>>>>> This patch is a preparation for removing the global lock during
>>>>> online_pages phase. Also this patch changes the documentation of
>>>>> node_size_lock to include the protectioin of nr_zones.
>>>>
>>>> I looked into locking recently, and there is more to it.
>>>>
>>>> Please read:
>>>>
>>>> commit dee6da22efac451d361f5224a60be2796d847b51
>>>> Author: David Hildenbrand <david@redhat.com>
>>>> Date:   Tue Oct 30 15:10:44 2018 -0700
>>>>
>>>>     memory-hotplug.rst: add some details about locking internals
>>>>     
>>>>     Let's document the magic a bit, especially why device_hotplug_lock is
>>>>     required when adding/removing memory and how it all play together with
>>>>     requests to online/offline memory from user space.
>>>>
>>>> Short summary: Onlining/offlining of memory requires the device_hotplug_lock
>>>> as of now.
>>>
>>> Well, I would tend to disagree here. You might be describing the current
>>> state of art but the device_hotplug_lock doesn't make much sense for the
>>> memory hotplug in principle. There is absolutely nothing in the core MM
>>
>> There are collisions with CPU hotplug that require this lock (when nodes
>> come and go as far as I remember). And there is the problematic lock
>> inversion that can happen when adding/remving memory. This all has to be
>> sorted out, we'll have to see if we really need it for
>> onlining/offlining, though, however ...
>>
> 
> Seems I get a little understanding on this part.
> 
> There are two hotplug:
>    * CPU hotplug 
>    * Memory hotplug.
> 
> There are two phase for Memory hotplug:
>    * physical add/remove 
>    * logical online/offline
> 
> All of them are protected by device_hotplug_lock now, so we need to be
> careful to release this in any case. Is my understanding correct?

Yes, e.g. the acpi driver always held the device_hotplug_lock (due to
possible problems with concurrent cpu/memory hot(un)plug). Onlining
offlining of devices (including cpu/memory) from user space always held
the device_hotplug_lock. So this part was executed sequentially for a
long time.

I recently made sure that any adding/removing/onlining/offlining
correctly grabs the device_hotplug_lock AND the mem_hotplug_lock in any
case (because it was inconsistent and broken), so it is all executed
sequentially.

So when getting rid of mem_hotplug_lock we only have to care about all
users that don't take the device_hotplug_lock.

> 
>>> that would require this lock. The current state just uses a BKL in some
>>> sense and we really want to get rid of that longterm. This patch is a tiny
>>> step in that direction and I suspect many more will need to come on the
>>> way. We really want to end up with a clear scope of each lock being
>>> taken. A project for a brave soul...
>>
>> ... for now I don't consider "optimize for parallel
>> onlining/offlining/adding/removing of memory and cpus" really necessary.
>> What is necessary indeed is to not slowdown the whole system just
>> because some memory is coming/going. Therefore I agree, this patch is a
>> step into the right direction.
>>
> 
> Agree.
> 
> The target is to accelerate the hot-plug without slow down the normal
> process. 

Indeed!


-- 

Thanks,

David / dhildenb

  reply	other threads:[~2018-11-26  9:24 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-20  1:48 [PATCH] mm, hotplug: protect nr_zones with pgdat_resize_lock() Wei Yang
2018-11-20  7:31 ` Michal Hocko
2018-11-20  7:58   ` osalvador
2018-11-20  8:48     ` Michal Hocko
2018-11-21  2:52     ` Wei Yang
2018-11-21  7:15       ` Michal Hocko
2018-11-22  1:52         ` Wei Yang
2018-11-22  8:39           ` Michal Hocko
2018-11-26  2:28         ` Wei Yang
2018-11-26  8:16           ` Michal Hocko
2018-11-26  9:06             ` Wei Yang
2018-11-26 10:03               ` Michal Hocko
2018-11-27  0:18                 ` Wei Yang
2018-11-27  3:12             ` Wei Yang
2018-11-27 13:16               ` Michal Hocko
2018-11-27 23:56                 ` Wei Yang
2018-11-21  8:24       ` osalvador
2018-11-21  2:44   ` Wei Yang
2018-11-21  7:14     ` Michal Hocko
2018-11-22 10:12 ` [PATCH v2] mm, hotplug: move init_currently_empty_zone() under zone_span_lock protection Wei Yang
2018-11-22 10:15   ` Wei Yang
2018-11-22 10:29     ` Michal Hocko
2018-11-22 14:27       ` Wei Yang
2018-11-22 10:37   ` osalvador
2018-11-22 14:28     ` Wei Yang
2018-11-22 15:26   ` David Hildenbrand
2018-11-22 21:28     ` Wei Yang
2018-11-22 21:53       ` David Hildenbrand
2018-11-22 23:53         ` Wei Yang
2018-11-23  8:42     ` Michal Hocko
2018-11-23  8:46       ` David Hildenbrand
2018-11-26  1:44         ` Wei Yang
2018-11-26  9:24           ` David Hildenbrand [this message]
2018-11-27  0:23             ` Wei Yang
2018-11-30  6:58   ` [PATCH v3] " Wei Yang
2018-11-30  9:30     ` David Hildenbrand
2018-12-01  0:27       ` Wei Yang
2018-12-03 10:09         ` David Hildenbrand
2018-12-03 20:37           ` Wei Yang
2018-12-03 20:50     ` [PATCH v4] " Wei Yang

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=424dd9eb-7295-5cf4-98b2-7a1bfd53b32e@redhat.com \
    --to=david@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@suse.com \
    --cc=osalvador@suse.de \
    --cc=richard.weiyang@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 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).