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:     Johannes Weiner <hannes@cmpxchg.org>
CC:     "linux-mm@kvack.org" <linux-mm@kvack.org>,
        Michal Hocko <mhocko@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Kernel Team <Kernel-team@fb.com>,
        "Shakeel Butt" <shakeelb@google.com>,
        Vladimir Davydov <vdavydov.dev@gmail.com>,
        "Waiman Long" <longman@redhat.com>,
        Christoph Lameter <cl@linux.com>
Subject: Re: [PATCH 09/16] mm: memcg/slab: charge individual slab objects
 instead of pages
Thread-Topic: [PATCH 09/16] mm: memcg/slab: charge individual slab objects
 instead of pages
Thread-Index: AQHVhUr89wbCEoSSmkSojIAthXBDTadrzcEAgAAFUQA=
Date:   Fri, 25 Oct 2019 20:00:32 +0000
Message-ID: <20191025200020.GA8325@castle.DHCP.thefacebook.com>
References: <20191018002820.307763-1-guro@fb.com>
 <20191018002820.307763-10-guro@fb.com> <20191025194118.GA393641@cmpxchg.org>
In-Reply-To: <20191025194118.GA393641@cmpxchg.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-messagesentrepresentingtype: 1
x-originating-ip: [2620:10d:c090:180::bdfd]
x-ms-publictraffictype: Email
x-ms-office365-filtering-correlation-id: f342b92d-bb98-4dfa-f87f-08d75985fde5
x-ms-traffictypediagnostic: BN8PR15MB2578:
x-ms-exchange-transport-forked: True
x-microsoft-antispam-prvs: <BN8PR15MB25789ED87994F862F5A61AC5BE650@BN8PR15MB2578.namprd15.prod.outlook.com>
x-ms-oob-tlc-oobclassifiers: OLM:4714;
x-forefront-prvs: 02015246A9
x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(136003)(346002)(396003)(366004)(376002)(39860400002)(189003)(199004)(256004)(52116002)(186003)(9686003)(476003)(6116002)(4326008)(6512007)(6916009)(76176011)(446003)(86362001)(6486002)(8676002)(14454004)(11346002)(6436002)(229853002)(7736002)(305945005)(6246003)(486006)(81166006)(8936002)(46003)(102836004)(81156014)(316002)(25786009)(54906003)(66556008)(66446008)(66476007)(66946007)(64756008)(71190400001)(6506007)(1076003)(5660300002)(71200400001)(561944003)(386003)(478600001)(99286004)(33656002)(2906002);DIR:OUT;SFP:1102;SCL:1;SRVR:BN8PR15MB2578;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: BCL:0;
x-microsoft-antispam-message-info: bETJpbE3mvAJ/CPPJWnp3QNaD/a/7DqvQSlCaCI5vVvUMpIRUF8dUWtVYYa1hJpdiRtT7eTOv/uBfeyBpZ6lVaKhZUvga5poXSj3YdMRpMXTNr3GnC49DmulV5BxyDydbZZG1Wjg11JrALZ0qV3H1/RugNQMRMpEzGTbDzMS0BGvxcraD/Dt2GyyZz7/86PXw0cEvg4ijnAMpWX4QEvAGXqlSvGst/3RyRPecpMzME561daSTAXpuA7L9FPaPVqhjpPLuF0NkxH8MHmaEGVe1TBTtHPKeXBndFUxDqr9z+41SA/Z4DqBezyeHvAboMKmH6axundd8oyVFXjY6f56zm51BIavic/Vm2SECfcARN+WU2mGf+0StiZk3o3W22wUJI2ZZC1MGfEiCBiodgp9R0uCRIrYZBtZhiuQa4ARGRp1+LiRsJHwCi8jbxAUbADI
Content-Type: text/plain; charset="us-ascii"
Content-ID: <54289716AAA5304A98D8988A365F07C7@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: f342b92d-bb98-4dfa-f87f-08d75985fde5
X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Oct 2019 20:00:32.4482
 (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-CrossTenant-userprincipalname: /VaoAQjvEz6SC6rbfYmLzsA00foy3TvdSZ8WIoKg+EO75AJrWjL8W5Ns3XbES5ea
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR15MB2578
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.95,1.0.8
 definitions=2019-10-25_10:2019-10-25,2019-10-25 signatures=0
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 phishscore=0 adultscore=0
 mlxlogscore=999 impostorscore=0 suspectscore=0 clxscore=1015
 priorityscore=1501 bulkscore=0 malwarescore=0 mlxscore=0 spamscore=0
 lowpriorityscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-1908290000 definitions=main-1910250182
X-FB-Internal: deliver
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, Oct 25, 2019 at 03:41:18PM -0400, Johannes Weiner wrote:
> On Thu, Oct 17, 2019 at 05:28:13PM -0700, Roman Gushchin wrote:
> > +static inline struct kmem_cache *memcg_slab_pre_alloc_hook(struct kmem=
_cache *s,
> > +						struct mem_cgroup **memcgp,
> > +						size_t size, gfp_t flags)
> > +{
> > +	struct kmem_cache *cachep;
> > +
> > +	cachep =3D memcg_kmem_get_cache(s, memcgp);
> > +	if (is_root_cache(cachep))
> > +		return s;
> > +
> > +	if (__memcg_kmem_charge_subpage(*memcgp, size * s->size, flags)) {
> > +		mem_cgroup_put(*memcgp);
> > +		memcg_kmem_put_cache(cachep);
> > +		cachep =3D NULL;
> > +	}
> > +
> > +	return cachep;
> > +}
> > +
> >  static inline void memcg_slab_post_alloc_hook(struct kmem_cache *s,
> >  					      struct mem_cgroup *memcg,
> >  					      size_t size, void **p)
> >  {
> >  	struct mem_cgroup_ptr *memcg_ptr;
> > +	struct lruvec *lruvec;
> >  	struct page *page;
> >  	unsigned long off;
> >  	size_t i;
> > @@ -439,6 +393,11 @@ static inline void memcg_slab_post_alloc_hook(stru=
ct kmem_cache *s,
> >  			off =3D obj_to_index(s, page, p[i]);
> >  			mem_cgroup_ptr_get(memcg_ptr);
> >  			page->mem_cgroup_vec[off] =3D memcg_ptr;
> > +			lruvec =3D mem_cgroup_lruvec(page_pgdat(page), memcg);
> > +			mod_lruvec_memcg_state(lruvec, cache_vmstat_idx(s),
> > +					       s->size);
> > +		} else {
> > +			__memcg_kmem_uncharge_subpage(memcg, s->size);
> >  		}
> >  	}
> >  	mem_cgroup_ptr_put(memcg_ptr);
>=20
> The memcg_ptr as a collection vessel for object references makes a lot
> of sense. But this code showcases that it should be a first-class
> memory tracking API that the allocator interacts with, rather than
> having to deal with a combination of memcg_ptr and memcg.
>=20
> In the two hunks here, on one hand we charge bytes to the memcg
> object, and then handle all the refcounting through a different
> bucketing object. To support that in the first place, we have to
> overload the memcg API all the way down to try_charge() to support
> bytes and pages. This is difficult to follow throughout all layers.
>=20
> What would be better is for this to be an abstraction layer for a
> subpage object tracker that sits on top of the memcg page tracker -
> not unlike the page allocator and the slab allocators themselves.
>=20
> And then the slab allocator would only interact with the subpage
> object tracker, and the object tracker would deal with the memcg page
> tracker under the hood.

Yes, the idea makes total sense to me. I'm not sure I like the new naming
(I have to spend some time with it first), but the idea of moving
stocks and leftovers to the memcg_ptr/obj_cgroup level is really good.

I'll include something based on your proposal into the next version
of the patchset.

Thank you!