Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1730200 > unrolled thread

Re: [PATCH 1/2] mm, memory_hotplug: do not fail offlining too early

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] mm, memory_hotplug: do not fail offlining too early Michal Hocko <mhocko@kernel.org> - 2017-09-11 10:20 +0200
    Re: [PATCH 1/2] mm, memory_hotplug: do not fail offlining too early Vlastimil Babka <vbabka@suse.cz> - 2017-09-13 13:50 +0200
      Re: [PATCH 1/2] mm, memory_hotplug: do not fail offlining too early Vlastimil Babka <vbabka@suse.cz> - 2017-09-13 14:20 +0200
        Re: [PATCH 1/2] mm, memory_hotplug: do not fail offlining too early Michal Hocko <mhocko@kernel.org> - 2017-09-13 14:40 +0200
      Re: [PATCH 1/2] mm, memory_hotplug: do not fail offlining too early Michal Hocko <mhocko@kernel.org> - 2017-09-13 14:20 +0200
      Re: [PATCH 1/2] mm, memory_hotplug: do not fail offlining too early Michal Hocko <mhocko@kernel.org> - 2017-09-13 14:20 +0200

#1730200 — Re: [PATCH 1/2] mm, memory_hotplug: do not fail offlining too early

On Fri 08-09-17 19:26:06, Vlastimil Babka wrote:
> On 09/04/2017 10:21 AM, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > Memory offlining can fail just too eagerly under a heavy memory pressure.
> > 
> > [ 5410.336792] page:ffffea22a646bd00 count:255 mapcount:252 mapping:ffff88ff926c9f38 index:0x3
> > [ 5410.336809] flags: 0x9855fe40010048(uptodate|active|mappedtodisk)
> > [ 5410.336811] page dumped because: isolation failed
> > [ 5410.336813] page->mem_cgroup:ffff8801cd662000
> > [ 5420.655030] memory offlining [mem 0x18b580000000-0x18b5ffffffff] failed
> > 
> > Isolation has failed here because the page is not on LRU. Most probably
> > because it was on the pcp LRU cache or it has been removed from the LRU
> > already but it hasn't been freed yet. In both cases the page doesn't look
> > non-migrable so retrying more makes sense.
> > 
> > __offline_pages seems rather cluttered when it comes to the retry
> > logic. We have 5 retries at maximum and a timeout. We could argue
> > whether the timeout makes sense but failing just because of a race when
> > somebody isoltes a page from LRU or puts it on a pcp LRU lists is just
> > wrong. It only takes it to race with a process which unmaps some pages
> > and remove them from the LRU list and we can fail the whole offline
> > because of something that is a temporary condition and actually not
> > harmful for the offline. Please note that unmovable pages should be
> > already excluded during start_isolate_page_range.
> 
> Hmm, the has_unmovable_pages() check doesn't offer any strict guarantees due to
> races, per its comment. Also at the very quick glance, I see a check where it
> assumes that MIGRATE_MOVABLE pageblock will have no unmovable pages. There is no
> such guarantee even without races.

Yes, you are right that there are races possible but practically
speaking non-movable memblocks (in !MOVABLE_ZONE) would be very likely
to have reliably unmovable pages and so has_unmovable_pages would bail
out. And ZONE_MOVABLE memblocks with permanently pinned pages sound like
a bug to me.

> > Fix this by removing the max retry count and only rely on the timeout
> > resp. interruption by a signal from the userspace. Also retry rather
> > than fail when check_pages_isolated sees some !free pages because those
> > could be a result of the race as well.
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> Even within a movable node where has_unmovable_pages() is a non-issue, you could
> have pinned movable pages where the pinning is not temporary.

Who would pin those pages? Such a page would be unreclaimable as well
and thus a memory leak and I would argue it would be a bug.

> So after this
> patch, this will really keep retrying forever. I'm not saying it's wrong, just
> pointing it out, since the changelog seems to assume there would be only
> temporary failures possible and thus unbound retries are always correct.
> The obvious problem if we wanted to avoid this, is how to recognize
> non-temporary failures...

Yes, we should be able to distinguish the two and hopefully we can teach
the migration code to distinguish between EBUSY (likely permanent) and
EGAIN (temporal) failure. This sound like something we should aim for
longterm I guess. Anyway as I've said in other email. If somebody really
wants to have a guaratee of a bounded retry then it is trivial to set up
an alarm and send a signal itself to bail out.

Do you think that the changelog should be more clear about this?
-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1731554

On 09/11/2017 10:17 AM, Michal Hocko wrote:
> On Fri 08-09-17 19:26:06, Vlastimil Babka wrote:
>> On 09/04/2017 10:21 AM, Michal Hocko wrote:
>>> From: Michal Hocko <mhocko@suse.com>
>>>
>>> Fix this by removing the max retry count and only rely on the timeout
>>> resp. interruption by a signal from the userspace. Also retry rather
>>> than fail when check_pages_isolated sees some !free pages because those
>>> could be a result of the race as well.
>>>
>>> Signed-off-by: Michal Hocko <mhocko@suse.com>
>>
>> Even within a movable node where has_unmovable_pages() is a non-issue, you could
>> have pinned movable pages where the pinning is not temporary.
> 
> Who would pin those pages? Such a page would be unreclaimable as well
> and thus a memory leak and I would argue it would be a bug.

I don't know who exactly, but generally it's a problem for CMA and a
reason why there was some effort from PeterZ to introduce an API for
long-term pinning.

>> So after this
>> patch, this will really keep retrying forever. I'm not saying it's wrong, just
>> pointing it out, since the changelog seems to assume there would be only
>> temporary failures possible and thus unbound retries are always correct.
>> The obvious problem if we wanted to avoid this, is how to recognize
>> non-temporary failures...
> 
> Yes, we should be able to distinguish the two and hopefully we can teach
> the migration code to distinguish between EBUSY (likely permanent) and
> EGAIN (temporal) failure. This sound like something we should aim for
> longterm I guess. Anyway as I've said in other email. If somebody really
> wants to have a guaratee of a bounded retry then it is trivial to set up
> an alarm and send a signal itself to bail out.

Sure, I would just be careful about not breaking existing userspace
(udev?) when offline triggered via ACPI from some management interface
(or whatever the exact mechanism is).

> Do you think that the changelog should be more clear about this?

It certainly wouldn't hurt :)

[toc] | [prev] | [next] | [standalone]

#1731579

On 09/13/2017 02:14 PM, Michal Hocko wrote:
>>>> Do you think that the changelog should be more clear about this?
>>>
>>> It certainly wouldn't hurt :)
>>
>> So what do you think about the following wording:
> 
> Ups, wrong patch
> 
> 
> From 8639496a834b4a7c24972ec23b17e50f0d6a304c Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Mon, 14 Aug 2017 10:46:12 +0200
> Subject: [PATCH 1/2] mm, memory_hotplug: do not fail offlining too early
> 
> Memory offlining can fail just too eagerly under a heavy memory pressure.
> 
> [ 5410.336792] page:ffffea22a646bd00 count:255 mapcount:252 mapping:ffff88ff926c9f38 index:0x3
> [ 5410.336809] flags: 0x9855fe40010048(uptodate|active|mappedtodisk)
> [ 5410.336811] page dumped because: isolation failed
> [ 5410.336813] page->mem_cgroup:ffff8801cd662000
> [ 5420.655030] memory offlining [mem 0x18b580000000-0x18b5ffffffff] failed
> 
> Isolation has failed here because the page is not on LRU. Most probably
> because it was on the pcp LRU cache or it has been removed from the LRU
> already but it hasn't been freed yet. In both cases the page doesn't look
> non-migrable so retrying more makes sense.
> 
> __offline_pages seems rather cluttered when it comes to the retry
> logic. We have 5 retries at maximum and a timeout. We could argue
> whether the timeout makes sense but failing just because of a race when
> somebody isoltes a page from LRU or puts it on a pcp LRU lists is just
> wrong. It only takes it to race with a process which unmaps some pages
> and remove them from the LRU list and we can fail the whole offline
> because of something that is a temporary condition and actually not
> harmful for the offline.
> 
> Please note that unmovable pages should be already excluded during
> start_isolate_page_range. We could argue that has_unmovable_pages is
> racy and MIGRATE_MOVABLE check doesn't provide any hard guarantee either
> but kernel zones (aka < ZONE_MOVABLE) will very likely detect unmovable
> pages in most cases and movable zone shouldn't contain unmovable pages
> at all. Some of those pages might be pinned but not for ever because
> that would be a bug on its own. In any case the context is still
> interruptible and so the userspace can easily bail out when the
> operation takes too long. This is certainly better behavior than a
> hardcoded retry loop which is racy.
> 
> Fix this by removing the max retry count and only rely on the timeout
> resp. interruption by a signal from the userspace. Also retry rather
> than fail when check_pages_isolated sees some !free pages because those
> could be a result of the race as well.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Yeah, that's better, thanks.

Acked-by: Vlastimil Babka <vbabka@suse.cz>

[toc] | [prev] | [next] | [standalone]

#1731590

On Wed 13-09-17 14:19:19, Vlastimil Babka wrote:
> On 09/13/2017 02:14 PM, Michal Hocko wrote:
> >>>> Do you think that the changelog should be more clear about this?
> >>>
> >>> It certainly wouldn't hurt :)
> >>
> >> So what do you think about the following wording:
> > 
> > Ups, wrong patch
> > 
> > 
> > From 8639496a834b4a7c24972ec23b17e50f0d6a304c Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.com>
> > Date: Mon, 14 Aug 2017 10:46:12 +0200
> > Subject: [PATCH 1/2] mm, memory_hotplug: do not fail offlining too early
> > 
> > Memory offlining can fail just too eagerly under a heavy memory pressure.
> > 
> > [ 5410.336792] page:ffffea22a646bd00 count:255 mapcount:252 mapping:ffff88ff926c9f38 index:0x3
> > [ 5410.336809] flags: 0x9855fe40010048(uptodate|active|mappedtodisk)
> > [ 5410.336811] page dumped because: isolation failed
> > [ 5410.336813] page->mem_cgroup:ffff8801cd662000
> > [ 5420.655030] memory offlining [mem 0x18b580000000-0x18b5ffffffff] failed
> > 
> > Isolation has failed here because the page is not on LRU. Most probably
> > because it was on the pcp LRU cache or it has been removed from the LRU
> > already but it hasn't been freed yet. In both cases the page doesn't look
> > non-migrable so retrying more makes sense.
> > 
> > __offline_pages seems rather cluttered when it comes to the retry
> > logic. We have 5 retries at maximum and a timeout. We could argue
> > whether the timeout makes sense but failing just because of a race when
> > somebody isoltes a page from LRU or puts it on a pcp LRU lists is just
> > wrong. It only takes it to race with a process which unmaps some pages
> > and remove them from the LRU list and we can fail the whole offline
> > because of something that is a temporary condition and actually not
> > harmful for the offline.
> > 
> > Please note that unmovable pages should be already excluded during
> > start_isolate_page_range. We could argue that has_unmovable_pages is
> > racy and MIGRATE_MOVABLE check doesn't provide any hard guarantee either
> > but kernel zones (aka < ZONE_MOVABLE) will very likely detect unmovable
> > pages in most cases and movable zone shouldn't contain unmovable pages
> > at all. Some of those pages might be pinned but not for ever because
> > that would be a bug on its own. In any case the context is still
> > interruptible and so the userspace can easily bail out when the
> > operation takes too long. This is certainly better behavior than a
> > hardcoded retry loop which is racy.
> > 
> > Fix this by removing the max retry count and only rely on the timeout
> > resp. interruption by a signal from the userspace. Also retry rather
> > than fail when check_pages_isolated sees some !free pages because those
> > could be a result of the race as well.
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> Yeah, that's better, thanks.
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>

Thanks. I will give it a day and repost the series. If somebody still
have some concerns please speak up.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1731583

On Wed 13-09-17 14:10:01, Michal Hocko wrote:
> On Wed 13-09-17 13:41:20, Vlastimil Babka wrote:
> > On 09/11/2017 10:17 AM, Michal Hocko wrote:
> [...]
> > > Yes, we should be able to distinguish the two and hopefully we can teach
> > > the migration code to distinguish between EBUSY (likely permanent) and
> > > EGAIN (temporal) failure. This sound like something we should aim for
> > > longterm I guess. Anyway as I've said in other email. If somebody really
> > > wants to have a guaratee of a bounded retry then it is trivial to set up
> > > an alarm and send a signal itself to bail out.
> > 
> > Sure, I would just be careful about not breaking existing userspace
> > (udev?) when offline triggered via ACPI from some management interface
> > (or whatever the exact mechanism is).
> 
> The thing is that there is absolutely no timing guarantee even with
> retry limit in place. We are doing allocations, potentially bouncing on
> locks which can be taken elsewhere etc... So if somebody really depend
> on this then it is pretty much broken already.
> 
> > > Do you think that the changelog should be more clear about this?
> > 
> > It certainly wouldn't hurt :)
> 
> So what do you think about the following wording:

Ups, wrong patch


From 8639496a834b4a7c24972ec23b17e50f0d6a304c Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Mon, 14 Aug 2017 10:46:12 +0200
Subject: [PATCH 1/2] mm, memory_hotplug: do not fail offlining too early

Memory offlining can fail just too eagerly under a heavy memory pressure.

[ 5410.336792] page:ffffea22a646bd00 count:255 mapcount:252 mapping:ffff88ff926c9f38 index:0x3
[ 5410.336809] flags: 0x9855fe40010048(uptodate|active|mappedtodisk)
[ 5410.336811] page dumped because: isolation failed
[ 5410.336813] page->mem_cgroup:ffff8801cd662000
[ 5420.655030] memory offlining [mem 0x18b580000000-0x18b5ffffffff] failed

Isolation has failed here because the page is not on LRU. Most probably
because it was on the pcp LRU cache or it has been removed from the LRU
already but it hasn't been freed yet. In both cases the page doesn't look
non-migrable so retrying more makes sense.

__offline_pages seems rather cluttered when it comes to the retry
logic. We have 5 retries at maximum and a timeout. We could argue
whether the timeout makes sense but failing just because of a race when
somebody isoltes a page from LRU or puts it on a pcp LRU lists is just
wrong. It only takes it to race with a process which unmaps some pages
and remove them from the LRU list and we can fail the whole offline
because of something that is a temporary condition and actually not
harmful for the offline.

Please note that unmovable pages should be already excluded during
start_isolate_page_range. We could argue that has_unmovable_pages is
racy and MIGRATE_MOVABLE check doesn't provide any hard guarantee either
but kernel zones (aka < ZONE_MOVABLE) will very likely detect unmovable
pages in most cases and movable zone shouldn't contain unmovable pages
at all. Some of those pages might be pinned but not for ever because
that would be a bug on its own. In any case the context is still
interruptible and so the userspace can easily bail out when the
operation takes too long. This is certainly better behavior than a
hardcoded retry loop which is racy.

Fix this by removing the max retry count and only rely on the timeout
resp. interruption by a signal from the userspace. Also retry rather
than fail when check_pages_isolated sees some !free pages because those
could be a result of the race as well.

Signed-off-by: Michal Hocko <mhocko@suse.com>

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1731584

On Wed 13-09-17 13:41:20, Vlastimil Babka wrote:
> On 09/11/2017 10:17 AM, Michal Hocko wrote:
[...]
> > Yes, we should be able to distinguish the two and hopefully we can teach
> > the migration code to distinguish between EBUSY (likely permanent) and
> > EGAIN (temporal) failure. This sound like something we should aim for
> > longterm I guess. Anyway as I've said in other email. If somebody really
> > wants to have a guaratee of a bounded retry then it is trivial to set up
> > an alarm and send a signal itself to bail out.
> 
> Sure, I would just be careful about not breaking existing userspace
> (udev?) when offline triggered via ACPI from some management interface
> (or whatever the exact mechanism is).

The thing is that there is absolutely no timing guarantee even with
retry limit in place. We are doing allocations, potentially bouncing on
locks which can be taken elsewhere etc... So if somebody really depend
on this then it is pretty much broken already.

> > Do you think that the changelog should be more clear about this?
> 
> It certainly wouldn't hurt :)

So what do you think about the following wording:

commit 23c4ded55c2ba880165a9f5b8a67694361fb6bc7
Author: Michal Hocko <mhocko@suse.com>
Date:   Mon Aug 28 13:13:06 2017 +0200

    mm, memory_hotplug: remove timeout from __offline_memory
    
    We have a hardcoded 120s timeout after which the memory offline fails
    basically since the hot remove has been introduced. This is essentially
    a policy implemented in the kernel. Moreover there is no way to adjust
    the timeout and so we are sometimes facing memory offline failures if
    the system is under a heavy memory pressure or very intensive CPU
    workload on large machines.
    
    It is not very clear what purpose the timeout actually serves. The
    offline operation is interruptible by a signal so if userspace wants
    some timeout based termination this can be done trivially by sending a
    signal.
    
    If there is a strong usecase to do this from the kernel then we should
    do it properly and have a it tunable from the userspace with the timeout
    disabled by default along with the explanation who uses it and for what
    purporse.
    
    Acked-by: Vlastimil Babka <vbabka@suse.cz>
    Signed-off-by: Michal Hocko <mhocko@suse.com>
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web