Messages in this thread Patch in this message | | | Date | Sun, 30 Nov 2008 23:04:49 +0900 | From | "KOSAKI Motohiro" <> | Subject | Re: [PATCH 02/09] memcg: make inactive_anon_is_low() |
| |
>> make inactive_anon_is_low for memcgroup. >> it improve active_anon vs inactive_anon ratio balancing. > > The subject line of this patch seems to be truncated and the changelog > seems bit terse. While the change may be obvious to memcg developers, > it's not for the casual reader.
Yes, I'm wrong. Will fix.
>> +static inline int >> +mem_cgroup_inactive_anon_is_low(struct mem_cgroup *memcg, struct zone *zone) >> +{ >> + return 1; >> +} >> + >> + > > An extra newline here.
Will fix.
=================================================================== >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -156,6 +156,9 @@ struct mem_cgroup { >> unsigned long last_oom_jiffies; >> int obsolete; >> atomic_t refcnt; >> + >> + int inactive_ratio; >> + > > Is there a reason why this is not unsigned long? A comment here > explaining what ->inactive_ratio is used for would be nice.
Ah sorry. the type of zone->inactive_ratio is unsigned int.
Then, I'd like to change it to unsigned int. because difference of the global reclaim easily cause silly mistake and bug.
>> +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; >> + ratio = int_sqrt(10 * gb); > > You might want to consider adding a comment explaining what the above > calculation is supposed to be doing.
Yes, Of cource. Thanks.
>> static DEFINE_MUTEX(set_limit_mutex); >> >> static int mem_cgroup_resize_limit(struct mem_cgroup *memcg, >> @@ -1381,6 +1411,11 @@ static int mem_cgroup_resize_limit(struc >> GFP_HIGHUSER_MOVABLE, false); >> if (!progress) retry_count--; >> } >> + >> + if (!ret) >> + mem_cgroup_set_inactive_ratio(memcg); >> + >> + > > An extra newline here.
Will fix.
>> @@ -1423,6 +1458,7 @@ int mem_cgroup_resize_memsw_limit(struct >> if (curusage >= oldusage) >> retry_count--; >> } >> + >> return ret; >> } > > There's some diff noise here.
ditto. thanks.
| |