| Message ID | 20181211035128.43256-1-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm, sparse: remove check with __highest_present_section_nr in for_each_present_section_nr() | expand |
On Tue, Dec 11, 2018 at 11:51:28AM +0800, Wei Yang wrote: >A valid present section number is in [0, __highest_present_section_nr]. >And the return value of next_present_section_nr() meets this >requirement. This means it is not necessary to check it with ^ , if it is not (-1) Would like to add this to be more exact. >__highest_present_section_nr again in for_each_present_section_nr(). > >Since we pass an unsigned long *section_nr* to >for_each_present_section_nr(), we need to cast it to int before >comparing. > >Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >--- > mm/sparse.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > >diff --git a/mm/sparse.c b/mm/sparse.c >index a4fdbcb21514..9eaa8f98a3d2 100644 >--- a/mm/sparse.c >+++ b/mm/sparse.c >@@ -197,8 +197,7 @@ static inline int next_present_section_nr(int section_nr) > } > #define for_each_present_section_nr(start, section_nr) \ > for (section_nr = next_present_section_nr(start-1); \ >- ((section_nr >= 0) && \ >- (section_nr <= __highest_present_section_nr)); \ >+ (int)section_nr >= 0; \ > section_nr = next_present_section_nr(section_nr)) > > static inline unsigned long first_present_section_nr(void) >-- >2.15.1
On Tue 11-12-18 11:51:28, Wei Yang wrote: > A valid present section number is in [0, __highest_present_section_nr]. > And the return value of next_present_section_nr() meets this > requirement. This means it is not necessary to check it with > __highest_present_section_nr again in for_each_present_section_nr(). > > Since we pass an unsigned long *section_nr* to > for_each_present_section_nr(), we need to cast it to int before > comparing. Why do we want this patch? Is it an improvement? If yes, it is performance visible change or does it make the code easier to maintain? To me at least the later seems dubious to be honest because it adds a non-obvious dependency of the terminal condition to the next_present_section_nr implementation and that might turn out error prone. > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > --- > mm/sparse.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/mm/sparse.c b/mm/sparse.c > index a4fdbcb21514..9eaa8f98a3d2 100644 > --- a/mm/sparse.c > +++ b/mm/sparse.c > @@ -197,8 +197,7 @@ static inline int next_present_section_nr(int section_nr) > } > #define for_each_present_section_nr(start, section_nr) \ > for (section_nr = next_present_section_nr(start-1); \ > - ((section_nr >= 0) && \ > - (section_nr <= __highest_present_section_nr)); \ > + (int)section_nr >= 0; \ > section_nr = next_present_section_nr(section_nr)) > > static inline unsigned long first_present_section_nr(void) > -- > 2.15.1 >
On Tue, Dec 11, 2018 at 10:44:41AM +0100, Michal Hocko wrote: >On Tue 11-12-18 11:51:28, Wei Yang wrote: >> A valid present section number is in [0, __highest_present_section_nr]. >> And the return value of next_present_section_nr() meets this >> requirement. This means it is not necessary to check it with >> __highest_present_section_nr again in for_each_present_section_nr(). >> >> Since we pass an unsigned long *section_nr* to >> for_each_present_section_nr(), we need to cast it to int before >> comparing. > >Why do we want this patch? Is it an improvement? If yes, it is >performance visible change or does it make the code easier to maintain? > Michal I know you concern, maintainance is a very critical part of review. >To me at least the later seems dubious to be honest because it adds a >non-obvious dependency of the terminal condition to the >next_present_section_nr implementation and that might turn out error >prone. > While I think the original code is not that clear about the syntax. When we look at the next_present_section_nr(section_nr), the return value falls into two categories: -1 : no more present section after section_nr other: the next present section number after section_nr Based on this syntax, the iteration could be simpler to terminate when the return value is less than 0. This is what the patch tries to do. Maybe I could do more to help the maintainance: * add some comment about the return value of next_present_section_nr * terminate the loop when section_nr == -1 Hope this would help a little.
On Tue 11-12-18 10:19:05, Wei Yang wrote: > On Tue, Dec 11, 2018 at 10:44:41AM +0100, Michal Hocko wrote: > >On Tue 11-12-18 11:51:28, Wei Yang wrote: > >> A valid present section number is in [0, __highest_present_section_nr]. > >> And the return value of next_present_section_nr() meets this > >> requirement. This means it is not necessary to check it with > >> __highest_present_section_nr again in for_each_present_section_nr(). > >> > >> Since we pass an unsigned long *section_nr* to > >> for_each_present_section_nr(), we need to cast it to int before > >> comparing. > > > >Why do we want this patch? Is it an improvement? If yes, it is > >performance visible change or does it make the code easier to maintain? > > > > Michal > > I know you concern, maintainance is a very critical part of review. > > >To me at least the later seems dubious to be honest because it adds a > >non-obvious dependency of the terminal condition to the > >next_present_section_nr implementation and that might turn out error > >prone. > > > > While I think the original code is not that clear about the syntax. > > When we look at the next_present_section_nr(section_nr), the return > value falls into two categories: > > -1 : no more present section after section_nr > other: the next present section number after section_nr > > Based on this syntax, the iteration could be simpler to terminate > when the return value is less than 0. This is what the patch tries to > do. > > Maybe I could do more to help the maintainance: > > * add some comment about the return value of next_present_section_nr > * terminate the loop when section_nr == -1 > > Hope this would help a little. Well, not really. Nothing of the above seems to matter to callers of the code. So I do not see this as a general improvement and as such no strong reason to merge it. It is basicly polishing a code without any obvious issues.
On Tue, Dec 11, 2018 at 11:23:13AM +0100, Michal Hocko wrote: >On Tue 11-12-18 10:19:05, Wei Yang wrote: >> On Tue, Dec 11, 2018 at 10:44:41AM +0100, Michal Hocko wrote: >> >On Tue 11-12-18 11:51:28, Wei Yang wrote: >> >> A valid present section number is in [0, __highest_present_section_nr]. >> >> And the return value of next_present_section_nr() meets this >> >> requirement. This means it is not necessary to check it with >> >> __highest_present_section_nr again in for_each_present_section_nr(). >> >> >> >> Since we pass an unsigned long *section_nr* to >> >> for_each_present_section_nr(), we need to cast it to int before >> >> comparing. >> > >> >Why do we want this patch? Is it an improvement? If yes, it is >> >performance visible change or does it make the code easier to maintain? >> > >> >> Michal >> >> I know you concern, maintainance is a very critical part of review. >> >> >To me at least the later seems dubious to be honest because it adds a >> >non-obvious dependency of the terminal condition to the >> >next_present_section_nr implementation and that might turn out error >> >prone. >> > >> >> While I think the original code is not that clear about the syntax. >> >> When we look at the next_present_section_nr(section_nr), the return >> value falls into two categories: >> >> -1 : no more present section after section_nr >> other: the next present section number after section_nr >> >> Based on this syntax, the iteration could be simpler to terminate >> when the return value is less than 0. This is what the patch tries to >> do. >> >> Maybe I could do more to help the maintainance: >> >> * add some comment about the return value of next_present_section_nr >> * terminate the loop when section_nr == -1 >> >> Hope this would help a little. > >Well, not really. Nothing of the above seems to matter to callers of the >code. So I do not see this as a general improvement and as such no >strong reason to merge it. It is basicly polishing a code without any >obvious issues. Er... but I don't see the reason to keep a redundant check in the code. Even this is an internal function, it would be better to make it clean and neat. Would you mind sharing your concern about this polishing? If there is no issue, we would prefer no polishing of the code? >-- >Michal Hocko >SUSE Labs
diff --git a/mm/sparse.c b/mm/sparse.c index a4fdbcb21514..9eaa8f98a3d2 100644 --- a/mm/sparse.c +++ b/mm/sparse.c @@ -197,8 +197,7 @@ static inline int next_present_section_nr(int section_nr) } #define for_each_present_section_nr(start, section_nr) \ for (section_nr = next_present_section_nr(start-1); \ - ((section_nr >= 0) && \ - (section_nr <= __highest_present_section_nr)); \ + (int)section_nr >= 0; \ section_nr = next_present_section_nr(section_nr)) static inline unsigned long first_present_section_nr(void)
A valid present section number is in [0, __highest_present_section_nr]. And the return value of next_present_section_nr() meets this requirement. This means it is not necessary to check it with __highest_present_section_nr again in for_each_present_section_nr(). Since we pass an unsigned long *section_nr* to for_each_present_section_nr(), we need to cast it to int before comparing. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> --- mm/sparse.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)