lkml.org 
[lkml]   [2010]   [Jul]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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



    \
     
     \ /
      Last update: 2010-07-14 16:49    [from the cache]
    ©2003-2014 Jasper Spaans. hosted at Digital Ocean