All of lore.kernel.org
 help / color / mirror / Atom feed
From: Johannes Weiner <hannes@cmpxchg.org>
To: Michal Hocko <mhocko@suse.cz>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Hugh Dickins <hughd@google.com>, Ying Han <yinghan@google.com>
Subject: Re: [PATCH v4] memcg: remove PCG_CACHE page_cgroup flag
Date: Tue, 24 Jan 2012 17:44:49 +0100	[thread overview]
Message-ID: <20120124164449.GH1660@cmpxchg.org> (raw)
In-Reply-To: <20120124160140.GH26289@tiehlicka.suse.cz>

On Tue, Jan 24, 2012 at 05:01:40PM +0100, Michal Hocko wrote:
> On Tue 24-01-12 15:54:11, Johannes Weiner wrote:
> > Hold on, I think this patch is still not complete: end_migration()
> > directly uses __mem_cgroup_uncharge_common() with the FORCE charge
> > type.  This will uncharge all migrated anon pages as cache, when it
> > should decide based on PageAnon(used), which is the page where
> > ->mapping is intact after migration.
> 
> You are right, I've missed that one as well. Anyway
> MEM_CGROUP_CHARGE_TYPE_FORCE is used only in mem_cgroup_end_migration
> these days and it got out of sync with its documentation (used by
> force_empty) quite some time ago (f817ed48). What about something like
> the following on top of the previous patch?
> --- 
> Should be foldet into the previous patch with the updated changelog:
> 
> Mapping of the unused page is not touched during migration (see

used one, not unused.  unused->mapping is globbered during migration.

> page_remove_rmap) so we can rely on it and push the correct charge type
> down to __mem_cgroup_uncharge_common from end_migration. The force flag
> was misleading anyway.

Kinda.  It had the effect of skipping the needless page_mapped() /
PageCgroupMigration() check, we know the unused page is no longer
mapped and cleared the migration flag just a few lines up.  But doing
the checks is no biggie and it's not worth adding another flag just to
skip them.  But I guess this should be mentioned in the changelog.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

WARNING: multiple messages have this Message-ID (diff)
From: Johannes Weiner <hannes@cmpxchg.org>
To: Michal Hocko <mhocko@suse.cz>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Hugh Dickins <hughd@google.com>, Ying Han <yinghan@google.com>
Subject: Re: [PATCH v4] memcg: remove PCG_CACHE page_cgroup flag
Date: Tue, 24 Jan 2012 17:44:49 +0100	[thread overview]
Message-ID: <20120124164449.GH1660@cmpxchg.org> (raw)
In-Reply-To: <20120124160140.GH26289@tiehlicka.suse.cz>

On Tue, Jan 24, 2012 at 05:01:40PM +0100, Michal Hocko wrote:
> On Tue 24-01-12 15:54:11, Johannes Weiner wrote:
> > Hold on, I think this patch is still not complete: end_migration()
> > directly uses __mem_cgroup_uncharge_common() with the FORCE charge
> > type.  This will uncharge all migrated anon pages as cache, when it
> > should decide based on PageAnon(used), which is the page where
> > ->mapping is intact after migration.
> 
> You are right, I've missed that one as well. Anyway
> MEM_CGROUP_CHARGE_TYPE_FORCE is used only in mem_cgroup_end_migration
> these days and it got out of sync with its documentation (used by
> force_empty) quite some time ago (f817ed48). What about something like
> the following on top of the previous patch?
> --- 
> Should be foldet into the previous patch with the updated changelog:
> 
> Mapping of the unused page is not touched during migration (see

used one, not unused.  unused->mapping is globbered during migration.

> page_remove_rmap) so we can rely on it and push the correct charge type
> down to __mem_cgroup_uncharge_common from end_migration. The force flag
> was misleading anyway.

Kinda.  It had the effect of skipping the needless page_mapped() /
PageCgroupMigration() check, we know the unused page is no longer
mapped and cleared the migration flag just a few lines up.  But doing
the checks is no biggie and it's not worth adding another flag just to
skip them.  But I guess this should be mentioned in the changelog.

  reply	other threads:[~2012-01-24 16:45 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-01-19  9:17 [PATCH] memcg: remove PCG_CACHE page_cgroup flag KAMEZAWA Hiroyuki
2012-01-19  9:17 ` KAMEZAWA Hiroyuki
2012-01-19 13:30 ` Johannes Weiner
2012-01-19 13:30   ` Johannes Weiner
2012-01-19 23:55   ` KAMEZAWA Hiroyuki
2012-01-19 23:55     ` KAMEZAWA Hiroyuki
2012-01-19 13:43 ` Michal Hocko
2012-01-19 13:43   ` Michal Hocko
2012-01-19 23:56   ` KAMEZAWA Hiroyuki
2012-01-19 23:56     ` KAMEZAWA Hiroyuki
2012-01-20  3:26 ` [PATCH v3] " KAMEZAWA Hiroyuki
2012-01-20  3:26   ` KAMEZAWA Hiroyuki
2012-01-20  8:45   ` Michal Hocko
2012-01-20  8:45     ` Michal Hocko
2012-01-24  3:16     ` [PATCH v4] " KAMEZAWA Hiroyuki
2012-01-24  3:16       ` KAMEZAWA Hiroyuki
2012-01-24  8:56       ` Michal Hocko
2012-01-24  8:56         ` Michal Hocko
2012-01-24 11:16       ` Johannes Weiner
2012-01-24 11:16         ` Johannes Weiner
2012-01-24 14:54         ` Johannes Weiner
2012-01-24 14:54           ` Johannes Weiner
2012-01-24 16:01           ` Michal Hocko
2012-01-24 16:01             ` Michal Hocko
2012-01-24 16:44             ` Johannes Weiner [this message]
2012-01-24 16:44               ` Johannes Weiner
2012-01-24 17:23               ` Michal Hocko
2012-01-24 17:23                 ` Michal Hocko
2012-01-24 18:09                 ` Michal Hocko
2012-01-24 18:09                   ` Michal Hocko
2012-01-25  0:00                   ` KAMEZAWA Hiroyuki
2012-01-25  0:00                     ` KAMEZAWA Hiroyuki
2012-01-25  5:41                     ` [PATCH v5] " KAMEZAWA Hiroyuki
2012-01-25  5:41                       ` KAMEZAWA Hiroyuki
2012-01-25 12:24                       ` Michal Hocko
2012-01-25 12:24                         ` Michal Hocko
2012-01-25 13:36                       ` Johannes Weiner
2012-01-25 13:36                         ` Johannes Weiner
2012-01-20 10:33   ` [PATCH v3] " Johannes Weiner
2012-01-20 10:33     ` Johannes Weiner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20120124164449.GH1660@cmpxchg.org \
    --to=hannes@cmpxchg.org \
    --cc=akpm@linux-foundation.org \
    --cc=hughd@google.com \
    --cc=kamezawa.hiroyu@jp.fujitsu.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@suse.cz \
    --cc=yinghan@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.