2022-07-07 22:13:05

by Conor Dooley

[permalink] [raw]
Subject: [RFC 2/4] arch-topology: add a default implementation of store_cpu_topology()

From: Conor Dooley <[email protected]>

RISC-V & arm64 both use an almost identical method of filling in
default vales for arch topology. Create a weakly defined default
implementation with the intent of migrating both archs to use it.

Signed-off-by: Conor Dooley <[email protected]>
---
drivers/base/arch_topology.c | 19 +++++++++++++++++++
include/linux/arch_topology.h | 1 +
2 files changed, 20 insertions(+)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 441e14ac33a4..07e84c6ac5c2 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -765,6 +765,25 @@ void update_siblings_masks(unsigned int cpuid)
}
}

+void __weak store_cpu_topology(unsigned int cpuid)
+{
+ struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
+
+ if (cpuid_topo->package_id != -1)
+ goto topology_populated;
+
+ cpuid_topo->thread_id = -1;
+ cpuid_topo->core_id = cpuid;
+ cpuid_topo->package_id = cpu_to_node(cpuid);
+
+ pr_debug("CPU%u: package %d core %d thread %d\n",
+ cpuid, cpuid_topo->package_id, cpuid_topo->core_id,
+ cpuid_topo->thread_id);
+
+topology_populated:
+ update_siblings_masks(cpuid);
+}
+
static void clear_cpu_topology(int cpu)
{
struct cpu_topology *cpu_topo = &cpu_topology[cpu];
diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
index a07b510e7dc5..fee306b8a541 100644
--- a/include/linux/arch_topology.h
+++ b/include/linux/arch_topology.h
@@ -92,6 +92,7 @@ void update_siblings_masks(unsigned int cpu);
void remove_cpu_topology(unsigned int cpuid);
void reset_cpu_topology(void);
int parse_acpi_topology(void);
+
#endif

#endif /* _LINUX_ARCH_TOPOLOGY_H_ */
--
2.37.0


2022-07-08 08:48:19

by Conor Dooley

[permalink] [raw]
Subject: Re: [RFC 2/4] arch-topology: add a default implementation of store_cpu_topology()

On 08/07/2022 09:24, Sudeep Holla wrote:
> On Thu, Jul 07, 2022 at 11:04:35PM +0100, Conor Dooley wrote:
>> From: Conor Dooley <[email protected]>
>>
>> RISC-V & arm64 both use an almost identical method of filling in
>> default vales for arch topology. Create a weakly defined default
>> implementation with the intent of migrating both archs to use it.
>>
>> Signed-off-by: Conor Dooley <[email protected]>
>> ---
>> drivers/base/arch_topology.c | 19 +++++++++++++++++++
>> include/linux/arch_topology.h | 1 +
>> 2 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
>> index 441e14ac33a4..07e84c6ac5c2 100644
>> --- a/drivers/base/arch_topology.c
>> +++ b/drivers/base/arch_topology.c
>> @@ -765,6 +765,25 @@ void update_siblings_masks(unsigned int cpuid)
>> }
>> }
>>
>> +void __weak store_cpu_topology(unsigned int cpuid)

Does using __weak here make sense to you?

>
> I prefer to have this as default implementation. So just get the risc-v
> one pushed to upstream first(for v5.20) and get all the backports if required.
> Next cycle(i.e. v5.21), you can move both RISC-V and arm64.
>

Yeah, that was my intention. I meant to label patch 1/4 as "PATCH"
and (2,3,4)/4 as RFC but forgot. I talked with Palmer about doing
the risc-v impl. and then migrate both on IRC & he seemed happy with
it.

If you're okay with patch 1/4, I'll resubmit it as a standalone v2.

Thanks,
Conor.

2022-07-08 09:22:34

by Sudeep Holla

[permalink] [raw]
Subject: Re: [RFC 2/4] arch-topology: add a default implementation of store_cpu_topology()

On Thu, Jul 07, 2022 at 11:04:35PM +0100, Conor Dooley wrote:
> From: Conor Dooley <[email protected]>
>
> RISC-V & arm64 both use an almost identical method of filling in
> default vales for arch topology. Create a weakly defined default
> implementation with the intent of migrating both archs to use it.
>
> Signed-off-by: Conor Dooley <[email protected]>
> ---
> drivers/base/arch_topology.c | 19 +++++++++++++++++++
> include/linux/arch_topology.h | 1 +
> 2 files changed, 20 insertions(+)
>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index 441e14ac33a4..07e84c6ac5c2 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -765,6 +765,25 @@ void update_siblings_masks(unsigned int cpuid)
> }
> }
>
> +void __weak store_cpu_topology(unsigned int cpuid)

I prefer to have this as default implementation. So just get the risc-v
one pushed to upstream first(for v5.20) and get all the backports if required.
Next cycle(i.e. v5.21), you can move both RISC-V and arm64.

--
Regards,
Sudeep

2022-07-08 09:34:39

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [RFC 2/4] arch-topology: add a default implementation of store_cpu_topology()

Hi Sudeep,

On Fri, Jul 8, 2022 at 11:22 AM Sudeep Holla <[email protected]> wrote:
> On Fri, Jul 08, 2022 at 08:35:57AM +0000, [email protected] wrote:
> > On 08/07/2022 09:24, Sudeep Holla wrote:
> > > On Thu, Jul 07, 2022 at 11:04:35PM +0100, Conor Dooley wrote:
> > >> From: Conor Dooley <[email protected]>
> > >>
> > >> RISC-V & arm64 both use an almost identical method of filling in
> > >> default vales for arch topology. Create a weakly defined default
> > >> implementation with the intent of migrating both archs to use it.
> > >>
> > >> Signed-off-by: Conor Dooley <[email protected]>
> > >> ---
> > >> drivers/base/arch_topology.c | 19 +++++++++++++++++++
> > >> include/linux/arch_topology.h | 1 +
> > >> 2 files changed, 20 insertions(+)
> > >>
> > >> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> > >> index 441e14ac33a4..07e84c6ac5c2 100644
> > >> --- a/drivers/base/arch_topology.c
> > >> +++ b/drivers/base/arch_topology.c
> > >> @@ -765,6 +765,25 @@ void update_siblings_masks(unsigned int cpuid)
> > >> }
> > >> }
> > >>
> > >> +void __weak store_cpu_topology(unsigned int cpuid)
> >
> > Does using __weak here make sense to you?
> >
>
> I don't want any weak definition and arch to override as we know only
> arm64 and RISC-V are the only users and they are aligned to have same
> implementation. So weak definition doesn't make sense to me.
>
> > >
> > > I prefer to have this as default implementation. So just get the risc-v
> > > one pushed to upstream first(for v5.20) and get all the backports if required.
> > > Next cycle(i.e. v5.21), you can move both RISC-V and arm64.
> > >
> >
> > Yeah, that was my intention. I meant to label patch 1/4 as "PATCH"
> > and (2,3,4)/4 as RFC but forgot. I talked with Palmer about doing
> > the risc-v impl. and then migrate both on IRC & he seemed happy with
> > it.
> >
>
> Ah OK, good.
>
> > If you're okay with patch 1/4, I'll resubmit it as a standalone v2.
> >
>
> That would be great, thanks. You can most the code to move to generic from
> both arm64 and risc-v once we have this in v5.20-rc1

Why not ignore risc-v for now, and move the arm64 implementation to
the generic code for v5.20, so every arch will have it at once?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2022-07-08 09:47:07

by Sudeep Holla

[permalink] [raw]
Subject: Re: [RFC 2/4] arch-topology: add a default implementation of store_cpu_topology()

On Fri, Jul 08, 2022 at 08:35:57AM +0000, [email protected] wrote:
> On 08/07/2022 09:24, Sudeep Holla wrote:
> > On Thu, Jul 07, 2022 at 11:04:35PM +0100, Conor Dooley wrote:
> >> From: Conor Dooley <[email protected]>
> >>
> >> RISC-V & arm64 both use an almost identical method of filling in
> >> default vales for arch topology. Create a weakly defined default
> >> implementation with the intent of migrating both archs to use it.
> >>
> >> Signed-off-by: Conor Dooley <[email protected]>
> >> ---
> >> drivers/base/arch_topology.c | 19 +++++++++++++++++++
> >> include/linux/arch_topology.h | 1 +
> >> 2 files changed, 20 insertions(+)
> >>
> >> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> >> index 441e14ac33a4..07e84c6ac5c2 100644
> >> --- a/drivers/base/arch_topology.c
> >> +++ b/drivers/base/arch_topology.c
> >> @@ -765,6 +765,25 @@ void update_siblings_masks(unsigned int cpuid)
> >> }
> >> }
> >>
> >> +void __weak store_cpu_topology(unsigned int cpuid)
>
> Does using __weak here make sense to you?
>

I don't want any weak definition and arch to override as we know only
arm64 and RISC-V are the only users and they are aligned to have same
implementation. So weak definition doesn't make sense to me.

> >
> > I prefer to have this as default implementation. So just get the risc-v
> > one pushed to upstream first(for v5.20) and get all the backports if required.
> > Next cycle(i.e. v5.21), you can move both RISC-V and arm64.
> >
>
> Yeah, that was my intention. I meant to label patch 1/4 as "PATCH"
> and (2,3,4)/4 as RFC but forgot. I talked with Palmer about doing
> the risc-v impl. and then migrate both on IRC & he seemed happy with
> it.
>

Ah OK, good.

> If you're okay with patch 1/4, I'll resubmit it as a standalone v2.
>

That would be great, thanks. You can most the code to move to generic from
both arm64 and risc-v once we have this in v5.20-rc1

--
Regards,
Sudeep

2022-07-08 10:04:45

by Sudeep Holla

[permalink] [raw]
Subject: Re: [RFC 2/4] arch-topology: add a default implementation of store_cpu_topology()

On Fri, Jul 08, 2022 at 11:28:19AM +0200, Geert Uytterhoeven wrote:
> Hi Sudeep,
>
> On Fri, Jul 8, 2022 at 11:22 AM Sudeep Holla <[email protected]> wrote:
> > On Fri, Jul 08, 2022 at 08:35:57AM +0000, [email protected] wrote:
> > > On 08/07/2022 09:24, Sudeep Holla wrote:
> > > > On Thu, Jul 07, 2022 at 11:04:35PM +0100, Conor Dooley wrote:
> > > >> From: Conor Dooley <[email protected]>
> > > >>
> > > >> RISC-V & arm64 both use an almost identical method of filling in
> > > >> default vales for arch topology. Create a weakly defined default
> > > >> implementation with the intent of migrating both archs to use it.
> > > >>
> > > >> Signed-off-by: Conor Dooley <[email protected]>
> > > >> ---
> > > >> drivers/base/arch_topology.c | 19 +++++++++++++++++++
> > > >> include/linux/arch_topology.h | 1 +
> > > >> 2 files changed, 20 insertions(+)
> > > >>
> > > >> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> > > >> index 441e14ac33a4..07e84c6ac5c2 100644
> > > >> --- a/drivers/base/arch_topology.c
> > > >> +++ b/drivers/base/arch_topology.c
> > > >> @@ -765,6 +765,25 @@ void update_siblings_masks(unsigned int cpuid)
> > > >> }
> > > >> }
> > > >>
> > > >> +void __weak store_cpu_topology(unsigned int cpuid)
> > >
> > > Does using __weak here make sense to you?
> > >
> >
> > I don't want any weak definition and arch to override as we know only
> > arm64 and RISC-V are the only users and they are aligned to have same
> > implementation. So weak definition doesn't make sense to me.
> >
> > > >
> > > > I prefer to have this as default implementation. So just get the risc-v
> > > > one pushed to upstream first(for v5.20) and get all the backports if required.
> > > > Next cycle(i.e. v5.21), you can move both RISC-V and arm64.
> > > >
> > >
> > > Yeah, that was my intention. I meant to label patch 1/4 as "PATCH"
> > > and (2,3,4)/4 as RFC but forgot. I talked with Palmer about doing
> > > the risc-v impl. and then migrate both on IRC & he seemed happy with
> > > it.
> > >
> >
> > Ah OK, good.
> >
> > > If you're okay with patch 1/4, I'll resubmit it as a standalone v2.
> > >
> >
> > That would be great, thanks. You can most the code to move to generic from
> > both arm64 and risc-v once we have this in v5.20-rc1
>
> Why not ignore risc-v for now, and move the arm64 implementation to
> the generic code for v5.20, so every arch will have it at once?
>

We could but,
1. This arch_topology is new and has been going through lot of changes
recently and having code there might make it difficult to backport
changes that are required for RISC-V(my guess)

2. May be too late for v5.20, I would like to see if we can even drop tiny
arm64 bit in the code. It may be risky to try that this late and also
with other topology changes we already have queued.

Let me know if that makes sense.

--
Regards,
Sudeep

2022-07-08 10:06:20

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [RFC 2/4] arch-topology: add a default implementation of store_cpu_topology()

On Fri, Jul 08, 2022 at 10:47:10AM +0100, Sudeep Holla wrote:
> On Fri, Jul 08, 2022 at 11:28:19AM +0200, Geert Uytterhoeven wrote:
> > Hi Sudeep,
> >
> > On Fri, Jul 8, 2022 at 11:22 AM Sudeep Holla <[email protected]> wrote:
> > > On Fri, Jul 08, 2022 at 08:35:57AM +0000, [email protected] wrote:
> > > > On 08/07/2022 09:24, Sudeep Holla wrote:
> > > > > On Thu, Jul 07, 2022 at 11:04:35PM +0100, Conor Dooley wrote:
> > > > >> From: Conor Dooley <[email protected]>
> > > > >>
> > > > >> RISC-V & arm64 both use an almost identical method of filling in
> > > > >> default vales for arch topology. Create a weakly defined default
> > > > >> implementation with the intent of migrating both archs to use it.
> > > > >>
> > > > >> Signed-off-by: Conor Dooley <[email protected]>
> > > > >> ---
> > > > >> drivers/base/arch_topology.c | 19 +++++++++++++++++++
> > > > >> include/linux/arch_topology.h | 1 +
> > > > >> 2 files changed, 20 insertions(+)
> > > > >>
> > > > >> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> > > > >> index 441e14ac33a4..07e84c6ac5c2 100644
> > > > >> --- a/drivers/base/arch_topology.c
> > > > >> +++ b/drivers/base/arch_topology.c
> > > > >> @@ -765,6 +765,25 @@ void update_siblings_masks(unsigned int cpuid)
> > > > >> }
> > > > >> }
> > > > >>
> > > > >> +void __weak store_cpu_topology(unsigned int cpuid)
> > > >
> > > > Does using __weak here make sense to you?
> > > >
> > >
> > > I don't want any weak definition and arch to override as we know only
> > > arm64 and RISC-V are the only users and they are aligned to have same
> > > implementation. So weak definition doesn't make sense to me.
> > >
> > > > >
> > > > > I prefer to have this as default implementation. So just get the risc-v
> > > > > one pushed to upstream first(for v5.20) and get all the backports if required.
> > > > > Next cycle(i.e. v5.21), you can move both RISC-V and arm64.
> > > > >
> > > >
> > > > Yeah, that was my intention. I meant to label patch 1/4 as "PATCH"
> > > > and (2,3,4)/4 as RFC but forgot. I talked with Palmer about doing
> > > > the risc-v impl. and then migrate both on IRC & he seemed happy with
> > > > it.
> > > >
> > >
> > > Ah OK, good.
> > >
> > > > If you're okay with patch 1/4, I'll resubmit it as a standalone v2.
> > > >
> > >
> > > That would be great, thanks. You can most the code to move to generic from
> > > both arm64 and risc-v once we have this in v5.20-rc1
> >
> > Why not ignore risc-v for now, and move the arm64 implementation to
> > the generic code for v5.20, so every arch will have it at once?
> >
>
> We could but,
> 1. This arch_topology is new and has been going through lot of changes
> recently and having code there might make it difficult to backport
> changes that are required for RISC-V(my guess)

Worry about future issues in the future. Make it simple now as you know
what you are dealing with at the moment.

thanks,

greg k-h

2022-07-08 10:43:02

by Conor Dooley

[permalink] [raw]
Subject: Re: [RFC 2/4] arch-topology: add a default implementation of store_cpu_topology()

On 08/07/2022 10:21, Sudeep Holla wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Fri, Jul 08, 2022 at 08:35:57AM +0000, [email protected] wrote:
>> On 08/07/2022 09:24, Sudeep Holla wrote:
>>> On Thu, Jul 07, 2022 at 11:04:35PM +0100, Conor Dooley wrote:
>>>> From: Conor Dooley <[email protected]>
>>>>
>>>> RISC-V & arm64 both use an almost identical method of filling in
>>>> default vales for arch topology. Create a weakly defined default
>>>> implementation with the intent of migrating both archs to use it.
>>>>
>>>> Signed-off-by: Conor Dooley <[email protected]>
>>>> ---
>>>> drivers/base/arch_topology.c | 19 +++++++++++++++++++
>>>> include/linux/arch_topology.h | 1 +
>>>> 2 files changed, 20 insertions(+)
>>>>
>>>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
>>>> index 441e14ac33a4..07e84c6ac5c2 100644
>>>> --- a/drivers/base/arch_topology.c
>>>> +++ b/drivers/base/arch_topology.c
>>>> @@ -765,6 +765,25 @@ void update_siblings_masks(unsigned int cpuid)
>>>> }
>>>> }
>>>>
>>>> +void __weak store_cpu_topology(unsigned int cpuid)
>>
>> Does using __weak here make sense to you?
>>
>
> I don't want any weak definition and arch to override as we know only
> arm64 and RISC-V are the only users and they are aligned to have same
> implementation. So weak definition doesn't make sense to me.

Right. I had used __weak b/c I didn't know how to split the migration
into smaller patches per arch without breaking the build due to
multiple definitions of store_cpu_topology().

>
>>>
>>> I prefer to have this as default implementation. So just get the risc-v
>>> one pushed to upstream first(for v5.20) and get all the backports if required.
>>> Next cycle(i.e. v5.21), you can move both RISC-V and arm64.
>>>
>>
>> Yeah, that was my intention. I meant to label patch 1/4 as "PATCH"
>> and (2,3,4)/4 as RFC but forgot. I talked with Palmer about doing
>> the risc-v impl. and then migrate both on IRC & he seemed happy with
>> it.
>>
>
> Ah OK, good.
>
>> If you're okay with patch 1/4, I'll resubmit it as a standalone v2.
>>
>
> That would be great, thanks. You can most the code to move to generic from
> both arm64 and risc-v once we have this in v5.20-rc1

Right, that sounds like a plan (well, pending geert's concerns).
Could I have your R-b on patch 1? The comments you made about
removing the duplicate function should be resolved.

Thanks,
Conor.

2022-07-08 11:58:29

by Sudeep Holla

[permalink] [raw]
Subject: Re: [RFC 2/4] arch-topology: add a default implementation of store_cpu_topology()

On Fri, Jul 08, 2022 at 12:03:41PM +0200, Greg KH wrote:
> On Fri, Jul 08, 2022 at 10:47:10AM +0100, Sudeep Holla wrote:
> > On Fri, Jul 08, 2022 at 11:28:19AM +0200, Geert Uytterhoeven wrote:
> > > Hi Sudeep,
> > >
> > > On Fri, Jul 8, 2022 at 11:22 AM Sudeep Holla <[email protected]> wrote:
> > > > On Fri, Jul 08, 2022 at 08:35:57AM +0000, [email protected] wrote:
> > > > > On 08/07/2022 09:24, Sudeep Holla wrote:
> > > > > > On Thu, Jul 07, 2022 at 11:04:35PM +0100, Conor Dooley wrote:
> > > > > >> From: Conor Dooley <[email protected]>
> > > > > >>
> > > > > >> RISC-V & arm64 both use an almost identical method of filling in
> > > > > >> default vales for arch topology. Create a weakly defined default
> > > > > >> implementation with the intent of migrating both archs to use it.
> > > > > >>
> > > > > >> Signed-off-by: Conor Dooley <[email protected]>
> > > > > >> ---
> > > > > >> drivers/base/arch_topology.c | 19 +++++++++++++++++++
> > > > > >> include/linux/arch_topology.h | 1 +
> > > > > >> 2 files changed, 20 insertions(+)
> > > > > >>
> > > > > >> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> > > > > >> index 441e14ac33a4..07e84c6ac5c2 100644
> > > > > >> --- a/drivers/base/arch_topology.c
> > > > > >> +++ b/drivers/base/arch_topology.c
> > > > > >> @@ -765,6 +765,25 @@ void update_siblings_masks(unsigned int cpuid)
> > > > > >> }
> > > > > >> }
> > > > > >>
> > > > > >> +void __weak store_cpu_topology(unsigned int cpuid)
> > > > >
> > > > > Does using __weak here make sense to you?
> > > > >
> > > >
> > > > I don't want any weak definition and arch to override as we know only
> > > > arm64 and RISC-V are the only users and they are aligned to have same
> > > > implementation. So weak definition doesn't make sense to me.
> > > >
> > > > > >
> > > > > > I prefer to have this as default implementation. So just get the risc-v
> > > > > > one pushed to upstream first(for v5.20) and get all the backports if required.
> > > > > > Next cycle(i.e. v5.21), you can move both RISC-V and arm64.
> > > > > >
> > > > >
> > > > > Yeah, that was my intention. I meant to label patch 1/4 as "PATCH"
> > > > > and (2,3,4)/4 as RFC but forgot. I talked with Palmer about doing
> > > > > the risc-v impl. and then migrate both on IRC & he seemed happy with
> > > > > it.
> > > > >
> > > >
> > > > Ah OK, good.
> > > >
> > > > > If you're okay with patch 1/4, I'll resubmit it as a standalone v2.
> > > > >
> > > >
> > > > That would be great, thanks. You can most the code to move to generic from
> > > > both arm64 and risc-v once we have this in v5.20-rc1
> > >
> > > Why not ignore risc-v for now, and move the arm64 implementation to
> > > the generic code for v5.20, so every arch will have it at once?
> > >
> >
> > We could but,
> > 1. This arch_topology is new and has been going through lot of changes
> > recently and having code there might make it difficult to backport
> > changes that are required for RISC-V(my guess)
>
> Worry about future issues in the future. Make it simple now as you know
> what you are dealing with at the moment.
>

Sure, I was just suggesting and expecting someone from RISC-V community or
maintainers to make a call. As I said it is based on my understanding.
hence I have mentioned as guess. So I am not against it as such.


--
Regards,
Sudeep

2022-07-08 12:25:40

by Conor Dooley

[permalink] [raw]
Subject: Re: [RFC 2/4] arch-topology: add a default implementation of store_cpu_topology()

On 08/07/2022 12:39, Sudeep Holla wrote:
> On Fri, Jul 08, 2022 at 12:03:41PM +0200, Greg KH wrote:
>> On Fri, Jul 08, 2022 at 10:47:10AM +0100, Sudeep Holla wrote:
>>> On Fri, Jul 08, 2022 at 11:28:19AM +0200, Geert Uytterhoeven wrote:
>>>> Hi Sudeep,
>>>>
>>>> On Fri, Jul 8, 2022 at 11:22 AM Sudeep Holla <[email protected]> wrote:
>>>>> On Fri, Jul 08, 2022 at 08:35:57AM +0000, [email protected] wrote:
>>>>>> If you're okay with patch 1/4, I'll resubmit it as a standalone v2.
>>>>>>
>>>>>
>>>>> That would be great, thanks. You can most the code to move to generic from
>>>>> both arm64 and risc-v once we have this in v5.20-rc1
>>>>
>>>> Why not ignore risc-v for now, and move the arm64 implementation to
>>>> the generic code for v5.20, so every arch will have it at once?
>>>>
>>>
>>> We could but,
>>> 1. This arch_topology is new and has been going through lot of changes
>>> recently and having code there might make it difficult to backport
>>> changes that are required for RISC-V(my guess)
>>
>> Worry about future issues in the future. Make it simple now as you know
>> what you are dealing with at the moment.
>>
>
> Sure, I was just suggesting and expecting someone from RISC-V community or
> maintainers to make a call. As I said it is based on my understanding.
> hence I have mentioned as guess. So I am not against it as such.

I did a little bit of poking in the git history.
The last code touching the arm implementation was:
3102bc0e6ac7 ("arm64: topology: Stop using MPIDR for topology information")
on Fri Oct 2 12:01:41 2020 +0100

The introduction of arch-topology stuff to RISC-V was:
03f11f03dbfe ("RISC-V: Parse cpu topology during boot.")
on Thu Jun 27 12:53:00 2019 -0700

Backporting as far as v5.10 should be no real effort and I don't think
to v5.4 that should be meaninfully harder. If 3102bc0e6ac7 hasn't been
backported already, maybe it should be since it appears to have been
fixing a problem too.

Based on that, I think doing this the straightforward way in the first
place is a better idea.

I'll respin the series as:
patch 1: Move arm64 to the generic implementation
patch 2: Make RISC-V use the generic implementation

Thanks,
Conor.

2022-07-08 14:20:38

by Sudeep Holla

[permalink] [raw]
Subject: Re: [RFC 2/4] arch-topology: add a default implementation of store_cpu_topology()

On Fri, Jul 08, 2022 at 11:57:05AM +0000, [email protected] wrote:
>
> I did a little bit of poking in the git history.
> The last code touching the arm implementation was:
> 3102bc0e6ac7 ("arm64: topology: Stop using MPIDR for topology information")
> on Fri Oct 2 12:01:41 2020 +0100
>
> The introduction of arch-topology stuff to RISC-V was:
> 03f11f03dbfe ("RISC-V: Parse cpu topology during boot.")
> on Thu Jun 27 12:53:00 2019 -0700
>
> Backporting as far as v5.10 should be no real effort and I don't think
> to v5.4 that should be meaninfully harder. If 3102bc0e6ac7 hasn't been
> backported already, maybe it should be since it appears to have been
> fixing a problem too.
>

Thanks for doing the research and sorry for the noise earlier.

> Based on that, I think doing this the straightforward way in the first
> place is a better idea.
>
> I'll respin the series as:
> patch 1: Move arm64 to the generic implementation

I don't think the mpidr check we have there is of much use IMO. You can
drop that and see if arm64 maintainers and/or others agree. As you have
already figured, since 3102bc0e6ac7 we are not using MPIDR and the one
check we have is optional IMO. So you can either drop it or keep it as
in your RFC and then post updates.

--
Regards,
Sudeep