On Tue, Dec 7, 2010 at 4:33 AM, Mel Gorman <mel@xxxxxxxxx> wrote:
> On Mon, Nov 29, 2010 at 10:49:42PM -0800, Ying Han wrote:
>> There is a kswapd kernel thread for each memory node. We add a different kswapd
>> for each cgroup.
>
> What is considered a normal number of cgroups in production? 10, 50, 10000?
Normally it is less than 100. I assume there is a cap of number of
cgroups can be created
per system.
If it's a really large number and all the cgroups kswapds wake at the same time,
> the zone LRU lock will be very heavily contended.
Thanks for reviewing the patch~
Agree. The zone->lru_lock is another thing we are looking at.
Eventually, we need to break the lock to
per-zone per-memcg lru.
Potentially there will
> also be a very large number of new IO sources. I confess I haven't read the
> thread yet so maybe this has already been thought of but it might make sense
> to have a 1:N relationship between kswapd and memcgroups and cycle between
> containers. The difficulty will be a latency between when kswapd wakes up
> and when a particular container is scanned. The closer the ratio is to 1:1,
> the less the latency will be but the higher the contenion on the LRU lock
> and IO will be.
No, we weren't talked about the mapping anywhere in the thread. Having
many kswapd threads
at the same time isn't a problem as long as no locking contention (
ext, 1k kswapd threads on
1k fake numa node system). So breaking the zone->lru_lock should work.
>
>> The kswapd is sleeping in the wait queue headed at kswapd_wait
>> field of a kswapd descriptor. The kswapd descriptor stores information of node
>> or cgroup and it allows the global and per cgroup background reclaim to share
>> common reclaim algorithms.
>>
>> This patch addes the kswapd descriptor and changes per zone kswapd_wait to the
>> common data structure.
>>
>> Signed-off-by: Ying Han <yinghan@xxxxxxxxxx>
>> ---
>> include/linux/mmzone.h | 3 +-
>> include/linux/swap.h | 10 +++++
>> mm/memcontrol.c | 2 +
>> mm/mmzone.c | 2 +-
>> mm/page_alloc.c | 9 +++-
>> mm/vmscan.c | 98 +++++++++++++++++++++++++++++++++--------------
>> 6 files changed, 90 insertions(+), 34 deletions(-)
>>
>> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
>> index 39c24eb..c77dfa2 100644
>> --- a/include/linux/mmzone.h
>> +++ b/include/linux/mmzone.h
>> @@ -642,8 +642,7 @@ typedef struct pglist_data {
>> unsigned long node_spanned_pages; /* total size of physical page
>> range, including holes */
>> int node_id;
>> - wait_queue_head_t kswapd_wait;
>> - struct task_struct *kswapd;
>> + wait_queue_head_t *kswapd_wait;
>> int kswapd_max_order;
>> } pg_data_t;
>>
>> diff --git a/include/linux/swap.h b/include/linux/swap.h
>> index eba53e7..2e6cb58 100644
>> --- a/include/linux/swap.h
>> +++ b/include/linux/swap.h
>> @@ -26,6 +26,16 @@ static inline int current_is_kswapd(void)
>> return current->flags & PF_KSWAPD;
>> }
>>
>> +struct kswapd {
>> + struct task_struct *kswapd_task;
>> + wait_queue_head_t kswapd_wait;
>> + struct mem_cgroup *kswapd_mem;
>> + pg_data_t *kswapd_pgdat;
>> +};
>> +
>> +#define MAX_KSWAPDS MAX_NUMNODES
>> +extern struct kswapd kswapds[MAX_KSWAPDS];
>
> This is potentially very large for a static structure. Can they not be
> dynamically allocated and kept on a list? Yes, there will be a list walk
> involved if yonu need a particular structure but that looks like it's a
> rare operation at this point.
>
>> +int kswapd(void *p);
>> /*
>> * MAX_SWAPFILES defines the maximum number of swaptypes: things which can
>> * be swapped to. The swap type and the offset into that swap type are
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index a4034b6..dca3590 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -263,6 +263,8 @@ struct mem_cgroup {
>> */
>> struct mem_cgroup_stat_cpu nocpu_base;
>> spinlock_t pcp_counter_lock;
>> +
>> + wait_queue_head_t *kswapd_wait;
>> };
>>
>> /* Stuffs for move charges at task migration. */
>> diff --git a/mm/mmzone.c b/mm/mmzone.c
>> index e35bfb8..c7cbed5 100644
>> --- a/mm/mmzone.c
>> +++ b/mm/mmzone.c
>> @@ -102,7 +102,7 @@ unsigned long zone_nr_free_pages(struct zone *zone)
>> * free pages are low, get a better estimate for free pages
>> */
>> if (nr_free_pages < zone->percpu_drift_mark &&
>> - !waitqueue_active(&zone->zone_pgdat->kswapd_wait))
>> + !waitqueue_active(zone->zone_pgdat->kswapd_wait))
>> return zone_page_state_snapshot(zone, NR_FREE_PAGES);
>>
>> return nr_free_pages;
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index b48dea2..a15bc1c 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -4070,13 +4070,18 @@ static void __paginginit free_area_init_core(struct pglist_data *pgdat,
>> int nid = pgdat->node_id;
>> unsigned long zone_start_pfn = pgdat->node_start_pfn;
>> int ret;
>> + struct kswapd *kswapd_p;
>>
>> pgdat_resize_init(pgdat);
>> pgdat->nr_zones = 0;
>> - init_waitqueue_head(&pgdat->kswapd_wait);
>> pgdat->kswapd_max_order = 0;
>> pgdat_page_cgroup_init(pgdat);
>> -
>> +
>> + kswapd_p = &kswapds[nid];
>> + init_waitqueue_head(&kswapd_p->kswapd_wait);
>> + pgdat->kswapd_wait = &kswapd_p->kswapd_wait;
>> + kswapd_p->kswapd_pgdat = pgdat;
>> +
>> for (j = 0; j < MAX_NR_ZONES; j++) {
>> struct zone *zone = pgdat->node_zones + j;
>> unsigned long size, realsize, memmap_pages;
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index b8a6fdc..e08005e 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -2115,12 +2115,18 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *mem_cont,
>>
>> return nr_reclaimed;
>> }
>> +
>> #endif
>>
>
> Unnecessary whitespace there.
>
>> +DEFINE_SPINLOCK(kswapds_spinlock);
>> +struct kswapd kswapds[MAX_KSWAPDS];
>> +
>> /* is kswapd sleeping prematurely? */
>> -static int sleeping_prematurely(pg_data_t *pgdat, int order, long remaining)
>> +static int sleeping_prematurely(struct kswapd *kswapd, int order,
>> + long remaining)
>> {
>> int i;
>> + pg_data_t *pgdat = kswapd->kswapd_pgdat;
>>
>
> This will behave strangely. You are using information from a *node* to
> determine if the kswapd belonging to a cgroup should sleep or not.
The
> risk is that a cgroup kswapd never goes to sleep because even when all
> of its pages are discarded, the node itself is still not balanced.
The kswapd descriptor is one per-node and one per-cgroup. I believe I have the
logic on the later patch to separate them out. And per-cgroup kswapd is using
the wmark calculated based on the limits. Like this:
static int sleeping_prematurely(struct kswapd *kswapd, int order,
>------->------->------->-------long remaining)
>-------if (mem) {
>------->-------if (!mem_cgroup_watermark_ok(mem, CHARGE_WMARK_HIGH))
>------->------->-------return 1;
>------->-------return 0;
>-------}
>
>> /* If a direct reclaimer woke kswapd within HZ/10, it's premature */
>> if (remaining)
>> @@ -2377,21 +2383,28 @@ out:
>> * If there are applications that are active memory-allocators
>> * (most normal use), this basically shouldn't matter.
>> */
>> -static int kswapd(void *p)
>> +int kswapd(void *p)
>> {
>> unsigned long order;
>> - pg_data_t *pgdat = (pg_data_t*)p;
>> + struct kswapd *kswapd_p = (struct kswapd *)p;
>> + pg_data_t *pgdat = kswapd_p->kswapd_pgdat;
>> + struct mem_cgroup *mem = kswapd_p->kswapd_mem;
>> + wait_queue_head_t *wait_h = &kswapd_p->kswapd_wait;
>> struct task_struct *tsk = current;
>> DEFINE_WAIT(wait);
>> struct reclaim_state reclaim_state = {
>> .reclaimed_slab = 0,
>> };
>> - const struct cpumask *cpumask = cpumask_of_node(pgdat->node_id);
>> + const struct cpumask *cpumask;
>>
>> lockdep_set_current_reclaim_state(GFP_KERNEL);
>>
>> - if (!cpumask_empty(cpumask))
>> - set_cpus_allowed_ptr(tsk, cpumask);
>> + if (pgdat) {
>> + BUG_ON(pgdat->kswapd_wait != wait_h);
>> + cpumask = cpumask_of_node(pgdat->node_id);
>> + if (!cpumask_empty(cpumask))
>> + set_cpus_allowed_ptr(tsk, cpumask);
>> + }
>> current->reclaim_state = &reclaim_state;
>>
>> /*
>> @@ -2414,9 +2427,13 @@ static int kswapd(void *p)
>> unsigned long new_order;
>> int ret;
>>
>> - prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
>> - new_order = pgdat->kswapd_max_order;
>> - pgdat->kswapd_max_order = 0;
>> + prepare_to_wait(wait_h, &wait, TASK_INTERRUPTIBLE);
>> + if (pgdat) {
>> + new_order = pgdat->kswapd_max_order;
>> + pgdat->kswapd_max_order = 0;
>> + } else
>> + new_order = 0;
>> +
>> if (order < new_order) {
>> /*
>> * Don't sleep if someone wants a larger 'order'
>> @@ -2428,10 +2445,12 @@ static int kswapd(void *p)
>> long remaining = 0;
>>
>> /* Try to sleep for a short interval */
>> - if (!sleeping_prematurely(pgdat, order, remaining)) {
>> + if (!sleeping_prematurely(kswapd_p, order,
>> + remaining)) {
>> remaining = schedule_timeout(HZ/10);
>> - finish_wait(&pgdat->kswapd_wait, &wait);
>> - prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
>> + finish_wait(wait_h, &wait);
>> + prepare_to_wait(wait_h, &wait,
>> + TASK_INTERRUPTIBLE);
>
> It would be nice if patch 1 did nothing but move the wait queue outside of
> the node structure without any other functional change. It'll be then be
> far easier to review a patch that introduces background reclaim for containers.
I will see if to splitting this patch into two.
--Ying
>
>> }
>>
>> /*
>> @@ -2439,20 +2458,25 @@ static int kswapd(void *p)
>> * premature sleep. If not, then go fully
>> * to sleep until explicitly woken up
>> */
>> - if (!sleeping_prematurely(pgdat, order, remaining)) {
>> - trace_mm_vmscan_kswapd_sleep(pgdat->node_id);
>> + if (!sleeping_prematurely(kswapd_p, order,
>> + remaining)) {
>> + if (pgdat)
>> + trace_mm_vmscan_kswapd_sleep(
>> + pgdat->node_id);
>> schedule();
>> } else {
>> if (remaining)
>> - count_vm_event(KSWAPD_LOW_WMARK_HIT_QUICKLY);
>> + count_vm_event(
>> + KSWAPD_LOW_WMARK_HIT_QUICKLY);
>> else
>> - count_vm_event(KSWAPD_HIGH_WMARK_HIT_QUICKLY);
>> + count_vm_event(
>> + KSWAPD_HIGH_WMARK_HIT_QUICKLY);
>> }
>> }
>> -
>> - order = pgdat->kswapd_max_order;
>> + if (pgdat)
>> + order = pgdat->kswapd_max_order;
>> }
>> - finish_wait(&pgdat->kswapd_wait, &wait);
>> + finish_wait(wait_h, &wait);
>>
>> ret = try_to_freeze();
>> if (kthread_should_stop())
>> @@ -2476,6 +2500,7 @@ static int kswapd(void *p)
>> void wakeup_kswapd(struct zone *zone, int order)
>> {
>> pg_data_t *pgdat;
>> + wait_queue_head_t *wait;
>>
>> if (!populated_zone(zone))
>> return;
>> @@ -2488,9 +2513,10 @@ void wakeup_kswapd(struct zone *zone, int order)
>> trace_mm_vmscan_wakeup_kswapd(pgdat->node_id, zone_idx(zone), order);
>> if (!cpuset_zone_allowed_hardwall(zone, GFP_KERNEL))
>> return;
>> - if (!waitqueue_active(&pgdat->kswapd_wait))
>> + wait = pgdat->kswapd_wait;
>> + if (!waitqueue_active(wait))
>> return;
>> - wake_up_interruptible(&pgdat->kswapd_wait);
>> + wake_up_interruptible(wait);
>> }
>>
>> /*
>> @@ -2587,7 +2613,10 @@ static int __devinit cpu_callback(struct notifier_block *nfb,
>>
>> if (cpumask_any_and(cpu_online_mask, mask) < nr_cpu_ids)
>> /* One of our CPUs online: restore mask */
>> - set_cpus_allowed_ptr(pgdat->kswapd, mask);
>> + if (kswapds[nid].kswapd_task)
>> + set_cpus_allowed_ptr(
>> + kswapds[nid].kswapd_task,
>> + mask);
>> }
>> }
>> return NOTIFY_OK;
>> @@ -2599,19 +2628,20 @@ static int __devinit cpu_callback(struct notifier_block *nfb,
>> */
>> int kswapd_run(int nid)
>> {
>> - pg_data_t *pgdat = NODE_DATA(nid);
>> + struct task_struct *thr;
>> int ret = 0;
>>
>> - if (pgdat->kswapd)
>> + if (kswapds[nid].kswapd_task)
>> return 0;
>>
>> - pgdat->kswapd = kthread_run(kswapd, pgdat, "kswapd%d", nid);
>> - if (IS_ERR(pgdat->kswapd)) {
>> + thr = kthread_run(kswapd, &kswapds[nid], "kswapd%d", nid);
>> + if (IS_ERR(thr)) {
>> /* failure at boot is fatal */
>> BUG_ON(system_state == SYSTEM_BOOTING);
>> printk("Failed to start kswapd on node %d\n",nid);
>> ret = -1;
>> }
>> + kswapds[nid].kswapd_task = thr;
>> return ret;
>> }
>>
>> @@ -2620,10 +2650,20 @@ int kswapd_run(int nid)
>> */
>> void kswapd_stop(int nid)
>> {
>> - struct task_struct *kswapd = NODE_DATA(nid)->kswapd;
>> + struct task_struct *thr;
>> + struct kswapd *kswapd_p;
>> + wait_queue_head_t *wait;
>> +
>> + pg_data_t *pgdat = NODE_DATA(nid);
>> +
>> + spin_lock(&kswapds_spinlock);
>> + wait = pgdat->kswapd_wait;
>> + kswapd_p = container_of(wait, struct kswapd, kswapd_wait);
>> + thr = kswapd_p->kswapd_task;
>> + spin_unlock(&kswapds_spinlock);
>>
>> - if (kswapd)
>> - kthread_stop(kswapd);
>> + if (thr)
>> + kthread_stop(thr);
>> }
>>
>> static int __init kswapd_init(void)
>> --
>> 1.7.3.1
>>
>
> --
> Mel Gorman
> Part-time Phd Student Linux Technology Center
> University of Limerick IBM Dublin Software Lab
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href