[176/178] mm/page_alloc: redundant definition variables of pfn in for loop

Commit Message

Andrew Morton April 30, 2021, 6:02 a.m. UTC

From: huxiang <[e-mail]>
Subject: mm/page_alloc: redundant definition variables of pfn in for loop

This variable pfn is defined repeatedly, so it can be deleted.

Link: https://lkml.kernel.org/r/[e-mail]
Signed-off-by: huxiang <[e-mail]>
Signed-off-by: Andrew Morton <[e-mail]>
---

 mm/page_alloc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Linus Torvalds April 30, 2021, 5:10 p.m. UTC | #1

On Thu, Apr 29, 2021 at 11:02 PM Andrew Morton
<[e-mail]> wrote:
>
> This variable pfn is defined repeatedly, so it can be deleted.

I'd actually much prefer this patch to be done exactly the other way:
avoid the variable name shadowing not by removing the second
declaration, but by actually making *both* declarations local to the
loops.

The lifetime of those 'pfn' variables really is just the inner body of
the loop, not the whole function.

Now, a good compiler will notice that the uses are entirely disjoint,
and not care about how the programmer used the same variable for two
different cases, but it's good practice to just minimize the scope of
a variable.

So I'm dropping this, but I would apply a patch that did something
like this instead

   void free_unref_page_list(struct list_head *list)
   {
        struct page *page, *next;
  -     unsigned long flags, pfn;
  +     unsigned long flags;
        int batch_count = 0;

        /* Prepare pages for freeing */
        list_for_each_entry_safe(page, next, list, lru) {
  -             pfn = page_to_pfn(page);
  +             unsigned long pfn = page_to_pfn(page);
                if (!free_unref_page_prepare(page, pfn))
                        list_del(&page->lru);
                set_page_private(page, pfn);

(UNTESTED, and intentionally whitespace-damaged, you get the idea).

        Linus

Patch

--- a/mm/page_alloc.c~mm-page_alloc-redundant-definition-variables-of-pfn-in-for-loop
+++ a/mm/page_alloc.c
@@ -3322,7 +3322,7 @@  void free_unref_page_list(struct list_he
 
 	local_irq_save(flags);
 	list_for_each_entry_safe(page, next, list, lru) {
-		unsigned long pfn = page_private(page);
+		pfn = page_private(page);
 
 		set_page_private(page, 0);
 		trace_mm_page_free_batched(page);