From: Yong Xuan Wang <yongxuan.wang@sifive.com>
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
"Rafael J. Wysocki" <rafael@kernel.org>,
linux-kernel@vger.kernel.org,
Pierre Gondois <pierre.gondois@arm.com>,
Vincent Chen <vincent.chen@sifive.com>,
Greentime Hu <greentime.hu@sifive.com>
Subject: Re: [PATCH -next v3] drivers: base: cacheinfo: fix shared_cpu_map
Date: Tue, 17 Jan 2023 18:02:56 +0800 [thread overview]
Message-ID: <CAMWQL2ivgNxTA73tmXeF9BEfJX1agQyWe1JqV6R8H8ksCF-csQ@mail.gmail.com> (raw)
In-Reply-To: <20230104105939.vdiq77xbn45agj22@bogus>
Hi Sudeep,
> On Wed, Jan 4, 2023 at 6:59 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Wed, Dec 28, 2022 at 03:24:19AM +0000, Yong-Xuan Wang wrote:
> > The cacheinfo sets up the shared_cpu_map by checking whether the caches
> > with the same index are shared between CPUs. However, this will trigger
> > slab-out-of-bounds access if the CPUs do not have the same cache hierarchy.
> > Another problem is the mismatched shared_cpu_map when the shared cache does
> > not have the same index between CPUs.
> >
> > CPU0 I D L3
> > index 0 1 2 x
> > ^ ^ ^ ^
> > index 0 1 2 3
> > CPU1 I D L2 L3
> >
> > This patch checks each cache is shared with all caches on other CPUs.
> >
>
> Just curious to know if this is just Qemu config or a real platform.
> I had intentionally not supported this to just to get to know when such
> h/w appears in the real world 😁.
>
We are trying to build such kind of config in QEMU.
> > Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
> > Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> > ---
> > drivers/base/cacheinfo.c | 25 +++++++++++++++----------
> > 1 file changed, 15 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> > index 950b22cdb5f7..dfa804bcf3cc 100644
> > --- a/drivers/base/cacheinfo.c
> > +++ b/drivers/base/cacheinfo.c
> > @@ -256,7 +256,7 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
> > {
> > struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
> > struct cacheinfo *this_leaf, *sib_leaf;
> > - unsigned int index;
> > + unsigned int index, sib_index;
> > int ret = 0;
> >
> > if (this_cpu_ci->cpu_map_populated)
> > @@ -284,11 +284,12 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
> >
> > if (i == cpu || !sib_cpu_ci->info_list)
> > continue;/* skip if itself or no cacheinfo */
> > -
> > - sib_leaf = per_cpu_cacheinfo_idx(i, index);
> > - if (cache_leaves_are_shared(this_leaf, sib_leaf)) {
> > - cpumask_set_cpu(cpu, &sib_leaf->shared_cpu_map);
> > - cpumask_set_cpu(i, &this_leaf->shared_cpu_map);
> > + for (sib_index = 0; sib_index < cache_leaves(i); sib_index++) {
> > + sib_leaf = per_cpu_cacheinfo_idx(i, sib_index);
> > + if (cache_leaves_are_shared(this_leaf, sib_leaf)) {
> > + cpumask_set_cpu(cpu, &sib_leaf->shared_cpu_map);
> > + cpumask_set_cpu(i, &this_leaf->shared_cpu_map);
>
> Does it make sense to break here once we match as it is unlikely to match
> with any other indices ?
>
Yeah. We can break here once we find the shared instance. I'll send a
new version to fix it.
Thank you!
> > + }
> > }
> > }
> > /* record the maximum cache line size */
> > @@ -302,7 +303,7 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
> > static void cache_shared_cpu_map_remove(unsigned int cpu)
> > {
> > struct cacheinfo *this_leaf, *sib_leaf;
> > - unsigned int sibling, index;
> > + unsigned int sibling, index, sib_index;
> >
> > for (index = 0; index < cache_leaves(cpu); index++) {
> > this_leaf = per_cpu_cacheinfo_idx(cpu, index);
> > @@ -313,9 +314,13 @@ static void cache_shared_cpu_map_remove(unsigned int cpu)
> > if (sibling == cpu || !sib_cpu_ci->info_list)
> > continue;/* skip if itself or no cacheinfo */
> >
> > - sib_leaf = per_cpu_cacheinfo_idx(sibling, index);
> > - cpumask_clear_cpu(cpu, &sib_leaf->shared_cpu_map);
> > - cpumask_clear_cpu(sibling, &this_leaf->shared_cpu_map);
> > + for (sib_index = 0; sib_index < cache_leaves(sibling); sib_index++) {
> > + sib_leaf = per_cpu_cacheinfo_idx(sibling, sib_index);
> > + if (cache_leaves_are_shared(this_leaf, sib_leaf)) {
> > + cpumask_clear_cpu(cpu, &sib_leaf->shared_cpu_map);
> > + cpumask_clear_cpu(sibling, &this_leaf->shared_cpu_map);
>
> Same comment as above.
>
> --
> Regards,
> Sudeep
Regards,
Yong-Xuan
prev parent reply other threads:[~2023-01-17 10:03 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-28 3:24 [PATCH -next v3 0/1] drivers: base: cacheinfo: fix shared_cpu_map Yong-Xuan Wang
2022-12-28 3:24 ` [PATCH -next v3] " Yong-Xuan Wang
2023-01-04 10:59 ` Sudeep Holla
2023-01-17 10:02 ` Yong Xuan Wang [this message]
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=CAMWQL2ivgNxTA73tmXeF9BEfJX1agQyWe1JqV6R8H8ksCF-csQ@mail.gmail.com \
--to=yongxuan.wang@sifive.com \
--cc=greentime.hu@sifive.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pierre.gondois@arm.com \
--cc=rafael@kernel.org \
--cc=sudeep.holla@arm.com \
--cc=vincent.chen@sifive.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.