Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Roman Gushchin <guro@fb.com>
To:     Chris Down <chris@chrisdown.name>
CC:     Andrew Morton <akpm@linux-foundation.org>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Michal Hocko <mhocko@kernel.org>, Tejun Heo <tj@kernel.org>,
        Dennis Zhou <dennis@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "cgroups@vger.kernel.org" <cgroups@vger.kernel.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        Kernel Team <Kernel-team@fb.com>
Subject: Re: [PATCH REBASED] mm, memcg: Make scan aggression always exclude
 protection
Thread-Topic: [PATCH REBASED] mm, memcg: Make scan aggression always exclude
 protection
Thread-Index: AQHU4MjBAXhBvPMCI0qZ3+WtTr5x4qYXxmuAgAB7AIA=
Date:   Fri, 22 Mar 2019 22:49:28 +0000
Message-ID: <20190322224922.GA7729@tower.DHCP.thefacebook.com>
References: <20190228213050.GA28211@chrisdown.name>
 <20190322160307.GA3316@chrisdown.name>
 <20190322222907.GA17496@tower.DHCP.thefacebook.com>
In-Reply-To: <20190322222907.GA17496@tower.DHCP.thefacebook.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-messagesentrepresentingtype: 1
x-originating-ip: [2620:10d:c090:200::2:41f8]
x-ms-publictraffictype: Email
x-ms-office365-filtering-correlation-id: 8f237f79-b4ed-4967-2367-08d6af18a3fa
x-microsoft-antispam: BCL:0;PCL:0;RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600127)(711020)(4605104)(2017052603328)(7153060)(7193020);SRVR:BN8PR15MB3153;
x-ms-traffictypediagnostic: BN8PR15MB3153:
x-microsoft-antispam-prvs: <BN8PR15MB3153CD534EA2C14CD65CDBCFBE430@BN8PR15MB3153.namprd15.prod.outlook.com>
x-forefront-prvs: 09840A4839
x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(39860400002)(396003)(366004)(346002)(136003)(376002)(189003)(199004)(476003)(6486002)(256004)(186003)(76176011)(52116002)(486006)(6506007)(4326008)(81156014)(71200400001)(478600001)(386003)(105586002)(54906003)(102836004)(11346002)(33656002)(6916009)(8676002)(14454004)(99286004)(7736002)(305945005)(446003)(25786009)(1076003)(106356001)(71190400001)(8936002)(53936002)(229853002)(81166006)(68736007)(316002)(6512007)(46003)(9686003)(6436002)(97736004)(2906002)(6246003)(6116002)(14444005)(86362001)(5660300002);DIR:OUT;SFP:1102;SCL:1;SRVR:BN8PR15MB3153;H:BN8PR15MB2626.namprd15.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;MX:1;A:1;
received-spf: None (protection.outlook.com: fb.com does not designate
 permitted sender hosts)
x-ms-exchange-senderadcheck: 1
x-microsoft-antispam-message-info: H0r83NdEsEExzVMLwJT6cs/DTHUNrzvw9l66QU71KmrZZEJhK2HaTbHF0IUYbAifngkOWkafRZ+EuoxyUp3S0Mw0OHiCZkSLbIDT4Iupqb/ReyqB+5cPe/fXcAJ+JzCyZiRbmDX3865+3CVdrpA5cYIa5RF0TTmMZ/u5zccZe7e2rtqpubcKvb2VlNgaiFid1tsdt11vH8m6aMZkVq7ZZJyT6uIx3WKBDAH7LllTWKbwhZevf6h1GGQo1QxaRiyH91bQUjmsqEXwmRkLngu50ASgyae7Z0JdA7COflF5qkReiG1rKOQgd81f+u1bZrWxMaq1Uv3ZdVT/5+prSv8fvDcNrCtyzDHFtlPxZHdAie3BdNhef9WJRecy/Fe5fewBIaoHpBZJDnTmm34JLal1LJ3UGdUd48ddWVKBS0OUMmU=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <138A374FD0ADE743A805FFC790F7E616@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 8f237f79-b4ed-4967-2367-08d6af18a3fa
X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Mar 2019 22:49:28.7240
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR15MB3153
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-03-22_13:,,
 signatures=0
X-Proofpoint-Spam-Reason: safe
X-FB-Internal: Safe
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Mar 22, 2019 at 03:29:10PM -0700, Roman Gushchin wrote:
> On Fri, Mar 22, 2019 at 04:03:07PM +0000, Chris Down wrote:
> > This patch is an incremental improvement on the existing
> > memory.{low,min} relative reclaim work to base its scan pressure
> > calculations on how much protection is available compared to the curren=
t
> > usage, rather than how much the current usage is over some protection
> > threshold.
> >=20
> > Previously the way that memory.low protection works is that if you are
> > 50% over a certain baseline, you get 50% of your normal scan pressure.
> > This is certainly better than the previous cliff-edge behaviour, but it
> > can be improved even further by always considering memory under the
> > currently enforced protection threshold to be out of bounds. This means
> > that we can set relatively low memory.low thresholds for variable or
> > bursty workloads while still getting a reasonable level of protection,
> > whereas with the previous version we may still trivially hit the 100%
> > clamp. The previous 100% clamp is also somewhat arbitrary, whereas this
> > one is more concretely based on the currently enforced protection
> > threshold, which is likely easier to reason about.
> >=20
> > There is also a subtle issue with the way that proportional reclaim
> > worked previously -- it promotes having no memory.low, since it makes
> > pressure higher during low reclaim. This happens because we base our
> > scan pressure modulation on how far memory.current is between memory.mi=
n
> > and memory.low, but if memory.low is unset, we only use the overage
> > method. In most cromulent configurations, this then means that we end u=
p
> > with *more* pressure than with no memory.low at all when we're in low
> > reclaim, which is not really very usable or expected.
> >=20
> > With this patch, memory.low and memory.min affect reclaim pressure in a
> > more understandable and composable way. For example, from a user
> > standpoint, "protected" memory now remains untouchable from a reclaim
> > aggression standpoint, and users can also have more confidence that
> > bursty workloads will still receive some amount of guaranteed
> > protection.
> >=20
> > Signed-off-by: Chris Down <chris@chrisdown.name>
> > Reviewed-by: Roman Gushchin <guro@fb.com>
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Michal Hocko <mhocko@kernel.org>
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: Roman Gushchin <guro@fb.com>
> > Cc: Dennis Zhou <dennis@kernel.org>
> > Cc: linux-kernel@vger.kernel.org
> > Cc: cgroups@vger.kernel.org
> > Cc: linux-mm@kvack.org
> > Cc: kernel-team@fb.com
> > ---
> >  include/linux/memcontrol.h | 25 ++++++++--------
> >  mm/vmscan.c                | 61 +++++++++++++-------------------------
> >  2 files changed, 32 insertions(+), 54 deletions(-)
> >=20
> > No functional changes, just rebased.
> >=20
> > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > index b226c4bafc93..799de23edfb7 100644
> > --- a/include/linux/memcontrol.h
> > +++ b/include/linux/memcontrol.h
> > @@ -333,17 +333,17 @@ static inline bool mem_cgroup_disabled(void)
> >  	return !cgroup_subsys_enabled(memory_cgrp_subsys);
> >  }
> > =20
> > -static inline void mem_cgroup_protection(struct mem_cgroup *memcg,
> > -					 unsigned long *min, unsigned long *low)
> > +static inline unsigned long mem_cgroup_protection(struct mem_cgroup *m=
emcg,
> > +						  bool in_low_reclaim)
> >  {
> > -	if (mem_cgroup_disabled()) {
> > -		*min =3D 0;
> > -		*low =3D 0;
> > -		return;
> > -	}
> > +	if (mem_cgroup_disabled())
> > +		return 0;
> > +
> > +	if (in_low_reclaim)
> > +		return READ_ONCE(memcg->memory.emin);
> > =20
> > -	*min =3D READ_ONCE(memcg->memory.emin);
> > -	*low =3D READ_ONCE(memcg->memory.elow);
> > +	return max(READ_ONCE(memcg->memory.emin),
> > +		   READ_ONCE(memcg->memory.elow));
> >  }
> > =20
> >  enum mem_cgroup_protection mem_cgroup_protected(struct mem_cgroup *roo=
t,
> > @@ -845,11 +845,10 @@ static inline void memcg_memory_event_mm(struct m=
m_struct *mm,
> >  {
> >  }
> > =20
> > -static inline void mem_cgroup_protection(struct mem_cgroup *memcg,
> > -					 unsigned long *min, unsigned long *low)
> > +static inline unsigned long mem_cgroup_protection(struct mem_cgroup *m=
emcg,
> > +						  bool in_low_reclaim)
> >  {
> > -	*min =3D 0;
> > -	*low =3D 0;
> > +	return 0;
> >  }
> > =20
> >  static inline enum mem_cgroup_protection mem_cgroup_protected(
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index f6b9b45f731d..d5daa224364d 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -2374,12 +2374,13 @@ static void get_scan_count(struct lruvec *lruve=
c, struct mem_cgroup *memcg,
> >  		int file =3D is_file_lru(lru);
> >  		unsigned long lruvec_size;
> >  		unsigned long scan;
> > -		unsigned long min, low;
> > +		unsigned long protection;
> > =20
> >  		lruvec_size =3D lruvec_lru_size(lruvec, lru, sc->reclaim_idx);
> > -		mem_cgroup_protection(memcg, &min, &low);
> > +		protection =3D mem_cgroup_protection(memcg,
> > +						   sc->memcg_low_reclaim);
> > =20
> > -		if (min || low) {
> > +		if (protection) {
> >  			/*
> >  			 * Scale a cgroup's reclaim pressure by proportioning
> >  			 * its current usage to its memory.low or memory.min
> > @@ -2392,13 +2393,10 @@ static void get_scan_count(struct lruvec *lruve=
c, struct mem_cgroup *memcg,
> >  			 * setting extremely liberal protection thresholds. It
> >  			 * also means we simply get no protection at all if we
> >  			 * set it too low, which is not ideal.
> > -			 */
> > -			unsigned long cgroup_size =3D mem_cgroup_size(memcg);
> > -
> > -			/*
> > -			 * If there is any protection in place, we adjust scan
> > -			 * pressure in proportion to how much a group's current
> > -			 * usage exceeds that, in percent.
> > +			 *
> > +			 * If there is any protection in place, we reduce scan
> > +			 * pressure by how much of the total memory used is
> > +			 * within protection thresholds.
> >  			 *
> >  			 * There is one special case: in the first reclaim pass,
> >  			 * we skip over all groups that are within their low
> > @@ -2408,43 +2406,24 @@ static void get_scan_count(struct lruvec *lruve=
c, struct mem_cgroup *memcg,
> >  			 * ideally want to honor how well-behaved groups are in
> >  			 * that case instead of simply punishing them all
> >  			 * equally. As such, we reclaim them based on how much
> > -			 * of their best-effort protection they are using. Usage
> > -			 * below memory.min is excluded from consideration when
> > -			 * calculating utilisation, as it isn't ever
> > -			 * reclaimable, so it might as well not exist for our
> > -			 * purposes.
> > +			 * memory they are using, reducing the scan pressure
> > +			 * again by how much of the total memory used is under
> > +			 * hard protection.
> >  			 */
> > -			if (sc->memcg_low_reclaim && low > min) {
> > -				/*
> > -				 * Reclaim according to utilisation between min
> > -				 * and low
> > -				 */
> > -				scan =3D lruvec_size * (cgroup_size - min) /
> > -					(low - min);
> > -			} else {
> > -				/* Reclaim according to protection overage */
> > -				scan =3D lruvec_size * cgroup_size /
> > -					max(min, low) - lruvec_size;
>=20
> I've noticed that the old version is just wrong: if cgroup_size is way sm=
aller
> than max(min, low), scan will be set to -lruvec_size.
> Given that it's unsigned long, we'll end up with scanning the whole list
> (due to clamp() below).

Just to clarify: in most cases it works fine because we skip cgroups with
cgroup_size < max(min, low). So we just don't call the code above.

However, we can race with the emin/elow update and end up with negative sca=
n,
especially if cgroup_size is about the effective protection size

The new version looks much more secure.

Thanks!