On Tue, 09 Oct 2007 16:39:48 +0530
Balbir Singh <[EMAIL PROTECTED]> wrote:
> > +static inline int
> > +page_cgroup_assign_new_page_cgroup(struct page *page, struct page_cgroup
> > *pc)
> > +{
> > + int ret = 0;
> > +
> > + lock_page_cgroup(page);
> > + if (!page_get_page_cgroup(page))
> > + page_assign_page_cgroup(page, pc);
> > + else
> > + ret = 1;
> > + unlock_page_cgroup(page);
> > + return ret;
> > +}
> > +
>
> Some comment on when the assignment can fail, for example if page
> already has a page_cgroup associated with it, would be nice.
>
Sure ,will add.
> > +
> > +static inline struct page_cgroup *
> > +clear_page_cgroup(struct page *page, struct page_cgroup *pc)
> > +{
> > + struct page_cgroup *ret;
> > + /* lock and clear */
> > + lock_page_cgroup(page);
> > + ret = page_get_page_cgroup(page);
> > + if (likely(ret == pc))
> > + page_assign_page_cgroup(page, NULL);
> > + unlock_page_cgroup(page);
> > + return ret;
> > +}
> > +
>
> We could add a comment stating that clearing would fail if the page's
> cgroup is not pc
>
will add, too.
> > +
> > static void __mem_cgroup_move_lists(struct page_cgroup *pc, bool active)
> > {
> > if (active)
> > @@ -260,7 +289,7 @@ int mem_cgroup_charge(struct page *page,
> > gfp_t gfp_mask)
> > {
> > struct mem_cgroup *mem;
> > - struct page_cgroup *pc, *race_pc;
> > + struct page_cgroup *pc;
> > unsigned long flags;
> > unsigned long nr_retries = MEM_CGROUP_RECLAIM_RETRIES;
> >
> > @@ -353,24 +382,16 @@ noreclaim:
> > 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);
> > - res_counter_uncharge(&mem->res, PAGE_SIZE);
> > - css_put(&mem->css);
> > - goto done;
> > - }
> > -
> > atomic_set(&pc->ref_cnt, 1);
> > pc->mem_cgroup = mem;
> > pc->page = page;
> > - page_assign_page_cgroup(page, pc);
> > + if (page_cgroup_assign_new_page_cgroup(page, pc)) {
> > + /* race ... undo and retry */
> > + res_counter_uncharge(&mem->res, PAGE_SIZE);
> > + css_put(&mem->css);
> > + kfree(pc);
> > + goto retry;
>
> This part is a bit confusing, why do we want to retry. If someone
> else charged the page already, we just continue, we let the other
> task take the charge and add this page to it's cgroup
>
Okay. will add precise text.
> > + }
> >
> > spin_lock_irqsave(&mem->lru_lock, flags);
> > list_add(&pc->lru, &mem->active_list);
> > @@ -421,17 +442,18 @@ void mem_cgroup_uncharge(struct page_cgr
> >
> > if (atomic_dec_and_test(&pc->ref_cnt)) {
> > page = pc->page;
> > - lock_page_cgroup(page);
> > - mem = pc->mem_cgroup;
> > - css_put(&mem->css);
> > - page_assign_page_cgroup(page, NULL);
> > - unlock_page_cgroup(page);
> > - res_counter_uncharge(&mem->res, PAGE_SIZE);
> > -
> > - spin_lock_irqsave(&mem->lru_lock, flags);
> > - list_del_init(&pc->lru);
> > - spin_unlock_irqrestore(&mem->lru_lock, flags);
> > - kfree(pc);
> > + /*
> > + * Obetaion page->cgroup and clear it under lock.
> ^^^^^^^^
> Not sure if I've come across this word before
Sorry (>_<;
Get page->cgroup and clear it under lock.
>
> > + */
> > + if (clear_page_cgroup(page, pc) == pc) {
>
> OK.. so we've come so far and seen that pc has changed underneath us,
> what do we do with this pc?
>
Hmm... How about this ?
==
if (clear_page_cgroup(page, pc) == pc) {
/* do usual work */
} else {
BUG();
}
== or BUG_ON(clear_page_cgroup(page, pc) != pc)
I have no clear idea when this race will occur.
But this "lock and clear" behavior is sane, I think.
Thanks,
-kame
_______________________________________________
Containers mailing list
[EMAIL PROTECTED]
https://lists.linux-foundation.org/mailman/listinfo/containers
_______________________________________________
Devel mailing list
[email protected]
https://openvz.org/mailman/listinfo/devel
