Currently, if __region_intersects() finds any overlapped but unmatched
resource, it walks the descendant resource tree to check for
overlapped and matched descendant resources.  This is achieved using
for_each_resource(), which iterates not only the descent tree, but
also subsequent sibling trees in certain scenarios.  While this
doesn't introduce bugs, it makes code hard to be understood and
potentially inefficient.

So, the patch renames next_resource() to __next_resource() and
modified it to return NULL after traversing all descent resources.
Test shows that this avoids unnecessary resource tree walking in
__region_intersects().

It appears even better to revise for_each_resource() to traverse the
descendant resource tree of "_root" only.  But that will cause "_root"
to be evaluated twice, which I don't find a good way to eliminate.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Alistair Popple <apopple@nvidia.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Baoquan He <bhe@redhat.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
---
 kernel/resource.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index b730bd28b422..3ded4c5d4418 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -50,15 +50,28 @@ EXPORT_SYMBOL(iomem_resource);
 
 static DEFINE_RWLOCK(resource_lock);
 
-static struct resource *next_resource(struct resource *p, bool skip_children)
+static struct resource *__next_resource(struct resource *root, struct resource *p,
+					bool skip_children)
 {
 	if (!skip_children && p->child)
 		return p->child;
-	while (!p->sibling && p->parent)
+	while (!p->sibling && p->parent) {
 		p = p->parent;
+		if (p == root)
+			return NULL;
+	}
 	return p->sibling;
 }
 
+static struct resource *next_resource(struct resource *p, bool skip_children)
+{
+	return __next_resource(NULL, p, skip_children);
+}
+
+/*
+ * Traverse the whole resource tree (NOTE: not descendant tree under
+ * _root) from _root->child on.
+ */
 #define for_each_resource(_root, _p, _skip_children) \
 	for ((_p) = (_root)->child; (_p); (_p) = next_resource(_p, _skip_children))
 
@@ -572,7 +585,7 @@ static int __region_intersects(struct resource *parent, resource_size_t start,
 		covered = false;
 		ostart = max(res.start, p->start);
 		oend = min(res.end, p->end);
-		for_each_resource(p, dp, false) {
+		for (dp = p->child; dp; dp = __next_resource(p, dp, false)) {
 			if (!resource_overlaps(dp, &res))
 				continue;
 			is_type = (dp->flags & flags) == flags &&
-- 
2.39.2

On 10.10.24 08:55, Huang Ying wrote:
> Currently, if __region_intersects() finds any overlapped but unmatched
> resource, it walks the descendant resource tree to check for
> overlapped and matched descendant resources.  This is achieved using
> for_each_resource(), which iterates not only the descent tree, but
> also subsequent sibling trees in certain scenarios.  While this
> doesn't introduce bugs, it makes code hard to be understood and
> potentially inefficient.
> 
> So, the patch renames next_resource() to __next_resource() and
> modified it to return NULL after traversing all descent resources.
> Test shows that this avoids unnecessary resource tree walking in
> __region_intersects().
> 
> It appears even better to revise for_each_resource() to traverse the
> descendant resource tree of "_root" only.  But that will cause "_root"
> to be evaluated twice, which I don't find a good way to eliminate.

I'm not sure I'm enjoying below code, it makes it harder for me to
understand what's happening.

I'm also not 100% sure why "p" becomes "root" and "dp" becomes "p" when
calling the function :) Likely this works as intended, but it's confusing
(IOW, bad naming, especially for dp).


I think you should just leave next_resource() alone and rather add
a new function that doesn't conditionally consume NULL pointers
(and also no skip_children because you're passing false either way).

static struct resource *next_resource_XXX(struct resource *root,
		struct resource *p)
{
	while (!p->sibling && p->parent) {
		p = p->parent;
		if (p == root)
			return NULL;
	}
	return p->sibling;
}

Maybe even better, add a new for_each_resource() macro that expresses the intended semantics.

#define for_each_resource_XXX(_root, _p) \
	for ((_p) = (_root)->child; (_p); (_p) = next_resource_XXX(_root, _p))

XXX TBD

Or do you think this should not only be "improved" for the __region_intersects() use case
but for all for_each_resource() users? I cannot tell easily.

-- 
Cheers,

David / dhildenb

David Hildenbrand <david@redhat.com> writes:

> On 10.10.24 08:55, Huang Ying wrote:
>> Currently, if __region_intersects() finds any overlapped but unmatched
>> resource, it walks the descendant resource tree to check for
>> overlapped and matched descendant resources.  This is achieved using
>> for_each_resource(), which iterates not only the descent tree, but
>> also subsequent sibling trees in certain scenarios.  While this
>> doesn't introduce bugs, it makes code hard to be understood and
>> potentially inefficient.
>> So, the patch renames next_resource() to __next_resource() and
>> modified it to return NULL after traversing all descent resources.
>> Test shows that this avoids unnecessary resource tree walking in
>> __region_intersects().
>> It appears even better to revise for_each_resource() to traverse the
>> descendant resource tree of "_root" only.  But that will cause "_root"
>> to be evaluated twice, which I don't find a good way to eliminate.
>
> I'm not sure I'm enjoying below code, it makes it harder for me to
> understand what's happening.
>
> I'm also not 100% sure why "p" becomes "root" and "dp" becomes "p" when
> calling the function :) Likely this works as intended, but it's confusing
> (IOW, bad naming, especially for dp).
>
>
> I think you should just leave next_resource() alone and rather add
> a new function that doesn't conditionally consume NULL pointers
> (and also no skip_children because you're passing false either way).
>
> static struct resource *next_resource_XXX(struct resource *root,
> 		struct resource *p)
> {
> 	while (!p->sibling && p->parent) {
> 		p = p->parent;
> 		if (p == root)
> 			return NULL;
> 	}
> 	return p->sibling;
> }
>
> Maybe even better, add a new for_each_resource() macro that expresses the intended semantics.
>
> #define for_each_resource_XXX(_root, _p) \
> 	for ((_p) = (_root)->child; (_p); (_p) = next_resource_XXX(_root, _p))

Yes.  This can improve code readability.

A possible issue is that "_root" will be evaluated twice in above macro
definition.  IMO, this should be avoided.  Do you have some idea about
how to do that?  Something like below?

#define for_each_resource_XXX(_root, _p)                                \
	for (typeof(_root) __root = (_root), __p = (_p) = (__root)->child; \
	     __p && (_p); (_p) = next_resource_XXX(__root, _p))

> XXX TBD
>
> Or do you think this should not only be "improved" for the __region_intersects() use case
> but for all for_each_resource() users? I cannot tell easily.

I prefer to make for_each_resource() to traverse only descendant
resource tree of "_root".  This helps code reusing and make the
interface easier to be understood.  The difficulty lies in twice
evaluation as above.

--
Best Regards,
Huang, Ying

On Fri, Oct 11, 2024 at 09:06:37AM +0800, Huang, Ying wrote:
> David Hildenbrand <david@redhat.com> writes:
> > On 10.10.24 08:55, Huang Ying wrote:

...

> > 	for ((_p) = (_root)->child; (_p); (_p) = next_resource_XXX(_root, _p))
> 
> Yes.  This can improve code readability.
> 
> A possible issue is that "_root" will be evaluated twice in above macro
> definition.  IMO, this should be avoided.

Ideally, yes. But how many for_each type of macros you see that really try hard
to achieve that? I believe we shouldn't worry right now about this and rely on
the fact that root is the given variable. Or do you have an example of what you
suggested in the other reply, i.e. where it's an evaluation of the heavy call?

> Do you have some idea about
> how to do that?  Something like below?
> 
> #define for_each_resource_XXX(_root, _p)                                \
> 	for (typeof(_root) __root = (_root), __p = (_p) = (__root)->child; \
> 	     __p && (_p); (_p) = next_resource_XXX(__root, _p))

This is a bit ugly :-( I would avoid ugliness as long as we have no problem to
solve (see above).

> > XXX TBD
> >
> > Or do you think this should not only be "improved" for the __region_intersects() use case
> > but for all for_each_resource() users? I cannot tell easily.
> 
> I prefer to make for_each_resource() to traverse only descendant
> resource tree of "_root".  This helps code reusing and make the
> interface easier to be understood.  The difficulty lies in twice
> evaluation as above.

-- 
With Best Regards,
Andy Shevchenko

[ I was sent here from 87msiw4j1m.fsf@yhuang6-desk2.ccr.corp.intel.com,
can we please just create a for_each_resource_descendant() as Ying has
proposed? ]

Andy Shevchenko wrote:
> On Fri, Oct 11, 2024 at 09:06:37AM +0800, Huang, Ying wrote:
> > David Hildenbrand <david@redhat.com> writes:
> > > On 10.10.24 08:55, Huang Ying wrote:
> 
> ...
> 
> > > 	for ((_p) = (_root)->child; (_p); (_p) = next_resource_XXX(_root, _p))
> > 
> > Yes.  This can improve code readability.
> > 
> > A possible issue is that "_root" will be evaluated twice in above macro
> > definition.  IMO, this should be avoided.
> 
> Ideally, yes. But how many for_each type of macros you see that really try hard
> to achieve that? I believe we shouldn't worry right now about this and rely on
> the fact that root is the given variable. Or do you have an example of what you
> suggested in the other reply, i.e. where it's an evaluation of the heavy call?
> 
> > Do you have some idea about
> > how to do that?  Something like below?
> > 
> > #define for_each_resource_XXX(_root, _p)                                \
> > 	for (typeof(_root) __root = (_root), __p = (_p) = (__root)->child; \
> > 	     __p && (_p); (_p) = next_resource_XXX(__root, _p))
> 
> This is a bit ugly :-( I would avoid ugliness as long as we have no problem to
> solve (see above).

Using a local defined variable to avoid double evaluation is standard
practice. I do not understand "avoid ugliness as long as we have no problem to
solve", the problem to solve will be if someone accidentally does
something like "for_each_resource_descendant(root++, res)". *That* will
be a problem when someone finally realizes that the macro is hiding a
double evaluation.

So no, this proposal is not "ugly", it is a best practice. See the
definition of min_not_zero() for example.

On Wed, Oct 23, 2024 at 02:07:52PM -0700, Dan Williams wrote:
> Andy Shevchenko wrote:
> > On Fri, Oct 11, 2024 at 09:06:37AM +0800, Huang, Ying wrote:
> > > David Hildenbrand <david@redhat.com> writes:
> > > > On 10.10.24 08:55, Huang Ying wrote:

...

> > > > 	for ((_p) = (_root)->child; (_p); (_p) = next_resource_XXX(_root, _p))
> > > 
> > > Yes.  This can improve code readability.
> > > 
> > > A possible issue is that "_root" will be evaluated twice in above macro
> > > definition.  IMO, this should be avoided.
> > 
> > Ideally, yes. But how many for_each type of macros you see that really try hard
> > to achieve that? I believe we shouldn't worry right now about this and rely on
> > the fact that root is the given variable. Or do you have an example of what you
> > suggested in the other reply, i.e. where it's an evaluation of the heavy call?
> > 
> > > Do you have some idea about
> > > how to do that?  Something like below?
> > > 
> > > #define for_each_resource_XXX(_root, _p)                                \
> > > 	for (typeof(_root) __root = (_root), __p = (_p) = (__root)->child; \
> > > 	     __p && (_p); (_p) = next_resource_XXX(__root, _p))
> > 
> > This is a bit ugly :-( I would avoid ugliness as long as we have no problem to
> > solve (see above).
> 
> Using a local defined variable to avoid double evaluation is standard
> practice. I do not understand "avoid ugliness as long as we have no problem to
> solve", the problem to solve will be if someone accidentally does
> something like "for_each_resource_descendant(root++, res)". *That* will
> be a problem when someone finally realizes that the macro is hiding a
> double evaluation.

Can you explain, why do we need __p and how can we get rid of that?
I understand the part of the local variable for root.

> So no, this proposal is not "ugly", it is a best practice. See the
> definition of min_not_zero() for example.

I know that there are a lot of macros that look uglier that this one.

-- 
With Best Regards,
Andy Shevchenko

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> On Wed, Oct 23, 2024 at 02:07:52PM -0700, Dan Williams wrote:
>> Andy Shevchenko wrote:
>> > On Fri, Oct 11, 2024 at 09:06:37AM +0800, Huang, Ying wrote:
>> > > David Hildenbrand <david@redhat.com> writes:
>> > > > On 10.10.24 08:55, Huang Ying wrote:
>
> ...
>
>> > > > 	for ((_p) = (_root)->child; (_p); (_p) = next_resource_XXX(_root, _p))
>> > > 
>> > > Yes.  This can improve code readability.
>> > > 
>> > > A possible issue is that "_root" will be evaluated twice in above macro
>> > > definition.  IMO, this should be avoided.
>> > 
>> > Ideally, yes. But how many for_each type of macros you see that really try hard
>> > to achieve that? I believe we shouldn't worry right now about this and rely on
>> > the fact that root is the given variable. Or do you have an example of what you
>> > suggested in the other reply, i.e. where it's an evaluation of the heavy call?
>> > 
>> > > Do you have some idea about
>> > > how to do that?  Something like below?
>> > > 
>> > > #define for_each_resource_XXX(_root, _p)                                \
>> > > 	for (typeof(_root) __root = (_root), __p = (_p) = (__root)->child; \
>> > > 	     __p && (_p); (_p) = next_resource_XXX(__root, _p))
>> > 
>> > This is a bit ugly :-( I would avoid ugliness as long as we have no problem to
>> > solve (see above).
>> 
>> Using a local defined variable to avoid double evaluation is standard
>> practice. I do not understand "avoid ugliness as long as we have no problem to
>> solve", the problem to solve will be if someone accidentally does
>> something like "for_each_resource_descendant(root++, res)". *That* will
>> be a problem when someone finally realizes that the macro is hiding a
>> double evaluation.
>
> Can you explain, why do we need __p and how can we get rid of that?
> I understand the part of the local variable for root.

If don't use '__p', the macro becomes

#define for_each_resource_XXX(_root, _p)                                \
	for (typeof(_root) __root = (_root), (_p) = (__root)->child; \
	     (_p); (_p) = next_resource_XXX(__root, _p))

Where, '_p' must be a variable name, and it will be a new variable
inside for loop and mask the variable with same name outside of macro.
IIUC, this breaks the macro convention in kernel and has subtle variable
masking semantics.

>> So no, this proposal is not "ugly", it is a best practice. See the
>> definition of min_not_zero() for example.
>
> I know that there are a lot of macros that look uglier that this one.

--
Best Regards,
Huang, Ying

On Thu, Oct 24, 2024 at 08:30:39PM +0800, Huang, Ying wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> > On Wed, Oct 23, 2024 at 02:07:52PM -0700, Dan Williams wrote:
> >> Andy Shevchenko wrote:
> >> > On Fri, Oct 11, 2024 at 09:06:37AM +0800, Huang, Ying wrote:
> >> > > David Hildenbrand <david@redhat.com> writes:
> >> > > > On 10.10.24 08:55, Huang Ying wrote:

...

> >> > > > 	for ((_p) = (_root)->child; (_p); (_p) = next_resource_XXX(_root, _p))
> >> > > 
> >> > > Yes.  This can improve code readability.
> >> > > 
> >> > > A possible issue is that "_root" will be evaluated twice in above macro
> >> > > definition.  IMO, this should be avoided.
> >> > 
> >> > Ideally, yes. But how many for_each type of macros you see that really try hard
> >> > to achieve that? I believe we shouldn't worry right now about this and rely on
> >> > the fact that root is the given variable. Or do you have an example of what you
> >> > suggested in the other reply, i.e. where it's an evaluation of the heavy call?
> >> > 
> >> > > Do you have some idea about
> >> > > how to do that?  Something like below?
> >> > > 
> >> > > #define for_each_resource_XXX(_root, _p)                                \
> >> > > 	for (typeof(_root) __root = (_root), __p = (_p) = (__root)->child; \
> >> > > 	     __p && (_p); (_p) = next_resource_XXX(__root, _p))
> >> > 
> >> > This is a bit ugly :-( I would avoid ugliness as long as we have no problem to
> >> > solve (see above).
> >> 
> >> Using a local defined variable to avoid double evaluation is standard
> >> practice. I do not understand "avoid ugliness as long as we have no problem to
> >> solve", the problem to solve will be if someone accidentally does
> >> something like "for_each_resource_descendant(root++, res)". *That* will
> >> be a problem when someone finally realizes that the macro is hiding a
> >> double evaluation.
> >
> > Can you explain, why do we need __p and how can we get rid of that?
> > I understand the part of the local variable for root.
> 
> If don't use '__p', the macro becomes
> 
> #define for_each_resource_XXX(_root, _p)                                \
> 	for (typeof(_root) __root = (_root), (_p) = (__root)->child; \
> 	     (_p); (_p) = next_resource_XXX(__root, _p))
> 
> Where, '_p' must be a variable name, and it will be a new variable
> inside for loop and mask the variable with same name outside of macro.
> IIUC, this breaks the macro convention in kernel and has subtle variable
> masking semantics.

Yep.

In property.h nobody cares about evaluation which makes the macro as simple as

#define for_each_resource_XXX(_root, _p)		\
	for (_p = next_resource_XXX(__root, NULL); _p;	\
	     _p = next_resource_XXX(__root, _p))

(Dan,
 that's what I called to avoid solving issues we don't have and most likely
 will never have.)

but if you want to stick with your variant some improvements can be done:

#define for_each_resource_XXX(_root, _p)				\
	for (typeof(_root) __root = (_root), __p = _p = __root->child;	\
	     __p && _p; _p = next_resource_XXX(__root, _p))


1) no need to have local variable in parentheses;
2) no need to have iterator in parentheses, otherwise it would be crazy code
that has put something really wrong there and still expect the thing to work.

> >> So no, this proposal is not "ugly", it is a best practice. See the
> >> definition of min_not_zero() for example.
> >
> > I know that there are a lot of macros that look uglier that this one.

-- 
With Best Regards,
Andy Shevchenko

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> On Thu, Oct 24, 2024 at 08:30:39PM +0800, Huang, Ying wrote:
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> > On Wed, Oct 23, 2024 at 02:07:52PM -0700, Dan Williams wrote:
>> >> Andy Shevchenko wrote:
>> >> > On Fri, Oct 11, 2024 at 09:06:37AM +0800, Huang, Ying wrote:
>> >> > > David Hildenbrand <david@redhat.com> writes:
>> >> > > > On 10.10.24 08:55, Huang Ying wrote:
>
> ...
>
>> >> > > > 	for ((_p) = (_root)->child; (_p); (_p) = next_resource_XXX(_root, _p))
>> >> > > 
>> >> > > Yes.  This can improve code readability.
>> >> > > 
>> >> > > A possible issue is that "_root" will be evaluated twice in above macro
>> >> > > definition.  IMO, this should be avoided.
>> >> > 
>> >> > Ideally, yes. But how many for_each type of macros you see that really try hard
>> >> > to achieve that? I believe we shouldn't worry right now about this and rely on
>> >> > the fact that root is the given variable. Or do you have an example of what you
>> >> > suggested in the other reply, i.e. where it's an evaluation of the heavy call?
>> >> > 
>> >> > > Do you have some idea about
>> >> > > how to do that?  Something like below?
>> >> > > 
>> >> > > #define for_each_resource_XXX(_root, _p)                                \
>> >> > > 	for (typeof(_root) __root = (_root), __p = (_p) = (__root)->child; \
>> >> > > 	     __p && (_p); (_p) = next_resource_XXX(__root, _p))
>> >> > 
>> >> > This is a bit ugly :-( I would avoid ugliness as long as we have no problem to
>> >> > solve (see above).
>> >> 
>> >> Using a local defined variable to avoid double evaluation is standard
>> >> practice. I do not understand "avoid ugliness as long as we have no problem to
>> >> solve", the problem to solve will be if someone accidentally does
>> >> something like "for_each_resource_descendant(root++, res)". *That* will
>> >> be a problem when someone finally realizes that the macro is hiding a
>> >> double evaluation.
>> >
>> > Can you explain, why do we need __p and how can we get rid of that?
>> > I understand the part of the local variable for root.
>> 
>> If don't use '__p', the macro becomes
>> 
>> #define for_each_resource_XXX(_root, _p)                                \
>> 	for (typeof(_root) __root = (_root), (_p) = (__root)->child; \
>> 	     (_p); (_p) = next_resource_XXX(__root, _p))
>> 
>> Where, '_p' must be a variable name, and it will be a new variable
>> inside for loop and mask the variable with same name outside of macro.
>> IIUC, this breaks the macro convention in kernel and has subtle variable
>> masking semantics.
>
> Yep.
>
> In property.h nobody cares about evaluation which makes the macro as simple as
>
> #define for_each_resource_XXX(_root, _p)		\
> 	for (_p = next_resource_XXX(__root, NULL); _p;	\
> 	     _p = next_resource_XXX(__root, _p))
>
> (Dan,
>  that's what I called to avoid solving issues we don't have and most likely
>  will never have.)
>
> but if you want to stick with your variant some improvements can be done:

I still prefer to solve possible issues if the solution isn't too
complex.

> #define for_each_resource_XXX(_root, _p)				\
> 	for (typeof(_root) __root = (_root), __p = _p = __root->child;	\
> 	     __p && _p; _p = next_resource_XXX(__root, _p))
>
>
> 1) no need to have local variable in parentheses;
> 2) no need to have iterator in parentheses, otherwise it would be crazy code
> that has put something really wrong there and still expect the thing to work.

Thanks!  You are right.  Will use this in the future versions.

>> >> So no, this proposal is not "ugly", it is a best practice. See the
>> >> definition of min_not_zero() for example.
>> >
>> > I know that there are a lot of macros that look uglier that this one.

--
Best Regards,
Huang, Ying

Andy Shevchenko wrote:
> On Thu, Oct 24, 2024 at 08:30:39PM +0800, Huang, Ying wrote:
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> > > On Wed, Oct 23, 2024 at 02:07:52PM -0700, Dan Williams wrote:
> > >> Andy Shevchenko wrote:
> > >> > On Fri, Oct 11, 2024 at 09:06:37AM +0800, Huang, Ying wrote:
> > >> > > David Hildenbrand <david@redhat.com> writes:
> > >> > > > On 10.10.24 08:55, Huang Ying wrote:
> 
> ...
> 
> > >> > > > 	for ((_p) = (_root)->child; (_p); (_p) = next_resource_XXX(_root, _p))
> > >> > > 
> > >> > > Yes.  This can improve code readability.
> > >> > > 
> > >> > > A possible issue is that "_root" will be evaluated twice in above macro
> > >> > > definition.  IMO, this should be avoided.
> > >> > 
> > >> > Ideally, yes. But how many for_each type of macros you see that really try hard
> > >> > to achieve that? I believe we shouldn't worry right now about this and rely on
> > >> > the fact that root is the given variable. Or do you have an example of what you
> > >> > suggested in the other reply, i.e. where it's an evaluation of the heavy call?
> > >> > 
> > >> > > Do you have some idea about
> > >> > > how to do that?  Something like below?
> > >> > > 
> > >> > > #define for_each_resource_XXX(_root, _p)                                \
> > >> > > 	for (typeof(_root) __root = (_root), __p = (_p) = (__root)->child; \
> > >> > > 	     __p && (_p); (_p) = next_resource_XXX(__root, _p))
> > >> > 
> > >> > This is a bit ugly :-( I would avoid ugliness as long as we have no problem to
> > >> > solve (see above).
> > >> 
> > >> Using a local defined variable to avoid double evaluation is standard
> > >> practice. I do not understand "avoid ugliness as long as we have no problem to
> > >> solve", the problem to solve will be if someone accidentally does
> > >> something like "for_each_resource_descendant(root++, res)". *That* will
> > >> be a problem when someone finally realizes that the macro is hiding a
> > >> double evaluation.
> > >
> > > Can you explain, why do we need __p and how can we get rid of that?
> > > I understand the part of the local variable for root.
> > 
> > If don't use '__p', the macro becomes
> > 
> > #define for_each_resource_XXX(_root, _p)                                \
> > 	for (typeof(_root) __root = (_root), (_p) = (__root)->child; \
> > 	     (_p); (_p) = next_resource_XXX(__root, _p))
> > 
> > Where, '_p' must be a variable name, and it will be a new variable
> > inside for loop and mask the variable with same name outside of macro.
> > IIUC, this breaks the macro convention in kernel and has subtle variable
> > masking semantics.
> 
> Yep.

Oh, due to the comment expression, good catch.

> 
> In property.h nobody cares about evaluation which makes the macro as simple as
> 
> #define for_each_resource_XXX(_root, _p)		\
> 	for (_p = next_resource_XXX(__root, NULL); _p;	\
> 	     _p = next_resource_XXX(__root, _p))
> 
> (Dan,
>  that's what I called to avoid solving issues we don't have and most likely
>  will never have.)

Ah, my apologies, I thought the objection was to the macro altogether. 

> but if you want to stick with your variant some improvements can be done:
> 
> #define for_each_resource_XXX(_root, _p)				\
> 	for (typeof(_root) __root = (_root), __p = _p = __root->child;	\
> 	     __p && _p; _p = next_resource_XXX(__root, _p))
> 
> 
> 1) no need to have local variable in parentheses;
> 2) no need to have iterator in parentheses, otherwise it would be crazy code
> that has put something really wrong there and still expect the thing to work.

Why not:

#define for_each_resource_XXX(_root, _p)				\
	for (typeof(_root) __root = (_root), __p = _p = __root->child;	\
	     _p; _p = next_resource_XXX(__root, _p))

The __p is only to allow for _p to be initialized in the first statement
without causing a new "_p" shadow to be declared.

On Thu, Oct 24, 2024 at 02:57:38PM -0700, Dan Williams wrote:
> Andy Shevchenko wrote:
> > On Thu, Oct 24, 2024 at 08:30:39PM +0800, Huang, Ying wrote:
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> > > > On Wed, Oct 23, 2024 at 02:07:52PM -0700, Dan Williams wrote:
> > > >> Andy Shevchenko wrote:
> > > >> > On Fri, Oct 11, 2024 at 09:06:37AM +0800, Huang, Ying wrote:
> > > >> > > David Hildenbrand <david@redhat.com> writes:
> > > >> > > > On 10.10.24 08:55, Huang Ying wrote:

...

> > > >> > > > 	for ((_p) = (_root)->child; (_p); (_p) = next_resource_XXX(_root, _p))
> > > >> > > 
> > > >> > > Yes.  This can improve code readability.
> > > >> > > 
> > > >> > > A possible issue is that "_root" will be evaluated twice in above macro
> > > >> > > definition.  IMO, this should be avoided.
> > > >> > 
> > > >> > Ideally, yes. But how many for_each type of macros you see that really try hard
> > > >> > to achieve that? I believe we shouldn't worry right now about this and rely on
> > > >> > the fact that root is the given variable. Or do you have an example of what you
> > > >> > suggested in the other reply, i.e. where it's an evaluation of the heavy call?
> > > >> > 
> > > >> > > Do you have some idea about
> > > >> > > how to do that?  Something like below?
> > > >> > > 
> > > >> > > #define for_each_resource_XXX(_root, _p)                                \
> > > >> > > 	for (typeof(_root) __root = (_root), __p = (_p) = (__root)->child; \
> > > >> > > 	     __p && (_p); (_p) = next_resource_XXX(__root, _p))
> > > >> > 
> > > >> > This is a bit ugly :-( I would avoid ugliness as long as we have no problem to
> > > >> > solve (see above).
> > > >> 
> > > >> Using a local defined variable to avoid double evaluation is standard
> > > >> practice. I do not understand "avoid ugliness as long as we have no problem to
> > > >> solve", the problem to solve will be if someone accidentally does
> > > >> something like "for_each_resource_descendant(root++, res)". *That* will
> > > >> be a problem when someone finally realizes that the macro is hiding a
> > > >> double evaluation.
> > > >
> > > > Can you explain, why do we need __p and how can we get rid of that?
> > > > I understand the part of the local variable for root.
> > > 
> > > If don't use '__p', the macro becomes
> > > 
> > > #define for_each_resource_XXX(_root, _p)                                \
> > > 	for (typeof(_root) __root = (_root), (_p) = (__root)->child; \
> > > 	     (_p); (_p) = next_resource_XXX(__root, _p))
> > > 
> > > Where, '_p' must be a variable name, and it will be a new variable
> > > inside for loop and mask the variable with same name outside of macro.
> > > IIUC, this breaks the macro convention in kernel and has subtle variable
> > > masking semantics.
> > 
> > Yep.
> 
> Oh, due to the comment expression, good catch.
> 
> > In property.h nobody cares about evaluation which makes the macro as simple as
> > 
> > #define for_each_resource_XXX(_root, _p)		\
> > 	for (_p = next_resource_XXX(__root, NULL); _p;	\
> > 	     _p = next_resource_XXX(__root, _p))
> > 
> > (Dan,
> >  that's what I called to avoid solving issues we don't have and most likely
> >  will never have.)
> 
> Ah, my apologies, I thought the objection was to the macro altogether. 

No, no, I'm supporting the idea!

> > but if you want to stick with your variant some improvements can be done:
> > 
> > #define for_each_resource_XXX(_root, _p)				\
> > 	for (typeof(_root) __root = (_root), __p = _p = __root->child;	\
> > 	     __p && _p; _p = next_resource_XXX(__root, _p))
> > 
> > 
> > 1) no need to have local variable in parentheses;
> > 2) no need to have iterator in parentheses, otherwise it would be crazy code
> > that has put something really wrong there and still expect the thing to work.
> 
> Why not:
> 
> #define for_each_resource_XXX(_root, _p)				\
> 	for (typeof(_root) __root = (_root), __p = _p = __root->child;	\
> 	     _p; _p = next_resource_XXX(__root, _p))
> 
> The __p is only to allow for _p to be initialized in the first statement
> without causing a new "_p" shadow to be declared.

If people think this would be better than the existing patterns, okay. fine.

-- 
With Best Regards,
Andy Shevchenko

Andy Shevchenko wrote:
[..]
> > > but if you want to stick with your variant some improvements can be done:
> > > 
> > > #define for_each_resource_XXX(_root, _p)				\
> > > 	for (typeof(_root) __root = (_root), __p = _p = __root->child;	\
> > > 	     __p && _p; _p = next_resource_XXX(__root, _p))
> > > 
> > > 
> > > 1) no need to have local variable in parentheses;
> > > 2) no need to have iterator in parentheses, otherwise it would be crazy code
> > > that has put something really wrong there and still expect the thing to work.
> > 
> > Why not:
> > 
> > #define for_each_resource_XXX(_root, _p)				\
> > 	for (typeof(_root) __root = (_root), __p = _p = __root->child;	\
> > 	     _p; _p = next_resource_XXX(__root, _p))
> > 
> > The __p is only to allow for _p to be initialized in the first statement
> > without causing a new "_p" shadow to be declared.
> 
> If people think this would be better than the existing patterns, okay. fine.

I think this case is different than the existing patterns in that the
iterator variable needs to be initiatlized from a declared variable, and
as Ying said, my proposal is busted.

To your point though, lets add a comment on why this macro is a bit
different to avoid people like me making bad cleanup suggestions.

Dan Williams <dan.j.williams@intel.com> writes:

> Andy Shevchenko wrote:
> [..]
>> > > but if you want to stick with your variant some improvements can be done:
>> > > 
>> > > #define for_each_resource_XXX(_root, _p)				\
>> > > 	for (typeof(_root) __root = (_root), __p = _p = __root->child;	\
>> > > 	     __p && _p; _p = next_resource_XXX(__root, _p))
>> > > 
>> > > 
>> > > 1) no need to have local variable in parentheses;
>> > > 2) no need to have iterator in parentheses, otherwise it would be crazy code
>> > > that has put something really wrong there and still expect the thing to work.
>> > 
>> > Why not:
>> > 
>> > #define for_each_resource_XXX(_root, _p)				\
>> > 	for (typeof(_root) __root = (_root), __p = _p = __root->child;	\
>> > 	     _p; _p = next_resource_XXX(__root, _p))
>> > 
>> > The __p is only to allow for _p to be initialized in the first statement
>> > without causing a new "_p" shadow to be declared.
>> 
>> If people think this would be better than the existing patterns, okay. fine.
>
> I think this case is different than the existing patterns in that the
> iterator variable needs to be initiatlized from a declared variable, and
> as Ying said, my proposal is busted.
>
> To your point though, lets add a comment on why this macro is a bit
> different to avoid people like me making bad cleanup suggestions.

Sure.  Will do that.

--
Best Regards,
Huang, Ying

Dan Williams <dan.j.williams@intel.com> writes:

> Andy Shevchenko wrote:
>> On Thu, Oct 24, 2024 at 08:30:39PM +0800, Huang, Ying wrote:
>> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> > > On Wed, Oct 23, 2024 at 02:07:52PM -0700, Dan Williams wrote:
>> > >> Andy Shevchenko wrote:
>> > >> > On Fri, Oct 11, 2024 at 09:06:37AM +0800, Huang, Ying wrote:
>> > >> > > David Hildenbrand <david@redhat.com> writes:
>> > >> > > > On 10.10.24 08:55, Huang Ying wrote:
>> 
>> ...
>> 
>> > >> > > > 	for ((_p) = (_root)->child; (_p); (_p) = next_resource_XXX(_root, _p))
>> > >> > > 
>> > >> > > Yes.  This can improve code readability.
>> > >> > > 
>> > >> > > A possible issue is that "_root" will be evaluated twice in above macro
>> > >> > > definition.  IMO, this should be avoided.
>> > >> > 
>> > >> > Ideally, yes. But how many for_each type of macros you see that really try hard
>> > >> > to achieve that? I believe we shouldn't worry right now about this and rely on
>> > >> > the fact that root is the given variable. Or do you have an example of what you
>> > >> > suggested in the other reply, i.e. where it's an evaluation of the heavy call?
>> > >> > 
>> > >> > > Do you have some idea about
>> > >> > > how to do that?  Something like below?
>> > >> > > 
>> > >> > > #define for_each_resource_XXX(_root, _p)                                \
>> > >> > > 	for (typeof(_root) __root = (_root), __p = (_p) = (__root)->child; \
>> > >> > > 	     __p && (_p); (_p) = next_resource_XXX(__root, _p))
>> > >> > 
>> > >> > This is a bit ugly :-( I would avoid ugliness as long as we have no problem to
>> > >> > solve (see above).
>> > >> 
>> > >> Using a local defined variable to avoid double evaluation is standard
>> > >> practice. I do not understand "avoid ugliness as long as we have no problem to
>> > >> solve", the problem to solve will be if someone accidentally does
>> > >> something like "for_each_resource_descendant(root++, res)". *That* will
>> > >> be a problem when someone finally realizes that the macro is hiding a
>> > >> double evaluation.
>> > >
>> > > Can you explain, why do we need __p and how can we get rid of that?
>> > > I understand the part of the local variable for root.
>> > 
>> > If don't use '__p', the macro becomes
>> > 
>> > #define for_each_resource_XXX(_root, _p)                                \
>> > 	for (typeof(_root) __root = (_root), (_p) = (__root)->child; \
>> > 	     (_p); (_p) = next_resource_XXX(__root, _p))
>> > 
>> > Where, '_p' must be a variable name, and it will be a new variable
>> > inside for loop and mask the variable with same name outside of macro.
>> > IIUC, this breaks the macro convention in kernel and has subtle variable
>> > masking semantics.
>> 
>> Yep.
>
> Oh, due to the comment expression, good catch.
>
>> 
>> In property.h nobody cares about evaluation which makes the macro as simple as
>> 
>> #define for_each_resource_XXX(_root, _p)		\
>> 	for (_p = next_resource_XXX(__root, NULL); _p;	\
>> 	     _p = next_resource_XXX(__root, _p))
>> 
>> (Dan,
>>  that's what I called to avoid solving issues we don't have and most likely
>>  will never have.)
>
> Ah, my apologies, I thought the objection was to the macro altogether. 
>
>> but if you want to stick with your variant some improvements can be done:
>> 
>> #define for_each_resource_XXX(_root, _p)				\
>> 	for (typeof(_root) __root = (_root), __p = _p = __root->child;	\
>> 	     __p && _p; _p = next_resource_XXX(__root, _p))
>> 
>> 
>> 1) no need to have local variable in parentheses;
>> 2) no need to have iterator in parentheses, otherwise it would be crazy code
>> that has put something really wrong there and still expect the thing to work.
>
> Why not:
>
> #define for_each_resource_XXX(_root, _p)				\
> 	for (typeof(_root) __root = (_root), __p = _p = __root->child;	\
> 	     _p; _p = next_resource_XXX(__root, _p))
>
> The __p is only to allow for _p to be initialized in the first statement
> without causing a new "_p" shadow to be declared.

I have tries this before.  Compiler will complain with

warning: unused variable ‘__p’ [-Wunused-variable]

--
Best Regards,
Huang, Ying

On 11.10.24 12:49, Andy Shevchenko wrote:
> On Fri, Oct 11, 2024 at 09:06:37AM +0800, Huang, Ying wrote:
>> David Hildenbrand <david@redhat.com> writes:
>>> On 10.10.24 08:55, Huang Ying wrote:
> 
> ...
> 
>>> 	for ((_p) = (_root)->child; (_p); (_p) = next_resource_XXX(_root, _p))
>>
>> Yes.  This can improve code readability.
>>
>> A possible issue is that "_root" will be evaluated twice in above macro
>> definition.  IMO, this should be avoided.
> 
> Ideally, yes. But how many for_each type of macros you see that really try hard
> to achieve that? I believe we shouldn't worry right now about this and rely on
> the fact that root is the given variable. Or do you have an example of what you
> suggested in the other reply, i.e. where it's an evaluation of the heavy call?
> 
>> Do you have some idea about
>> how to do that?  Something like below?
>>
>> #define for_each_resource_XXX(_root, _p)                                \
>> 	for (typeof(_root) __root = (_root), __p = (_p) = (__root)->child; \
>> 	     __p && (_p); (_p) = next_resource_XXX(__root, _p))
> 
> This is a bit ugly :-( I would avoid ugliness as long as we have no problem to
> solve (see above).

Fully agreed, I didn't quite understand the concern about "evaluation" 
at first. If it's just reading a variable twice, it doesn't matter at 
all right now.

-- 
Cheers,

David / dhildenb

On Fri, Oct 11, 2024 at 12:51:09PM +0200, David Hildenbrand wrote:
> On 11.10.24 12:49, Andy Shevchenko wrote:
> > On Fri, Oct 11, 2024 at 09:06:37AM +0800, Huang, Ying wrote:
> > > David Hildenbrand <david@redhat.com> writes:
> > > > On 10.10.24 08:55, Huang Ying wrote:

...

> > > > 	for ((_p) = (_root)->child; (_p); (_p) = next_resource_XXX(_root, _p))
> > > 
> > > Yes.  This can improve code readability.
> > > 
> > > A possible issue is that "_root" will be evaluated twice in above macro
> > > definition.  IMO, this should be avoided.
> > 
> > Ideally, yes. But how many for_each type of macros you see that really try hard
> > to achieve that? I believe we shouldn't worry right now about this and rely on
> > the fact that root is the given variable. Or do you have an example of what you
> > suggested in the other reply, i.e. where it's an evaluation of the heavy call?
> > 
> > > Do you have some idea about
> > > how to do that?  Something like below?
> > > 
> > > #define for_each_resource_XXX(_root, _p)                                \
> > > 	for (typeof(_root) __root = (_root), __p = (_p) = (__root)->child; \
> > > 	     __p && (_p); (_p) = next_resource_XXX(__root, _p))
> > 
> > This is a bit ugly :-( I would avoid ugliness as long as we have no problem to
> > solve (see above).
> 
> Fully agreed, I didn't quite understand the concern about "evaluation" at
> first.

It's a basic concept for macros and a good mine field even for the simple
cases.

> If it's just reading a variable twice, it doesn't matter at all right
> now.

The problem (even if it's a variable) is that the content of variable can be
changed when run in non-atomic context, i.e. two evaluations will give two
different results. Most "simple" for_each macros leave this exercise to the
caller. That's what I also suggest for now.

-- 
With Best Regards,
Andy Shevchenko

On Fri, Oct 11, 2024 at 02:15:55PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 11, 2024 at 12:51:09PM +0200, David Hildenbrand wrote:
> > On 11.10.24 12:49, Andy Shevchenko wrote:
> > > On Fri, Oct 11, 2024 at 09:06:37AM +0800, Huang, Ying wrote:
> > > > David Hildenbrand <david@redhat.com> writes:
> > > > > On 10.10.24 08:55, Huang Ying wrote:

...

> > > > > 	for ((_p) = (_root)->child; (_p); (_p) = next_resource_XXX(_root, _p))
> > > > 
> > > > Yes.  This can improve code readability.
> > > > 
> > > > A possible issue is that "_root" will be evaluated twice in above macro
> > > > definition.  IMO, this should be avoided.
> > > 
> > > Ideally, yes. But how many for_each type of macros you see that really try hard
> > > to achieve that? I believe we shouldn't worry right now about this and rely on
> > > the fact that root is the given variable. Or do you have an example of what you
> > > suggested in the other reply, i.e. where it's an evaluation of the heavy call?
> > > 
> > > > Do you have some idea about
> > > > how to do that?  Something like below?
> > > > 
> > > > #define for_each_resource_XXX(_root, _p)                                \
> > > > 	for (typeof(_root) __root = (_root), __p = (_p) = (__root)->child; \
> > > > 	     __p && (_p); (_p) = next_resource_XXX(__root, _p))
> > > 
> > > This is a bit ugly :-( I would avoid ugliness as long as we have no problem to
> > > solve (see above).
> > 
> > Fully agreed, I didn't quite understand the concern about "evaluation" at
> > first.
> 
> It's a basic concept for macros and a good mine field even for the simple
> cases.
> 
> > If it's just reading a variable twice, it doesn't matter at all right
> > now.
> 
> The problem (even if it's a variable) is that the content of variable can be
> changed when run in non-atomic context, i.e. two evaluations will give two
> different results. Most "simple" for_each macros leave this exercise to the
> caller. That's what I also suggest for now.

For any context as Ying provided an example with calls, they have to be
idempotent, or you definitely get two different pointers for these, which is
bigger issue that what I described above.

-- 
With Best Regards,
Andy Shevchenko

On 11.10.24 13:19, Andy Shevchenko wrote:
> On Fri, Oct 11, 2024 at 02:15:55PM +0300, Andy Shevchenko wrote:
>> On Fri, Oct 11, 2024 at 12:51:09PM +0200, David Hildenbrand wrote:
>>> On 11.10.24 12:49, Andy Shevchenko wrote:
>>>> On Fri, Oct 11, 2024 at 09:06:37AM +0800, Huang, Ying wrote:
>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>> On 10.10.24 08:55, Huang Ying wrote:
> 
> ...
> 
>>>>>> 	for ((_p) = (_root)->child; (_p); (_p) = next_resource_XXX(_root, _p))
>>>>>
>>>>> Yes.  This can improve code readability.
>>>>>
>>>>> A possible issue is that "_root" will be evaluated twice in above macro
>>>>> definition.  IMO, this should be avoided.
>>>>
>>>> Ideally, yes. But how many for_each type of macros you see that really try hard
>>>> to achieve that? I believe we shouldn't worry right now about this and rely on
>>>> the fact that root is the given variable. Or do you have an example of what you
>>>> suggested in the other reply, i.e. where it's an evaluation of the heavy call?
>>>>
>>>>> Do you have some idea about
>>>>> how to do that?  Something like below?
>>>>>
>>>>> #define for_each_resource_XXX(_root, _p)                                \
>>>>> 	for (typeof(_root) __root = (_root), __p = (_p) = (__root)->child; \
>>>>> 	     __p && (_p); (_p) = next_resource_XXX(__root, _p))
>>>>
>>>> This is a bit ugly :-( I would avoid ugliness as long as we have no problem to
>>>> solve (see above).
>>>
>>> Fully agreed, I didn't quite understand the concern about "evaluation" at
>>> first.
>>
>> It's a basic concept for macros and a good mine field even for the simple
>> cases.
>>
>>> If it's just reading a variable twice, it doesn't matter at all right
>>> now.
>>
>> The problem (even if it's a variable) is that the content of variable can be
>> changed when run in non-atomic context, i.e. two evaluations will give two
>> different results. Most "simple" for_each macros leave this exercise to the
>> caller. That's what I also suggest for now.
> 
> For any context as Ying provided an example with calls, they have to be
> idempotent, or you definitely get two different pointers for these, which is
> bigger issue that what I described above.

Ah, now I understood what Ying meant: if the root pointer is modified 
within the loop body we'd be in trouble.

-- 
Cheers,

David / dhildenb

David Hildenbrand <david@redhat.com> writes:

> On 11.10.24 13:19, Andy Shevchenko wrote:
>> On Fri, Oct 11, 2024 at 02:15:55PM +0300, Andy Shevchenko wrote:
>>> On Fri, Oct 11, 2024 at 12:51:09PM +0200, David Hildenbrand wrote:
>>>> On 11.10.24 12:49, Andy Shevchenko wrote:
>>>>> On Fri, Oct 11, 2024 at 09:06:37AM +0800, Huang, Ying wrote:
>>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>>> On 10.10.24 08:55, Huang Ying wrote:
>> ...
>> 
>>>>>>> 	for ((_p) = (_root)->child; (_p); (_p) = next_resource_XXX(_root, _p))
>>>>>>
>>>>>> Yes.  This can improve code readability.
>>>>>>
>>>>>> A possible issue is that "_root" will be evaluated twice in above macro
>>>>>> definition.  IMO, this should be avoided.
>>>>>
>>>>> Ideally, yes. But how many for_each type of macros you see that really try hard
>>>>> to achieve that? I believe we shouldn't worry right now about this and rely on
>>>>> the fact that root is the given variable. Or do you have an example of what you
>>>>> suggested in the other reply, i.e. where it's an evaluation of the heavy call?
>>>>>
>>>>>> Do you have some idea about
>>>>>> how to do that?  Something like below?
>>>>>>
>>>>>> #define for_each_resource_XXX(_root, _p)                                \
>>>>>> 	for (typeof(_root) __root = (_root), __p = (_p) = (__root)->child; \
>>>>>> 	     __p && (_p); (_p) = next_resource_XXX(__root, _p))
>>>>>
>>>>> This is a bit ugly :-( I would avoid ugliness as long as we have no problem to
>>>>> solve (see above).
>>>>
>>>> Fully agreed, I didn't quite understand the concern about "evaluation" at
>>>> first.
>>>
>>> It's a basic concept for macros and a good mine field even for the simple
>>> cases.
>>>
>>>> If it's just reading a variable twice, it doesn't matter at all right
>>>> now.
>>>
>>> The problem (even if it's a variable) is that the content of variable can be
>>> changed when run in non-atomic context, i.e. two evaluations will give two
>>> different results. Most "simple" for_each macros leave this exercise to the
>>> caller. That's what I also suggest for now.
>> For any context as Ying provided an example with calls, they have to
>> be
>> idempotent, or you definitely get two different pointers for these, which is
>> bigger issue that what I described above.
>
> Ah, now I understood what Ying meant: if the root pointer is modified
> within the loop body we'd be in trouble.

Given we cannot provide a good macro implementation to traverse only the
descendant tree of _root, I suggest to just keep current
for_each_resource() implementation.  There is only one user of the
proposed new macro to traverse the descendant tree.  So, I suggest to
open coded the for loop instead.  More comments can be added to make it
clear.

--
Best Regards,
Huang, Ying

On 11.10.24 03:06, Huang, Ying wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>> On 10.10.24 08:55, Huang Ying wrote:
>>> Currently, if __region_intersects() finds any overlapped but unmatched
>>> resource, it walks the descendant resource tree to check for
>>> overlapped and matched descendant resources.  This is achieved using
>>> for_each_resource(), which iterates not only the descent tree, but
>>> also subsequent sibling trees in certain scenarios.  While this
>>> doesn't introduce bugs, it makes code hard to be understood and
>>> potentially inefficient.
>>> So, the patch renames next_resource() to __next_resource() and
>>> modified it to return NULL after traversing all descent resources.
>>> Test shows that this avoids unnecessary resource tree walking in
>>> __region_intersects().
>>> It appears even better to revise for_each_resource() to traverse the
>>> descendant resource tree of "_root" only.  But that will cause "_root"
>>> to be evaluated twice, which I don't find a good way to eliminate.
>>
>> I'm not sure I'm enjoying below code, it makes it harder for me to
>> understand what's happening.
>>
>> I'm also not 100% sure why "p" becomes "root" and "dp" becomes "p" when
>> calling the function :) Likely this works as intended, but it's confusing
>> (IOW, bad naming, especially for dp).
>>
>>
>> I think you should just leave next_resource() alone and rather add
>> a new function that doesn't conditionally consume NULL pointers
>> (and also no skip_children because you're passing false either way).
>>
>> static struct resource *next_resource_XXX(struct resource *root,
>> 		struct resource *p)
>> {
>> 	while (!p->sibling && p->parent) {
>> 		p = p->parent;
>> 		if (p == root)
>> 			return NULL;
>> 	}
>> 	return p->sibling;
>> }
>>
>> Maybe even better, add a new for_each_resource() macro that expresses the intended semantics.
>>
>> #define for_each_resource_XXX(_root, _p) \
>> 	for ((_p) = (_root)->child; (_p); (_p) = next_resource_XXX(_root, _p))
> 
> Yes.  This can improve code readability.
> 
> A possible issue is that "_root" will be evaluated twice in above macro
> definition. 

Do you mean that we would process it twice in the loop body, or what 
exactly do you mean with "evaluate" ?


And just I understand what we want to achieve: we want to walk the 
subtree below "root" and prevent going to root->sibling or root->parent 
if "root" is not actually the "real root", correct?

X
|--------|
A----D   E
|
B--C


So assume we start walking at A, we want to evaluate A,B,C but not D,E,X.

Does that sum up what we want to achieve?

-- 
Cheers,

David / dhildenb

David Hildenbrand <david@redhat.com> writes:

> On 11.10.24 03:06, Huang, Ying wrote:
>> David Hildenbrand <david@redhat.com> writes:
>> 
>>> On 10.10.24 08:55, Huang Ying wrote:
>>>> Currently, if __region_intersects() finds any overlapped but unmatched
>>>> resource, it walks the descendant resource tree to check for
>>>> overlapped and matched descendant resources.  This is achieved using
>>>> for_each_resource(), which iterates not only the descent tree, but
>>>> also subsequent sibling trees in certain scenarios.  While this
>>>> doesn't introduce bugs, it makes code hard to be understood and
>>>> potentially inefficient.
>>>> So, the patch renames next_resource() to __next_resource() and
>>>> modified it to return NULL after traversing all descent resources.
>>>> Test shows that this avoids unnecessary resource tree walking in
>>>> __region_intersects().
>>>> It appears even better to revise for_each_resource() to traverse the
>>>> descendant resource tree of "_root" only.  But that will cause "_root"
>>>> to be evaluated twice, which I don't find a good way to eliminate.
>>>
>>> I'm not sure I'm enjoying below code, it makes it harder for me to
>>> understand what's happening.
>>>
>>> I'm also not 100% sure why "p" becomes "root" and "dp" becomes "p" when
>>> calling the function :) Likely this works as intended, but it's confusing
>>> (IOW, bad naming, especially for dp).
>>>
>>>
>>> I think you should just leave next_resource() alone and rather add
>>> a new function that doesn't conditionally consume NULL pointers
>>> (and also no skip_children because you're passing false either way).
>>>
>>> static struct resource *next_resource_XXX(struct resource *root,
>>> 		struct resource *p)
>>> {
>>> 	while (!p->sibling && p->parent) {
>>> 		p = p->parent;
>>> 		if (p == root)
>>> 			return NULL;
>>> 	}
>>> 	return p->sibling;
>>> }
>>>
>>> Maybe even better, add a new for_each_resource() macro that expresses the intended semantics.
>>>
>>> #define for_each_resource_XXX(_root, _p) \
>>> 	for ((_p) = (_root)->child; (_p); (_p) = next_resource_XXX(_root, _p))
>> Yes.  This can improve code readability.
>> A possible issue is that "_root" will be evaluated twice in above
>> macro
>> definition. 
>
> Do you mean that we would process it twice in the loop body, or what
> exactly do you mean with "evaluate" ?

In the macro definition above, _root is used twice.  For example, if
"_root" is a time consuming function call, the function will run twice.
That's not expected.

> And just I understand what we want to achieve: we want to walk the
> subtree below "root" and prevent going to root->sibling or
> root->parent if "root" is not actually the "real root", correct?
>
> X
> |--------|
> A----D   E
> |
> B--C
>
>
> So assume we start walking at A, we want to evaluate A,B,C but not D,E,X.
>
> Does that sum up what we want to achieve?

Yes.

--
Best Regards,
Huang, Ying

On Fri, Oct 11, 2024 at 04:48:25PM +0800, Huang, Ying wrote:
> David Hildenbrand <david@redhat.com> writes:
> > On 11.10.24 03:06, Huang, Ying wrote:

...

> > And just I understand what we want to achieve: we want to walk the
> > subtree below "root" and prevent going to root->sibling or
> > root->parent if "root" is not actually the "real root", correct?
> >
> > X
> > |--------|
> > A----D   E
> > |
> > B--C
> >
> > So assume we start walking at A, we want to evaluate A,B,C but not D,E,X.
> >
> > Does that sum up what we want to achieve?
> 
> Yes.

Can this explanation be added to the commit message of the next version of
the patch?

-- 
With Best Regards,
Andy Shevchenko