All of lore.kernel.org
 help / color / mirror / Atom feed
From: Guenter Roeck <linux@roeck-us.net>
To: "Odzioba, Lukasz" <lukasz.odzioba@intel.com>,
	Jean Delvare <jdelvare@suse.de>
Cc: "Yu, Fenghua" <fenghua.yu@intel.com>,
	"lm-sensors@lm-sensors.org" <lm-sensors@lm-sensors.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [lm-sensors] [PATCH] hwmon: coretemp: use list instead of fixed size array for temp data
Date: Fri, 17 Jul 2015 18:01:39 +0000	[thread overview]
Message-ID: <55A94303.5040805@roeck-us.net> (raw)
In-Reply-To: <D6EDEBF1F91015459DB866AC4EE162CCF76BDA@IRSMSX103.ger.corp.intel.com>

On 07/17/2015 10:28 AM, Odzioba, Lukasz wrote:
> From: Guenter Roeck [mailto:linux@roeck-us.net]
> On Friday, July 17, 2015 6:55 PM Guenter Roeck wrote:
>
>> You don't really explain why your approach would be better than
>> allocating an array of pointers to struct temp_data and increasing
>> its size using krealloc if needed.
>
> Let's consider two cases of such implementation:
> a) we use array of pointers with O(n) access algorithm
> b) we use array of pointers with O(1) access algorithm
>
> In both cases an array will have greater memory footprint unless
> we implement reallocation ourselves when cpus are disabled which will
> make code harder to maintain.

Please explain why krealloc() won't work, why using krealloc(()
would result in a larger memory footprint than using lists,
and why disabling CPUs would require any action in the first place.

> Case b) does not handle huge core ids and sparse enumeration well -
> it is still to discuss whether we really need it since there is no
> such hardware yet.
>
"yet" is a key term here. Presumably you have insider information.
Unless you can share this information, I don't see the point of
replacing an O(1) algorithm with O(n), especially since there
is a relatively simple alternative available to support more CPUs.

> I am not saying that my solution is the best of possible ones.
> I am saying that "the best" can vary depending on which criteria do you
> choose from (time, memory, clean code...). Some may say that O(n) is
> fine unless we have thousands of cores and this code is not on hot path,
> others may be concerned more about memory on small/old devices.
> I don't see holy grail here, If you see one please let me know.
>

Unless you clarify that Intel will introduce CPU IDs which can not be used
as array index because they are too sparse, I don't really see how the list
solution would consume less memory than an array of pointers, even if the
array is somewhat sparse. After all, a list consumes at least two pointers
per entry.

Thanks,
Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

WARNING: multiple messages have this Message-ID (diff)
From: Guenter Roeck <linux@roeck-us.net>
To: "Odzioba, Lukasz" <lukasz.odzioba@intel.com>,
	Jean Delvare <jdelvare@suse.de>
Cc: "Yu, Fenghua" <fenghua.yu@intel.com>,
	"lm-sensors@lm-sensors.org" <lm-sensors@lm-sensors.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] hwmon: coretemp: use list instead of fixed size array for temp data
Date: Fri, 17 Jul 2015 11:01:39 -0700	[thread overview]
Message-ID: <55A94303.5040805@roeck-us.net> (raw)
In-Reply-To: <D6EDEBF1F91015459DB866AC4EE162CCF76BDA@IRSMSX103.ger.corp.intel.com>

On 07/17/2015 10:28 AM, Odzioba, Lukasz wrote:
> From: Guenter Roeck [mailto:linux@roeck-us.net]
> On Friday, July 17, 2015 6:55 PM Guenter Roeck wrote:
>
>> You don't really explain why your approach would be better than
>> allocating an array of pointers to struct temp_data and increasing
>> its size using krealloc if needed.
>
> Let's consider two cases of such implementation:
> a) we use array of pointers with O(n) access algorithm
> b) we use array of pointers with O(1) access algorithm
>
> In both cases an array will have greater memory footprint unless
> we implement reallocation ourselves when cpus are disabled which will
> make code harder to maintain.

Please explain why krealloc() won't work, why using krealloc(()
would result in a larger memory footprint than using lists,
and why disabling CPUs would require any action in the first place.

> Case b) does not handle huge core ids and sparse enumeration well -
> it is still to discuss whether we really need it since there is no
> such hardware yet.
>
"yet" is a key term here. Presumably you have insider information.
Unless you can share this information, I don't see the point of
replacing an O(1) algorithm with O(n), especially since there
is a relatively simple alternative available to support more CPUs.

> I am not saying that my solution is the best of possible ones.
> I am saying that "the best" can vary depending on which criteria do you
> choose from (time, memory, clean code...). Some may say that O(n) is
> fine unless we have thousands of cores and this code is not on hot path,
> others may be concerned more about memory on small/old devices.
> I don't see holy grail here, If you see one please let me know.
>

Unless you clarify that Intel will introduce CPU IDs which can not be used
as array index because they are too sparse, I don't really see how the list
solution would consume less memory than an array of pointers, even if the
array is somewhat sparse. After all, a list consumes at least two pointers
per entry.

Thanks,
Guenter


  reply	other threads:[~2015-07-17 18:01 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-04-30 13:18 [lm-sensors] [PATCH] hwmon: coretemp: fix oops on cpu unplug Kirill A. Shutemov
2012-04-30 13:18 ` Kirill A. Shutemov
2012-04-30 15:28 ` [lm-sensors] " Guenter Roeck
2012-04-30 15:28   ` Guenter Roeck
2012-04-30 16:19   ` [lm-sensors] " Kirill A. Shutemov
2012-04-30 16:19     ` Kirill A. Shutemov
2012-04-30 16:59     ` [lm-sensors] " Guenter Roeck
2012-04-30 16:59       ` Guenter Roeck
2012-05-01 15:20 ` [lm-sensors] " Guenter Roeck
2012-05-01 15:20   ` Guenter Roeck
2012-05-01 21:00   ` [lm-sensors] " Kirill A. Shutemov
2012-05-01 21:00     ` Kirill A. Shutemov
2012-05-01 21:25     ` [lm-sensors] " Guenter Roeck
2012-05-01 21:25       ` Guenter Roeck
2012-05-02 15:10       ` [lm-sensors] [PATCH] hwmon: coretemp: use list instead of fixed size array for temp data Kirill A. Shutemov
2012-05-02 15:10         ` Kirill A. Shutemov
2012-05-03  5:29         ` [lm-sensors] " R, Durgadoss
2012-05-03  5:29           ` R, Durgadoss
2012-05-03 10:04           ` Kirill A. Shutemov
2012-05-03 10:04             ` Kirill A. Shutemov
2012-05-03 11:18         ` [lm-sensors] [PATCH, v2] hwmon: coretemp: use list instead of fixed size array for temp Kirill A. Shutemov
2012-05-03 11:18           ` [PATCH, v2] hwmon: coretemp: use list instead of fixed size array for temp data Kirill A. Shutemov
2012-05-04  5:41           ` [lm-sensors] [PATCH, v2] hwmon: coretemp: use list instead of fixed size array for temp Guenter Roeck
2012-05-04  5:41             ` [PATCH, v2] hwmon: coretemp: use list instead of fixed size array for temp data Guenter Roeck
2012-05-04  6:46             ` [lm-sensors] [PATCH, v2] hwmon: coretemp: use list instead of fixed size array for temp Kirill A. Shutemov
2012-05-04  6:46               ` [PATCH, v2] hwmon: coretemp: use list instead of fixed size array for temp data Kirill A. Shutemov
2012-05-04 13:34               ` [lm-sensors] [PATCH, v2] hwmon: coretemp: use list instead of fixed size array for temp Guenter Roeck
2012-05-04 13:34                 ` [PATCH, v2] hwmon: coretemp: use list instead of fixed size array for temp data Guenter Roeck
2012-05-04 13:42                 ` [lm-sensors] [PATCH, v2] hwmon: coretemp: use list instead of fixed size array for temp Kirill A. Shutemov
2012-05-04 13:42                   ` [PATCH, v2] hwmon: coretemp: use list instead of fixed size array for temp data Kirill A. Shutemov
2015-07-15 16:04         ` [lm-sensors] [PATCH] " Lukasz Odzioba
2015-07-15 16:04           ` Lukasz Odzioba
2015-07-15 21:07           ` [lm-sensors] " Jean Delvare
2015-07-15 21:07             ` Jean Delvare
2015-07-16 13:17             ` [lm-sensors] " Odzioba, Lukasz
2015-07-16 13:17               ` Odzioba, Lukasz
2015-07-17 16:55               ` [lm-sensors] " Guenter Roeck
2015-07-17 16:55                 ` Guenter Roeck
2015-07-17 17:28                 ` [lm-sensors] " Odzioba, Lukasz
2015-07-17 17:28                   ` Odzioba, Lukasz
2015-07-17 18:01                   ` Guenter Roeck [this message]
2015-07-17 18:01                     ` Guenter Roeck
2015-07-17 19:23                     ` [lm-sensors] " Odzioba, Lukasz
2015-07-17 19:23                       ` Odzioba, Lukasz
2015-07-17 21:33                       ` [lm-sensors] " Jean Delvare
2015-07-17 21:33                         ` Jean Delvare
2015-07-17 19:11                   ` [lm-sensors] " Jean Delvare
2015-07-17 19:11                     ` Jean Delvare
2015-07-17 19:36                     ` [lm-sensors] " Guenter Roeck
2015-07-17 19:36                       ` Guenter Roeck
2015-07-17 21:25                       ` [lm-sensors] " Jean Delvare
2015-07-17 21:25                         ` Jean Delvare

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=55A94303.5040805@roeck-us.net \
    --to=linux@roeck-us.net \
    --cc=fenghua.yu@intel.com \
    --cc=jdelvare@suse.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lm-sensors@lm-sensors.org \
    --cc=lukasz.odzioba@intel.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.