[PATCH] hwmon: adt7470: Allow faster removal

[PATCH] hwmon: adt7470: Allow faster removal

[Joshua Scott]

adt7470_remove will wait for the update thread to complete before
returning. This has a worst-case time of up to the user-configurable
auto_update_interval.

Break this delay into smaller slices to allow the thread to exit quickly
when adt7470_remove is called.

Signed-off-by: Joshua Scott <joshua.scott@alliedtelesis.co.nz>
---
 drivers/hwmon/adt7470.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
index 7d185a9..ba97392 100644
--- a/drivers/hwmon/adt7470.c
+++ b/drivers/hwmon/adt7470.c
@@ -268,14 +268,21 @@ static int adt7470_update_thread(void *p)
 {
 	struct i2c_client *client = p;
 	struct adt7470_data *data = i2c_get_clientdata(client);
+	unsigned long next_read = jiffies - 1;
 
 	while (!kthread_should_stop()) {
-		mutex_lock(&data->lock);
-		adt7470_read_temperatures(client, data);
-		mutex_unlock(&data->lock);
+
+		if (time_after_eq(jiffies, next_read)) {
+			next_read = jiffies + data->auto_update_interval * HZ / 1000;
+			mutex_lock(&data->lock);
+			adt7470_read_temperatures(client, data);
+			mutex_unlock(&data->lock);
+		}
+
+		msleep_interruptible(1);
+
 		if (kthread_should_stop())
 			break;
-		msleep_interruptible(data->auto_update_interval);
 	}
 
 	complete_all(&data->auto_update_stop);
-- 
2.9.0

Re: [PATCH] hwmon: adt7470: Allow faster removal

[Jean Delvare]

Hi Joshua,

On Thu,  1 Sep 2016 17:17:21 +1200, Joshua Scott wrote:
> adt7470_remove will wait for the update thread to complete before
> returning. This has a worst-case time of up to the user-configurable
> auto_update_interval.
> 
> Break this delay into smaller slices to allow the thread to exit quickly
> when adt7470_remove is called.
> 
> Signed-off-by: Joshua Scott <joshua.scott@alliedtelesis.co.nz>
> ---
>  drivers/hwmon/adt7470.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index 7d185a9..ba97392 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c
> @@ -268,14 +268,21 @@ static int adt7470_update_thread(void *p)
>  {
>  	struct i2c_client *client = p;
>  	struct adt7470_data *data = i2c_get_clientdata(client);
> +	unsigned long next_read = jiffies - 1;
>  
>  	while (!kthread_should_stop()) {
> -		mutex_lock(&data->lock);
> -		adt7470_read_temperatures(client, data);
> -		mutex_unlock(&data->lock);
> +
> +		if (time_after_eq(jiffies, next_read)) {
> +			next_read = jiffies + data->auto_update_interval * HZ / 1000;
> +			mutex_lock(&data->lock);
> +			adt7470_read_temperatures(client, data);
> +			mutex_unlock(&data->lock);
> +		}
> +
> +		msleep_interruptible(1);
> +
>  		if (kthread_should_stop())
>  			break;
> -		msleep_interruptible(data->auto_update_interval);
>  	}
>  
>  	complete_all(&data->auto_update_stop);

This change looks terribly costly to me. In order to shorten the
duration of a rare event (you don't "rmmod adt7470" on a regular basis,
do you?) you increase the cost of a kernel thread which runs all the
time. I'm afraid this msleep_interruptible(1) is going to prevent the
CPU from entering low power states at all, leading to an increased
system temperature and power consumption. Have you compared the output
of "powertop" (specifically the "Idle stats" section) before and after
your patch?

Is there no way to voluntarily interrupt this long msleep_interruptible?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] hwmon: adt7470: Allow faster removal

[Guenter Roeck]

On 08/31/2016 10:17 PM, Joshua Scott wrote:
> adt7470_remove will wait for the update thread to complete before
> returning. This has a worst-case time of up to the user-configurable
> auto_update_interval.
>
> Break this delay into smaller slices to allow the thread to exit quickly
> when adt7470_remove is called.
>
> Signed-off-by: Joshua Scott <joshua.scott@alliedtelesis.co.nz>
> ---
>  drivers/hwmon/adt7470.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index 7d185a9..ba97392 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c
> @@ -268,14 +268,21 @@ static int adt7470_update_thread(void *p)
>  {
>  	struct i2c_client *client = p;
>  	struct adt7470_data *data = i2c_get_clientdata(client);
> +	unsigned long next_read = jiffies - 1;
>
>  	while (!kthread_should_stop()) {
> -		mutex_lock(&data->lock);
> -		adt7470_read_temperatures(client, data);
> -		mutex_unlock(&data->lock);
> +
> +		if (time_after_eq(jiffies, next_read)) {
> +			next_read = jiffies + data->auto_update_interval * HZ / 1000;
> +			mutex_lock(&data->lock);
> +			adt7470_read_temperatures(client, data);
> +			mutex_unlock(&data->lock);
> +		}
> +
> +		msleep_interruptible(1);
> +


This puts a heavy burden on the system, forcing it to run every ms, just for
the unlikely case of driver removal. Why is quick removal so important ?
If it is, we'll have to find a better solution.

Guenter

>  		if (kthread_should_stop())
>  			break;
> -		msleep_interruptible(data->auto_update_interval);
>  	}
>
>  	complete_all(&data->auto_update_stop);
>

Re: [PATCH] hwmon: adt7470: Allow faster removal

[Chris Packham]

Hi Guenter, Jean,

On 09/02/2016 12:12 AM, Guenter Roeck wrote:
> On 08/31/2016 10:17 PM, Joshua Scott wrote:
>> adt7470_remove will wait for the update thread to complete before
>> returning. This has a worst-case time of up to the user-configurable
>> auto_update_interval.
>>
>> Break this delay into smaller slices to allow the thread to exit quickly
>> when adt7470_remove is called.
>>
>> Signed-off-by: Joshua Scott <joshua.scott@alliedtelesis.co.nz>
>> ---
>>  drivers/hwmon/adt7470.c | 15 +++++++++++----
>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
>> index 7d185a9..ba97392 100644
>> --- a/drivers/hwmon/adt7470.c
>> +++ b/drivers/hwmon/adt7470.c
>> @@ -268,14 +268,21 @@ static int adt7470_update_thread(void *p)
>>  {
>>  	struct i2c_client *client = p;
>>  	struct adt7470_data *data = i2c_get_clientdata(client);
>> +	unsigned long next_read = jiffies - 1;
>>
>>  	while (!kthread_should_stop()) {
>> -		mutex_lock(&data->lock);
>> -		adt7470_read_temperatures(client, data);
>> -		mutex_unlock(&data->lock);
>> +
>> +		if (time_after_eq(jiffies, next_read)) {
>> +			next_read = jiffies + data->auto_update_interval * HZ / 1000;
>> +			mutex_lock(&data->lock);
>> +			adt7470_read_temperatures(client, data);
>> +			mutex_unlock(&data->lock);
>> +		}
>> +
>> +		msleep_interruptible(1);
>> +
>
>
> This puts a heavy burden on the system, forcing it to run every ms, just for
> the unlikely case of driver removal. Why is quick removal so important ?
> If it is, we'll have to find a better solution.
>
> Guenter
>

The particular use case we have is a chassis based system with an 
adt7470 on a removable fan-tray. The problem is that when the fan tray 
is removed it takes auto_update_interval/1000 seconds for the update 
thread to stop and the i2c device to be removed. In the intervening time 
a new fan-tray could have been installed.

>>  		if (kthread_should_stop())
>>  			break;
>> -		msleep_interruptible(data->auto_update_interval);
>>  	}
>>
>>  	complete_all(&data->auto_update_stop);
>>

On 09/01/2016 08:18 PM, Jean Delvare wrote:
>
> This change looks terribly costly to me. In order to shorten the
> duration of a rare event (you don't "rmmod adt7470" on a regular basis,
> do you?)

It's worse than that. We're not doing rmmod, hardware is physically 
being removed.

> you increase the cost of a kernel thread which runs all the
> time. I'm afraid this msleep_interruptible(1) is going to prevent the
> CPU from entering low power states at all, leading to an increased
> system temperature and power consumption. Have you compared the output
> of "powertop" (specifically the "Idle stats" section) before and after
> your patch?
>
> Is there no way to voluntarily interrupt this long msleep_interruptible?
>

If there is I'd like to know. That would be the ideal solution for us.

Re: [PATCH] hwmon: adt7470: Allow faster removal

[Jean Delvare]

Hi Chris,

On Thu, 1 Sep 2016 21:08:54 +0000, Chris Packham wrote:
> On 09/02/2016 12:12 AM, Guenter Roeck wrote:
> > This puts a heavy burden on the system, forcing it to run every ms, just for
> > the unlikely case of driver removal. Why is quick removal so important ?
> > If it is, we'll have to find a better solution.
> 
> The particular use case we have is a chassis based system with an 
> adt7470 on a removable fan-tray. The problem is that when the fan tray 
> is removed it takes auto_update_interval/1000 seconds for the update 
> thread to stop and the i2c device to be removed. In the intervening time 
> a new fan-tray could have been installed.

Thanks for the clarification. An actual use case makes it easier to
think about solutions.

> On 09/01/2016 08:18 PM, Jean Delvare wrote:
> >
> > This change looks terribly costly to me. In order to shorten the
> > duration of a rare event (you don't "rmmod adt7470" on a regular basis,
> > do you?)
> 
> It's worse than that. We're not doing rmmod, hardware is physically 
> being removed.

I wouldn't call it "worse", it's pretty much the same to me.

> > you increase the cost of a kernel thread which runs all the
> > time. I'm afraid this msleep_interruptible(1) is going to prevent the
> > CPU from entering low power states at all, leading to an increased
> > system temperature and power consumption. Have you compared the output
> > of "powertop" (specifically the "Idle stats" section) before and after
> > your patch?
> >
> > Is there no way to voluntarily interrupt this long msleep_interruptible?
> 
> If there is I'd like to know. That would be the ideal solution for us.

I've never done that before myself so I don't know more than you.
stackoverflow has a few promising pointers though:

http://stackoverflow.com/questions/26050745/is-there-a-way-inside-the-kernel-of-killing-a-kernel-kthread-just-like-kill-9
http://stackoverflow.com/questions/36344295/wakeup-a-kernel-thread-that-is-in-sleep-using-msleep

My own research suggests that maybe calling
wake_up_process(data->auto_update) right after kthread_stop() may work.
Have you tried that? I only find it suspect that nobody else in the
kernel is doing that.

-- 
Jean Delvare
SUSE L3 Support

[PATCH] hwmon: adt7470: Allow faster removal

[Joshua Scott]

adt7470_remove will wait for the update thread to complete before
returning. This had a worst-case time of up to the user-configurable
auto_update_interval.

Replace msleep_interruptible with set_current_state and schedule_timeout
so that kthread_stop will interrupt the sleep.

Signed-off-by: Joshua Scott <joshua.scott@alliedtelesis.co.nz>
---
 drivers/hwmon/adt7470.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
index 7d185a9..e93c528 100644
--- a/drivers/hwmon/adt7470.c
+++ b/drivers/hwmon/adt7470.c
@@ -273,9 +273,12 @@ static int adt7470_update_thread(void *p)
 		mutex_lock(&data->lock);
 		adt7470_read_temperatures(client, data);
 		mutex_unlock(&data->lock);
+
+		set_current_state(TASK_INTERRUPTIBLE);
 		if (kthread_should_stop())
 			break;
-		msleep_interruptible(data->auto_update_interval);
+
+		schedule_timeout(data->auto_update_interval * HZ / 1000);
 	}
 
 	complete_all(&data->auto_update_stop);
-- 
2.9.0

Re: [PATCH] hwmon: adt7470: Allow faster removal

[Guenter Roeck]

On 09/06/2016 03:49 PM, Joshua Scott wrote:
> adt7470_remove will wait for the update thread to complete before
> returning. This had a worst-case time of up to the user-configurable
> auto_update_interval.
>
> Replace msleep_interruptible with set_current_state and schedule_timeout
> so that kthread_stop will interrupt the sleep.
>
> Signed-off-by: Joshua Scott <joshua.scott@alliedtelesis.co.nz>
> ---
>  drivers/hwmon/adt7470.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index 7d185a9..e93c528 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c
> @@ -273,9 +273,12 @@ static int adt7470_update_thread(void *p)
>  		mutex_lock(&data->lock);
>  		adt7470_read_temperatures(client, data);
>  		mutex_unlock(&data->lock);
> +
> +		set_current_state(TASK_INTERRUPTIBLE);
>  		if (kthread_should_stop())
>  			break;
> -		msleep_interruptible(data->auto_update_interval);
> +
> +		schedule_timeout(data->auto_update_interval * HZ / 1000);

Please use msecs_to_jiffies(data->auto_update_interval).

Thanks,
Guenter