Re: [PATCH 1/1] resource: Fixed iomem resource release failed on release_mem_region_adjustable() when memory node or cpu node hot-remove.

[Bjorn Helgaas]

On Sat, Jun 23, 2018 at 05:53:40AM +0000, Guomin Chen wrote:
> Hi 
> 	The report link for this issue is: https://bugzilla.suse.com/show_bug.cgi?id=1092687

If/when you update this patch, please include the URL in the commit log,
e.g.,

  Link: https://bugzilla.suse.com/show_bug.cgi?id=1092687

It would also be nice if you could make that report public so anybody can
read it.

> > -----Original Message-----
> > From: Bjorn Helgaas [mailto:helgaas@xxxxxxxxxx]
> > Sent: 2018å6æ23æ 7:20
> > To: Guomin Chen <guomin.chen@xxxxxxxx>
> > Cc: Brijesh Singh <brijesh.singh@xxxxxxx>; Tom Lendacky
> > <thomas.lendacky@xxxxxxx>; Yaowei Bai
> > <baiyaowei@xxxxxxxxxxxxxxxxxxxx>; Bjorn Helgaas <bhelgaas@xxxxxxxxxx>;
> > Toshi Kani <toshi.kani@xxxxxxx>; Dan Williams <dan.j.williams@xxxxxxxxx>;
> > Thomas Gleixner <tglx@xxxxxxxxxxxxx>; Andrew Morton
> > <akpm@xxxxxxxxxxxxxxxxxxxx>; Joey Lee <JLee@xxxxxxxx>; Borislav Petkov
> > <bp@xxxxxxx>; Takashi Iwai <tiwai@xxxxxxx>; linux-efi@xxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH 1/1] resource: Fixed iomem resource release failed on
> > release_mem_region_adjustable() when memory node or cpu node
> > hot-remove.
> > 
> > [+cc Toshi]
> > 
> > On Fri, Jun 22, 2018 at 08:01:38PM +0800, guomin chen wrote:
> > > We've got a bug report indicating the hot-remove node resource release
> > > failed,when the memory on this node is divided into several
> > > sections.because the release_mem_region_adjustable() can only release
> > > one resource that must be [start,end].
> > 
> > Can you please include a URL for the bug report?  That's useful for additional
> > details and gives hints about how future changes in this area might be tested.
> > 
> > release_mem_region_adjustable() and the only call to it were added by Toshi
> > (cc'd):
> > 
> >   825f787bb496 ("resource: add release_mem_region_adjustable()")
> >   fe74ebb106a5 ("mm: change __remove_pages() to call
> > release_mem_region_adjustable()")
> > 
> > > In my case, the BIOS supports faulty memory isolation. if BIOS
> > > detected bad memory block, the BIOS will isolates this badblock.
> > > And set this badblock memory to EfiUnusableMemory in EFI memory map
> > > base on UEFI 2.7 spec.For example in my system, the memory range on
> > > node2 is [mem 0x0000080000000000-0x00000807ffffffff].but the BIOS
> > > detected the [8004e000000-8004e0fffff] is a badblock memory.
> > > So the memory on node2 seem like this:
> > >    		80000000000-8004dffffff : System RAM
> > > 		8004e000000-8004e0fffff : Unusable memory
> > > 		8004e100000-807ffffffff : System RAM
> > >
> > > Now, when offline the cpu node2,the kernel will try to release
> > > ioresource [mem 0x0000080000000000-0x00000807ffffffff]. at this time,
> > > the kernel will release failed,and output error message:
> > > "Unable to release resource <0x0000080000000000-0x00000807ffffffff>
> > > (-22)".
> > > Because the release_mem_region_adjustable() can only release one
> > > resource that must be [0x0000080000000000 , 0x00000807ffffffff].
> > > but now,the iomem resource on node2 [0x0000080000000000,
> > > 0x00000807ffffffff] are divided into three resources [80000000000-
> > > 8004dffffff],[8004e000000-8004e0fffff]and[8004e100000-807ffffffff].
> > >
> > > This patch help to Release multiple iomem resources at once when node
> > > hot-remove. Such as in above case, when hot-remove the cpu node2,the
> > > kernel will try to release resource [0x0000080000000000-
> > > 0x00000807ffffffff].And this patch will release three resources
> > > [80000000000-8004dffffff],[8004e000000-8004e0fffff] and
> > > [8004e100000-807ffffffff].
> > >
> > > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > > Cc: Brijesh Singh <brijesh.singh@xxxxxxx>
> > > Cc: Borislav Petkov <bp@xxxxxxx>
> > > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > > Cc: Tom Lendacky <thomas.lendacky@xxxxxxx>
> > > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > > Cc: Yaowei Bai <baiyaowei@xxxxxxxxxxxxxxxxxxxx>
> > > Cc: Takashi Iwai <tiwai@xxxxxxx>
> > > Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> > > Cc: linux-efi@xxxxxxxxxxxxxxx
> > > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > > Cc: Lee Chun-Yi <JLee@xxxxxxxx>
> > > Signed-off-by: guomin chen <guomin.chen@xxxxxxxx>
> > > ---
> > >  kernel/resource.c | 73
> > > ++++++++++++++++++++++++++++++++-----------------------
> > >  1 file changed, 43 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/kernel/resource.c b/kernel/resource.c index
> > > 30e1bc68503b..959bcce4c405 100644
> > > --- a/kernel/resource.c
> > > +++ b/kernel/resource.c
> > > @@ -1240,6 +1240,7 @@ int release_mem_region_adjustable(struct
> > resource *parent,
> > >  	struct resource *res;
> > >  	struct resource *new_res;
> > >  	resource_size_t end;
> > > +	resource_size_t new_start = start;
> > >  	int ret = -EINVAL;
> > >
> > >  	end = start + size - 1;
> > > @@ -1257,7 +1258,7 @@ int release_mem_region_adjustable(struct
> > resource *parent,
> > >  			break;
> > >
> > >  		/* look for the next resource if it does not fit into */
> > > -		if (res->start > start || res->end < end) {
> > > +		if (res->end < new_start) {
> > >  			p = &res->sibling;
> > >  			continue;
> > >  		}
> > > @@ -1271,42 +1272,54 @@ int release_mem_region_adjustable(struct
> > resource *parent,
> > >  		}
> > >
> > >  		/* found the target resource; let's adjust accordingly */
> > > -		if (res->start == start && res->end == end) {
> > > +		if (res->start == new_start && res->end == end) {
> > >  			/* free the whole entry */
> > >  			*p = res->sibling;
> > >  			free_resource(res);
> > >  			ret = 0;
> > > -		} else if (res->start == start && res->end != end) {
> > > -			/* adjust the start */
> > > -			ret = __adjust_resource(res, end + 1,
> > > -						res->end - end);
> > > -		} else if (res->start != start && res->end == end) {
> > > -			/* adjust the end */
> > > -			ret = __adjust_resource(res, res->start,
> > > -						start - res->start);
> > > +		} else if (res->end > end) {
> > > +			if (res->start >= new_start) {
> > > +				/* adjust the start */
> > > +				ret = __adjust_resource(res, end + 1,
> > > +							res->end - end);
> > > +			} else {
> > > +				/* split into two entries */
> > > +				if (!new_res) {
> > > +					ret = -ENOMEM;
> > > +					break;
> > > +				}
> > > +				new_res->name = res->name;
> > > +				new_res->start = end + 1;
> > > +				new_res->end = res->end;
> > > +				new_res->flags = res->flags;
> > > +				new_res->desc = res->desc;
> > > +				new_res->parent = res->parent;
> > > +				new_res->sibling = res->sibling;
> > > +				new_res->child = NULL;
> > > +
> > > +				ret = __adjust_resource(res, res->start,
> > > +							new_start - res->start);
> > > +				if (ret)
> > > +					break;
> > > +				res->sibling = new_res;
> > > +				new_res = NULL;
> > > +			}
> > >  		} else {
> > > -			/* split into two entries */
> > > -			if (!new_res) {
> > > -				ret = -ENOMEM;
> > > -				break;
> > > +			if (res->start < new_start) {
> > > +				/* adjust the end */
> > > +				ret = __adjust_resource(res, res->start,
> > > +							new_start - res->start);
> > > +				new_start = res->end+1;
> > > +				p = &res->sibling;
> > > +			} else {
> > > +				new_start = res->end+1;
> > > +				*p = res->sibling;
> > > +				free_resource(res);
> > > +				ret = 0;
> > >  			}
> > > -			new_res->name = res->name;
> > > -			new_res->start = end + 1;
> > > -			new_res->end = res->end;
> > > -			new_res->flags = res->flags;
> > > -			new_res->desc = res->desc;
> > > -			new_res->parent = res->parent;
> > > -			new_res->sibling = res->sibling;
> > > -			new_res->child = NULL;
> > > -
> > > -			ret = __adjust_resource(res, res->start,
> > > -						start - res->start);
> > > -			if (ret)
> > > -				break;
> > > -			res->sibling = new_res;
> > > -			new_res = NULL;
> > > +			if (res->end < end)
> > > +				continue;
> > >  		}
> > > -
> > >  		break;
> > >  	}
> > >
> > > --
> > > 2.12.3
> > >
>