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

Groups > linux.kernel > #1620004 > unrolled thread

Re: [PATCH -v2 0/9] mm: make movable onlining suck less

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 -v2 0/9] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-10 17:00 +0200
    Re: [PATCH -v2 0/9] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-10 17:30 +0200
      Re: [PATCH -v2 0/9] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-10 17:40 +0200
    Re: [PATCH -v2 0/9] mm: make movable onlining suck less Igor Mammedov <imammedo@redhat.com> - 2017-04-11 10:10 +0200
      Re: [PATCH -v2 0/9] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-11 10:50 +0200
        Re: [PATCH -v2 0/9] mm: make movable onlining suck less Igor Mammedov <imammedo@redhat.com> - 2017-04-11 12:00 +0200
          Re: [PATCH -v2 0/9] mm: make movable onlining suck less Michal Hocko <mhocko@kernel.org> - 2017-04-11 12:50 +0200

#1620004 — Re: [PATCH -v2 0/9] mm: make movable onlining suck less

On Mon 10-04-17 16:27:49, Igor Mammedov wrote:
[...]
> Hi Michal,
> 
> I've given series some dumb testing, see below for unexpected changes I've noticed.
> 
> Using the same CLI as above plus hotpluggable dimms present at startup
> (it still uses hotplug path as dimms aren't reported in e820)
> 
> -object memory-backend-ram,id=mem1,size=256M -object memory-backend-ram,id=mem0,size=256M \
> -device pc-dimm,id=dimm1,memdev=mem1,slot=1,node=0 -device pc-dimm,id=dimm0,memdev=mem0,slot=0,node=0
> 
> so dimm1 => memory3[23] and dimm0 => memory3[45]
> 
> #issue1:
> unable to online memblock as NORMAL adjacent to onlined MOVABLE
> 
> 1: after boot
> memory32:offline removable: 0  zones: Normal Movable
> memory33:offline removable: 0  zones: Normal Movable
> memory34:offline removable: 0  zones: Normal Movable
> memory35:offline removable: 0  zones: Normal Movable
> 
> 2: online as movable 1st dimm
> 
> #echo online_movable > memory32/state
> #echo online_movable > memory33/state
> 
> everything is as expected:
> memory32:online removable: 1  zones: Movable
> memory33:online removable: 1  zones: Movable
> memory34:offline removable: 0  zones: Movable
> memory35:offline removable: 0  zones: Movable
> 
> 3: try to offline memory32 and online as NORMAL
> 
> #echo offline > memory32/state
> memory32:offline removable: 1  zones: Normal Movable
> memory33:online removable: 1  zones: Movable
> memory34:offline removable: 0  zones: Movable
> memory35:offline removable: 0  zones: Movable

OK, this is not expected. We are not shifting zones anymore so the range
which was online_movable will not become available to the zone Normal.
So this must be something broken down the show_valid_zones path. I will
investigate.

> 
> #echo online_kernel > memory32/state
> write error: Invalid argument
> // that's not what's expected

this is proper behavior with the current implementation. Does anything
depend on the zone reusing?

> memory32:offline removable: 1  zones: Normal Movable
> memory33:online removable: 1  zones: Movable
> memory34:offline removable: 0  zones: Movable
> memory35:offline removable: 0  zones: Movable
> 
> 
> ======
> #issue2: dimm1 assigned to node 1 on qemu CLI
> memblock is onlined as movable by default
> 
> // after boot
> memory32:offline removable: 1  zones: Normal
> memory33:offline removable: 1  zones: Normal Movable
> memory34:offline removable: 1  zones: Normal
> memory35:offline removable: 1  zones: Normal Movable
> // not related to this issue but notice not all blocks are
> // "Normal Movable" when compared when both dimms on node 0 /#issue1/

yes they should be

> #echo online_movable > memory33/state
> #echo online > memory32/state
> 
> memory32:online removable: 1  zones: Movable
> memory33:online removable: 1  zones: Movable
> 
> before series memory32 goes to zone NORMAL as expected
> memory32:online removable: 0  zones: Normal Movable
> memory33:online removable: 1  zones: Movable Normal

OK, I will double check.
 
> ======
> #issue3:
> removable flag flipped to non-removable state
> 
> // before series at commit ef0b577b6:
> memory32:offline removable: 0  zones: Normal Movable
> memory33:offline removable: 0  zones: Normal Movable
> memory34:offline removable: 0  zones: Normal Movable
> memory35:offline removable: 0  zones: Normal Movable
> 
> // after series at commit 6a010434
> memory32:offline removable: 1  zones: Normal
> memory33:offline removable: 1  zones: Normal
> memory34:offline removable: 1  zones: Normal
> memory35:offline removable: 1  zones: Normal Movable
> 
> also looking at #issue1 removable flag state doesn't
> seem to be consistent between state changes but maybe that's
> been broken before

OK, will have a look.

Thanks for your testing!

-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1620033

[dropping Lai Jiangshan whose email bounces]

On Mon 10-04-17 16:56:39, Michal Hocko wrote:
> On Mon 10-04-17 16:27:49, Igor Mammedov wrote:
> [...]
> > Hi Michal,
> > 
> > I've given series some dumb testing, see below for unexpected changes I've noticed.
> > 
> > Using the same CLI as above plus hotpluggable dimms present at startup
> > (it still uses hotplug path as dimms aren't reported in e820)
> > 
> > -object memory-backend-ram,id=mem1,size=256M -object memory-backend-ram,id=mem0,size=256M \
> > -device pc-dimm,id=dimm1,memdev=mem1,slot=1,node=0 -device pc-dimm,id=dimm0,memdev=mem0,slot=0,node=0
> > 
> > so dimm1 => memory3[23] and dimm0 => memory3[45]
> > 
> > #issue1:
> > unable to online memblock as NORMAL adjacent to onlined MOVABLE
> > 
> > 1: after boot
> > memory32:offline removable: 0  zones: Normal Movable
> > memory33:offline removable: 0  zones: Normal Movable
> > memory34:offline removable: 0  zones: Normal Movable
> > memory35:offline removable: 0  zones: Normal Movable
> > 
> > 2: online as movable 1st dimm
> > 
> > #echo online_movable > memory32/state
> > #echo online_movable > memory33/state
> > 
> > everything is as expected:
> > memory32:online removable: 1  zones: Movable
> > memory33:online removable: 1  zones: Movable
> > memory34:offline removable: 0  zones: Movable
> > memory35:offline removable: 0  zones: Movable
> > 
> > 3: try to offline memory32 and online as NORMAL
> > 
> > #echo offline > memory32/state
> > memory32:offline removable: 1  zones: Normal Movable
> > memory33:online removable: 1  zones: Movable
> > memory34:offline removable: 0  zones: Movable
> > memory35:offline removable: 0  zones: Movable
> 
> OK, this is not expected. We are not shifting zones anymore so the range
> which was online_movable will not become available to the zone Normal.
> So this must be something broken down the show_valid_zones path. I will
> investigate.

Heh, this one is embarrassing
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 1c6fdacbccd3..9677b6b711b0 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -402,7 +402,7 @@ static ssize_t show_valid_zones(struct device *dev,
 		return sprintf(buf, "none\n");
 
 	start_pfn = valid_start_pfn;
-	nr_pages = valid_end_pfn - valid_end_pfn;
+	nr_pages = valid_end_pfn - start_pfn;
 
 	/*
 	 * Check the existing zone. Make sure that we do that only on the
-- 
Michal Hocko
SUSE Labs

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

#1620041

On Mon 10-04-17 17:22:28, Michal Hocko wrote:
[...]
> Heh, this one is embarrassing
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 1c6fdacbccd3..9677b6b711b0 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -402,7 +402,7 @@ static ssize_t show_valid_zones(struct device *dev,
>  		return sprintf(buf, "none\n");
>  
>  	start_pfn = valid_start_pfn;
> -	nr_pages = valid_end_pfn - valid_end_pfn;
> +	nr_pages = valid_end_pfn - start_pfn;
>  
>  	/*
>  	 * Check the existing zone. Make sure that we do that only on the

Btw. while starting into the code I think that allow_online_pfn_range is
also wrong and we need the following
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 94e96ca790f6..035165ceefef 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -858,7 +858,7 @@ bool allow_online_pfn_range(int nid, unsigned long pfn, unsigned long nr_pages,
 	 * TODO make sure we do not overlap with ZONE_DEVICE
 	 */
 	if (online_type == MMOP_ONLINE_KERNEL) {
-		if (!populated_zone(movable_zone))
+		if (!movable_zone->spanned_pages)
 			return true;
 		return movable_zone->zone_start_pfn >= pfn + nr_pages;
 	} else if (online_type == MMOP_ONLINE_MOVABLE) {

because we would allow ZONE_NORMAL after the full movable zone has been
offlined.
-- 
Michal Hocko
SUSE Labs

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

#1621009

On Mon, 10 Apr 2017 16:56:39 +0200
Michal Hocko <mhocko@kernel.org> wrote:

> On Mon 10-04-17 16:27:49, Igor Mammedov wrote:
> [...]
> > Hi Michal,
> > 
> > I've given series some dumb testing, see below for unexpected changes I've noticed.
> > 
> > Using the same CLI as above plus hotpluggable dimms present at startup
> > (it still uses hotplug path as dimms aren't reported in e820)
> > 
> > -object memory-backend-ram,id=mem1,size=256M -object memory-backend-ram,id=mem0,size=256M \
> > -device pc-dimm,id=dimm1,memdev=mem1,slot=1,node=0 -device pc-dimm,id=dimm0,memdev=mem0,slot=0,node=0
> > 
> > so dimm1 => memory3[23] and dimm0 => memory3[45]
> > 
> > #issue1:
> > unable to online memblock as NORMAL adjacent to onlined MOVABLE
> > 
> > 1: after boot
> > memory32:offline removable: 0  zones: Normal Movable
> > memory33:offline removable: 0  zones: Normal Movable
> > memory34:offline removable: 0  zones: Normal Movable
> > memory35:offline removable: 0  zones: Normal Movable
> > 
> > 2: online as movable 1st dimm
> > 
> > #echo online_movable > memory32/state
> > #echo online_movable > memory33/state
> > 
> > everything is as expected:
> > memory32:online removable: 1  zones: Movable
> > memory33:online removable: 1  zones: Movable
> > memory34:offline removable: 0  zones: Movable
> > memory35:offline removable: 0  zones: Movable
> > 
> > 3: try to offline memory32 and online as NORMAL
> > 
> > #echo offline > memory32/state
> > memory32:offline removable: 1  zones: Normal Movable
> > memory33:online removable: 1  zones: Movable
> > memory34:offline removable: 0  zones: Movable
> > memory35:offline removable: 0  zones: Movable  
> 
> OK, this is not expected. We are not shifting zones anymore so the range
> which was online_movable will not become available to the zone Normal.
> So this must be something broken down the show_valid_zones path. I will
> investigate.
> 
> > 
> > #echo online_kernel > memory32/state
> > write error: Invalid argument
> > // that's not what's expected  
> 
> this is proper behavior with the current implementation. Does anything
> depend on the zone reusing?
if we didn't have zone imbalance issue in design,
the it wouldn't matter but as it stands it's not
minore issue.

Consider following,
one hotplugs some memory and onlines it as movable,
then one needs to hotplug some more but to do so 
one one needs more memory from zone NORMAL and to keep
zone balance some memory in MOVABLE should be reonlined
as NORMAL

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

#1621036

On Tue 11-04-17 10:01:52, Igor Mammedov wrote:
> On Mon, 10 Apr 2017 16:56:39 +0200
> Michal Hocko <mhocko@kernel.org> wrote:
[...]
> > > #echo online_kernel > memory32/state
> > > write error: Invalid argument
> > > // that's not what's expected  
> > 
> > this is proper behavior with the current implementation. Does anything
> > depend on the zone reusing?
> if we didn't have zone imbalance issue in design,
> the it wouldn't matter but as it stands it's not
> minore issue.
> 
> Consider following,
> one hotplugs some memory and onlines it as movable,
> then one needs to hotplug some more but to do so 
> one one needs more memory from zone NORMAL and to keep
> zone balance some memory in MOVABLE should be reonlined
> as NORMAL

Is this something that we absolutely have to have right _now_? Or are you
OK if I address this in follow up series? Because it will make the
current code slightly more complex and to be honest I would rather like
to see this "core" merge and build more on top.
-- 
Michal Hocko
SUSE Labs

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

#1621109

On Tue, 11 Apr 2017 10:41:42 +0200
Michal Hocko <mhocko@kernel.org> wrote:

> On Tue 11-04-17 10:01:52, Igor Mammedov wrote:
> > On Mon, 10 Apr 2017 16:56:39 +0200
> > Michal Hocko <mhocko@kernel.org> wrote:  
> [...]
> > > > #echo online_kernel > memory32/state
> > > > write error: Invalid argument
> > > > // that's not what's expected    
> > > 
> > > this is proper behavior with the current implementation. Does anything
> > > depend on the zone reusing?  
> > if we didn't have zone imbalance issue in design,
> > the it wouldn't matter but as it stands it's not
> > minore issue.
> > 
> > Consider following,
> > one hotplugs some memory and onlines it as movable,
> > then one needs to hotplug some more but to do so 
> > one one needs more memory from zone NORMAL and to keep
> > zone balance some memory in MOVABLE should be reonlined
> > as NORMAL  
> 
> Is this something that we absolutely have to have right _now_? Or are you
> OK if I address this in follow up series? Because it will make the
> current code slightly more complex and to be honest I would rather like
> to see this "core" merge and build more on top.
It's fine by me to do it on top.

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

#1621136

On Tue 11-04-17 11:53:22, Igor Mammedov wrote:
> On Tue, 11 Apr 2017 10:41:42 +0200
> Michal Hocko <mhocko@kernel.org> wrote:
> 
> > On Tue 11-04-17 10:01:52, Igor Mammedov wrote:
> > > On Mon, 10 Apr 2017 16:56:39 +0200
> > > Michal Hocko <mhocko@kernel.org> wrote:  
> > [...]
> > > > > #echo online_kernel > memory32/state
> > > > > write error: Invalid argument
> > > > > // that's not what's expected    
> > > > 
> > > > this is proper behavior with the current implementation. Does anything
> > > > depend on the zone reusing?  
> > > if we didn't have zone imbalance issue in design,
> > > the it wouldn't matter but as it stands it's not
> > > minore issue.
> > > 
> > > Consider following,
> > > one hotplugs some memory and onlines it as movable,
> > > then one needs to hotplug some more but to do so 
> > > one one needs more memory from zone NORMAL and to keep
> > > zone balance some memory in MOVABLE should be reonlined
> > > as NORMAL  
> > 
> > Is this something that we absolutely have to have right _now_? Or are you
> > OK if I address this in follow up series? Because it will make the
> > current code slightly more complex and to be honest I would rather like
> > to see this "core" merge and build more on top.
>
> It's fine by me to do it on top.

OK, I will document this in the changelog of the patch 6.

"
Please note that this patch also changes the original behavior when
offlining a memory block adjacent to another zone (Normal vs. Movable)
used to allow to change its movable type. This will be handled later.
"
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web