* [PATCH] mm, sparse: remove check with __highest_present_section_nr in for_each_present_section_nr()
@ 2018-12-11 3:51 Wei Yang
2018-12-11 5:09 ` Wei Yang
2018-12-11 9:44 ` Michal Hocko
0 siblings, 2 replies; 6+ messages in thread
From: Wei Yang @ 2018-12-11 3:51 UTC (permalink / raw)
To: linux-mm; +Cc: akpm, mhocko, osalvador, Wei Yang
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(-)
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm, sparse: remove check with __highest_present_section_nr in for_each_present_section_nr()
2018-12-11 3:51 [PATCH] mm, sparse: remove check with __highest_present_section_nr in for_each_present_section_nr() Wei Yang
@ 2018-12-11 5:09 ` Wei Yang
2018-12-11 9:44 ` Michal Hocko
1 sibling, 0 replies; 6+ messages in thread
From: Wei Yang @ 2018-12-11 5:09 UTC (permalink / raw)
To: Wei Yang; +Cc: linux-mm, akpm, mhocko, osalvador
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
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mm, sparse: remove check with __highest_present_section_nr in for_each_present_section_nr()
2018-12-11 3:51 [PATCH] mm, sparse: remove check with __highest_present_section_nr in for_each_present_section_nr() Wei Yang
2018-12-11 5:09 ` Wei Yang
@ 2018-12-11 9:44 ` Michal Hocko
2018-12-11 10:19 ` Wei Yang
1 sibling, 1 reply; 6+ messages in thread
From: Michal Hocko @ 2018-12-11 9:44 UTC (permalink / raw)
To: Wei Yang; +Cc: linux-mm, akpm, osalvador
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
>
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm, sparse: remove check with __highest_present_section_nr in for_each_present_section_nr()
2018-12-11 9:44 ` Michal Hocko
@ 2018-12-11 10:19 ` Wei Yang
2018-12-11 10:23 ` Michal Hocko
0 siblings, 1 reply; 6+ messages in thread
From: Wei Yang @ 2018-12-11 10:19 UTC (permalink / raw)
To: Michal Hocko; +Cc: Wei Yang, linux-mm, akpm, osalvador
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.
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm, sparse: remove check with __highest_present_section_nr in for_each_present_section_nr()
2018-12-11 10:19 ` Wei Yang
@ 2018-12-11 10:23 ` Michal Hocko
2018-12-11 14:41 ` Wei Yang
0 siblings, 1 reply; 6+ messages in thread
From: Michal Hocko @ 2018-12-11 10:23 UTC (permalink / raw)
To: Wei Yang; +Cc: linux-mm, akpm, osalvador
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.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm, sparse: remove check with __highest_present_section_nr in for_each_present_section_nr()
2018-12-11 10:23 ` Michal Hocko
@ 2018-12-11 14:41 ` Wei Yang
0 siblings, 0 replies; 6+ messages in thread
From: Wei Yang @ 2018-12-11 14:41 UTC (permalink / raw)
To: Michal Hocko; +Cc: Wei Yang, linux-mm, akpm, osalvador
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
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-12-11 14:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-11 3:51 [PATCH] mm, sparse: remove check with __highest_present_section_nr in for_each_present_section_nr() Wei Yang
2018-12-11 5:09 ` Wei Yang
2018-12-11 9:44 ` Michal Hocko
2018-12-11 10:19 ` Wei Yang
2018-12-11 10:23 ` Michal Hocko
2018-12-11 14:41 ` Wei Yang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox