On Thu, 2007-09-13 at 15:19 +0530, Balbir Singh wrote: > VM_BUG_ON(pc && !locked)
Even better :-)
> >> +/*
> >> + * Charge the memory controller for page usage.
> >> + * Return
> >> + * 0 if the charge was successful
> >> + * < 0 if the cgroup is over its limit
> >> + */
> >> +int mem_cgroup_charge(struct page *page, struct mm_struct *mm)
> >> +{
> >> + struct mem_cgroup *mem;
> >> + struct page_cgroup *pc, *race_pc;
> >> +
> >> + /*
> >> + * Should page_cgroup's go to their own slab?
> >> + * One could optimize the performance of the charging routine
> >> + * by saving a bit in the page_flags and using it as a lock
> >> + * to see if the cgroup page already has a page_cgroup associated
> >> + * with it
> >> + */
> >> + lock_page_cgroup(page);
> >> + pc = page_get_page_cgroup(page);
> >> + /*
> >> + * The page_cgroup exists and the page has already been accounted
> >> + */
> >> + if (pc) {
> >> + atomic_inc(&pc->ref_cnt);
> >> + goto done;
> >> + }
> >> +
> >> + unlock_page_cgroup(page);
> >> +
> >> + pc = kzalloc(sizeof(struct page_cgroup), GFP_KERNEL);
> >> + if (pc == NULL)
> >> + goto err;
> >> +
> >> + rcu_read_lock();
> >> + /*
> >> + * We always charge the cgroup the mm_struct belongs to
> >> + * the mm_struct's mem_cgroup changes on task migration if the
> >> + * thread group leader migrates. It's possible that mm is not
> >> + * set, if so charge the init_mm (happens for pagecache usage).
> >> + */
> >> + if (!mm)
> >> + mm = &init_mm;
> >> +
> >> + mem = rcu_dereference(mm->mem_cgroup);
> >> + /*
> >> + * For every charge from the cgroup, increment reference
> >> + * count
> >> + */
> >> + css_get(&mem->css);
> >> + rcu_read_unlock();
> >> +
> >> + /*
> >> + * If we created the page_cgroup, we should free it on exceeding
> >> + * the cgroup limit.
> >> + */
> >> + if (res_counter_charge(&mem->res, 1)) {
> >> + css_put(&mem->css);
> >> + goto free_pc;
> >> + }
> >> +
> >> + lock_page_cgroup(page);
> >> + /*
> >> + * Check if somebody else beat us to allocating the page_cgroup
> >> + */
> >> + race_pc = page_get_page_cgroup(page);
> >> + if (race_pc) {
> >> + kfree(pc);
> >> + pc = race_pc;
> >> + atomic_inc(&pc->ref_cnt);
> >
> > This inc
> >
> >> + res_counter_uncharge(&mem->res, 1);
> >> + css_put(&mem->css);
> >> + goto done;
> >> + }
> >> +
> >> + atomic_set(&pc->ref_cnt, 1);
> >
> > combined with this set make me wonder...
> >
>
> I am not sure I understand this comment.
Is that inc needed? the pc is already associated with the page and
should thus already have a reference, so this inc would do 1->2, but we
then set it to 1 again. seems like a superfluous operation.
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Containers mailing list [EMAIL PROTECTED] https://lists.linux-foundation.org/mailman/listinfo/containers
_______________________________________________ Devel mailing list [email protected] https://openvz.org/mailman/listinfo/devel
