| Message ID | 1472226699.14381.186.camel@edumazet-glaptop3.roam.corp.google.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Eric Dumazet <eric.dumazet@gmail.com> wrote: > From: Eric Dumazet <edumazet@google.com> > > If vmalloc() was successful, do not attempt a kmalloc_array() > > Fixes: 4cf0b354d92e ("rhashtable: avoid large lock-array allocations") > Reported-by: CAI Qian <caiqian@redhat.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Florian Westphal <fw@strlen.de> > --- > lib/rhashtable.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/lib/rhashtable.c b/lib/rhashtable.c > index 5ba520b544d7..56054e541a0f 100644 > --- a/lib/rhashtable.c > +++ b/lib/rhashtable.c > @@ -77,17 +77,18 @@ static int alloc_bucket_locks(struct rhashtable *ht, struct bucket_table *tbl, > size = min_t(unsigned int, size, tbl->size >> 1); > > if (sizeof(spinlock_t) != 0) { > + tbl->locks = NULL; > #ifdef CONFIG_NUMA > if (size * sizeof(spinlock_t) > PAGE_SIZE && > gfp == GFP_KERNEL) > tbl->locks = vmalloc(size * sizeof(spinlock_t)); > - else > #endif Argh. Thanks Eric for fixing this :-/
On Fri, Aug 26, 2016 at 08:51:39AM -0700, Eric Dumazet wrote: > From: Eric Dumazet <edumazet@google.com> > > If vmalloc() was successful, do not attempt a kmalloc_array() > > Fixes: 4cf0b354d92e ("rhashtable: avoid large lock-array allocations") > Reported-by: CAI Qian <caiqian@redhat.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Florian Westphal <fw@strlen.de> Acked-by: Herbert Xu <herbert@gondor.apana.org.au> Thanks Eric!
On Fri, Aug 26, 2016 at 8:51 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > From: Eric Dumazet <edumazet@google.com> > > If vmalloc() was successful, do not attempt a kmalloc_array() > > Fixes: 4cf0b354d92e ("rhashtable: avoid large lock-array allocations") > Reported-by: CAI Qian <caiqian@redhat.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Florian Westphal <fw@strlen.de> > --- > lib/rhashtable.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/lib/rhashtable.c b/lib/rhashtable.c > index 5ba520b544d7..56054e541a0f 100644 > --- a/lib/rhashtable.c > +++ b/lib/rhashtable.c > @@ -77,17 +77,18 @@ static int alloc_bucket_locks(struct rhashtable *ht, struct bucket_table *tbl, > size = min_t(unsigned int, size, tbl->size >> 1); > > if (sizeof(spinlock_t) != 0) { > + tbl->locks = NULL; > #ifdef CONFIG_NUMA > if (size * sizeof(spinlock_t) > PAGE_SIZE && > gfp == GFP_KERNEL) > tbl->locks = vmalloc(size * sizeof(spinlock_t)); > - else > #endif Not directly for your patch, but why did we have this CONFIG_NUMA for vmalloc()? I think this macro is the real cause of the bug. :-P
After applied to this patch and ran the reproducer (compiling gcc), had no bucket_table_alloc in kmemleak report anymore. Hence, Tested-by: CAI Qian <caiqian@redhat.com> Funny enough, it now gave me this, [ 3406.807461] kmemleak: 1353 new suspected memory leaks (see /sys/kernel/debug/kmemleak) http://people.redhat.com/qcai/tmp/kmemleak.log CAI Qian ----- Original Message ----- > From: "Eric Dumazet" <eric.dumazet@gmail.com> > To: "David Miller" <davem@davemloft.net> > Cc: "CAI Qian" <caiqian@redhat.com>, "Thomas Graf" <tgraf@suug.ch>, "Herbert Xu" <herbert@gondor.apana.org.au>, "Eric > Dumazet" <edumazet@google.com>, "Network Development" <netdev@vger.kernel.org>, "Linus Torvalds" > <torvalds@linux-foundation.org>, "Florian Westphal" <fw@strlen.de> > Sent: Friday, August 26, 2016 11:51:39 AM > Subject: [PATCH net] rhashtable: fix a memory leak in alloc_bucket_locks() > > From: Eric Dumazet <edumazet@google.com> > > If vmalloc() was successful, do not attempt a kmalloc_array() > > Fixes: 4cf0b354d92e ("rhashtable: avoid large lock-array allocations") > Reported-by: CAI Qian <caiqian@redhat.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Florian Westphal <fw@strlen.de> > --- > lib/rhashtable.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/lib/rhashtable.c b/lib/rhashtable.c > index 5ba520b544d7..56054e541a0f 100644 > --- a/lib/rhashtable.c > +++ b/lib/rhashtable.c > @@ -77,17 +77,18 @@ static int alloc_bucket_locks(struct rhashtable *ht, > struct bucket_table *tbl, > size = min_t(unsigned int, size, tbl->size >> 1); > > if (sizeof(spinlock_t) != 0) { > + tbl->locks = NULL; > #ifdef CONFIG_NUMA > if (size * sizeof(spinlock_t) > PAGE_SIZE && > gfp == GFP_KERNEL) > tbl->locks = vmalloc(size * sizeof(spinlock_t)); > - else > #endif > if (gfp != GFP_KERNEL) > gfp |= __GFP_NOWARN | __GFP_NORETRY; > > - tbl->locks = kmalloc_array(size, sizeof(spinlock_t), > - gfp); > + if (!tbl->locks) > + tbl->locks = kmalloc_array(size, sizeof(spinlock_t), > + gfp); > if (!tbl->locks) > return -ENOMEM; > for (i = 0; i < size; i++) > > >
On Fri, Aug 26, 2016 at 10:40 AM, CAI Qian <caiqian@redhat.com> wrote: > After applied to this patch and ran the reproducer (compiling gcc), had > no bucket_table_alloc in kmemleak report anymore. Hence, > > Tested-by: CAI Qian <caiqian@redhat.com> > > Funny enough, it now gave me this, > > [ 3406.807461] kmemleak: 1353 new suspected memory leaks (see /sys/kernel/debug/kmemleak) Those logs look incomprehensible. This one looks more like a kmemleak bug than anything else. Linus
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Fri, 26 Aug 2016 08:51:39 -0700 > From: Eric Dumazet <edumazet@google.com> > > If vmalloc() was successful, do not attempt a kmalloc_array() > > Fixes: 4cf0b354d92e ("rhashtable: avoid large lock-array allocations") > Reported-by: CAI Qian <caiqian@redhat.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> Applied, thanks Eric.
diff --git a/lib/rhashtable.c b/lib/rhashtable.c index 5ba520b544d7..56054e541a0f 100644 --- a/lib/rhashtable.c +++ b/lib/rhashtable.c @@ -77,17 +77,18 @@ static int alloc_bucket_locks(struct rhashtable *ht, struct bucket_table *tbl, size = min_t(unsigned int, size, tbl->size >> 1); if (sizeof(spinlock_t) != 0) { + tbl->locks = NULL; #ifdef CONFIG_NUMA if (size * sizeof(spinlock_t) > PAGE_SIZE && gfp == GFP_KERNEL) tbl->locks = vmalloc(size * sizeof(spinlock_t)); - else #endif if (gfp != GFP_KERNEL) gfp |= __GFP_NOWARN | __GFP_NORETRY; - tbl->locks = kmalloc_array(size, sizeof(spinlock_t), - gfp); + if (!tbl->locks) + tbl->locks = kmalloc_array(size, sizeof(spinlock_t), + gfp); if (!tbl->locks) return -ENOMEM; for (i = 0; i < size; i++)