On Tue, 11 May 2010 08:54:46 +0900, KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx> wrote:
> On Mon, 10 May 2010 15:25:54 -0700
> Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> > On Thu, 8 Apr 2010 14:10:20 +0900
> > Daisuke Nishimura <nishimura@xxxxxxxxxxxxxxxxx> wrote:
> >
> > > This patch cleans up move charge code by:
> > >
> > > - define functions to handle pte for each types, and make is_target_pte_for_mc()
> > > cleaner.
> > > - instead of checking the MOVE_CHARGE_TYPE_ANON bit, define a function that
> > > checks the bit.
> > >
> > > ...
> > >
> >
> > > @@ -4241,13 +4263,15 @@ static int is_target_pte_for_mc(struct vm_area_struct *vma,
> > > if (!ret || !target)
> > > put_page(page);
> > > }
> > > - /* throught */
> > > - if (ent.val && do_swap_account && !ret &&
> > > - css_id(&mc.from->css) == lookup_swap_cgroup(ent)) {
> > > - ret = MC_TARGET_SWAP;
> > > - if (target)
> > > - target->ent = ent;
> > > + /* Threre is a swap entry and a page doesn't exist or isn't charged */
> > > + if (ent.val && !ret) {
> > > + if (css_id(&mc.from->css) == lookup_swap_cgroup(ent)) {
> > > + ret = MC_TARGET_SWAP;
> > > + if (target)
> > > + target->ent = ent;
> > > + }
> > > }
> > > +
> > > return ret;
> > > }
> >
> > Are you sure that the test of do_swap_account should be removed here?
> > it didn't seem to be covered in the changelog.
> >
> Hmmm...thank you for pointing out. I think it should be checked.
>
> Nishimura-san ?
>
mc_handle_swap_pte() will set ent.val only when do_swap_account,
so it's all right to remove the check here.
(snip)
> > +static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
> > + unsigned long addr, pte_t ptent, swp_entry_t *entry)
> > +{
> > + int usage_count;
> > + struct page *page = NULL;
> > + swp_entry_t ent = pte_to_swp_entry(ptent);
> > +
> > + if (!move_anon() || non_swap_entry(ent))
> > + return NULL;
> > + usage_count = mem_cgroup_count_swap_user(ent, &page);
> > + if (usage_count > 1) { /* we don't move shared anon */
> > if (page)
> > put_page(page);
> > - return 0;
> > + return NULL;
> > }
> > + if (do_swap_account)
> > + entry->val = ent.val;
>
> Maybe page should be set to NULL here. if !do_swap_account....
>
I leave the "page"(it may store a page in swapcache) as it is intentionally
to move a charge of an unmapped swapcache.
Thanks,
Daisuke Nishimura.
--
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/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>