| Message ID | 20160901051721.13806-1-joshua.scott@alliedtelesis.co.nz (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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?
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); > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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);
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(-)