[glommer-memcg:kmemcg-slab 53/62] mm/memcontrol.c:755:6: sparse: symbol 'memcg_flush_cache_create_qu

[glommer-memcg:kmemcg-slab 53/62] mm/memcontrol.c:755:6: sparse: symbol 'memcg_flush_cache_create_qu

[Fengguang Wu]

[-- Attachment #1: Type: text/plain, Size: 2122 bytes --]

Hi Glauber,

FYI, there are new sparse warnings show up in

tree:   git://git.kernel.org/pub/scm/linux/kernel/git/glommer/memcg.git kmemcg-slab
head:   b20a4abf14992a969b761691f8978a28cacdecb4
commit: db85c41ae676eb65f37b31a59111f2b9b56621ed [53/62] memcg: infrastructure to match an allocation to the right cache

All sparse warnings:

  mm/memcontrol.c:585:12: sparse: symbol 'cache_types' was not declared. Should it be static?
+ mm/memcontrol.c:755:6: sparse: symbol 'memcg_flush_cache_create_queue' was not declared. Should it be static?
  include/linux/cgroup.h:559:16: sparse: cast removes address space of expression
  include/linux/cgroup.h:559:16: sparse: incompatible types in comparison expression (different address spaces)
  include/linux/cgroup.h:559:16: sparse: incompatible types in comparison expression (different address spaces)
  mm/memcontrol.c:4763:21: sparse: incompatible types in comparison expression (different address spaces)
  mm/memcontrol.c:4765:21: sparse: incompatible types in comparison expression (different address spaces)
  mm/memcontrol.c:5946:31: sparse: incompatible types in comparison expression (different address spaces)

vim +755 mm/memcontrol.c

   748	/*
   749	 * Flush the queue of kmem_caches to create, because we're creating a cgroup.
   750	 *
   751	 * We might end up flushing other cgroups' creation requests as well, but
   752	 * they will just get queued again next time someone tries to make a slab
   753	 * allocation for them.
   754	 */
 > 755	void memcg_flush_cache_create_queue(void)
   756	{
   757		struct create_work *cw, *tmp;
   758		unsigned long flags;
   759	
   760		spin_lock_irqsave(&cache_queue_lock, flags);
   761		list_for_each_entry_safe(cw, tmp, &create_queue, list) {
   762			list_del(&cw->list);
   763			kfree(cw);
   764		}
   765		spin_unlock_irqrestore(&cache_queue_lock, flags);
   766	}

Please consider folding the attached diff and ignore conflicts with
the previous one :-)

---
0-DAY kernel build testing backend         Open Source Technology Centre
Fengguang Wu <wfg@linux.intel.com>                     Intel Corporation

[-- Attachment #2: make-it-static-db85c41.diff --]
[-- Type: text/x-diff, Size: 677 bytes --]

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b85814a..fb7f1ca 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -582,7 +582,7 @@ static struct kmem_cache *kmem_cache_dup(struct mem_cgroup *memcg,
 	return new;
 }
 
-struct ida cache_types;
+static struct ida cache_types;
 
 void memcg_register_cache(struct mem_cgroup *memcg, struct kmem_cache *cachep)
 {
@@ -752,7 +752,7 @@ static LIST_HEAD(create_queue);
  * they will just get queued again next time someone tries to make a slab
  * allocation for them.
  */
-void memcg_flush_cache_create_queue(void)
+static void memcg_flush_cache_create_queue(void)
 {
 	struct create_work *cw, *tmp;
 	unsigned long flags;

[glommer-memcg:kmemcg-slab 57/62] drivers/video/riva/fbdev.c:281:9: sparse: preprocessor token MAX_L

[Fengguang Wu]

Glauber,

The patch entitled

 sl[au]b: Allocate objects from memcg cache

changes

 include/linux/slub_def.h |   15 ++++++++++-----

which triggers this warning:

drivers/video/riva/fbdev.c:281:9: sparse: preprocessor token MAX_LEVEL redefined

It's the MAX_LEVEL that is defined in include/linux/idr.h.

MAX_LEVEL is obviously too generic. Better adding some prefix to it?

Thanks,
Fengguang

Re: [glommer-memcg:kmemcg-slab 57/62] drivers/video/riva/fbdev.c:281:9: sparse: preprocessor token M

[Glauber Costa]

On 09/10/2012 04:57 PM, Fengguang Wu wrote:
> Glauber,
> 
> The patch entitled
> 
>  sl[au]b: Allocate objects from memcg cache
> 
> changes
> 
>  include/linux/slub_def.h |   15 ++++++++++-----
> 
> which triggers this warning:
> 
> drivers/video/riva/fbdev.c:281:9: sparse: preprocessor token MAX_LEVEL redefined
> 
> It's the MAX_LEVEL that is defined in include/linux/idr.h.
> 
> MAX_LEVEL is obviously too generic. Better adding some prefix to it?
> 

I don't see any MAX_LEVEL definition in this patch. You say it is
defined in include/linux/idr.h, and as the diffstat shows, I am not
touching this file.

I think this needs patching independently.

Re: [glommer-memcg:kmemcg-slab 57/62] drivers/video/riva/fbdev.c:281:9: sparse: preprocessor token M

[Fengguang Wu]

On Mon, Sep 10, 2012 at 04:58:02PM +0400, Glauber Costa wrote:
> On 09/10/2012 04:57 PM, Fengguang Wu wrote:
> > Glauber,
> > 
> > The patch entitled
> > 
> >  sl[au]b: Allocate objects from memcg cache
> > 
> > changes
> > 
> >  include/linux/slub_def.h |   15 ++++++++++-----
> > 
> > which triggers this warning:
> > 
> > drivers/video/riva/fbdev.c:281:9: sparse: preprocessor token MAX_LEVEL redefined
> > 
> > It's the MAX_LEVEL that is defined in include/linux/idr.h.
> > 
> > MAX_LEVEL is obviously too generic. Better adding some prefix to it?
> > 
> 
> I don't see any MAX_LEVEL definition in this patch. You say it is
> defined in include/linux/idr.h, and as the diffstat shows, I am not
> touching this file.

It's a rather *unexpected* side effect. You changed slub_def.h to
include memcontrol.h/cgroup.h which in turn includes idr.h.

> I think this needs patching independently.

Yes, sure. And perhaps send it for quick inclusion before your patches?

Thanks,
Fengguang

Re: [glommer-memcg:kmemcg-slab 57/62] drivers/video/riva/fbdev.c:281:9: sparse: preprocessor token M

[Glauber Costa]

On 09/10/2012 05:06 PM, Fengguang Wu wrote:
> On Mon, Sep 10, 2012 at 04:58:02PM +0400, Glauber Costa wrote:
>> On 09/10/2012 04:57 PM, Fengguang Wu wrote:
>>> Glauber,
>>>
>>> The patch entitled
>>>
>>>  sl[au]b: Allocate objects from memcg cache
>>>
>>> changes
>>>
>>>  include/linux/slub_def.h |   15 ++++++++++-----
>>>
>>> which triggers this warning:
>>>
>>> drivers/video/riva/fbdev.c:281:9: sparse: preprocessor token MAX_LEVEL redefined
>>>
>>> It's the MAX_LEVEL that is defined in include/linux/idr.h.
>>>
>>> MAX_LEVEL is obviously too generic. Better adding some prefix to it?
>>>
>>
>> I don't see any MAX_LEVEL definition in this patch. You say it is
>> defined in include/linux/idr.h, and as the diffstat shows, I am not
>> touching this file.
> 
> It's a rather *unexpected* side effect. You changed slub_def.h to
> include memcontrol.h/cgroup.h which in turn includes idr.h.
> 
Indeed.
>> I think this needs patching independently.
> 
> Yes, sure. And perhaps send it for quick inclusion before your patches?
> 

I agree.