Message-ID: <D6EDEBF1F91015459DB866AC4EE162CC022FB40D@IRSMSX103.ger.corp.intel.com>
Date:	Fri, 18 Sep 2015 15:33:34 +0000
From:	"Odzioba, Lukasz" <lukasz.odzioba@...el.com>
To:	'Guenter Roeck' <linux@...ck-us.net>,
	"Yu, Fenghua" <fenghua.yu@...el.com>
CC:	"jdelvare@...e.de" <jdelvare@...e.de>,
	"lm-sensors@...sensors.org" <lm-sensors@...sensors.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/1] hwmon: coretemp: use dynamically allocated array to
 store per core data

On 09/11/2015 04:28 AM, Guenter Roeck wrote:
> You can return NULL but never check for this condition in the calling code.
> The only time you check in the calling code is when you want to know
> if pdata->core_data[index] is NULL, which is distinctly different.
> As such, this check does not really make sense. It would indicate
> a severe driver error if it is encountered, meaning it would be better
> to just let the driver crash if that happens.

Good catch, I'll remove (index >= pdata->core_data_size) condition.

> This lock doesn't help you anything. If create_core_data can be called _now_,
> after the lock is released, tdata is no longer valid.

I don't get it, could you explain it?
core_data contains pointers and address and "length" of it can change after
krealloc, but the data in it should still point to the valid memory which is
not changing during reallocation, so tdata should be ok. Am I missing something?

> Really, I don't understand the point of this function. Using pdata->core_data[index]
> directly without lock would be just as (un-)safe. For the lock to have any value,
> you would have to keep it while accessing tdata (which is presumably what it is
> supposed to protect).

This lock is to protect core_data from beeing reallocated while accessing core_data[index].
>From my understanding without it we would have to assume/know that operations like
tdata *x=core_data[index] and core_data substitution in krealloc are atomic.
Once we have an address of temp_data struct krealloc cannot hurt us.

> At the same time, the lock is too aggressive. You don't need to protect
> a read from another read. All you need to protect is a read from a write.

> It might make more sense to have both get_core_data() to acquire the
> (read ?) lock, and put_core_data() to release it. The function would
> then not get the pointer, but just acquire the lock - the calling code
> can get the pointer directly, after all.

This has even wider scope than my lock, but I can implement it if you wish.

> I do not understand what the get_online_cpus() and put_online_cpus() while allocating
> the memory is supposed to accomplish. Please elaborate.

I couldn't find a way to avoid deadlock and race condition on during cpu removal
using just one lock - when sysfs is accessed while cpu is beeing removed.
As a background you could read history of Kirill A. Shutemov's patches from 2012:
http://permalink.gmane.org/gmane.linux.drivers.sensors/29589

Instead of get/put_online_cpus I could use another lock.
I am using it during memory allocation because put_core_offline could be executed
simoultaneously with create_core_data, can't it?
Maybe I am just trying to reinvent a wheel and you have a better idea how to ensure that.

> Also, it might be more efficient to allocate memory in chunks, not one at a time.

I'll change step from 1 to 16, unless you have a better idea.

> What is the purpose of this variable ? You might as well use a constant.

I wanted to keep it for readability, but after all I agree that it would be better to remove it.

Regarding the rest of the comments I'll apply them.

Thank you for your time and sorry for my delayed response.
Lukas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists