From: Yanfei Xu <[email protected]>
There are two 'start_pfn' declared in compact_zone() which have
different meaning. Rename the second one to 'iteration_start_pfn'
to prevent trace_mm_compaction_end() from tracing an undesirable
value.
BTW, remove an useless semicolon.
Acked-by: David Hildenbrand <[email protected]>
Acked-by: Vlastimil Babka <[email protected]>
Signed-off-by: Yanfei Xu <[email protected]>
---
v1->v2:
Rename 'start_pfn' to 'iteration_start_pfn' and change commit messages.
mm/compaction.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/mm/compaction.c b/mm/compaction.c
index 176dcded298e..ccd27c739fd6 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2272,7 +2272,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
while ((ret = compact_finished(cc)) == COMPACT_CONTINUE) {
int err;
- unsigned long start_pfn = cc->migrate_pfn;
+ unsigned long iteration_start_pfn = cc->migrate_pfn;
/*
* Avoid multiple rescans which can happen if a page cannot be
@@ -2284,7 +2284,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
*/
cc->rescan = false;
if (pageblock_start_pfn(last_migrated_pfn) ==
- pageblock_start_pfn(start_pfn)) {
+ pageblock_start_pfn(iteration_start_pfn)) {
cc->rescan = true;
}
@@ -2308,8 +2308,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
goto check_drain;
case ISOLATE_SUCCESS:
update_cached = false;
- last_migrated_pfn = start_pfn;
- ;
+ last_migrated_pfn = iteration_start_pfn;
}
err = migrate_pages(&cc->migratepages, compaction_alloc,
--
2.18.2
> From: Yanfei Xu <[email protected]>
>
> There are two 'start_pfn' declared in compact_zone() which have
> different meaning. Rename the second one to 'iteration_start_pfn'
> to prevent trace_mm_compaction_end() from tracing an undesirable
> value.
>
> BTW, remove an useless semicolon.
>
> Acked-by: David Hildenbrand <[email protected]>
> Acked-by: Vlastimil Babka <[email protected]>
> Signed-off-by: Yanfei Xu <[email protected]>
> ---
> v1->v2:
> Rename 'start_pfn' to 'iteration_start_pfn' and change commit messages.
>
> mm/compaction.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 176dcded298e..ccd27c739fd6 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -2272,7 +2272,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
>
> while ((ret = compact_finished(cc)) == COMPACT_CONTINUE) {
> int err;
> - unsigned long start_pfn = cc->migrate_pfn;
> + unsigned long iteration_start_pfn = cc->migrate_pfn;
>
> /*
> * Avoid multiple rescans which can happen if a page cannot be
> @@ -2284,7 +2284,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
> */
> cc->rescan = false;
> if (pageblock_start_pfn(last_migrated_pfn) ==
> - pageblock_start_pfn(start_pfn)) {
> + pageblock_start_pfn(iteration_start_pfn)) {
> cc->rescan = true;
> }
>
> @@ -2308,8 +2308,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
> goto check_drain;
> case ISOLATE_SUCCESS:
> update_cached = false;
> - last_migrated_pfn = start_pfn;
> - ;
> + last_migrated_pfn = iteration_start_pfn;
> }
>
> err = migrate_pages(&cc->migratepages, compaction_alloc,
Improves readability.
Acked-by: Pankaj Gupta <[email protected]>
On 10/19/20 6:48 PM, Vlastimil Babka wrote:
> On 10/19/20 12:29 PM, Xu, Yanfei wrote:
>>
>>
>> On 10/19/20 5:40 PM, Vlastimil Babka wrote:
>>> On 10/19/20 10:36 AM, [email protected] wrote:
>>>> From: Yanfei Xu <[email protected]>
>>>>
>>>> There are two 'start_pfn' declared in compact_zone() which have
>>>> different meaning. Rename the second one to 'iteration_start_pfn'
>>>> to prevent trace_mm_compaction_end() from tracing an undesirable
>>>> value.
>>>
>>> "to prevent confusion.", because trace_mm_compaction_end() has the
>>> correct value even before the patch - the second start_pfn is out
>>> of scope at that point.
>>>
>>> Thanks
>>>
>> In the while-statement, the second start_pfn is always be reassigned the
>> value of cc->migrate_pfn in every loop, also the cc->migrate_pfn might
>> be changed in the loop. Does trace_mm_compaction_end() really want to
>> trace the new assinged start_pfn?
>
> compact_zone()
> {
> unsigned long start_pfn = cc->zone->zone_start_pfn;
>
> while ((ret = compact_finished(cc)) == COMPACT_CONTINUE) {
> unsigned long start_pfn = cc->migrate_pfn;
> ...
> }
>
> trace_mm_compaction_end(start_pfn, cc->migrate_pfn, ...)
> }
>
> Unless my C knowledge fails me completely, the start_pfn in the while
> loop is a new different local variable that shadows the start_pfn from
> compact_zone() level, but does not modify its value. After while loop
> finishes, start_pfn has still the value assigned at
> compact_zone() beginning and that's what tracepoint sees.
>
You are right! and I got your point. As you said, the second start_pfn
is out of scope at that point.
Will send v3.
Many thanks,
Yanfei
> So renaming the variable in while loop is not a bug fix, but removing
> confusion.
>
>> Without the patch： 566e54e11（mm, compaction: remove last_migrated_pfn
>> from compact_control）, there is only one start_pfn which has a fixed
>> value. The trace_mm_compaction_end() trace it too.
>>
>> Thus, I think the tracepoint might get an undesireble value.:)
>>
>> Thanks,
>> Yanfei
>>
>>>> BTW, remove an useless semicolon.
>>>>
>>>> Acked-by: David Hildenbrand <[email protected]>
>>>> Acked-by: Vlastimil Babka <[email protected]>
>>>> Signed-off-by: Yanfei Xu <[email protected]>
>>>> ---
>>>> v1->v2:
>>>> Rename 'start_pfn' to 'iteration_start_pfn' and change commit messages.
>>>>
>>>> mm/compaction.c | 7 +++----
>>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>>> index 176dcded298e..ccd27c739fd6 100644
>>>> --- a/mm/compaction.c
>>>> +++ b/mm/compaction.c
>>>> @@ -2272,7 +2272,7 @@ compact_zone(struct compact_control *cc,
>>>> struct capture_control *capc)
>>>> while ((ret = compact_finished(cc)) == COMPACT_CONTINUE) {
>>>> int err;
>>>> - unsigned long start_pfn = cc->migrate_pfn;
>>>> + unsigned long iteration_start_pfn = cc->migrate_pfn;
>>>> /*
>>>> * Avoid multiple rescans which can happen if a page
>>>> cannot be
>>>> @@ -2284,7 +2284,7 @@ compact_zone(struct compact_control *cc,
>>>> struct capture_control *capc)
>>>> */
>>>> cc->rescan = false;
>>>> if (pageblock_start_pfn(last_migrated_pfn) ==
>>>> - pageblock_start_pfn(start_pfn)) {
>>>> + pageblock_start_pfn(iteration_start_pfn)) {
>>>> cc->rescan = true;
>>>> }
>>>> @@ -2308,8 +2308,7 @@ compact_zone(struct compact_control *cc,
>>>> struct capture_control *capc)
>>>> goto check_drain;
>>>> case ISOLATE_SUCCESS:
>>>> update_cached = false;
>>>> - last_migrated_pfn = start_pfn;
>>>> - ;
>>>> + last_migrated_pfn = iteration_start_pfn;
>>>> }
>>>> err = migrate_pages(&cc->migratepages, compaction_alloc,
>>>>
>>>
>>>
>>
>
On 10/19/20 10:36 AM, [email protected] wrote:
> From: Yanfei Xu <[email protected]>
>
> There are two 'start_pfn' declared in compact_zone() which have
> different meaning. Rename the second one to 'iteration_start_pfn'
> to prevent trace_mm_compaction_end() from tracing an undesirable
> value.
"to prevent confusion.", because trace_mm_compaction_end() has the
correct value even before the patch - the second start_pfn is out
of scope at that point.
Thanks
> BTW, remove an useless semicolon.
>
> Acked-by: David Hildenbrand <[email protected]>
> Acked-by: Vlastimil Babka <[email protected]>
> Signed-off-by: Yanfei Xu <[email protected]>
> ---
> v1->v2:
> Rename 'start_pfn' to 'iteration_start_pfn' and change commit messages.
>
> mm/compaction.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 176dcded298e..ccd27c739fd6 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -2272,7 +2272,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
>
> while ((ret = compact_finished(cc)) == COMPACT_CONTINUE) {
> int err;
> - unsigned long start_pfn = cc->migrate_pfn;
> + unsigned long iteration_start_pfn = cc->migrate_pfn;
>
> /*
> * Avoid multiple rescans which can happen if a page cannot be
> @@ -2284,7 +2284,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
> */
> cc->rescan = false;
> if (pageblock_start_pfn(last_migrated_pfn) ==
> - pageblock_start_pfn(start_pfn)) {
> + pageblock_start_pfn(iteration_start_pfn)) {
> cc->rescan = true;
> }
>
> @@ -2308,8 +2308,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
> goto check_drain;
> case ISOLATE_SUCCESS:
> update_cached = false;
> - last_migrated_pfn = start_pfn;
> - ;
> + last_migrated_pfn = iteration_start_pfn;
> }
>
> err = migrate_pages(&cc->migratepages, compaction_alloc,
>
On 10/19/20 12:29 PM, Xu, Yanfei wrote:
>
>
> On 10/19/20 5:40 PM, Vlastimil Babka wrote:
>> On 10/19/20 10:36 AM, [email protected] wrote:
>>> From: Yanfei Xu <[email protected]>
>>>
>>> There are two 'start_pfn' declared in compact_zone() which have
>>> different meaning. Rename the second one to 'iteration_start_pfn'
>>> to prevent trace_mm_compaction_end() from tracing an undesirable
>>> value.
>>
>> "to prevent confusion.", because trace_mm_compaction_end() has the
>> correct value even before the patch - the second start_pfn is out
>> of scope at that point.
>>
>> Thanks
>>
> In the while-statement, the second start_pfn is always be reassigned the
> value of cc->migrate_pfn in every loop, also the cc->migrate_pfn might
> be changed in the loop. Does trace_mm_compaction_end() really want to
> trace the new assinged start_pfn?
compact_zone()
{
unsigned long start_pfn = cc->zone->zone_start_pfn;
while ((ret = compact_finished(cc)) == COMPACT_CONTINUE) {
unsigned long start_pfn = cc->migrate_pfn;
...
}
trace_mm_compaction_end(start_pfn, cc->migrate_pfn, ...)
}
Unless my C knowledge fails me completely, the start_pfn in the while loop is a
new different local variable that shadows the start_pfn from compact_zone()
level, but does not modify its value. After while loop finishes, start_pfn has
still the value assigned at
compact_zone() beginning and that's what tracepoint sees.
So renaming the variable in while loop is not a bug fix, but removing confusion.
> Without the patch： 566e54e11（mm, compaction: remove last_migrated_pfn
> from compact_control）, there is only one start_pfn which has a fixed
> value. The trace_mm_compaction_end() trace it too.
>
> Thus, I think the tracepoint might get an undesireble value.:)
>
> Thanks,
> Yanfei
>
>>> BTW, remove an useless semicolon.
>>>
>>> Acked-by: David Hildenbrand <[email protected]>
>>> Acked-by: Vlastimil Babka <[email protected]>
>>> Signed-off-by: Yanfei Xu <[email protected]>
>>> ---
>>> v1->v2:
>>> Rename 'start_pfn' to 'iteration_start_pfn' and change commit messages.
>>>
>>> mm/compaction.c | 7 +++----
>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>> index 176dcded298e..ccd27c739fd6 100644
>>> --- a/mm/compaction.c
>>> +++ b/mm/compaction.c
>>> @@ -2272,7 +2272,7 @@ compact_zone(struct compact_control *cc, struct
>>> capture_control *capc)
>>> while ((ret = compact_finished(cc)) == COMPACT_CONTINUE) {
>>> int err;
>>> - unsigned long start_pfn = cc->migrate_pfn;
>>> + unsigned long iteration_start_pfn = cc->migrate_pfn;
>>> /*
>>> * Avoid multiple rescans which can happen if a page cannot be
>>> @@ -2284,7 +2284,7 @@ compact_zone(struct compact_control *cc, struct
>>> capture_control *capc)
>>> */
>>> cc->rescan = false;
>>> if (pageblock_start_pfn(last_migrated_pfn) ==
>>> - pageblock_start_pfn(start_pfn)) {
>>> + pageblock_start_pfn(iteration_start_pfn)) {
>>> cc->rescan = true;
>>> }
>>> @@ -2308,8 +2308,7 @@ compact_zone(struct compact_control *cc, struct
>>> capture_control *capc)
>>> goto check_drain;
>>> case ISOLATE_SUCCESS:
>>> update_cached = false;
>>> - last_migrated_pfn = start_pfn;
>>> - ;
>>> + last_migrated_pfn = iteration_start_pfn;
>>> }
>>> err = migrate_pages(&cc->migratepages, compaction_alloc,
>>>
>>
>>
>
On 10/19/20 5:40 PM, Vlastimil Babka wrote:
> On 10/19/20 10:36 AM, [email protected] wrote:
>> From: Yanfei Xu <[email protected]>
>>
>> There are two 'start_pfn' declared in compact_zone() which have
>> different meaning. Rename the second one to 'iteration_start_pfn'
>> to prevent trace_mm_compaction_end() from tracing an undesirable
>> value.
>
> "to prevent confusion.", because trace_mm_compaction_end() has the
> correct value even before the patch - the second start_pfn is out
> of scope at that point.
>
> Thanks
>
In the while-statement, the second start_pfn is always be reassigned the
value of cc->migrate_pfn in every loop, also the cc->migrate_pfn might
be changed in the loop. Does trace_mm_compaction_end() really want to
trace the new assinged start_pfn?
Without the patch： 566e54e11（mm, compaction: remove last_migrated_pfn
from compact_control）, there is only one start_pfn which has a fixed
value. The trace_mm_compaction_end() trace it too.
Thus, I think the tracepoint might get an undesireble value.:)
Thanks,
Yanfei
>> BTW, remove an useless semicolon.
>>
>> Acked-by: David Hildenbrand <[email protected]>
>> Acked-by: Vlastimil Babka <[email protected]>
>> Signed-off-by: Yanfei Xu <[email protected]>
>> ---
>> v1->v2:
>> Rename 'start_pfn' to 'iteration_start_pfn' and change commit messages.
>>
>> mm/compaction.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index 176dcded298e..ccd27c739fd6 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -2272,7 +2272,7 @@ compact_zone(struct compact_control *cc, struct
>> capture_control *capc)
>> while ((ret = compact_finished(cc)) == COMPACT_CONTINUE) {
>> int err;
>> - unsigned long start_pfn = cc->migrate_pfn;
>> + unsigned long iteration_start_pfn = cc->migrate_pfn;
>> /*
>> * Avoid multiple rescans which can happen if a page cannot be
>> @@ -2284,7 +2284,7 @@ compact_zone(struct compact_control *cc, struct
>> capture_control *capc)
>> */
>> cc->rescan = false;
>> if (pageblock_start_pfn(last_migrated_pfn) ==
>> - pageblock_start_pfn(start_pfn)) {
>> + pageblock_start_pfn(iteration_start_pfn)) {
>> cc->rescan = true;
>> }
>> @@ -2308,8 +2308,7 @@ compact_zone(struct compact_control *cc, struct
>> capture_control *capc)
>> goto check_drain;
>> case ISOLATE_SUCCESS:
>> update_cached = false;
>> - last_migrated_pfn = start_pfn;
>> - ;
>> + last_migrated_pfn = iteration_start_pfn;
>> }
>> err = migrate_pages(&cc->migratepages, compaction_alloc,
>>
>
>