* [RFC] [PATCH] Memory controller remove control_type feature
@ 2007-12-20 18:53 Balbir Singh
2007-12-21 0:30 ` KAMEZAWA Hiroyuki
2007-12-22 6:59 ` Andrew Morton
0 siblings, 2 replies; 6+ messages in thread
From: Balbir Singh @ 2007-12-20 18:53 UTC (permalink / raw)
To: Hugh Dickins; +Cc: Balbir Singh, LKML, Andrew Morton, KAMEZAWA Hiroyuki
Based on the discussion at http://lkml.org/lkml/2007/12/20/383, it was
felt that control_type might not be a good thing to implement right away.
We can add this flexibility at a later point when required.
Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
---
include/linux/memcontrol.h | 6 --
mm/memcontrol.c | 91 ++++++++-------------------------------------
2 files changed, 18 insertions(+), 79 deletions(-)
diff -puN mm/memcontrol.c~memory-controller-no-more-control-type mm/memcontrol.c
--- linux-2.6.24-rc5/mm/memcontrol.c~memory-controller-no-more-control-type 2007-12-20 22:37:17.000000000 +0530
+++ linux-2.6.24-rc5-balbir/mm/memcontrol.c 2007-12-20 23:50:47.000000000 +0530
@@ -131,7 +131,6 @@ struct mem_cgroup {
*/
struct mem_cgroup_lru_info info;
- unsigned long control_type; /* control RSS or RSS+Pagecache */
int prev_priority; /* for recording reclaim priority */
/*
* statistics.
@@ -718,24 +717,17 @@ int mem_cgroup_cache_charge(struct page
gfp_t gfp_mask)
{
int ret = 0;
- struct mem_cgroup *mem;
if (!mm)
mm = &init_mm;
- rcu_read_lock();
- mem = rcu_dereference(mm->mem_cgroup);
- css_get(&mem->css);
- rcu_read_unlock();
- if (mem->control_type == MEM_CGROUP_TYPE_ALL)
- ret = mem_cgroup_charge_common(page, mm, gfp_mask,
+ ret = mem_cgroup_charge_common(page, mm, gfp_mask,
MEM_CGROUP_CHARGE_TYPE_CACHE);
- css_put(&mem->css);
return ret;
}
/*
* Uncharging is always a welcome operation, we never complain, simply
- * uncharge.
+ * uncharge. This routine should be called with lock_page_cgroup held
*/
void mem_cgroup_uncharge(struct page_cgroup *pc)
{
@@ -745,8 +737,7 @@ void mem_cgroup_uncharge(struct page_cgr
unsigned long flags;
/*
- * This can handle cases when a page is not charged at all and we
- * are switching between handling the control_type.
+ * Check if our page_cgroup is valid
*/
if (!pc)
return;
@@ -758,6 +749,7 @@ void mem_cgroup_uncharge(struct page_cgr
* get page->cgroup and clear it under lock.
* force_empty can drop page->cgroup without checking refcnt.
*/
+ unlock_page_cgroup(page);
if (clear_page_cgroup(page, pc) == pc) {
mem = pc->mem_cgroup;
css_put(&mem->css);
@@ -767,9 +759,17 @@ void mem_cgroup_uncharge(struct page_cgr
spin_unlock_irqrestore(&mz->lru_lock, flags);
kfree(pc);
}
+ lock_page_cgroup(page);
}
}
+void mem_cgroup_uncharge_page(struct page *page)
+{
+ lock_page_cgroup(page);
+ mem_cgroup_uncharge(page_get_page_cgroup(page));
+ unlock_page_cgroup(page);
+}
+
/*
* Returns non-zero if a page (under migration) has valid page_cgroup member.
* Refcnt of page_cgroup is incremented.
@@ -789,8 +789,12 @@ int mem_cgroup_prepare_migration(struct
void mem_cgroup_end_migration(struct page *page)
{
- struct page_cgroup *pc = page_get_page_cgroup(page);
+ struct page_cgroup *pc;
+
+ lock_page_cgroup(page);
+ pc = page_get_page_cgroup(page);
mem_cgroup_uncharge(pc);
+ unlock_page_cgroup(page);
}
/*
* We know both *page* and *newpage* are now not-on-LRU and Pg_locked.
@@ -945,61 +949,6 @@ static ssize_t mem_cgroup_write(struct c
mem_cgroup_write_strategy);
}
-static ssize_t mem_control_type_write(struct cgroup *cont,
- struct cftype *cft, struct file *file,
- const char __user *userbuf,
- size_t nbytes, loff_t *pos)
-{
- int ret;
- char *buf, *end;
- unsigned long tmp;
- struct mem_cgroup *mem;
-
- mem = mem_cgroup_from_cont(cont);
- buf = kmalloc(nbytes + 1, GFP_KERNEL);
- ret = -ENOMEM;
- if (buf == NULL)
- goto out;
-
- buf[nbytes] = 0;
- ret = -EFAULT;
- if (copy_from_user(buf, userbuf, nbytes))
- goto out_free;
-
- ret = -EINVAL;
- tmp = simple_strtoul(buf, &end, 10);
- if (*end != '\0')
- goto out_free;
-
- if (tmp <= MEM_CGROUP_TYPE_UNSPEC || tmp >= MEM_CGROUP_TYPE_MAX)
- goto out_free;
-
- mem->control_type = tmp;
- ret = nbytes;
-out_free:
- kfree(buf);
-out:
- return ret;
-}
-
-static ssize_t mem_control_type_read(struct cgroup *cont,
- struct cftype *cft,
- struct file *file, char __user *userbuf,
- size_t nbytes, loff_t *ppos)
-{
- unsigned long val;
- char buf[64], *s;
- struct mem_cgroup *mem;
-
- mem = mem_cgroup_from_cont(cont);
- s = buf;
- val = mem->control_type;
- s += sprintf(s, "%lu\n", val);
- return simple_read_from_buffer((void __user *)userbuf, nbytes,
- ppos, buf, s - buf);
-}
-
-
static ssize_t mem_force_empty_write(struct cgroup *cont,
struct cftype *cft, struct file *file,
const char __user *userbuf,
@@ -1098,11 +1047,6 @@ static struct cftype mem_cgroup_files[]
.read = mem_cgroup_read,
},
{
- .name = "control_type",
- .write = mem_control_type_write,
- .read = mem_control_type_read,
- },
- {
.name = "force_empty",
.write = mem_force_empty_write,
.read = mem_force_empty_read,
@@ -1170,7 +1114,6 @@ mem_cgroup_create(struct cgroup_subsys *
res_counter_init(&mem->res);
- mem->control_type = MEM_CGROUP_TYPE_ALL;
memset(&mem->info, 0, sizeof(mem->info));
for_each_node_state(node, N_POSSIBLE)
diff -puN include/linux/memcontrol.h~memory-controller-no-more-control-type include/linux/memcontrol.h
--- linux-2.6.24-rc5/include/linux/memcontrol.h~memory-controller-no-more-control-type 2007-12-20 22:37:17.000000000 +0530
+++ linux-2.6.24-rc5-balbir/include/linux/memcontrol.h 2007-12-20 22:40:00.000000000 +0530
@@ -35,6 +35,7 @@ extern struct page_cgroup *page_get_page
extern int mem_cgroup_charge(struct page *page, struct mm_struct *mm,
gfp_t gfp_mask);
extern void mem_cgroup_uncharge(struct page_cgroup *pc);
+extern void mem_cgroup_uncharge_page(struct page *page);
extern void mem_cgroup_move_lists(struct page_cgroup *pc, bool active);
extern unsigned long mem_cgroup_isolate_pages(unsigned long nr_to_scan,
struct list_head *dst,
@@ -52,11 +53,6 @@ static inline struct mem_cgroup *mm_cgro
return rcu_dereference(mm->mem_cgroup);
}
-static inline void mem_cgroup_uncharge_page(struct page *page)
-{
- mem_cgroup_uncharge(page_get_page_cgroup(page));
-}
-
extern int mem_cgroup_prepare_migration(struct page *page);
extern void mem_cgroup_end_migration(struct page *page);
extern void mem_cgroup_page_migration(struct page *page, struct page *newpage);
_
--
Warm Regards,
Balbir Singh
Linux Technology Center
IBM, ISTL
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] [PATCH] Memory controller remove control_type feature
2007-12-20 18:53 [RFC] [PATCH] Memory controller remove control_type feature Balbir Singh
@ 2007-12-21 0:30 ` KAMEZAWA Hiroyuki
2007-12-22 6:59 ` Andrew Morton
1 sibling, 0 replies; 6+ messages in thread
From: KAMEZAWA Hiroyuki @ 2007-12-21 0:30 UTC (permalink / raw)
To: Balbir Singh; +Cc: Hugh Dickins, LKML, Andrew Morton
On Fri, 21 Dec 2007 00:23:58 +0530
Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
>
>
> Based on the discussion at http://lkml.org/lkml/2007/12/20/383, it was
> felt that control_type might not be a good thing to implement right away.
> We can add this flexibility at a later point when required.
>
> Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
> ---
>
I agree with this direction.
Thanks,
-Kame
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] [PATCH] Memory controller remove control_type feature
2007-12-20 18:53 [RFC] [PATCH] Memory controller remove control_type feature Balbir Singh
2007-12-21 0:30 ` KAMEZAWA Hiroyuki
@ 2007-12-22 6:59 ` Andrew Morton
2007-12-22 7:32 ` Balbir Singh
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2007-12-22 6:59 UTC (permalink / raw)
To: Balbir Singh; +Cc: Hugh Dickins, LKML, KAMEZAWA Hiroyuki
On Fri, 21 Dec 2007 00:23:58 +0530 Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> Based on the discussion at http://lkml.org/lkml/2007/12/20/383, it was
> felt that control_type might not be a good thing to implement right away.
> We can add this flexibility at a later point when required.
Gee the memory controller patchset is turning into a mess.
memory-controller-add-documentation.patch
memcgroup-temporarily-revert-swapoff-mod.patch
memory-controller-resource-counters-v7.patch
memory-controller-containers-setup-v7.patch
memory-controller-accounting-setup-v7.patch
memory-controller-memory-accounting-v7.patch
memory-controller-memory-accounting-v7-move-page_assign_page_cgroup-to-vm_bug_on-in-free_hot_cold_page.patch
memory-controller-task-migration-v7.patch
memory-controller-add-per-container-lru-and-reclaim-v7.patch
memory-controller-add-per-container-lru-and-reclaim-v7-memcgroup-fix-try_to_free-order.patch
memory-controller-improve-user-interface.patch
memory-controller-improve-user-interface-memory-controller-enhancements-for-reclaiming-take2-possible-race-fix-in-res_counter.patch
memory-controller-oom-handling-v7.patch
memory-controller-add-switch-to-control-what-type-of-pages-to-limit-v7.patch
memory-controller-make-page_referenced-container-aware-v7.patch
memory-controller-make-charging-gfp-mask-aware.patch
memory-controller-make-charging-gfp-mask-aware-fix.patch
memcgroup-reinstate-swapoff-mod.patch
memory-controller-bug_on.patch
mem-controller-gfp-mask-fix.patch
memcontrol-move-mm_cgroup-to-header-file.patch
memcontrol-move-oom-task-exclusion-to-tasklist.patch
oom-add-sysctl-to-enable-task-memory-dump.patch
kswapd-should-only-wait-on-io-if-there-is-io.patch
bugfix-for-memory-cgroup-controller-charge-refcnt-race-fix.patch
bugfix-for-memory-cgroup-controller-fix-error-handling-path-in-mem_charge_cgroup.patch
bugfix-for-memory-controller-add-helper-function-for-assigning-cgroup-to-page.patch
bugfix-for-memory-cgroup-controller-avoid-pagelru-page-in-mem_cgroup_isolate_pages.patch
bugfix-for-memory-cgroup-controller-avoid-pagelru-page-in-mem_cgroup_isolate_pages-fix.patch
memcgroup-fix-zone-isolation-oom.patch
memcgroup-revert-swap_state-mods.patch
bugfix-for-memory-cgroup-controller-migration-under-memory-controller-fix.patch
#
memory-cgroup-enhancements-fix-zone-handling-in-try_to_free_mem_cgroup_page.patch
memory-cgroup-enhancements-fix-zone-handling-in-try_to_free_mem_cgroup_page-warning-fix.patch
memory-cgroup-enhancements-force_empty-interface-for-dropping-all-account-in-empty-cgroup.patch
memory-cgroup-enhancements-remember-a-page-is-charged-as-page-cache.patch
memory-controller-use-rcu_read_lock-in-mem_cgroup_cache_charge.patch
memcgroup-tidy-up-mem_cgroup_charge_common.patch
memcgroup-fix-hang-with-shmem-tmpfs.patch
memory-cgroup-enhancements-remember-a-page-is-on-active-list-of-cgroup-or-not.patch
memory-cgroup-enhancements-add-status-accounting-function-for-memory-cgroup.patch
memory-cgroup-enhancements-add-status-accounting-function-for-memory-cgroup-checkpatch-fixes.patch
memory-cgroup-enhancements-add-status-accounting-function-for-memory-cgroup-fix-1.patch
memory-cgroup-enhancements-add-status-accounting-function-for-memory-cgroup-uninlining.patch
memory-cgroup-enhancements-add-status-accounting-function-for-memory-cgroup-fix-2.patch
memory-cgroup-enhancements-add-memorystat-file.patch
memory-cgroup-enhancements-add-memorystat-file-checkpatch-fixes.patch
memory-cgroup-enhancements-add-memorystat-file-printk-fix.patch
memory-cgroup-enhancements-add-pre_destroy-handler.patch
memory-cgroup-enhancements-implicit-force_empty-at-rmdir.patch
#
per-zone-and-reclaim-enhancements-for-memory-controller-take-3-add-scan_global_lru-macro.patch
per-zone-and-reclaim-enhancements-for-memory-controller-take-3-nid-zid-helper-function-for-cgroup.patch
per-zone-and-reclaim-enhancements-for-memory-controller-take-3-per-zone-active-inactive-counter.patch
per-zone-and-reclaim-enhancements-for-memory-controller-take-3-per-zone-active-inactive-counter-memory-controller-enhancements-for-reclaiming-take2-clean-up-remove-unused-variable.patch
per-zone-and-reclaim-enhancements-for-memory-controller-take-3-per-zone-active-inactive-counter-memory-controller-enhancements-for-reclaiming-take2-add-bug_on-in-mem_cgroup_zoneinfo.patch
per-zone-and-reclaim-enhancements-for-memory-controller-take-3-calculate-mapper_ratio-per-cgroup.patch
per-zone-and-reclaim-enhancements-for-memory-controller-take-3-calculate-active-inactive-imbalance-per-cgroup.patch
per-zone-and-reclaim-enhancements-for-memory-controller-take-3-remember-reclaim-priority-in-memory-cgroup.patch
per-zone-and-reclaim-enhancements-for-memory-controller-take-3-remember-reclaim-priority-in-memory-cgroup-fix.patch
per-zone-and-reclaim-enhancements-for-memory-controller-take-3-remember-reclaim-priority-in-memory-cgroup-fix-2.patch
per-zone-and-reclaim-enhancements-for-memory-controller-take-3-calculate-the-number-of-pages-to-be-scanned-per-cgroup.patch
per-zone-and-reclaim-enhancements-for-memory-controller-take-3-modifies-vmscanc-for-isolate-globa-cgroup-lru-activity.patch
per-zone-and-reclaim-enhancements-for-memory-controller-take-3-modifies-vmscanc-for-isolate-globa-cgroup-lru-activity-fix.patch
per-zone-and-reclaim-enhancements-for-memory-controller-take-3-modifies-vmscanc-for-isolate-globa-cgroup-lru-activity-fix-accounting-in-vmscanc-for-memory-controller.patch
per-zone-and-reclaim-enhancements-for-memory-controller-take-3-per-zone-lru-for-cgroup.patch
per-zone-and-reclaim-enhancements-for-memory-controller-take-3-per-zone-lru-for-cgroup-bugfix-for-memory-cgroup-per-zone-struct-allocation.patch
per-zone-and-reclaim-enhancements-for-memory-controller-take-3-per-zone-lru-for-cgroup-memory-controller-enhancements-for-reclaiming-take2-define-free_mem_cgroup_per_zone_info.patch
per-zone-and-reclaim-enhancements-for-memory-controller-take-3-per-zone-lock-for-cgroup.patch
memory-controller-remove-control_type-feature.patch
update-documentation-controller-memorytxt.patch
Hopefully it'll look a bit better once I do a big patch-folding but we
still have patches interesecting everywhere and now we have patches which
add a feature and later ones which take it away again.
But I don't think it's worth the time and risk of a huge
rip-up-and-refactor.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] [PATCH] Memory controller remove control_type feature
2007-12-22 6:59 ` Andrew Morton
@ 2007-12-22 7:32 ` Balbir Singh
2007-12-22 11:52 ` Hugh Dickins
0 siblings, 1 reply; 6+ messages in thread
From: Balbir Singh @ 2007-12-22 7:32 UTC (permalink / raw)
To: Andrew Morton; +Cc: Hugh Dickins, LKML, KAMEZAWA Hiroyuki
Andrew Morton wrote:
> On Fri, 21 Dec 2007 00:23:58 +0530 Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
>
>> Based on the discussion at http://lkml.org/lkml/2007/12/20/383, it was
>> felt that control_type might not be a good thing to implement right away.
>> We can add this flexibility at a later point when required.
>
> Gee the memory controller patchset is turning into a mess.
>
Yes, the patchset has expanded and we have several useful bug fixes and
cleanups and some new features.
> memory-controller-add-documentation.patch
> memcgroup-temporarily-revert-swapoff-mod.patch
> memory-controller-resource-counters-v7.patch
> memory-controller-containers-setup-v7.patch
> memory-controller-accounting-setup-v7.patch
> memory-controller-memory-accounting-v7.patch
> memory-controller-memory-accounting-v7-move-page_assign_page_cgroup-to-vm_bug_on-in-free_hot_cold_page.patch
> memory-controller-task-migration-v7.patch
> memory-controller-add-per-container-lru-and-reclaim-v7.patch
> memory-controller-add-per-container-lru-and-reclaim-v7-memcgroup-fix-try_to_free-order.patch
> memory-controller-improve-user-interface.patch
> memory-controller-improve-user-interface-memory-controller-enhancements-for-reclaiming-take2-possible-race-fix-in-res_counter.patch
> memory-controller-oom-handling-v7.patch
> memory-controller-add-switch-to-control-what-type-of-pages-to-limit-v7.patch
> memory-controller-make-page_referenced-container-aware-v7.patch
> memory-controller-make-charging-gfp-mask-aware.patch
> memory-controller-make-charging-gfp-mask-aware-fix.patch
> memcgroup-reinstate-swapoff-mod.patch
> memory-controller-bug_on.patch
> mem-controller-gfp-mask-fix.patch
> memcontrol-move-mm_cgroup-to-header-file.patch
> memcontrol-move-oom-task-exclusion-to-tasklist.patch
> oom-add-sysctl-to-enable-task-memory-dump.patch
> kswapd-should-only-wait-on-io-if-there-is-io.patch
> bugfix-for-memory-cgroup-controller-charge-refcnt-race-fix.patch
> bugfix-for-memory-cgroup-controller-fix-error-handling-path-in-mem_charge_cgroup.patch
> bugfix-for-memory-controller-add-helper-function-for-assigning-cgroup-to-page.patch
> bugfix-for-memory-cgroup-controller-avoid-pagelru-page-in-mem_cgroup_isolate_pages.patch
> bugfix-for-memory-cgroup-controller-avoid-pagelru-page-in-mem_cgroup_isolate_pages-fix.patch
> memcgroup-fix-zone-isolation-oom.patch
> memcgroup-revert-swap_state-mods.patch
> bugfix-for-memory-cgroup-controller-migration-under-memory-controller-fix.patch
> #
> memory-cgroup-enhancements-fix-zone-handling-in-try_to_free_mem_cgroup_page.patch
> memory-cgroup-enhancements-fix-zone-handling-in-try_to_free_mem_cgroup_page-warning-fix.patch
> memory-cgroup-enhancements-force_empty-interface-for-dropping-all-account-in-empty-cgroup.patch
> memory-cgroup-enhancements-remember-a-page-is-charged-as-page-cache.patch
> memory-controller-use-rcu_read_lock-in-mem_cgroup_cache_charge.patch
> memcgroup-tidy-up-mem_cgroup_charge_common.patch
> memcgroup-fix-hang-with-shmem-tmpfs.patch
> memory-cgroup-enhancements-remember-a-page-is-on-active-list-of-cgroup-or-not.patch
> memory-cgroup-enhancements-add-status-accounting-function-for-memory-cgroup.patch
> memory-cgroup-enhancements-add-status-accounting-function-for-memory-cgroup-checkpatch-fixes.patch
> memory-cgroup-enhancements-add-status-accounting-function-for-memory-cgroup-fix-1.patch
> memory-cgroup-enhancements-add-status-accounting-function-for-memory-cgroup-uninlining.patch
> memory-cgroup-enhancements-add-status-accounting-function-for-memory-cgroup-fix-2.patch
> memory-cgroup-enhancements-add-memorystat-file.patch
> memory-cgroup-enhancements-add-memorystat-file-checkpatch-fixes.patch
> memory-cgroup-enhancements-add-memorystat-file-printk-fix.patch
> memory-cgroup-enhancements-add-pre_destroy-handler.patch
> memory-cgroup-enhancements-implicit-force_empty-at-rmdir.patch
> #
> per-zone-and-reclaim-enhancements-for-memory-controller-take-3-add-scan_global_lru-macro.patch
> per-zone-and-reclaim-enhancements-for-memory-controller-take-3-nid-zid-helper-function-for-cgroup.patch
> per-zone-and-reclaim-enhancements-for-memory-controller-take-3-per-zone-active-inactive-counter.patch
> per-zone-and-reclaim-enhancements-for-memory-controller-take-3-per-zone-active-inactive-counter-memory-controller-enhancements-for-reclaiming-take2-clean-up-remove-unused-variable.patch
> per-zone-and-reclaim-enhancements-for-memory-controller-take-3-per-zone-active-inactive-counter-memory-controller-enhancements-for-reclaiming-take2-add-bug_on-in-mem_cgroup_zoneinfo.patch
> per-zone-and-reclaim-enhancements-for-memory-controller-take-3-calculate-mapper_ratio-per-cgroup.patch
> per-zone-and-reclaim-enhancements-for-memory-controller-take-3-calculate-active-inactive-imbalance-per-cgroup.patch
> per-zone-and-reclaim-enhancements-for-memory-controller-take-3-remember-reclaim-priority-in-memory-cgroup.patch
> per-zone-and-reclaim-enhancements-for-memory-controller-take-3-remember-reclaim-priority-in-memory-cgroup-fix.patch
> per-zone-and-reclaim-enhancements-for-memory-controller-take-3-remember-reclaim-priority-in-memory-cgroup-fix-2.patch
> per-zone-and-reclaim-enhancements-for-memory-controller-take-3-calculate-the-number-of-pages-to-be-scanned-per-cgroup.patch
> per-zone-and-reclaim-enhancements-for-memory-controller-take-3-modifies-vmscanc-for-isolate-globa-cgroup-lru-activity.patch
> per-zone-and-reclaim-enhancements-for-memory-controller-take-3-modifies-vmscanc-for-isolate-globa-cgroup-lru-activity-fix.patch
> per-zone-and-reclaim-enhancements-for-memory-controller-take-3-modifies-vmscanc-for-isolate-globa-cgroup-lru-activity-fix-accounting-in-vmscanc-for-memory-controller.patch
> per-zone-and-reclaim-enhancements-for-memory-controller-take-3-per-zone-lru-for-cgroup.patch
> per-zone-and-reclaim-enhancements-for-memory-controller-take-3-per-zone-lru-for-cgroup-bugfix-for-memory-cgroup-per-zone-struct-allocation.patch
> per-zone-and-reclaim-enhancements-for-memory-controller-take-3-per-zone-lru-for-cgroup-memory-controller-enhancements-for-reclaiming-take2-define-free_mem_cgroup_per_zone_info.patch
> per-zone-and-reclaim-enhancements-for-memory-controller-take-3-per-zone-lock-for-cgroup.patch
> memory-controller-remove-control_type-feature.patch
> update-documentation-controller-memorytxt.patch
>
> Hopefully it'll look a bit better once I do a big patch-folding but we
> still have patches interesecting everywhere and now we have patches which
> add a feature and later ones which take it away again.
>
I think folding will help. I understand your concern w.r.t getting the
correct set of patches with a good changelog.
> But I don't think it's worth the time and risk of a huge
> rip-up-and-refactor.
>
I agree, given the proximity of the new merge window for 2.6.25. We
could review the patches after folding them and see how to consolidate
further in case the patches continue to look messy.
--
Warm Regards,
Balbir Singh
Linux Technology Center
IBM, ISTL
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] [PATCH] Memory controller remove control_type feature
2007-12-22 7:32 ` Balbir Singh
@ 2007-12-22 11:52 ` Hugh Dickins
2007-12-23 13:55 ` Balbir Singh
0 siblings, 1 reply; 6+ messages in thread
From: Hugh Dickins @ 2007-12-22 11:52 UTC (permalink / raw)
To: Balbir Singh; +Cc: Andrew Morton, LKML, KAMEZAWA Hiroyuki
On Sat, 22 Dec 2007, Balbir Singh wrote:
> Andrew Morton wrote:
> > On Fri, 21 Dec 2007 00:23:58 +0530 Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> >
> >> Based on the discussion at http://lkml.org/lkml/2007/12/20/383, it was
> >> felt that control_type might not be a good thing to implement right away.
> >> We can add this flexibility at a later point when required.
Not studied closely, but your patch looks both too much and too
little to me, Balbir.
Too much in that it appears to bundle in some significant little
locking changes without any mention in the commment.
Too little in that it leaves behind lots of junk relating to the
different control_types: the enums, the different kinds of call
that needn't now be different, no change to the various callsites.
Needs more cleanup, I'd say. Of course, that could be yet another
separate patch.
> > Gee the memory controller patchset is turning into a mess.
A mess indeed.
> Yes, the patchset has expanded and we have several useful bug fixes and
> cleanups and some new features.
Hah, a career in politics beckons ;)
> > Hopefully it'll look a bit better once I do a big patch-folding but we
> > still have patches interesecting everywhere and now we have patches which
> > add a feature and later ones which take it away again.
> >
>
> I think folding will help. I understand your concern w.r.t getting the
> correct set of patches with a good changelog.
>
> > But I don't think it's worth the time and risk of a huge
> > rip-up-and-refactor.
> >
>
> I agree, given the proximity of the new merge window for 2.6.25. We
> could review the patches after folding them and see how to consolidate
> further in case the patches continue to look messy.
Personally, I think it could benefit a lot from a rip-up-and-refactor.
But if we're rushing headlong for 2.6.25, yes, I agree it's too late.
And I'm afraid it's not something I can volunteer for at this time.
Hugh
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] [PATCH] Memory controller remove control_type feature
2007-12-22 11:52 ` Hugh Dickins
@ 2007-12-23 13:55 ` Balbir Singh
0 siblings, 0 replies; 6+ messages in thread
From: Balbir Singh @ 2007-12-23 13:55 UTC (permalink / raw)
To: Hugh Dickins; +Cc: Andrew Morton, LKML, KAMEZAWA Hiroyuki
Hugh Dickins wrote:
> On Sat, 22 Dec 2007, Balbir Singh wrote:
>> Andrew Morton wrote:
>>> On Fri, 21 Dec 2007 00:23:58 +0530 Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
>>>
>>>> Based on the discussion at http://lkml.org/lkml/2007/12/20/383, it was
>>>> felt that control_type might not be a good thing to implement right away.
>>>> We can add this flexibility at a later point when required.
>
> Not studied closely, but your patch looks both too much and too
> little to me, Balbir.
>
> Too much in that it appears to bundle in some significant little
> locking changes without any mention in the commment.
>
I can create and send across a new changelog, but what it does is make
the check for !pc under the page_group lock (lock_page_group()). I sent
out a pointer to the URL of our discussion. Yes, I should have been more
willing to write a more detailed changelog.
> Too little in that it leaves behind lots of junk relating to the
> different control_types: the enums, the different kinds of call
> that needn't now be different, no change to the various callsites.
> Needs more cleanup, I'd say. Of course, that could be yet another
> separate patch.
>
Yes, my dilemma, that still remains (Andrew very aptly noted) is that we
have a bunch of patches adding in the feature and then a patch removing
it. I have noted your point, which is correct about cleaning up unused
parameters.
>>> Gee the memory controller patchset is turning into a mess.
>
> A mess indeed.
>
>> Yes, the patchset has expanded and we have several useful bug fixes and
>> cleanups and some new features.
>
> Hah, a career in politics beckons ;)
>
>>> Hopefully it'll look a bit better once I do a big patch-folding but we
>>> still have patches interesecting everywhere and now we have patches which
>>> add a feature and later ones which take it away again.
>>>
>> I think folding will help. I understand your concern w.r.t getting the
>> correct set of patches with a good changelog.
>>
>>> But I don't think it's worth the time and risk of a huge
>>> rip-up-and-refactor.
>>>
>> I agree, given the proximity of the new merge window for 2.6.25. We
>> could review the patches after folding them and see how to consolidate
>> further in case the patches continue to look messy.
>
> Personally, I think it could benefit a lot from a rip-up-and-refactor.
> But if we're rushing headlong for 2.6.25, yes, I agree it's too late.
> And I'm afraid it's not something I can volunteer for at this time.
>
> Hugh
I want to spend as much time as possible testing the memory controller.
--
Warm Regards,
Balbir Singh
Linux Technology Center
IBM, ISTL
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2007-12-23 13:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-12-20 18:53 [RFC] [PATCH] Memory controller remove control_type feature Balbir Singh
2007-12-21 0:30 ` KAMEZAWA Hiroyuki
2007-12-22 6:59 ` Andrew Morton
2007-12-22 7:32 ` Balbir Singh
2007-12-22 11:52 ` Hugh Dickins
2007-12-23 13:55 ` Balbir Singh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox