Re: [PATCH 06/11] memcg: make inactive_anon_is_low()

[Balbir Singh]

* KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx> [2008-12-01 21:15:46]:

> Changelog:
>  v1 -> v2:
>    - add detail patch description
>    - fix coding style in mem_cgroup_set_inactive_ratio()
>    - add comment to mem_cgroup_set_inactive_ratio
>    - remove extra newline
>    - memcg::inactiveratio change type to unsigned int
>

[snip]
 
> +/*
> + * The inactive anon list should be small enough that the VM never has to
> + * do too much work, but large enough that each inactive page has a chance
> + * to be referenced again before it is swapped out.
> + *
> + * this calculation is straightforward porting from
> + * page_alloc.c::setup_per_zone_inactive_ratio().
> + * it describe more detail.
> + */
> +static void mem_cgroup_set_inactive_ratio(struct mem_cgroup *memcg)
> +{
> +	unsigned int gb, ratio;
> +
> +	gb = res_counter_read_u64(&memcg->res, RES_LIMIT) >> 30;
> +	if (gb)
> +		ratio = int_sqrt(10 * gb);

I don't understand where the magic number 10 comes from?

> +	else
> +		ratio = 1;
> +
> +	memcg->inactive_ratio = ratio;
> +
> +}
> +
>  static DEFINE_MUTEX(set_limit_mutex);
> 
>  static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
> @@ -1384,6 +1424,10 @@ static int mem_cgroup_resize_limit(struc
>  				GFP_HIGHUSER_MOVABLE, false);
>    		if (!progress)			retry_count--;
>  	}
> +
> +	if (!ret)
> +		mem_cgroup_set_inactive_ratio(memcg);
> +
>  	return ret;
>  }
> 
> @@ -1968,7 +2012,7 @@ mem_cgroup_create(struct cgroup_subsys *
>  		res_counter_init(&mem->res, NULL);
>  		res_counter_init(&mem->memsw, NULL);
>  	}
> -
> +	mem_cgroup_set_inactive_ratio(mem);
>  	mem->last_scanned_child = NULL;
> 
>  	return &mem->css;
> Index: b/mm/vmscan.c
> ===================================================================
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1369,14 +1369,7 @@ static void shrink_active_list(unsigned 
>  	pagevec_release(&pvec);
>  }
> 
> -/**
> - * inactive_anon_is_low - check if anonymous pages need to be deactivated
> - * @zone: zone to check
> - *
> - * Returns true if the zone does not have enough inactive anon pages,
> - * meaning some active anon pages need to be deactivated.
> - */
> -static int inactive_anon_is_low(struct zone *zone)
> +static int inactive_anon_is_low_global(struct zone *zone)
>  {
>  	unsigned long active, inactive;
> 
> @@ -1389,6 +1382,25 @@ static int inactive_anon_is_low(struct z
>  	return 0;
>  }
> 
> +/**
> + * inactive_anon_is_low - check if anonymous pages need to be deactivated
> + * @zone: zone to check
> + * @sc:   scan control of this context
> + *
> + * Returns true if the zone does not have enough inactive anon pages,
> + * meaning some active anon pages need to be deactivated.
> + */
> +static int inactive_anon_is_low(struct zone *zone, struct scan_control *sc)
> +{
> +	int low;
> +
> +	if (scan_global_lru(sc))
> +		low = inactive_anon_is_low_global(zone);
> +	else
> +		low = mem_cgroup_inactive_anon_is_low(sc->mem_cgroup, zone);
> +	return low;
> +}
> +
>  static unsigned long shrink_list(enum lru_list lru, unsigned long nr_to_scan,
>  	struct zone *zone, struct scan_control *sc, int priority)
>  {
> @@ -1400,7 +1412,7 @@ static unsigned long shrink_list(enum lr
>  	}
> 
>  	if (lru == LRU_ACTIVE_ANON &&
> -	    (!scan_global_lru(sc) || inactive_anon_is_low(zone))) {
> +	    inactive_anon_is_low(zone, sc)) {

Can't we merge the line with the "if" statement

>  		shrink_active_list(nr_to_scan, zone, sc, priority, file);
>  		return 0;
>  	}
> @@ -1565,9 +1577,7 @@ static void shrink_zone(int priority, st
>  	 * Even if we did not try to evict anon pages at all, we want to
>  	 * rebalance the anon lru active/inactive ratio.
>  	 */
> -	if (!scan_global_lru(sc) || inactive_anon_is_low(zone))
> -		shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);
> -	else if (!scan_global_lru(sc))
> +	if (inactive_anon_is_low(zone, sc))
>  		shrink_active_list(SWAP_CLUSTER_MAX, zone, sc, priority, 0);
> 
>  	throttle_vm_writeout(sc->gfp_mask);
> @@ -1863,7 +1873,7 @@ loop_again:
>  			 * Do some background aging of the anon list, to give
>  			 * pages a chance to be referenced before reclaiming.
>  			 */
> -			if (inactive_anon_is_low(zone))
> +			if (inactive_anon_is_low(zone, &sc))
>  				shrink_active_list(SWAP_CLUSTER_MAX, zone,
>  							&sc, priority, 0);
> 
> 
> 

-- 
	Balbir
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/