| Message ID | 20200905143230.195049-3-nchatrad@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | RFC: hwmon: few improvements to amd_energy driver | expand |
On 9/5/20 7:32 AM, Naveen Krishna Chatradhi wrote: > On a system with course grain resolution of energy unit (milli J) the > accumulation thread can be executed less frequently than on the system > with fine grain resolution(micro J). > > This patch sets the accumulation thread interval to an optimum value > calculated based on the (energy unit) resolution supported by the > hardware (assuming a peak wattage of 240W). > > Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com> > --- > drivers/hwmon/amd_energy.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c > index 9580a16185b8..f0a13d6cc419 100644 > --- a/drivers/hwmon/amd_energy.c > +++ b/drivers/hwmon/amd_energy.c > @@ -48,6 +48,7 @@ struct amd_energy_data { > struct sensor_accumulator *accums; > /* Energy Status Units */ > u64 energy_units; > + unsigned int timeout; > int nr_cpus; > int nr_socks; > int core_id; > @@ -215,6 +216,7 @@ static umode_t amd_energy_is_visible(const void *_data, > static int energy_accumulator(void *p) > { > struct amd_energy_data *data = (struct amd_energy_data *)p; > + unsigned int timeout = data->timeout; > > while (!kthread_should_stop()) { > /* > @@ -234,7 +236,7 @@ static int energy_accumulator(void *p) > * > * let us accumulate for every 100secs > */ > - schedule_timeout(msecs_to_jiffies(100000)); > + schedule_timeout(msecs_to_jiffies(timeout)); Numbers below are in seconds, used as milli-seconds here. > } > return 0; > } > @@ -331,6 +333,14 @@ static int amd_energy_probe(struct platform_device *pdev) > if (IS_ERR(hwmon_dev)) > return PTR_ERR(hwmon_dev); > > + /* Once in 3 minutes for a resolution of 1/2*16 */ * 16 or ^ 16 ? > + if (data->energy_units == 0x10) > + data->timeout = 3 * 60; 180 ms ? I assume this is a bug and meant to be 3 * 60 * 1000. > + > + /* Once in 3 days for a resolution of 1/2^6 */ > + if (data->energy_units == 0x6) > + data->timeout = 3 * 24 * 60 * 60; > + ... and else ? This needs to cover all cases, including those not currently existing. I would suggest to define a formula based on data->energy_units. The energy units value can be anything from 0..31. Based on your numbers, something like timeout_ms = BIT(34 - data->energy_units); should do. It translates to about 3.1 days for energy_units=6, and 4.3 minutes for energy_units=16. If that is too much, you can make it timeout_ms = BIT(33 - data->energy_units); To avoid overflow, it might make sense to max out at BIT(31). timeout_ms = BIT((min(31, 33 - data->energy_units)); Guenter > data->wrap_accumulate = kthread_run(energy_accumulator, data, > "%s", dev_name(hwmon_dev)); > if (IS_ERR(data->wrap_accumulate)) >
Hi Guenter, > On 9/5/20 7:32 AM, Naveen Krishna Chatradhi wrote: > > On a system with course grain resolution of energy unit (milli J) the > > accumulation thread can be executed less frequently than on the system > > with fine grain resolution(micro J). > > > > This patch sets the accumulation thread interval to an optimum value > > calculated based on the (energy unit) resolution supported by the > > hardware (assuming a peak wattage of 240W). > > > > Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com> > > --- > > drivers/hwmon/amd_energy.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c > > index 9580a16185b8..f0a13d6cc419 100644 > > --- a/drivers/hwmon/amd_energy.c > > +++ b/drivers/hwmon/amd_energy.c > > @@ -48,6 +48,7 @@ struct amd_energy_data { > > struct sensor_accumulator *accums; > > /* Energy Status Units */ > > u64 energy_units; > > + unsigned int timeout; > > int nr_cpus; > > int nr_socks; > > int core_id; > > @@ -215,6 +216,7 @@ static umode_t amd_energy_is_visible(const void > > *_data, static int energy_accumulator(void *p) { > > struct amd_energy_data *data = (struct amd_energy_data *)p; > > + unsigned int timeout = data->timeout; > > > > while (!kthread_should_stop()) { > > /* > > @@ -234,7 +236,7 @@ static int energy_accumulator(void *p) > > * > > * let us accumulate for every 100secs > > */ > > - schedule_timeout(msecs_to_jiffies(100000)); > > + schedule_timeout(msecs_to_jiffies(timeout)); > > Numbers below are in seconds, used as milli-seconds here. > > > } > > return 0; > > } > > @@ -331,6 +333,14 @@ static int amd_energy_probe(struct platform_device *pdev) > > if (IS_ERR(hwmon_dev)) > > return PTR_ERR(hwmon_dev); > > > > + /* Once in 3 minutes for a resolution of 1/2*16 */ > > * 16 or ^ 16 ? > > > + if (data->energy_units == 0x10) > > + data->timeout = 3 * 60; > > 180 ms ? I assume this is a bug and meant to be 3 * 60 * 1000. > > > + > > + /* Once in 3 days for a resolution of 1/2^6 */ > > + if (data->energy_units == 0x6) > > + data->timeout = 3 * 24 * 60 * 60; > > + > > ... and else ? > > This needs to cover all cases, including those not currently existing. > I would suggest to define a formula based on data->energy_units. > The energy units value can be anything from 0..31. Based on your numbers, something like > timeout_ms = BIT(34 - data->energy_units); should do. It translates to about 3.1 days for energy_units=6, and > 4.3 minutes for energy_units=16. If that is too much, you can make it > timeout_ms = BIT(33 - data->energy_units); > > To avoid overflow, it might make sense to max out at BIT(31). > timeout_ms = BIT((min(31, 33 - data->energy_units)); > Thank you for the feedback and suggestions. I will implement and submit the updated version. > Guenter > > > data->wrap_accumulate = kthread_run(energy_accumulator, data, > > "%s", dev_name(hwmon_dev)); > > if (IS_ERR(data->wrap_accumulate)) > > Naveen
diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c index 9580a16185b8..f0a13d6cc419 100644 --- a/drivers/hwmon/amd_energy.c +++ b/drivers/hwmon/amd_energy.c @@ -48,6 +48,7 @@ struct amd_energy_data { struct sensor_accumulator *accums; /* Energy Status Units */ u64 energy_units; + unsigned int timeout; int nr_cpus; int nr_socks; int core_id; @@ -215,6 +216,7 @@ static umode_t amd_energy_is_visible(const void *_data, static int energy_accumulator(void *p) { struct amd_energy_data *data = (struct amd_energy_data *)p; + unsigned int timeout = data->timeout; while (!kthread_should_stop()) { /* @@ -234,7 +236,7 @@ static int energy_accumulator(void *p) * * let us accumulate for every 100secs */ - schedule_timeout(msecs_to_jiffies(100000)); + schedule_timeout(msecs_to_jiffies(timeout)); } return 0; } @@ -331,6 +333,14 @@ static int amd_energy_probe(struct platform_device *pdev) if (IS_ERR(hwmon_dev)) return PTR_ERR(hwmon_dev); + /* Once in 3 minutes for a resolution of 1/2*16 */ + if (data->energy_units == 0x10) + data->timeout = 3 * 60; + + /* Once in 3 days for a resolution of 1/2^6 */ + if (data->energy_units == 0x6) + data->timeout = 3 * 24 * 60 * 60; + data->wrap_accumulate = kthread_run(energy_accumulator, data, "%s", dev_name(hwmon_dev)); if (IS_ERR(data->wrap_accumulate))
On a system with course grain resolution of energy unit (milli J) the accumulation thread can be executed less frequently than on the system with fine grain resolution(micro J). This patch sets the accumulation thread interval to an optimum value calculated based on the (energy unit) resolution supported by the hardware (assuming a peak wattage of 240W). Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com> --- drivers/hwmon/amd_energy.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)