| Message ID | 20201014072349.34494-1-yanfei.xu@windriver.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/compaction: Remove some useless code in compact_zone() | expand |
On 14.10.20 09:23, yanfei.xu@windriver.com wrote: > From: Yanfei Xu <yanfei.xu@windriver.com> > > start_pfn has been declared at the begin of compact_zone(), it's > no need to declare it again. And remove an useless semicolon. > > Signed-off-by: Yanfei Xu <yanfei.xu@windriver.com> > --- > mm/compaction.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/mm/compaction.c b/mm/compaction.c > index 176dcded298e..5e69c1f94d96 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; > + start_pfn = cc->migrate_pfn; There is a user in trace_mm_compaction_end(start_pfn, cc->migrate_pfn, cc->free_pfn, end_pfn, sync, ret); we would now trace a different value, no? > > /* > * Avoid multiple rescans which can happen if a page cannot be > @@ -2309,7 +2309,6 @@ compact_zone(struct compact_control *cc, struct capture_control *capc) > case ISOLATE_SUCCESS: > update_cached = false; > last_migrated_pfn = start_pfn; > - ; Huh, how does something like that happen :)
On 10/14/20 2:28 PM, David Hildenbrand wrote: > On 14.10.20 09:23, yanfei.xu@windriver.com wrote: >> From: Yanfei Xu <yanfei.xu@windriver.com> >> >> start_pfn has been declared at the begin of compact_zone(), it's >> no need to declare it again. And remove an useless semicolon. >> >> Signed-off-by: Yanfei Xu <yanfei.xu@windriver.com> >> --- >> mm/compaction.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/mm/compaction.c b/mm/compaction.c >> index 176dcded298e..5e69c1f94d96 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; >> + start_pfn = cc->migrate_pfn; > > There is a user in > > trace_mm_compaction_end(start_pfn, cc->migrate_pfn, cc->free_pfn, > end_pfn, sync, ret); > > we would now trace a different value, no? Agreed. We should rather rename the while-local one to avoid confusion. Something like "iteration_start_pfn" ? >> >> /* >> * Avoid multiple rescans which can happen if a page cannot be >> @@ -2309,7 +2309,6 @@ compact_zone(struct compact_control *cc, struct capture_control *capc) >> case ISOLATE_SUCCESS: >> update_cached = false; >> last_migrated_pfn = start_pfn; >> - ; > > Huh, how does something like that happen :) Looks like "case ISOLATE_SUCCESS:" used to be an empty implementation, then statements got added, but semicolon not removed.
On 10/16/20 11:05 PM, Vlastimil Babka wrote: > On 10/14/20 2:28 PM, David Hildenbrand wrote: >> On 14.10.20 09:23, yanfei.xu@windriver.com wrote: >>> From: Yanfei Xu <yanfei.xu@windriver.com> >>> >>> start_pfn has been declared at the begin of compact_zone(), it's >>> no need to declare it again. And remove an useless semicolon. >>> >>> Signed-off-by: Yanfei Xu <yanfei.xu@windriver.com> >>> --- >>> mm/compaction.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/mm/compaction.c b/mm/compaction.c >>> index 176dcded298e..5e69c1f94d96 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; >>> + start_pfn = cc->migrate_pfn; >> >> There is a user in >> >> trace_mm_compaction_end(start_pfn, cc->migrate_pfn, cc->free_pfn, >> end_pfn, sync, ret); >> >> we would now trace a different value, no? > > Agreed. We should rather rename the while-local one to avoid confusion. > Something like "iteration_start_pfn" ? > Thank you, David and Vlastimil, for pointing out the impact to the tracepoint. I think "iteration_start_pfn" is appropriate and will send V2. >>> >>> /* >>> * Avoid multiple rescans which can happen if a page cannot be >>> @@ -2309,7 +2309,6 @@ compact_zone(struct compact_control *cc, struct >>> capture_control *capc) >>> case ISOLATE_SUCCESS: >>> update_cached = false; >>> last_migrated_pfn = start_pfn; >>> - ; >> >> Huh, how does something like that happen :) > > Looks like "case ISOLATE_SUCCESS:" used to be an empty implementation, > then statements got added, but semicolon not removed. Yup, this case used to be an empty.
diff --git a/mm/compaction.c b/mm/compaction.c index 176dcded298e..5e69c1f94d96 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; + start_pfn = cc->migrate_pfn; /* * Avoid multiple rescans which can happen if a page cannot be @@ -2309,7 +2309,6 @@ compact_zone(struct compact_control *cc, struct capture_control *capc) case ISOLATE_SUCCESS: update_cached = false; last_migrated_pfn = start_pfn; - ; } err = migrate_pages(&cc->migratepages, compaction_alloc,