| Message ID | 20241010065558.1347018-1-ying.huang@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] resource: Avoid unnecessary resource tree walking in __region_intersects() | expand |
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.
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 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?
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 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.
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.
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?
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.
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.
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.
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
[ 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.
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.
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.
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
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
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.
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
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 &&
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(-)