All of lore.kernel.org
 help / color / mirror / Atom feed
From: Wu Fengguang <fengguang.wu@intel.com>
To: Hugh Dickins <hugh.dickins@tiscali.co.uk>
Cc: Andi Kleen <andi@firstfloor.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>
Subject: Re: [PATCH] hwpoison: fix uninitialized warning
Date: Wed, 16 Sep 2009 09:08:53 +0800	[thread overview]
Message-ID: <20090916010853.GA11865@localhost> (raw)
In-Reply-To: <Pine.LNX.4.64.0909160137270.8639@sister.anvils>

On Wed, Sep 16, 2009 at 08:51:06AM +0800, Hugh Dickins wrote:
> On Wed, 16 Sep 2009, Wu Fengguang wrote:
> > On Wed, Sep 16, 2009 at 05:19:07AM +0800, Hugh Dickins wrote:
> > > Fix mmotm build warning, presumably also in linux-next:
> > > mm/memory.c: In function `do_swap_page':
> > > mm/memory.c:2498: warning: `pte' may be used uninitialized in this function
> > > 
> > > Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
> > > ---
> > > I've only noticed this warning on one machine, the powerpc: certainly it
> > > needs CONFIG_MIGRATION or CONFIG_MEMORY_FAILURE to see it, but I thought
> > > I had one of those set on other machines - just musing in case it's being
> > > masked elsewhere by some other bug...
> 
> > The lines was introduced in this patch:
> > 
> >         entry = pte_to_swp_entry(orig_pte);
> > -       if (is_migration_entry(entry)) {
> > -               migration_entry_wait(mm, pmd, address);
> > +       if (unlikely(non_swap_entry(entry))) {
> > +               if (is_migration_entry(entry)) {
> > +                       migration_entry_wait(mm, pmd, address);
> > +               } else if (is_hwpoison_entry(entry)) {
> > +                       ret = VM_FAULT_HWPOISON;
> > +               } else {
> > +                       print_bad_pte(vma, address, pte, NULL);
> > +                       ret = VM_FAULT_OOM;
> > +               }
> >                 goto out;
> >         }
> > 
> > Given that currently there are only two types of non swap entries:
> > migration/hwpoison, the last 'else' block is in fact dead code..
> 
> Ah, yes, I think it is dead code on x86 (32 and 64), where the
> swp_entry_t is well packed.  But not dead code on ppc64, which has
> 
> #define __swp_type(entry)	(((entry).val >> 1) & 0x3f)
> 
> which is allowing swap types up to 63, when in fact the highest
> we use is 31: that leaves space for 32 more non_swap_entry types.
> 
> So the compiler was absolutely right to complain about the
> uninitialized variable on ppc64, but not on x86.  It's a little
> surprising that ppc64 allows 64 swap types, but nothing wrong.

Ah I know. It seems that gcc is smart enough to remove that dead code
and hence the warning message in x86 :)

Thanks,
Fengguang

WARNING: multiple messages have this Message-ID (diff)
From: Wu Fengguang <fengguang.wu@intel.com>
To: Hugh Dickins <hugh.dickins@tiscali.co.uk>
Cc: Andi Kleen <andi@firstfloor.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>
Subject: Re: [PATCH] hwpoison: fix uninitialized warning
Date: Wed, 16 Sep 2009 09:08:53 +0800	[thread overview]
Message-ID: <20090916010853.GA11865@localhost> (raw)
In-Reply-To: <Pine.LNX.4.64.0909160137270.8639@sister.anvils>

On Wed, Sep 16, 2009 at 08:51:06AM +0800, Hugh Dickins wrote:
> On Wed, 16 Sep 2009, Wu Fengguang wrote:
> > On Wed, Sep 16, 2009 at 05:19:07AM +0800, Hugh Dickins wrote:
> > > Fix mmotm build warning, presumably also in linux-next:
> > > mm/memory.c: In function `do_swap_page':
> > > mm/memory.c:2498: warning: `pte' may be used uninitialized in this function
> > > 
> > > Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
> > > ---
> > > I've only noticed this warning on one machine, the powerpc: certainly it
> > > needs CONFIG_MIGRATION or CONFIG_MEMORY_FAILURE to see it, but I thought
> > > I had one of those set on other machines - just musing in case it's being
> > > masked elsewhere by some other bug...
> 
> > The lines was introduced in this patch:
> > 
> >         entry = pte_to_swp_entry(orig_pte);
> > -       if (is_migration_entry(entry)) {
> > -               migration_entry_wait(mm, pmd, address);
> > +       if (unlikely(non_swap_entry(entry))) {
> > +               if (is_migration_entry(entry)) {
> > +                       migration_entry_wait(mm, pmd, address);
> > +               } else if (is_hwpoison_entry(entry)) {
> > +                       ret = VM_FAULT_HWPOISON;
> > +               } else {
> > +                       print_bad_pte(vma, address, pte, NULL);
> > +                       ret = VM_FAULT_OOM;
> > +               }
> >                 goto out;
> >         }
> > 
> > Given that currently there are only two types of non swap entries:
> > migration/hwpoison, the last 'else' block is in fact dead code..
> 
> Ah, yes, I think it is dead code on x86 (32 and 64), where the
> swp_entry_t is well packed.  But not dead code on ppc64, which has
> 
> #define __swp_type(entry)	(((entry).val >> 1) & 0x3f)
> 
> which is allowing swap types up to 63, when in fact the highest
> we use is 31: that leaves space for 32 more non_swap_entry types.
> 
> So the compiler was absolutely right to complain about the
> uninitialized variable on ppc64, but not on x86.  It's a little
> surprising that ppc64 allows 64 swap types, but nothing wrong.

Ah I know. It seems that gcc is smart enough to remove that dead code
and hence the warning message in x86 :)

Thanks,
Fengguang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

  reply	other threads:[~2009-09-16  1:09 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-09-15 21:19 [PATCH] hwpoison: fix uninitialized warning Hugh Dickins
2009-09-15 21:19 ` Hugh Dickins
2009-09-15 23:59 ` Wu Fengguang
2009-09-15 23:59   ` Wu Fengguang
2009-09-16  0:12 ` Minchan Kim
2009-09-16  0:12   ` Minchan Kim
2009-09-16  0:23 ` Wu Fengguang
2009-09-16  0:23   ` Wu Fengguang
2009-09-16  0:51   ` Hugh Dickins
2009-09-16  0:51     ` Hugh Dickins
2009-09-16  1:08     ` Wu Fengguang [this message]
2009-09-16  1:08       ` Wu Fengguang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20090916010853.GA11865@localhost \
    --to=fengguang.wu@intel.com \
    --cc=akpm@linux-foundation.org \
    --cc=andi@firstfloor.org \
    --cc=hugh.dickins@tiscali.co.uk \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.