Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9020807789b70db0d84d142cbfed2bd8868f366a.camel@redhat.com>
Subject: Re: [PATCH v4 1/3] cacheinfo: Add arch specific early level
 initializer
From:   Radu Rendec <rrendec@redhat.com>
To:     Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
Cc:     linux-kernel@vger.kernel.org,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Pierre Gondois <Pierre.Gondois@arm.com>,
        Sudeep Holla <sudeep.holla@arm.com>,
        linux-arm-kernel@lists.infradead.org
Date:   Thu, 11 May 2023 15:55:18 -0400
In-Reply-To: <20230511000058.GD18514@ranerica-svr.sc.intel.com>
References: <20230412185759.755408-1-rrendec@redhat.com>
         <20230412185759.755408-2-rrendec@redhat.com>
         <20230510191207.GA18514@ranerica-svr.sc.intel.com>
         <b49e241d3ea8c679b81134e22c908ca64aeca18c.camel@redhat.com>
         <20230511000058.GD18514@ranerica-svr.sc.intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Wed, 2023-05-10 at 17:00 -0700, Ricardo Neri wrote:
> On Wed, May 10, 2023 at 04:44:49PM -0400, Radu Rendec wrote:
> > On Wed, 2023-05-10 at 12:12 -0700, Ricardo Neri wrote:
> > > On Wed, Apr 12, 2023 at 02:57:57PM -0400, Radu Rendec wrote:
> > > > This patch gives architecture specific code the ability to initiali=
ze
> > > > the cache level and allocate cacheinfo memory early, when cache lev=
el
> > > > initialization runs on the primary CPU for all possible CPUs.
> > [cut]
> > > > -int detect_cache_attributes(unsigned int cpu)
> > > > +static inline int init_level_allocate_ci(unsigned int cpu)
> > > > =C2=A0{
> > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int ret;
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned int early_leave=
s =3D cache_leaves(cpu);
> > > > =C2=A0
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* Since early init=
ialization/allocation of the cacheinfo is allowed
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * via fetch_cache_=
info() and this also gets called as CPU hotplug
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * callbacks via ca=
cheinfo_cpu_online, the init/alloc can be skipped
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * as it will happe=
n only once (the cacheinfo memory is never freed).
> > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * Just populate the cac=
heinfo.
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * Just populate the cac=
heinfo. However, if the cacheinfo has been
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * allocated early throu=
gh the arch-specific early_cache_level() call,
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * there is a chance the=
 info is wrong (this can happen on arm64). In
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * that case, call init_=
cache_level() anyway to give the arch-specific
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * code a chance to make=
 things right.
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 */
> > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (per_cpu_cacheinfo(cp=
u))
> > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0goto populate_leaves;
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (per_cpu_cacheinfo(cp=
u) && !ci_cacheinfo(cpu)->early_ci_levels)
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0return 0;
> > > > =C2=A0
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (init_cache_leve=
l(cpu) || !cache_leaves(cpu))
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return -ENOENT;
> > > > =C2=A0
> > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ret =3D allocate_cache_i=
nfo(cpu);
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/*
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * Now that we have prop=
erly initialized the cache level info, make
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * sure we don't try to =
do that again the next time we are called
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * (e.g. as CPU hotplug =
callbacks).
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 */
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ci_cacheinfo(cpu)->early=
_ci_levels =3D false;
> > > > +
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (cache_leaves(cpu) <=
=3D early_leaves)
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0return 0;
> > > > +
> > >=20
> > > I had posted a patchset[1] for x86 that initializes
> > > ci_cacheinfo(cpu)->num_leaves during SMP boot.
> > >=20
> > > This means that early_leaves and a late cache_leaves() are equal but
> > > per_cpu_cacheinfo(cpu) is never allocated. Currently, x86 does not us=
e
> > > fetch_cache_info().
> > >=20
> > > I think that we should check here that per_cpu_cacheinfo() has been a=
llocated to
> > > take care of the case in which early and late cache leaves remain the=
 same:
> > >=20
> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (cache_leaves(cpu) <=3D earl=
y_leaves)
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (cache_leaves(cpu) <=3D earl=
y_leaves && per_cpu_cacheinfo(cpu))
> > >=20
> > > Otherwise, in v6.4-rc1 + [1] I observe a NULL pointer dereference fro=
m
> > > last_level_cache_is_valid().
> > >=20
> > > I can post a patch with this fix if it makes sense.
> > >=20
> > > [1]. https://lore.kernel.org/all/20230424001956.21434-3-ricardo.neri-=
calderon@linux.intel.com/
> >=20
> > Thanks for bringing this to my attention. I need to run some tests on
> > x86 (I did all that work/testing on arm64) and wrap my head around it.
> >=20
> > While I don't see any problem with the fix you're proposing, I'm afraid
> > it may circle back to the other problem I tried to fix initially. Have
> > you tested this on an RT kernel by any chance?
>=20
> That is a good point. I did not test on an RT kernel. I'll try that.

It looks like the flow is much simpler on x86: detect_cache_attributes()
is called only once for each CPU, and it's called in kthread context.

I haven't tested on an RT kernel but I think it should be fine. I put a
msleep() there and saw no issues, which means kmalloc() on RT should be
fine as well.

> > I'm thinking that if we end up in init_level_allocate_ci() without the
> > cacheinfo memory having been allocated earlier, we're up for a "BUG"
> > splat on RT kernels.
> >=20
> > If early_leaves has the right value at that point, the cacheinfo memory
> > should be allocated early (on the primary CPU), so perhaps there's a
> > different problem somewhere else.
>=20
> That can work for x86, IMO. Not sure about other archs. As you mention,
> other archs still want the chance to correct the early cache info.

You're right. I got confused for a moment because I was used to the
arm64 flow. On x86, there is no "early" cache info per se because, as I
already mentioned, detect_cache_attributes() is called only once for
each CPU.

I was intrigued about how this worked without your changes, and I
looked closer. Between the initialization of the early_leaves variable
at the beginning of init_level_allocate_ci() and the comparison of
cache_leaves(cpu) and early_leaves, init_cache_level() gets called.
Before your changes, (struct cpu_cacheinfo).num_leaves was initialized
to 0 and then changed in init_cache_level(). That way, early_leaves
ended up as 0, which made the comparison evaluate to false.

At this point I think the patch you proposed is the right way to fix
this. I don't see any reason why it would interfere with other archs
that really use early allocation. This new patch should probably be
added to your series, since otherwise your other patches would
basically "introduce" a null-pointer deref.

My only suggestion would be to add a short comment before the
comparison, to explain that on x86 detect_cache_attributes() is called
only once for each CPU and so early allocation is not possible but
(struct cpu_cacheinfo).num_leaves is already initialized by the time
detect_cache_attributes() is called.

Regards,
Radu