[PATCH 1/1] Bumps limit of maximum core ID from 32 to 128.

[PATCH 1/1] Bumps limit of maximum core ID from 32 to 128.

[Lukasz Odzioba]

A new limit selected arbitrarily as power of two greater than
required minimum for Xeon Phi processor.

Currently driver is not able to handle cores with core ID greater than 32.
Such attempt ends up with the following error in dmesg:
coretemp coretemp.0: Adding Core XXX failed

Signed-off-by: Lukasz Odzioba <lukasz.odzioba@intel.com>
---
 drivers/hwmon/coretemp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 3e03379..6a27eb2 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -52,7 +52,7 @@ module_param_named(tjmax, force_tjmax, int, 0444);
 MODULE_PARM_DESC(tjmax, "TjMax value in degrees Celsius");
 
 #define BASE_SYSFS_ATTR_NO	2	/* Sysfs Base attr no for coretemp */
-#define NUM_REAL_CORES		32	/* Number of Real cores per cpu */
+#define NUM_REAL_CORES		128	/* Number of Real cores per cpu */
 #define CORETEMP_NAME_LENGTH	19	/* String Length of attrs */
 #define MAX_CORE_ATTRS		4	/* Maximum no of basic attrs */
 #define TOTAL_ATTRS		(MAX_CORE_ATTRS + 1)
-- 
1.7.1

Re: [PATCH 1/1] Bumps limit of maximum core ID from 32 to 128.

[Guenter Roeck]

On Mon, Oct 12, 2015 at 01:53:32PM +0200, Lukasz Odzioba wrote:
> A new limit selected arbitrarily as power of two greater than
> required minimum for Xeon Phi processor.
> 
> Currently driver is not able to handle cores with core ID greater than 32.
> Such attempt ends up with the following error in dmesg:
> coretemp coretemp.0: Adding Core XXX failed
> 

Why 128 instead of a more reasonable 64 ? What is the required minimum
for Xeon Phi ?

Also, please consider using the subject line commonly used in hwmon,
or at least point to the driver you are changing.

Thanks,
Guenter

> Signed-off-by: Lukasz Odzioba <lukasz.odzioba@intel.com>
> ---
>  drivers/hwmon/coretemp.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index 3e03379..6a27eb2 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -52,7 +52,7 @@ module_param_named(tjmax, force_tjmax, int, 0444);
>  MODULE_PARM_DESC(tjmax, "TjMax value in degrees Celsius");
>  
>  #define BASE_SYSFS_ATTR_NO	2	/* Sysfs Base attr no for coretemp */
> -#define NUM_REAL_CORES		32	/* Number of Real cores per cpu */
> +#define NUM_REAL_CORES		128	/* Number of Real cores per cpu */
>  #define CORETEMP_NAME_LENGTH	19	/* String Length of attrs */
>  #define MAX_CORE_ATTRS		4	/* Maximum no of basic attrs */
>  #define TOTAL_ATTRS		(MAX_CORE_ATTRS + 1)
> -- 
> 1.7.1
> 

Re: [lm-sensors] [PATCH 1/1] Bumps limit of maximum core ID from 32 to 128.

[Phil Pokorny]

On Tue, Oct 13, 2015 at 1:31 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Mon, Oct 12, 2015 at 01:53:32PM +0200, Lukasz Odzioba wrote:
> > A new limit selected arbitrarily as power of two greater than
> > required minimum for Xeon Phi processor.

> Why 128 instead of a more reasonable 64 ? What is the required minimum
> for Xeon Phi ?

Not meaning to be snarky, but this was answered in the first sentence.
64 is less than the required minimum for Xeon Phi processor.  So it
must be 65 or greater...

I wouldn't expect Intel to give you any more detail than that.  And it
might be that 64 is actually enough for now but would soon (months or
less than a year) be overrun by a newer processor.  So rather than
submit multiple minor patches, just submit one now that should be
"enough"

If you think this is a waste of RAM, we could make it a kernel
configuration option and let it be configured by the distro or user.
But if most distros select 128 to be able to support Xeon Phi, then
there might not be a reason for the additional complexity.

Phil P.

Re: [lm-sensors] [PATCH 1/1] Bumps limit of maximum core ID from 32 to 128.

[Guenter Roeck]

On 10/13/2015 02:05 PM, Phil Pokorny wrote:
> On Tue, Oct 13, 2015 at 1:31 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On Mon, Oct 12, 2015 at 01:53:32PM +0200, Lukasz Odzioba wrote:
>>> A new limit selected arbitrarily as power of two greater than
>>> required minimum for Xeon Phi processor.
>
>> Why 128 instead of a more reasonable 64 ? What is the required minimum
>> for Xeon Phi ?
>
> Not meaning to be snarky, but this was answered in the first sentence.
> 64 is less than the required minimum for Xeon Phi processor.  So it
> must be 65 or greater...
>

That is an assumption, not an answer, sorry.

Guenter

> I wouldn't expect Intel to give you any more detail than that.  And it
> might be that 64 is actually enough for now but would soon (months or
> less than a year) be overrun by a newer processor.  So rather than
> submit multiple minor patches, just submit one now that should be
> "enough"
>
> If you think this is a waste of RAM, we could make it a kernel
> configuration option and let it be configured by the distro or user.
> But if most distros select 128 to be able to support Xeon Phi, then
> there might not be a reason for the additional complexity.
>
> Phil P.
>

RE: [PATCH 1/1] Bumps limit of maximum core ID from 32 to 128.

[Odzioba, Lukasz]

On Tuesday, October 12, 2015 at 10:32 PM, Guenter Roeck wrote:
> Why 128 instead of a more reasonable 64 ? What is the required minimum
> for Xeon Phi ?

It would be fine today, but it will be not enough in 2016 and we would like to
give GNU/Linux distributions some time to propagate this patch.
For Knights Corner which is already on the market we need 61.
For Knights Landing we need 72.
For future generation it is not yet public information.

> Also, please consider using the subject line commonly used in hwmon,
> or at least point to the driver you are changing.

Right, I forgot this time, I'll send another patch just tell me whether 128
is ok, or please select any other value greater or equal known minimum
that you think will be more appropriate.

Thanks,
Lukas

 

Re: [PATCH 1/1] Bumps limit of maximum core ID from 32 to 128.

[Guenter Roeck]

On 10/13/2015 03:02 PM, Odzioba, Lukasz wrote:
> On Tuesday, October 12, 2015 at 10:32 PM, Guenter Roeck wrote:
>> Why 128 instead of a more reasonable 64 ? What is the required minimum
>> for Xeon Phi ?
>
> It would be fine today, but it will be not enough in 2016 and we would like to
> give GNU/Linux distributions some time to propagate this patch.
> For Knights Corner which is already on the market we need 61.
> For Knights Landing we need 72.

Pardon my ignorance ... those are Xeon Phi processors, and support up to
244 threads (for Knights Corner). Programming datasheet isn't easily available,
so I have to guess a bit. Following the processor numbering scheme of "ordinary"
processors, the CPU ID can therefore be up to 244 (at least) already today,
meaning the limit would have to be 256 (assuming that the processor does support
per-core temperature sensors). On the other side, the public datasheet suggests
that there are only three temperature sensors.

What am I missing here ?

Thanks,
Guenter

> For future generation it is not yet public information.
>
>> Also, please consider using the subject line commonly used in hwmon,
>> or at least point to the driver you are changing.
>
> Right, I forgot this time, I'll send another patch just tell me whether 128
> is ok, or please select any other value greater or equal known minimum
> that you think will be more appropriate.
>
> Thanks,
> Lukas
>
>
>

RE: [PATCH 1/1] Bumps limit of maximum core ID from 32 to 128.

[Odzioba, Lukasz]

On Wednesday, October 14, 2015 at 12:26 AM, Guenter Roeck wrote:
> Pardon my ignorance ... those are Xeon Phi processors, and support up to
> 244 threads (for Knights Corner). Programming datasheet isn't easily available,
> so I have to guess a bit. Following the processor numbering scheme of "ordinary"
> processors, the CPU ID can therefore be up to 244 (at least) already today,
> meaning the limit would have to be 256 (assuming that the processor does support
> per-core temperature sensors). On the other side, the public datasheet suggests
> that there are only three temperature sensors.

> What am I missing here ?

Knights Corner can have up to 61 cores with 4 threads per core, so up to 244 threads.
All HT siblings reads temperature from the same core and coretemp driver is aware of it.
That's it needs just one temp_data structure for all threads on given core.
I tested 128 on real HW myself.

Thanks,
Lukas

Re: [PATCH 1/1] Bumps limit of maximum core ID from 32 to 128.

[Guenter Roeck]

On 10/12/2015 04:53 AM, Lukasz Odzioba wrote:
> A new limit selected arbitrarily as power of two greater than
> required minimum for Xeon Phi processor.
>
> Currently driver is not able to handle cores with core ID greater than 32.
> Such attempt ends up with the following error in dmesg:
> coretemp coretemp.0: Adding Core XXX failed
>
> Signed-off-by: Lukasz Odzioba <lukasz.odzioba@intel.com>

Applied, after fixing up the subject and listing the current required limit
of 72 cores for Xeon Phi (per published information).

Guenter

> ---
>   drivers/hwmon/coretemp.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index 3e03379..6a27eb2 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -52,7 +52,7 @@ module_param_named(tjmax, force_tjmax, int, 0444);
>   MODULE_PARM_DESC(tjmax, "TjMax value in degrees Celsius");
>
>   #define BASE_SYSFS_ATTR_NO	2	/* Sysfs Base attr no for coretemp */
> -#define NUM_REAL_CORES		32	/* Number of Real cores per cpu */
> +#define NUM_REAL_CORES		128	/* Number of Real cores per cpu */
>   #define CORETEMP_NAME_LENGTH	19	/* String Length of attrs */
>   #define MAX_CORE_ATTRS		4	/* Maximum no of basic attrs */
>   #define TOTAL_ATTRS		(MAX_CORE_ATTRS + 1)
>

RE: [PATCH 1/1] Bumps limit of maximum core ID from 32 to 128.

[Odzioba, Lukasz]

On Wednesday, October 14, 2015 at 3:17 AM, Guenter Roeck wrote: 
> Applied, after fixing up the subject and listing the current required limit
> of 72 cores for Xeon Phi (per published information).

Guenter sorry for inconvenience I forgot that core enumeration on KNL 
is not continuous, so some core ID's are not used. Thus we have to
change limit to 76 instead of 72.

My mistake,
Lukas

Re: [PATCH 1/1] Bumps limit of maximum core ID from 32 to 128.

[Guenter Roeck]

On 10/14/2015 04:39 AM, Odzioba, Lukasz wrote:
> On Wednesday, October 14, 2015 at 3:17 AM, Guenter Roeck wrote:
>> Applied, after fixing up the subject and listing the current required limit
>> of 72 cores for Xeon Phi (per published information).
>
> Guenter sorry for inconvenience I forgot that core enumeration on KNL
> is not continuous, so some core ID's are not used. Thus we have to
> change limit to 76 instead of 72.
>
That is just in the comment. The actual limit is still 128.

Guenter

RE: [PATCH 1/1] Bumps limit of maximum core ID from 32 to 128.

[Odzioba, Lukasz]

On Wednesday, October 14, 2014 at 4:04 PM, Guenter Roeck wrote:
> That is just in the comment. The actual limit is still 128.

Ok, sure. Thank you for your help in driving this change upstream.

Thanks,
Lukas