Messages in this thread | | | Date | Wed, 14 Jul 2010 10:46:31 -0400 | From | Munehiro IKEDA <> | Subject | Re: [RFC][PATCH 02/11] blkiocg async: The main part of iotrack |
| |
KAMEZAWA Hiroyuki wrote: > On Fri, 09 Jul 2010 19:06:28 -0400 > Munehiro Ikeda <m-ikeda@ds.jp.nec.com> wrote: > >> OK, we can do it like: >> >> do { >> old = pc->flags; >> new = old & ((1UL << PAGE_TRACKING_ID_SHIFT) - 1); >> new |= (unsigned long)(id << PAGE_TRACKING_ID_SHIFT); >> } while (cmpxchg(&pc->flags, old, new) != old); >> >> >>> IIUC, there is an generic version now even if it's heavy. >> I couldn't find it out...is there already? Or you mean we should >> have generic one? >> > > generic cmpxchg in /include/asm-generic/cmpxchg.h > But...ahh...in some arch, you can't use cmpxchg, sorry. > > How about start from "a new field" for I/O cgroup in page_cgroup ? > > struct page_cgroup { > unsigned long flags; > struct mem_cgroup *mem_cgroup; > unsigned short blkio_cgroup_id; > struct page *page; > struct list_head lru; /* per cgroup LRU list */ > }; > > We can consider how we optimize it out, later. > (And, it's easier to debug and make development smooth.) > > For example, If we can create a vmalloced-array of mem_cgroup, > id->mem_cgroup lookup can be very fast and we can replace > pc->mem_cgroup link with pc->mem_cgroup_id.
Nice suggestion. Thanks Kame-san.
>>>> +unsigned long page_cgroup_get_owner(struct page *page); >>>> +int page_cgroup_set_owner(struct page *page, unsigned long id); >>>> +int page_cgroup_copy_owner(struct page *npage, struct page *opage); >>>> + >>>> void __meminit pgdat_page_cgroup_init(struct pglist_data *pgdat); >>>> >>>> #ifdef CONFIG_SPARSEMEM >>>> diff --git a/init/Kconfig b/init/Kconfig >>>> index 2e40f2f..337ee01 100644 >>>> --- a/init/Kconfig >>>> +++ b/init/Kconfig >>>> @@ -650,7 +650,7 @@ endif # CGROUPS >>>> >>>> config CGROUP_PAGE >>>> def_bool y >>>> - depends on CGROUP_MEM_RES_CTLR >>>> + depends on CGROUP_MEM_RES_CTLR || GROUP_IOSCHED_ASYNC >>>> >>>> config MM_OWNER >>>> bool >>>> diff --git a/mm/page_cgroup.c b/mm/page_cgroup.c >>>> index 6c00814..69e080c 100644 >>>> --- a/mm/page_cgroup.c >>>> +++ b/mm/page_cgroup.c >>>> @@ -9,6 +9,7 @@ >>>> #include<linux/vmalloc.h> >>>> #include<linux/cgroup.h> >>>> #include<linux/swapops.h> >>>> +#include<linux/blk-iotrack.h> >>>> >>>> static void __meminit >>>> __init_page_cgroup(struct page_cgroup *pc, unsigned long pfn) >>>> @@ -74,7 +75,7 @@ void __init page_cgroup_init_flatmem(void) >>>> >>>> int nid, fail; >>>> >>>> - if (mem_cgroup_disabled()) >>>> + if (mem_cgroup_disabled()&& blk_iotrack_disabled()) >>>> return; >>>> >>>> for_each_online_node(nid) { >>>> @@ -83,12 +84,13 @@ void __init page_cgroup_init_flatmem(void) >>>> goto fail; >>>> } >>>> printk(KERN_INFO "allocated %ld bytes of page_cgroup\n", total_usage); >>>> - printk(KERN_INFO "please try 'cgroup_disable=memory' option if you" >>>> - " don't want memory cgroups\n"); >>>> + printk(KERN_INFO "please try 'cgroup_disable=memory,blkio' option" >>>> + " if you don't want memory and blkio cgroups\n"); >>>> return; >>>> fail: >>>> printk(KERN_CRIT "allocation of page_cgroup failed.\n"); >>>> - printk(KERN_CRIT "please try 'cgroup_disable=memory' boot option\n"); >>>> + printk(KERN_CRIT >>>> + "please try 'cgroup_disable=memory,blkio' boot option\n"); >>>> panic("Out of memory"); >>>> } >>> Hmm, io-track is always done if blkio-cgroup is used, Right ? >> No, iotrack is enabled only when CONFIG_GROUP_IOSCHED_ASYNC=y. >> If =n, iotrack is disabled even if blkio-cgroup is enabled. >> > > >>> Then, why you have extra config as CONFIG_GROUP_IOSCHED_ASYNC ? >>> Is it necessary ? >> Current purpose of the option is only for debug because it is >> experimental functionality. >> It can be removed if this work is completed, or dynamic switch >> might be useful. >> >> In fact, just "cgroup_disable=memory" is enough for the failure >> case. Let me think right messages. >> > > IMHO, once you add boot-option or sysctl, it's very hard to remove it, lator. > So, if you think you'll remove it lator, don't add it or just add CONFIG.
OK. I understand we need to seriously think over to add a new boot option.
Thanks, Muuhh
-- IKEDA, Munehiro NEC Corporation of America m-ikeda@ds.jp.nec.com
| |