All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: Radu Rendec <rrendec@redhat.com>,
	linux-kernel@vger.kernel.org,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Pierre Gondois <Pierre.Gondois@arm.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4 1/3] cacheinfo: Add arch specific early level initializer
Date: Wed, 17 May 2023 18:27:03 -0700	[thread overview]
Message-ID: <20230518012703.GA19967@ranerica-svr.sc.intel.com> (raw)
In-Reply-To: <20230515093608.etfprpqn3lmgybe6@bogus>

On Mon, May 15, 2023 at 10:36:08AM +0100, Sudeep Holla wrote:
> On Wed, May 10, 2023 at 12:12:07PM -0700, Ricardo Neri wrote:
> > Hi,
> > 
> > I had posted a patchset[1] for x86 that initializes
> > ci_cacheinfo(cpu)->num_leaves during SMP boot.
> >
> 
> It is entirely clear to me if this is just a clean up or a fix to some
> issue you faced ? Just wanted to let you know Prateek from AMD has couple
> of fixes [2]

My first patch is a bug fix. The second patch is clean up that results
from fixing the bug in patch 1.

> 
> > This means that early_leaves and a late cache_leaves() are equal but
> > per_cpu_cacheinfo(cpu) is never allocated. Currently, x86 does not use
> > fetch_cache_info().
> > 
> > I think that we should check here that per_cpu_cacheinfo() has been allocated to
> > take care of the case in which early and late cache leaves remain the same:
> > 
> > -       if (cache_leaves(cpu) <= early_leaves)
> > +       if (cache_leaves(cpu) <= early_leaves && per_cpu_cacheinfo(cpu))
> > 
> > Otherwise, in v6.4-rc1 + [1] I observe a NULL pointer dereference from
> > last_level_cache_is_valid().
> >
> 
> I think this is different issue as Prateek was just observing wrong info
> after cpuhotplug operations. But the patches manage the cpumap_populated
> state better with the patches. Can you please look at that as weel ?

I verified that the patches from Prateek fix a different issue. I was able
to reproduce his issue. His patches fixes it.

I still see my issue after applying Prateek's patches.
> 
> -- 
> Regards,
> Sudeep
> 
> [2] https://lore.kernel.org/all/20230508084115.1157-1-kprateek.nayak@amd.com

Thank you for the suggestion!

BR,
Ricardo

WARNING: multiple messages have this Message-ID (diff)
From: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: Radu Rendec <rrendec@redhat.com>,
	linux-kernel@vger.kernel.org,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Pierre Gondois <Pierre.Gondois@arm.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4 1/3] cacheinfo: Add arch specific early level initializer
Date: Wed, 17 May 2023 18:27:03 -0700	[thread overview]
Message-ID: <20230518012703.GA19967@ranerica-svr.sc.intel.com> (raw)
In-Reply-To: <20230515093608.etfprpqn3lmgybe6@bogus>

On Mon, May 15, 2023 at 10:36:08AM +0100, Sudeep Holla wrote:
> On Wed, May 10, 2023 at 12:12:07PM -0700, Ricardo Neri wrote:
> > Hi,
> > 
> > I had posted a patchset[1] for x86 that initializes
> > ci_cacheinfo(cpu)->num_leaves during SMP boot.
> >
> 
> It is entirely clear to me if this is just a clean up or a fix to some
> issue you faced ? Just wanted to let you know Prateek from AMD has couple
> of fixes [2]

My first patch is a bug fix. The second patch is clean up that results
from fixing the bug in patch 1.

> 
> > This means that early_leaves and a late cache_leaves() are equal but
> > per_cpu_cacheinfo(cpu) is never allocated. Currently, x86 does not use
> > fetch_cache_info().
> > 
> > I think that we should check here that per_cpu_cacheinfo() has been allocated to
> > take care of the case in which early and late cache leaves remain the same:
> > 
> > -       if (cache_leaves(cpu) <= early_leaves)
> > +       if (cache_leaves(cpu) <= early_leaves && per_cpu_cacheinfo(cpu))
> > 
> > Otherwise, in v6.4-rc1 + [1] I observe a NULL pointer dereference from
> > last_level_cache_is_valid().
> >
> 
> I think this is different issue as Prateek was just observing wrong info
> after cpuhotplug operations. But the patches manage the cpumap_populated
> state better with the patches. Can you please look at that as weel ?

I verified that the patches from Prateek fix a different issue. I was able
to reproduce his issue. His patches fixes it.

I still see my issue after applying Prateek's patches.
> 
> -- 
> Regards,
> Sudeep
> 
> [2] https://lore.kernel.org/all/20230508084115.1157-1-kprateek.nayak@amd.com

Thank you for the suggestion!

BR,
Ricardo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2023-05-18  1:24 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-04-12 18:57 [PATCH v4 0/3] arch_topology: Pre-allocate cacheinfo from primary CPU Radu Rendec
2023-04-12 18:57 ` Radu Rendec
2023-04-12 18:57 ` [PATCH v4 1/3] cacheinfo: Add arch specific early level initializer Radu Rendec
2023-04-12 18:57   ` Radu Rendec
2023-05-10 19:12   ` Ricardo Neri
2023-05-10 20:44     ` Radu Rendec
2023-05-11  0:00       ` Ricardo Neri
2023-05-11 19:55         ` Radu Rendec
2023-05-19 21:44           ` Ricardo Neri
2023-05-19 21:44             ` Ricardo Neri
2023-05-19 22:02             ` Radu Rendec
2023-05-19 22:02               ` Radu Rendec
2023-05-15  9:36     ` Sudeep Holla
2023-05-15  9:36       ` Sudeep Holla
2023-05-18  1:27       ` Ricardo Neri [this message]
2023-05-18  1:27         ` Ricardo Neri
2023-05-18  9:34         ` Sudeep Holla
2023-05-18  9:34           ` Sudeep Holla
2023-05-31 12:22           ` Sudeep Holla
2023-05-31 12:22             ` Sudeep Holla
2023-05-31 17:03             ` Ricardo Neri
2023-05-31 17:03               ` Ricardo Neri
2023-08-07 23:23               ` Ricardo Neri
2023-08-07 23:23                 ` Ricardo Neri
2023-04-12 18:57 ` [PATCH v4 2/3] cacheinfo: Add arm64 early level initializer implementation Radu Rendec
2023-04-12 18:57   ` Radu Rendec
2023-04-13 10:22   ` Sudeep Holla
2023-04-13 10:22     ` Sudeep Holla
2023-04-13 14:45     ` Will Deacon
2023-04-13 14:45       ` Will Deacon
2023-04-13 15:05       ` Sudeep Holla
2023-04-13 15:05         ` Sudeep Holla
2023-04-14 12:46         ` Will Deacon
2023-04-14 12:46           ` Will Deacon
2023-04-12 18:57 ` [PATCH v4 3/3] cacheinfo: Allow early level detection when DT/ACPI info is missing/broken Radu Rendec
2023-04-12 18:57   ` Radu Rendec
2023-04-17 14:07 ` [PATCH v4 0/3] arch_topology: Pre-allocate cacheinfo from primary CPU Sudeep Holla
2023-04-17 14:07   ` Sudeep Holla

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=20230518012703.GA19967@ranerica-svr.sc.intel.com \
    --to=ricardo.neri-calderon@linux.intel.com \
    --cc=Pierre.Gondois@arm.com \
    --cc=catalin.marinas@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rrendec@redhat.com \
    --cc=sudeep.holla@arm.com \
    --cc=will@kernel.org \
    /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.