Huang, Ying wrote:
> Dan Williams <dan.j.williams@xxxxxxxxx> 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.
