[PATCH] Hibernation: Fix mark_nosave_pages() - Kernel

This is a discussion on [PATCH] Hibernation: Fix mark_nosave_pages() - Kernel ; Hi Len, The following patch fixes http://bugzilla.kernel.org/show_bug.cgi?id=9966 . Although the problem has been there for quite some time (since before 2.6.22, apparently), it would be good to have in 2.6.25, because it may fix booting problems on the affected systems. ...

+ Reply to Thread
Results 1 to 3 of 3

Thread: [PATCH] Hibernation: Fix mark_nosave_pages()

  1. [PATCH] Hibernation: Fix mark_nosave_pages()

    Hi Len,

    The following patch fixes http://bugzilla.kernel.org/show_bug.cgi?id=9966 .

    Although the problem has been there for quite some time (since before 2.6.22,
    apparently), it would be good to have in 2.6.25, because it may fix booting
    problems on the affected systems.

    Please note that it doesn't modify the code behavior on the systems which are
    not affected by bug #9966 .

    Thanks,
    Rafael


    ---
    From: Rafael J. Wysocki

    There is a problem in the hibernation code that triggers on some NUMA
    systems on which pfn_valid() returns 'true' for some PFNs that don't
    belong to any zone. Namely, there is a BUG_ON() in
    memory_bm_find_bit() that triggers for PFNs not belonging to any
    zone and passing the pfn_valid() test. On the affected systems it
    triggers when we mark PFNs reported by the platform as not saveable,
    because the PFNs in question belong to a region mapped directly using
    iorepam() (i.e. the ACPI data area) and they pass the pfn_valid()
    test.

    Modify memory_bm_find_bit() so that it returns an error if given PFN
    doesn't belong to any zone instead of crashing the kernel and ignore
    the result returned by it in mark_nosave_pages(), while marking the
    "nosave" memory regions.

    This doesn't affect the hibernation functionality, as we won't touch
    the PFNs in question anyway.

    Signed-off-by: Rafael J. Wysocki
    ---
    kernel/power/snapshot.c | 41 ++++++++++++++++++++++++++++++++++-------
    1 file changed, 34 insertions(+), 7 deletions(-)

    Index: linux-2.6/kernel/power/snapshot.c
    ================================================== =================
    --- linux-2.6.orig/kernel/power/snapshot.c
    +++ linux-2.6/kernel/power/snapshot.c
    @@ -447,7 +447,7 @@ static void memory_bm_free(struct memory
    * of @bm->cur_zone_bm are updated.
    */

    -static void memory_bm_find_bit(struct memory_bitmap *bm, unsigned long pfn,
    +static int memory_bm_find_bit(struct memory_bitmap *bm, unsigned long pfn,
    void **addr, unsigned int *bit_nr)
    {
    struct zone_bitmap *zone_bm;
    @@ -461,7 +461,8 @@ static void memory_bm_find_bit(struct me
    while (pfn < zone_bm->start_pfn || pfn >= zone_bm->end_pfn) {
    zone_bm = zone_bm->next;

    - BUG_ON(!zone_bm);
    + if (!zone_bm)
    + return -EFAULT;
    }
    bm->cur.zone_bm = zone_bm;
    }
    @@ -479,23 +480,40 @@ static void memory_bm_find_bit(struct me
    pfn -= bb->start_pfn;
    *bit_nr = pfn % BM_BITS_PER_CHUNK;
    *addr = bb->data + pfn / BM_BITS_PER_CHUNK;
    + return 0;
    }

    static void memory_bm_set_bit(struct memory_bitmap *bm, unsigned long pfn)
    {
    void *addr;
    unsigned int bit;
    + int error;

    - memory_bm_find_bit(bm, pfn, &addr, &bit);
    + error = memory_bm_find_bit(bm, pfn, &addr, &bit);
    + BUG_ON(error);
    set_bit(bit, addr);
    }

    +static int mem_bm_set_bit_check(struct memory_bitmap *bm, unsigned long pfn)
    +{
    + void *addr;
    + unsigned int bit;
    + int error;
    +
    + error = memory_bm_find_bit(bm, pfn, &addr, &bit);
    + if (!error)
    + set_bit(bit, addr);
    + return error;
    +}
    +
    static void memory_bm_clear_bit(struct memory_bitmap *bm, unsigned long pfn)
    {
    void *addr;
    unsigned int bit;
    + int error;

    - memory_bm_find_bit(bm, pfn, &addr, &bit);
    + error = memory_bm_find_bit(bm, pfn, &addr, &bit);
    + BUG_ON(error);
    clear_bit(bit, addr);
    }

    @@ -503,8 +521,10 @@ static int memory_bm_test_bit(struct mem
    {
    void *addr;
    unsigned int bit;
    + int error;

    - memory_bm_find_bit(bm, pfn, &addr, &bit);
    + error = memory_bm_find_bit(bm, pfn, &addr, &bit);
    + BUG_ON(error);
    return test_bit(bit, addr);
    }

    @@ -709,8 +729,15 @@ static void mark_nosave_pages(struct mem
    region->end_pfn << PAGE_SHIFT);

    for (pfn = region->start_pfn; pfn < region->end_pfn; pfn++)
    - if (pfn_valid(pfn))
    - memory_bm_set_bit(bm, pfn);
    + if (pfn_valid(pfn)) {
    + /*
    + * It is safe to ignore the result of
    + * mem_bm_set_bit_check() here, since we won't
    + * touch the PFNs for which the error is
    + * returned anyway.
    + */
    + mem_bm_set_bit_check(bm, pfn);
    + }
    }
    }

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH] Hibernation: Fix mark_nosave_pages()

    applied to acpi test tree.

    thanks,
    -len

    On Tuesday 11 March 2008, Rafael J. Wysocki wrote:
    > Hi Len,
    >
    > The following patch fixes http://bugzilla.kernel.org/show_bug.cgi?id=9966 .
    >
    > Although the problem has been there for quite some time (since before 2.6.22,
    > apparently), it would be good to have in 2.6.25, because it may fix booting
    > problems on the affected systems.
    >
    > Please note that it doesn't modify the code behavior on the systems which are
    > not affected by bug #9966 .
    >
    > Thanks,
    > Rafael
    >
    >
    > ---
    > From: Rafael J. Wysocki
    >
    > There is a problem in the hibernation code that triggers on some NUMA
    > systems on which pfn_valid() returns 'true' for some PFNs that don't
    > belong to any zone. Namely, there is a BUG_ON() in
    > memory_bm_find_bit() that triggers for PFNs not belonging to any
    > zone and passing the pfn_valid() test. On the affected systems it
    > triggers when we mark PFNs reported by the platform as not saveable,
    > because the PFNs in question belong to a region mapped directly using
    > iorepam() (i.e. the ACPI data area) and they pass the pfn_valid()
    > test.
    >
    > Modify memory_bm_find_bit() so that it returns an error if given PFN
    > doesn't belong to any zone instead of crashing the kernel and ignore
    > the result returned by it in mark_nosave_pages(), while marking the
    > "nosave" memory regions.
    >
    > This doesn't affect the hibernation functionality, as we won't touch
    > the PFNs in question anyway.
    >
    > Signed-off-by: Rafael J. Wysocki
    > ---
    > kernel/power/snapshot.c | 41 ++++++++++++++++++++++++++++++++++-------
    > 1 file changed, 34 insertions(+), 7 deletions(-)
    >
    > Index: linux-2.6/kernel/power/snapshot.c
    > ================================================== =================
    > --- linux-2.6.orig/kernel/power/snapshot.c
    > +++ linux-2.6/kernel/power/snapshot.c
    > @@ -447,7 +447,7 @@ static void memory_bm_free(struct memory
    > * of @bm->cur_zone_bm are updated.
    > */
    >
    > -static void memory_bm_find_bit(struct memory_bitmap *bm, unsigned long pfn,
    > +static int memory_bm_find_bit(struct memory_bitmap *bm, unsigned long pfn,
    > void **addr, unsigned int *bit_nr)
    > {
    > struct zone_bitmap *zone_bm;
    > @@ -461,7 +461,8 @@ static void memory_bm_find_bit(struct me
    > while (pfn < zone_bm->start_pfn || pfn >= zone_bm->end_pfn) {
    > zone_bm = zone_bm->next;
    >
    > - BUG_ON(!zone_bm);
    > + if (!zone_bm)
    > + return -EFAULT;
    > }
    > bm->cur.zone_bm = zone_bm;
    > }
    > @@ -479,23 +480,40 @@ static void memory_bm_find_bit(struct me
    > pfn -= bb->start_pfn;
    > *bit_nr = pfn % BM_BITS_PER_CHUNK;
    > *addr = bb->data + pfn / BM_BITS_PER_CHUNK;
    > + return 0;
    > }
    >
    > static void memory_bm_set_bit(struct memory_bitmap *bm, unsigned long pfn)
    > {
    > void *addr;
    > unsigned int bit;
    > + int error;
    >
    > - memory_bm_find_bit(bm, pfn, &addr, &bit);
    > + error = memory_bm_find_bit(bm, pfn, &addr, &bit);
    > + BUG_ON(error);
    > set_bit(bit, addr);
    > }
    >
    > +static int mem_bm_set_bit_check(struct memory_bitmap *bm, unsigned long pfn)
    > +{
    > + void *addr;
    > + unsigned int bit;
    > + int error;
    > +
    > + error = memory_bm_find_bit(bm, pfn, &addr, &bit);
    > + if (!error)
    > + set_bit(bit, addr);
    > + return error;
    > +}
    > +
    > static void memory_bm_clear_bit(struct memory_bitmap *bm, unsigned long pfn)
    > {
    > void *addr;
    > unsigned int bit;
    > + int error;
    >
    > - memory_bm_find_bit(bm, pfn, &addr, &bit);
    > + error = memory_bm_find_bit(bm, pfn, &addr, &bit);
    > + BUG_ON(error);
    > clear_bit(bit, addr);
    > }
    >
    > @@ -503,8 +521,10 @@ static int memory_bm_test_bit(struct mem
    > {
    > void *addr;
    > unsigned int bit;
    > + int error;
    >
    > - memory_bm_find_bit(bm, pfn, &addr, &bit);
    > + error = memory_bm_find_bit(bm, pfn, &addr, &bit);
    > + BUG_ON(error);
    > return test_bit(bit, addr);
    > }
    >
    > @@ -709,8 +729,15 @@ static void mark_nosave_pages(struct mem
    > region->end_pfn << PAGE_SHIFT);
    >
    > for (pfn = region->start_pfn; pfn < region->end_pfn; pfn++)
    > - if (pfn_valid(pfn))
    > - memory_bm_set_bit(bm, pfn);
    > + if (pfn_valid(pfn)) {
    > + /*
    > + * It is safe to ignore the result of
    > + * mem_bm_set_bit_check() here, since we won't
    > + * touch the PFNs for which the error is
    > + * returned anyway.
    > + */
    > + mem_bm_set_bit_check(bm, pfn);
    > + }
    > }
    > }
    >
    > --
    > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    > the body of a message to majordomo@vger.kernel.org
    > More majordomo info at http://vger.kernel.org/majordomo-info.html
    > Please read the FAQ at http://www.tux.org/lkml/
    >



    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH] Hibernation: Fix mark_nosave_pages()

    On Wed 2008-03-12 00:34:57, Rafael J. Wysocki wrote:
    > Hi Len,
    >
    > The following patch fixes http://bugzilla.kernel.org/show_bug.cgi?id=9966 .
    >
    > Although the problem has been there for quite some time (since before 2.6.22,
    > apparently), it would be good to have in 2.6.25, because it may fix booting
    > problems on the affected systems.
    >
    > Please note that it doesn't modify the code behavior on the systems which are
    > not affected by bug #9966 .
    >
    > Thanks,
    > Rafael
    >
    >
    > ---
    > From: Rafael J. Wysocki
    >
    > There is a problem in the hibernation code that triggers on some NUMA
    > systems on which pfn_valid() returns 'true' for some PFNs that don't
    > belong to any zone. Namely, there is a BUG_ON() in
    > memory_bm_find_bit() that triggers for PFNs not belonging to any
    > zone and passing the pfn_valid() test. On the affected systems it
    > triggers when we mark PFNs reported by the platform as not saveable,
    > because the PFNs in question belong to a region mapped directly using
    > iorepam() (i.e. the ACPI data area) and they pass the pfn_valid()
    > test.
    >
    > Modify memory_bm_find_bit() so that it returns an error if given PFN
    > doesn't belong to any zone instead of crashing the kernel and ignore
    > the result returned by it in mark_nosave_pages(), while marking the
    > "nosave" memory regions.
    >
    > This doesn't affect the hibernation functionality, as we won't touch
    > the PFNs in question anyway.
    >
    > Signed-off-by: Rafael J. Wysocki


    ACK. I hate how it adds more code to swsusp, but I could not find a
    simple way to avoid that.
    Pavel
    --
    (english) http://www.livejournal.com/~pavelmachek
    (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pav...rses/blog.html
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread