2.6.31-rc1-git3: Reported regressions 2.6.29 -> 2.6.30

[Bartlomiej Zolnierkiewicz]

On Friday 03 July 2009 05:58:25 Wu Zhangjin wrote:

> On Thu, 2009-07-02 at 18:13 +0200, Bartlomiej Zolnierkiewicz wrote:
> > On Thursday 02 July 2009 03:46:43 Wu Zhangjin wrote:
> > > On Wed, 2009-07-01 at 18:29 +0200, Bartlomiej Zolnierkiewicz wrote:
> > > > On Wednesday 01 July 2009 18:21:25 Bartlomiej Zolnierkiewicz wrote:
> > > > > On Wednesday 01 July 2009 16:47:41 Wu Zhangjin wrote:
> > > > > > On Wed, 2009-07-01 at 22:31 +0800, Jeff Chua wrote:
> > > > > > > On Tue, Jun 30, 2009 at 12:21 AM, Jeff Chua<jeff.chua.linux@...> wrote:
> > > > > > >
> > > > > > > > I just tried, and it "seems" to work. Will try a few more cycles.
> > > > > > >
> > > > > > > STD/STR survived quite a few cycles now. Patch seems to be doing the
> > > > > > > right thing.
> > > > > > >
> > > > > > > On Mon, Jun 29, 2009 at 11:51 PM, Etienne
> > > > > > > Basset<etienne.basset@...> wrote:
> > > > > > >
> > > > > > > > To have STR/resume work with current git, I have to :
> > > > > > >
> > > > > > > > 1) apply Bart's patch
> > > > > > >
> > > > > > > This is not yet in Linus's tree. And much needed to really fix the problem.
> > > > > > >
> > > > > > > > 2) revert this commit : a1317f714af7aed60ddc182d0122477cbe36ee9b
> > > > > > >
> > > > > >
> > > > > > Yes, This commit must be reverted, otherwise, STD/Hibernation will not
> > > > > > work either. I have tested it on two different loongson-based machines:
> > > > > > fuloong2e box and yeeloong2f netbook.(loongson is mips compatiable)
> > > > >
> > > > > Since it seems like Dave is taking his sweet time with doing the revert
> > > > > I stared at the code a bit more and I think that I finally found the bug
> > > > > (thanks to your debugging work for giving me the right hint!).
> > > > >
> > > > > The patch needs to take into the account a new code introduced by the recent
> > > > > block layer changes (commit 8f6205cd572fece673da0255d74843680f67f879):
> > > > >
> > > > > @@ -555,8 +560,11 @@ repeat:
> > > > >                 startstop = start_request(drive, rq);
> > > > >                 spin_lock_irq(&hwif->lock);
> > > > >  
> > > > > -               if (startstop == ide_stopped)
> > > > > +               if (startstop == ide_stopped) {
> > > > > +                       rq = hwif->rq;
> > > > > +                       hwif->rq = NULL;
> > > > >                         goto repeat;
> > > > > +               }
> > > > >         } else
> > > > >                 goto plug_device;
> > > > >  out:
> > > > >
> > > > > and not zero hwif->rq if the device is blocked.
> > > > >
> > > > > Could you try the attached patch and see if it fixes the issue?
> > > >
> > > > Here is the more complete version, also taking into the account changes
> > > > in ide_intr() and ide_timer_expiry():
> > > >
> > >
> > > Sorry, I can not apply this patch directly, which original version did
> > > you use? I used the one in the master branch of linux-mips development
> > > git repository.
> > >
> > > commit 5a4f13fad1ab5bd08dea78fc55321e429d83cddf
> > > Merge: ec9c45d e18ed14
> > > Author: Linus Torvalds <torvalds@...>
> > > Date:   Mon Jun 29 20:07:43 2009 -0700
> > >
> > >     Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide-2.6
> > >    
> > >     * git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide-2.6:
> > >       ide: memory overrun in ide_get_identity_ioctl() on big endian
> > > machines using ioctl HDIO_OBSOLETE_IDENTITY
> > >       ide: fix resume for CONFIG_BLK_DEV_IDEACPI=y
> > >       ide-cd: handle fragmented packet commands gracefully
> > >       ide: always kill the whole request on error
> > >       ide: fix ide_kill_rq() for special ide-{floppy,tape} driver
> > > requests
> > >
> > > it this too old? should i merge another git repository?
> >
> > Weird, I used linux-next but Linus' tree should also be fine
> > (as it matches linux-next w.r.t. ide currently).
>
> I just cloned the linux-next git repo, and tested your patch with
> STD/Hibernation, unfortunately, it also not work :-(
>
> here is the Call Trace:
>
> blk_delete_timer+0x0/0x20
> blk_requeue_request+0x24/0xd0
> ide_requeue_and_plug+0x38/0xb0
> ide_intr+0x120/0x300             --->  ide_intr....
> handle_IRQ_event+0x94/0x230
> handle_level_irq+0x7c/0x120
> mach_irq_dispatch+0xc8/0x158
> ret_from_irq+0x0/0x4
> cpu_idle+0x30/0x60
> start_kernel+0x330/0x34c
>
> If _NOT_ apply your patch and comment this part, it works:

OK, I see another gotcha added by recent changes, we need to explicitly
initialize rq_in_flight variables now.  Revised patch below..

From: Bartlomiej Zolnierkiewicz <bzolnier@...>
Subject: [PATCH] ide: make resume work again (for real)

It turns out that commit a1317f714af7aed60ddc182d0122477cbe36ee9b
("ide: improve handling of Power Management requests") needs to take
into the account a new code added by the recent block layer changes
in commit 8f6205cd572fece673da0255d74843680f67f879 ("ide: dequeue
in-flight request") and prevent clearing of hwif->rq if the device
is blocked.

Thanks to Etienne, Wu and Jeff for help in fixing the issue.

Reported-and-tested-by: Jeff Chua <jeff.chua.linux@...>
Reported-and-tested-by: Etienne Basset <etienne.basset@...>
Reported-by: Wu Zhangjin <wuzhangjin@...>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@...>
---
Added patch description, no other changes.

 drivers/ide/ide-io.c |   19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

Index: b/drivers/ide/ide-io.c
===================================================================
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -532,7 +532,8 @@ repeat:
 
  if (startstop == ide_stopped) {
  rq = hwif->rq;
- hwif->rq = NULL;
+ if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0)
+ hwif->rq = NULL;
  goto repeat;
  }
  } else
@@ -616,7 +617,7 @@ void ide_timer_expiry (unsigned long dat
  unsigned long flags;
  int wait = -1;
  int plug_device = 0;
- struct request *uninitialized_var(rq_in_flight);
+ struct request *rq_in_flight = NULL;
 
  spin_lock_irqsave(&hwif->lock, flags);
 
@@ -679,8 +680,10 @@ void ide_timer_expiry (unsigned long dat
  spin_lock_irq(&hwif->lock);
  enable_irq(hwif->irq);
  if (startstop == ide_stopped && hwif->polling == 0) {
- rq_in_flight = hwif->rq;
- hwif->rq = NULL;
+ if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
+ rq_in_flight = hwif->rq;
+ hwif->rq = NULL;
+ }
  ide_unlock_port(hwif);
  plug_device = 1;
  }
@@ -775,7 +778,7 @@ irqreturn_t ide_intr (int irq, void *dev
  ide_startstop_t startstop;
  irqreturn_t irq_ret = IRQ_NONE;
  int plug_device = 0;
- struct request *uninitialized_var(rq_in_flight);
+ struct request *rq_in_flight = NULL;
 
  if (host->host_flags & IDE_HFLAG_SERIALIZE) {
  if (hwif != host->cur_port)
@@ -856,8 +859,10 @@ irqreturn_t ide_intr (int irq, void *dev
  */
  if (startstop == ide_stopped && hwif->polling == 0) {
  BUG_ON(hwif->handler);
- rq_in_flight = hwif->rq;
- hwif->rq = NULL;
+ if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
+ rq_in_flight = hwif->rq;
+ hwif->rq = NULL;
+ }
  ide_unlock_port(hwif);
  plug_device = 1;
  }

[Wu Zhangjin]

Hi,

> OK, I see another gotcha added by recent changes, we need to explicitly
> initialize rq_in_flight variables now.  Revised patch below..
>

Sorry, STD also not work. if apply this patch, the same problem as not
apply it, it stopped at:

...
PM: Crete hibernation image:
PM: Need to copy ... pages
PM: Hibernation image created ...

I think it's better to revert this commit:
 a1317f714af7aed60ddc182d0122477cbe36ee9b ("ide: improve handling of
Power Management requests")

Regards,
Wu Zhangjin

> From: Bartlomiej Zolnierkiewicz <bzolnier@...>
> Subject: [PATCH] ide: make resume work again (for real)
>
> It turns out that commit a1317f714af7aed60ddc182d0122477cbe36ee9b
> ("ide: improve handling of Power Management requests") needs to take
> into the account a new code added by the recent block layer changes
> in commit 8f6205cd572fece673da0255d74843680f67f879 ("ide: dequeue
> in-flight request") and prevent clearing of hwif->rq if the device
> is blocked.
>
> Thanks to Etienne, Wu and Jeff for help in fixing the issue.
>
> Reported-and-tested-by: Jeff Chua <jeff.chua.linux@...>
> Reported-and-tested-by: Etienne Basset <etienne.basset@...>
> Reported-by: Wu Zhangjin <wuzhangjin@...>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@...>
> ---
> Added patch description, no other changes.
>
>  drivers/ide/ide-io.c |   19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> Index: b/drivers/ide/ide-io.c
> ===================================================================
> --- a/drivers/ide/ide-io.c
> +++ b/drivers/ide/ide-io.c
> @@ -532,7 +532,8 @@ repeat:
>  
>   if (startstop == ide_stopped) {
>   rq = hwif->rq;
> - hwif->rq = NULL;
> + if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0)
> + hwif->rq = NULL;
>   goto repeat;
>   }
>   } else
> @@ -616,7 +617,7 @@ void ide_timer_expiry (unsigned long dat
>   unsigned long flags;
>   int wait = -1;
>   int plug_device = 0;
> - struct request *uninitialized_var(rq_in_flight);
> + struct request *rq_in_flight = NULL;
>  
>   spin_lock_irqsave(&hwif->lock, flags);
>  
> @@ -679,8 +680,10 @@ void ide_timer_expiry (unsigned long dat
>   spin_lock_irq(&hwif->lock);
>   enable_irq(hwif->irq);
>   if (startstop == ide_stopped && hwif->polling == 0) {
> - rq_in_flight = hwif->rq;
> - hwif->rq = NULL;
> + if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
> + rq_in_flight = hwif->rq;
> + hwif->rq = NULL;
> + }
>   ide_unlock_port(hwif);
>   plug_device = 1;
>   }
> @@ -775,7 +778,7 @@ irqreturn_t ide_intr (int irq, void *dev
>   ide_startstop_t startstop;
>   irqreturn_t irq_ret = IRQ_NONE;
>   int plug_device = 0;
> - struct request *uninitialized_var(rq_in_flight);
> + struct request *rq_in_flight = NULL;
>  
>   if (host->host_flags & IDE_HFLAG_SERIALIZE) {
>   if (hwif != host->cur_port)
> @@ -856,8 +859,10 @@ irqreturn_t ide_intr (int irq, void *dev
>   */
>   if (startstop == ide_stopped && hwif->polling == 0) {
>   BUG_ON(hwif->handler);
> - rq_in_flight = hwif->rq;
> - hwif->rq = NULL;
> + if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
> + rq_in_flight = hwif->rq;
> + hwif->rq = NULL;
> + }
>   ide_unlock_port(hwif);
>   plug_device = 1;
>   }

[Bartlomiej Zolnierkiewicz]

On Friday 03 July 2009 17:31:36 Wu Zhangjin wrote:

> Hi,
>
> > OK, I see another gotcha added by recent changes, we need to explicitly
> > initialize rq_in_flight variables now.  Revised patch below..
> >
>
> Sorry, STD also not work. if apply this patch, the same problem as not
> apply it, it stopped at:
>
> ...
> PM: Crete hibernation image:
> PM: Need to copy ... pages
> PM: Hibernation image created ...
>
> I think it's better to revert this commit:
>  a1317f714af7aed60ddc182d0122477cbe36ee9b ("ide: improve handling of
> Power Management requests")

 
I completely agree and I've already requested this a week ago
(this commit was not meant for going straight to -rc tree anyway).

[David Miller-13]

From: Bartlomiej Zolnierkiewicz <bzolnier@...>
Date: Mon, 6 Jul 2009 16:57:59 +0200

>> I think it's better to revert this commit:
>>  a1317f714af7aed60ddc182d0122477cbe36ee9b ("ide: improve handling of
>> Power Management requests")
>  
> I completely agree and I've already requested this a week ago
> (this commit was not meant for going straight to -rc tree anyway).

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

[David Rientjes]

When debugging is enabled, slub requires that additional metadata be
stored in slabs for certain options: SLAB_RED_ZONE, SLAB_POISON, and
SLAB_STORE_USER.

Consequently, it may require that the minimum possible slab order needed
to allocate a single object be greater when using these options.  The
most notable example is for objects that are PAGE_SIZE bytes in size.

Higher minimum slab orders may cause page allocation failures when oom or
under heavy fragmentation.

This patch adds a new slub_debug option, which disables debugging by
default for caches that would have resulted in higher minimum orders:

        slub_debug=O

When this option is used on systems with 4K pages, kmalloc-4096, for
example, will not have debugging enabled by default even if
CONFIG_SLUB_DEBUG_ON is defined because it would have resulted in a
order-1 minimum slab order.

Cc: Christoph Lameter <cl@...>
Signed-off-by: David Rientjes <rientjes@...>
---
 Documentation/vm/slub.txt |   10 ++++++++++
 mm/slub.c                 |   42 +++++++++++++++++++++++++++++++++++++++---
 2 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/Documentation/vm/slub.txt b/Documentation/vm/slub.txt
--- a/Documentation/vm/slub.txt
+++ b/Documentation/vm/slub.txt
@@ -41,6 +41,8 @@ Possible debug options are
  P Poisoning (object and padding)
  U User tracking (free and alloc)
  T Trace (please only use on single slabs)
+ O Switch debugging off for caches that would have
+ caused higher minimum slab orders
  - Switch all debugging off (useful if the kernel is
  configured with CONFIG_SLUB_DEBUG_ON)
 
@@ -59,6 +61,14 @@ to the dentry cache with
 
  slub_debug=F,dentry
 
+Debugging options may require the minimum possible slab order to increase as
+a result of storing the metadata (for example, caches with PAGE_SIZE object
+sizes).  This has a higher liklihood of resulting in slab allocation errors
+in low memory situations or if there's high fragmentation of memory.  To
+switch off debugging for such caches by default, use
+
+ slub_debug=O
+
 In case you forgot to enable debugging on the kernel command line: It is
 possible to enable debugging manually when the kernel is up. Look at the
 contents of:
diff --git a/mm/slub.c b/mm/slub.c
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -142,6 +142,13 @@
  SLAB_POISON | SLAB_STORE_USER)
 
 /*
+ * Debugging flags that require metadata to be stored in the slab, up to
+ * DEBUG_SIZE in size.
+ */
+#define DEBUG_SIZE_FLAGS (SLAB_RED_ZONE | SLAB_POISON | SLAB_STORE_USER)
+#define DEBUG_SIZE (3 * sizeof(void *) + 2 * sizeof(struct track))
+
+/*
  * Set of flags that will prevent slab merging
  */
 #define SLUB_NEVER_MERGE (SLAB_RED_ZONE | SLAB_POISON | SLAB_STORE_USER | \
@@ -326,6 +333,7 @@ static int slub_debug;
 #endif
 
 static char *slub_debug_slabs;
+static int disable_higher_order_debug;
 
 /*
  * Object debugging
@@ -977,6 +985,15 @@ static int __init setup_slub_debug(char *str)
  */
  goto check_slabs;
 
+ if (tolower(*str) == 'o') {
+ /*
+ * Avoid enabling debugging on caches if its minimum order
+ * would increase as a result.
+ */
+ disable_higher_order_debug = 1;
+ goto out;
+ }
+
  slub_debug = 0;
  if (*str == '-')
  /*
@@ -1023,13 +1040,28 @@ static unsigned long kmem_cache_flags(unsigned long objsize,
  unsigned long flags, const char *name,
  void (*ctor)(void *))
 {
+ int debug_flags = slub_debug;
+
  /*
  * Enable debugging if selected on the kernel commandline.
  */
- if (slub_debug && (!slub_debug_slabs ||
-    strncmp(slub_debug_slabs, name, strlen(slub_debug_slabs)) == 0))
- flags |= slub_debug;
+ if (debug_flags) {
+ if (slub_debug_slabs &&
+    strncmp(slub_debug_slabs, name, strlen(slub_debug_slabs)))
+ goto out;
+
+ /*
+ * Disable debugging that increases slab size if the minimum
+ * slab order would have increased as a result.
+ */
+ if (disable_higher_order_debug &&
+    get_order(objsize + DEBUG_SIZE) > get_order(objsize))
+ debug_flags &= ~DEBUG_SIZE_FLAGS;
+ goto out;
 
+ flags |= debug_flags;
+ }
+out:
  return flags;
 }
 #else
@@ -1561,6 +1593,10 @@ slab_out_of_memory(struct kmem_cache *s, gfp_t gfpflags, int nid)
  "default order: %d, min order: %d\n", s->name, s->objsize,
  s->size, oo_order(s->oo), oo_order(s->min));
 
+ if (oo_order(s->min) > get_order(s->objsize))
+ printk(KERN_WARNING "  %s debugging increased min order, use "
+       "slub_debug=O to disable.\n", s->name);
+
  for_each_online_node(node) {
  struct kmem_cache_node *n = get_node(s, node);
  unsigned long nr_slabs;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[David Rientjes]

When debugging is enabled, slub requires that additional metadata be
stored in slabs for certain options: SLAB_RED_ZONE, SLAB_POISON, and
SLAB_STORE_USER.

Consequently, it may require that the minimum possible slab order needed
to allocate a single object be greater when using these options.  The
most notable example is for objects that are PAGE_SIZE bytes in size.

Higher minimum slab orders may cause page allocation failures when oom or
under heavy fragmentation.

This patch adds a new slub_debug option, which disables debugging by
default for caches that would have resulted in higher minimum orders:

        slub_debug=O

When this option is used on systems with 4K pages, kmalloc-4096, for
example, will not have debugging enabled by default even if
CONFIG_SLUB_DEBUG_ON is defined because it would have resulted in a
order-1 minimum slab order.

Cc: Christoph Lameter <cl@...>
Signed-off-by: David Rientjes <rientjes@...>
---

 V1 -> V2: Removed spurious `goto out'.

 Documentation/vm/slub.txt |   10 ++++++++++
 mm/slub.c                 |   41 ++++++++++++++++++++++++++++++++++++++---
 2 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/Documentation/vm/slub.txt b/Documentation/vm/slub.txt
--- a/Documentation/vm/slub.txt
+++ b/Documentation/vm/slub.txt
@@ -41,6 +41,8 @@ Possible debug options are
  P Poisoning (object and padding)
  U User tracking (free and alloc)
  T Trace (please only use on single slabs)
+ O Switch debugging off for caches that would have
+ caused higher minimum slab orders
  - Switch all debugging off (useful if the kernel is
  configured with CONFIG_SLUB_DEBUG_ON)
 
@@ -59,6 +61,14 @@ to the dentry cache with
 
  slub_debug=F,dentry
 
+Debugging options may require the minimum possible slab order to increase as
+a result of storing the metadata (for example, caches with PAGE_SIZE object
+sizes).  This has a higher liklihood of resulting in slab allocation errors
+in low memory situations or if there's high fragmentation of memory.  To
+switch off debugging for such caches by default, use
+
+ slub_debug=O
+
 In case you forgot to enable debugging on the kernel command line: It is
 possible to enable debugging manually when the kernel is up. Look at the
 contents of:
diff --git a/mm/slub.c b/mm/slub.c
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -142,6 +142,13 @@
  SLAB_POISON | SLAB_STORE_USER)
 
 /*
+ * Debugging flags that require metadata to be stored in the slab, up to
+ * DEBUG_SIZE in size.
+ */
+#define DEBUG_SIZE_FLAGS (SLAB_RED_ZONE | SLAB_POISON | SLAB_STORE_USER)
+#define DEBUG_SIZE (3 * sizeof(void *) + 2 * sizeof(struct track))
+
+/*
  * Set of flags that will prevent slab merging
  */
 #define SLUB_NEVER_MERGE (SLAB_RED_ZONE | SLAB_POISON | SLAB_STORE_USER | \
@@ -326,6 +333,7 @@ static int slub_debug;
 #endif
 
 static char *slub_debug_slabs;
+static int disable_higher_order_debug;
 
 /*
  * Object debugging
@@ -977,6 +985,15 @@ static int __init setup_slub_debug(char *str)
  */
  goto check_slabs;
 
+ if (tolower(*str) == 'o') {
+ /*
+ * Avoid enabling debugging on caches if its minimum order
+ * would increase as a result.
+ */
+ disable_higher_order_debug = 1;
+ goto out;
+ }
+
  slub_debug = 0;
  if (*str == '-')
  /*
@@ -1023,13 +1040,27 @@ static unsigned long kmem_cache_flags(unsigned long objsize,
  unsigned long flags, const char *name,
  void (*ctor)(void *))
 {
+ int debug_flags = slub_debug;
+
  /*
  * Enable debugging if selected on the kernel commandline.
  */
- if (slub_debug && (!slub_debug_slabs ||
-    strncmp(slub_debug_slabs, name, strlen(slub_debug_slabs)) == 0))
- flags |= slub_debug;
+ if (debug_flags) {
+ if (slub_debug_slabs &&
+    strncmp(slub_debug_slabs, name, strlen(slub_debug_slabs)))
+ goto out;
+
+ /*
+ * Disable debugging that increases slab size if the minimum
+ * slab order would have increased as a result.
+ */
+ if (disable_higher_order_debug &&
+    get_order(objsize + DEBUG_SIZE) > get_order(objsize))
+ debug_flags &= ~DEBUG_SIZE_FLAGS;
 
+ flags |= debug_flags;
+ }
+out:
  return flags;
 }
 #else
@@ -1561,6 +1592,10 @@ slab_out_of_memory(struct kmem_cache *s, gfp_t gfpflags, int nid)
  "default order: %d, min order: %d\n", s->name, s->objsize,
  s->size, oo_order(s->oo), oo_order(s->min));
 
+ if (oo_order(s->min) > get_order(s->objsize))
+ printk(KERN_WARNING "  %s debugging increased min order, use "
+       "slub_debug=O to disable.\n", s->name);
+
  for_each_online_node(node) {
  struct kmem_cache_node *n = get_node(s, node);
  unsigned long nr_slabs;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Américo Wang]

On Thu, Jul 2, 2009 at 4:36 AM, Joao Correia<joaomiguelcorreia@...> wrote:

> No formal patch has been sent yet, that i am aware of. I have made
> some changes following suggestion by Americo Wang advise, to the
> following:
>
> (patch by Ingo)
>
> diff --git a/kernel/lockdep_internals.h b/kernel/lockdep_internals.h
> index 699a2ac..031f4c6 100644
> --- a/kernel/lockdep_internals.h
> +++ b/kernel/lockdep_internals.h
> @@ -65,7 +65,7 @@ enum {
>  * Stack-trace: tightly packed array of stack backtrace
>  * addresses. Protected by the hash_lock.
>  */
> -#define MAX_STACK_TRACE_ENTRIES        262144UL
> +#define MAX_STACK_TRACE_ENTRIES        1048576UL
>
>  extern struct list_head all_lock_classes;
>  extern struct lock_chain lock_chains[];
>
> and afterwards, a new bug popped up, solved by changing
>
> include/linux/sched.h
>
> # define MAX_LOCK_DEPTH 48UL
>
> to
>
> # define MAX_LOCK_DEPTH 96UL
>
>
> I have now found a third limit bug, related to MAX_LOCKDEP_CHAINS,
> which was hidden so far, which im trying to raise and replicate. This
> is being discussed in detail in another message exchange on the lkml,
> between me and Americo.

How about changing MAX_LOCKDEP_CHAINS_BITS to 16?

kernel/lockdep_internals.h:59:#define MAX_LOCKDEP_CHAINS_BITS 15

And can you make a complete patch and send it to lkml with Peter and me
Cc'ed?

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

[Joao Correia]

Already testing the changes, just to see if something else breaks.

Any special notes on the patch (a basic guideline info on patches
would be great, just so i dont mess it up)? Never submited one before.

Joao Correia

On Tue, Jul 7, 2009 at 3:05 PM, Américo Wang<xiyou.wangcong@...> wrote:

> On Thu, Jul 2, 2009 at 4:36 AM, Joao Correia<joaomiguelcorreia@...> wrote:
>> No formal patch has been sent yet, that i am aware of. I have made
>> some changes following suggestion by Americo Wang advise, to the
>> following:
>>
>> (patch by Ingo)
>>
>> diff --git a/kernel/lockdep_internals.h b/kernel/lockdep_internals.h
>> index 699a2ac..031f4c6 100644
>> --- a/kernel/lockdep_internals.h
>> +++ b/kernel/lockdep_internals.h
>> @@ -65,7 +65,7 @@ enum {
>>  * Stack-trace: tightly packed array of stack backtrace
>>  * addresses. Protected by the hash_lock.
>>  */
>> -#define MAX_STACK_TRACE_ENTRIES        262144UL
>> +#define MAX_STACK_TRACE_ENTRIES        1048576UL
>>
>>  extern struct list_head all_lock_classes;
>>  extern struct lock_chain lock_chains[];
>>
>> and afterwards, a new bug popped up, solved by changing
>>
>> include/linux/sched.h
>>
>> # define MAX_LOCK_DEPTH 48UL
>>
>> to
>>
>> # define MAX_LOCK_DEPTH 96UL
>>
>>
>> I have now found a third limit bug, related to MAX_LOCKDEP_CHAINS,
>> which was hidden so far, which im trying to raise and replicate. This
>> is being discussed in detail in another message exchange on the lkml,
>> between me and Americo.
>
> How about changing MAX_LOCKDEP_CHAINS_BITS to 16?
>
> kernel/lockdep_internals.h:59:#define MAX_LOCKDEP_CHAINS_BITS   15
>
> And can you make a complete patch and send it to lkml with Peter and me
> Cc'ed?
>
> Thank you!
>

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

[Américo Wang]

On Tue, Jul 7, 2009 at 10:22 PM, Joao
Correia<joaomiguelcorreia@...> wrote:
> Already testing the changes, just to see if something else breaks.
>
> Any special notes on the patch (a basic guideline info on patches
> would be great, just so i dont mess it up)? Never submited one before.

Yes, check Documentation/SubmittingPatches and Documentation/email-clients.txt.

I am not sure if Peter likes them, but it is a good idea to split them
and send one by one.

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

[Christoph Lameter-7]

On Tue, 7 Jul 2009, David Rientjes wrote:

> + * Debugging flags that require metadata to be stored in the slab, up to
> + * DEBUG_SIZE in size.
> + */
> +#define DEBUG_SIZE_FLAGS (SLAB_RED_ZONE | SLAB_POISON | SLAB_STORE_USER)
> +#define DEBUG_SIZE (3 * sizeof(void *) + 2 * sizeof(struct track))

There is no need for DEBUG_SIZE since slub keeps both the size of the
object kmem_cache->objsize and the size with the metadata kmem_cache->size

If the order of both is different then the order would increase.

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

[David Rientjes]

On Tue, 7 Jul 2009, Christoph Lameter wrote:

> > + * Debugging flags that require metadata to be stored in the slab, up to
> > + * DEBUG_SIZE in size.
> > + */
> > +#define DEBUG_SIZE_FLAGS (SLAB_RED_ZONE | SLAB_POISON | SLAB_STORE_USER)
> > +#define DEBUG_SIZE (3 * sizeof(void *) + 2 * sizeof(struct track))
>
> There is no need for DEBUG_SIZE since slub keeps both the size of the
> object kmem_cache->objsize and the size with the metadata kmem_cache->size
>
> If the order of both is different then the order would increase.
>

Without DEBUG_SIZE_FLAGS, the only way to determine what flags have
increased the size is in calculate_sizes() and then disable them by
default if slub_debug=O is specified.  calculate_sizes() is used by
the `store', `poison', and `red_zone' callbacks, so the admin still has
the ability to enable these options even though slub_debug=O was used.

So we can either mask off the size-increasing debug bits when the cache is
created in kmem_cache_flags() like I did, or we can move the logic to
calculate_sizes() with an added formal to determine whether this is from
kmem_cache_open() or one of the attribute callbacks.

I think my solution is the cleanest and provides a single entity,
DEBUG_SIZE_FLAGS, which specifies the flags that slub_debug=O clears if
the minimum order increases.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Pekka Enberg-3]

On Tue, 7 Jul 2009, Christoph Lameter wrote:

> > > + * Debugging flags that require metadata to be stored in the slab, up to
> > > + * DEBUG_SIZE in size.
> > > + */
> > > +#define DEBUG_SIZE_FLAGS (SLAB_RED_ZONE | SLAB_POISON | SLAB_STORE_USER)
> > > +#define DEBUG_SIZE (3 * sizeof(void *) + 2 * sizeof(struct track))
> >
> > There is no need for DEBUG_SIZE since slub keeps both the size of the
> > object kmem_cache->objsize and the size with the metadata kmem_cache->size
> >
> > If the order of both is different then the order would increase.

On Thu, 2009-07-09 at 16:26 -0700, David Rientjes wrote:

> Without DEBUG_SIZE_FLAGS, the only way to determine what flags have
> increased the size is in calculate_sizes() and then disable them by
> default if slub_debug=O is specified.  calculate_sizes() is used by
> the `store', `poison', and `red_zone' callbacks, so the admin still has
> the ability to enable these options even though slub_debug=O was used.
>
> So we can either mask off the size-increasing debug bits when the cache is
> created in kmem_cache_flags() like I did, or we can move the logic to
> calculate_sizes() with an added formal to determine whether this is from
> kmem_cache_open() or one of the attribute callbacks.
>
> I think my solution is the cleanest and provides a single entity,
> DEBUG_SIZE_FLAGS, which specifies the flags that slub_debug=O clears if
> the minimum order increases.

Yup, agreed. I applied the patch, thanks everyone!

                        Pekka

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

[Christoph Lameter-7]

On Fri, 10 Jul 2009, Pekka Enberg wrote:

> On Thu, 2009-07-09 at 16:26 -0700, David Rientjes wrote:
> > Without DEBUG_SIZE_FLAGS, the only way to determine what flags have
> > increased the size is in calculate_sizes() and then disable them by
> > default if slub_debug=O is specified.  calculate_sizes() is used by
> > the `store', `poison', and `red_zone' callbacks, so the admin still has
> > the ability to enable these options even though slub_debug=O was used.
> >
> > So we can either mask off the size-increasing debug bits when the cache is
> > created in kmem_cache_flags() like I did, or we can move the logic to
> > calculate_sizes() with an added formal to determine whether this is from
> > kmem_cache_open() or one of the attribute callbacks.
> >
> > I think my solution is the cleanest and provides a single entity,
> > DEBUG_SIZE_FLAGS, which specifies the flags that slub_debug=O clears if
> > the minimum order increases.
>
> Yup, agreed. I applied the patch, thanks everyone!

There is a simpler solution. Call calculate sizes again if the resulting
sizes increased the order. Something like this.

Index: linux-2.6/mm/slub.c
===================================================================
--- linux-2.6.orig/mm/slub.c    2009-07-10 13:45:02.000000000 -0500
+++ linux-2.6/mm/slub.c 2009-07-10 13:46:07.000000000 -0500
@@ -2454,6 +2454,10 @@ static int kmem_cache_open(struct kmem_c
        if (!calculate_sizes(s, -1))
                goto error;

+       if (get_order(s->size) != get_order(s->objsize) && flag is set) {
+               switch off debug flags.
+               calculate_sizes(s, -1);
+       }
        /*
         * The larger the object size is, the more pages we want on the
partial
         * list to avoid pounding the page allocator excessively.

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