Re: [PATCH] Resource: fix region_intersects() for CXL memory

[Dan Williams <dan.j.williams@intel.com>]

Huang, Ying wrote:
> Dan Williams <dan.j.williams@intel.com> writes:
> > Huang Ying wrote:
[..]
> >>  kernel/resource.c | 44 +++++++++++++++++++++++++++++++++++++-------
> >>  1 file changed, 37 insertions(+), 7 deletions(-)
> >> 
> >> diff --git a/kernel/resource.c b/kernel/resource.c
> >> index 14777afb0a99..c97a5add9394 100644
> >> --- a/kernel/resource.c
> >> +++ b/kernel/resource.c
[..]
> >> +		/*
> >> +		 * Continue to search in descendant resources.  Unless
> >> +		 * the matched descendant resources cover the whole
> >> +		 * overlapped range, increase 'other', because it
> >> +		 * overlaps with 'p' at least.
> >> +		 */
> >> +		other++;
> >
> > This results in REGION_MIXED whenever the target of the search is found
> > as a descendant of @parent which I believe is unwanted.
> 
> This is not the behavior of this patch.  There's a "other--" later in
> this patch.
> 
> +		ostart = max(res.start, p->start);
> +		oend = min(res.end, p->end);
> +		for_each_resource(p, dp, false) {
> +			if (!resource_overlaps(dp, &res))
> +				continue;
> +			is_type = (((dp->flags & flags) == flags) &&
> +				   ((desc == IORES_DESC_NONE) ||
> +				    (desc == dp->desc)));
> +			if (is_type) {
> +				type++;
> +				if (dp->start > ostart)
> +					break;
> +				if (dp->end >= oend) {
> +					other--;     <====================== HERE!

Yes, I missed that.

> +					break;
> +				}
> +				ostart = dp->end + 1;
> +			}
> +		}
>  	}
>  
>  	if (type == 0)
> 
> 
> That is, if the overlapped range is covered by matched (is_type == true)
> descendant resources completely, other will not increase.
> 
> So, for resource tree as follows
> 
> 490000000-52fffffff : CXL Window 0
>   490000000-50fffffff : region0
>     490000000-50fffffff : dax0.0
>       490000000-50fffffff : System RAM (kmem)
>   510000000-52fffffff
>     510000000-52fffffff : dax0.1
> 
> region_intersects(, 0x490000000, PAGE_SIZE, IORESOURCE_SYSTEM_RAM,
> IORES_DESC_NONE) => REGION_INTERSECTS
> region_intersects(, 0x50f000000, 0x2000000, IORESOURCE_SYSTEM_RAM,
> IORES_DESC_NONE) => REGION_MIXED
> 
> Even for
> 
> 490000000-52fffffff : CXL Window 0
>   490000000-50fffffff : region0
>     490000000-50fffffff : dax0.0
>       490000000-50fffffff : System RAM (kmem)
> 
> region_intersects(, 0x50f000000, 0x2000000, IORESOURCE_SYSTEM_RAM,
> IORES_DESC_NONE) => REGION_MIXED
> 
> This isn't perfect, but it looks OK for me.  Because for
> 
> 490000000-50fffffff : System RAM
> 510000000-52fffffff : CXL Window 0
> 
> region_intersects(, 0x50f000000, 0x2000000, IORESOURCE_SYSTEM_RAM,
> IORES_DESC_NONE) => REGION_MIXED

That explanation makes sense and matches my expectation.

> However, I admit that the original code is hard to be understood,
> whether is something like below better?

I like that this proposal defers incrementing @other rather than
decrement after the fact.

> 
> 	for (p = parent->child; p ; p = p->sibling) {
> 		if (!resource_overlaps(p, &res))
> 			continue;
> 		is_type = (((p->flags & flags) == flags) &&
> 			   ((desc == IORES_DESC_NONE) || (desc == p->desc)));
> 		if (is_type) {
> 			type++;
> 			continue;
> 		}
> 		/*
> 		 * Continue to search in descendant resources.  Unless
> 		 * the matched descendant resources cover the whole
> 		 * overlapped range, increase 'other', because it
> 		 * overlaps with 'p' at least.
> 		 */
> 		covered = false;

I would call @covered, @single_descendant. Since @covered is ambiguous.

> 		ostart = max(res.start, p->start);
> 		oend = min(res.end, p->end);
> 		for_each_resource(p, dp, false) {
> 			if (!resource_overlaps(dp, &res))
> 				continue;
> 			is_type = (((dp->flags & flags) == flags) &&
> 				   ((desc == IORES_DESC_NONE) ||
> 				    (desc == dp->desc)));
> 			if (is_type) {
> 				type++;
> 				if (dp->start > ostart)

...this should have a comment:

/* partial descendant overlap indicates overlap with a descendant hole */

> 					break;
> 				if (dp->end >= oend) {
> 					covered = true;
> 					break;

...then per above this because easier to read as:

	single_descendant = true;

> 				}
> 				ostart = dp->end + 1;
> 			}
> 		}
> 		if (!covered)
> 			other++;
> 	}
> 
> > The semantics of region_intersects() has always been within a single
> > sibling level to date. So, I don't think @other should be incremented
> > until @is_type is non-zero. It follows that if @is_type is set and
> > !resource_contains(p, &res) then there is no point in descending because
> > it is known at that there are no descendants to worry about.
> 
> Sorry, I don't understand your words here.  Can you show your idea with
> some examples or pseudo code?

I think your proposed updates address my concern.