On Thu, Mar 13, 2025 at 05:59:15PM +0000, Matthew Wilcox wrote:
> On Thu, Mar 13, 2025 at 12:01:14PM -0400, Johannes Weiner wrote:
> > > void split_page_memcg(struct page *first, unsigned order)
> > > {
> > > - struct folio *folio = page_folio(first);
> > > + struct obj_cgroup *objcg = page_objcg(first);
> > > unsigned int i, nr = 1 << order;
> > >
> > > - if (mem_cgroup_disabled() || !folio_memcg_charged(folio))
> > > + if (!objcg)
> > > return;
> >
> > mem_cgroup_disabled() is a jump label, so a bit cheaper than the
> > branch. Can you keep that and do page_objcg() after it?
>
> I think this might be better?
>
> -static struct obj_cgroup *page_objcg(const struct page *first)
> +static struct obj_cgroup *page_objcg(const struct page *page)
> {
> - unsigned long memcg_data = first->memcg_data;
> + unsigned long memcg_data = page->memcg_data;
>
> - if (!memcg_data)
> + if (mem_cgroup_disabled() || !memcg_data)
> return NULL;
>
> I'd expect the compiler to inline page_objcg() into its caller and
> optimise away the NULL test if mem_cgroup_disabled().
That sounds good to me. Thanks!
