Re: [RFC RESEND 0/3] Add watermark support to devfreq

Re: [RFC RESEND 0/3] Add watermark support to devfreq

[MyungJoo Ham]

> (Sorry for the spam. I am resending the series because I noted that
> some of the email addresses were mistyped)
> 
> Currently main mechanism to implement scaling using devfreq is
> polling and the device profile is free to set polling interval.
> However, in many cases this approach is not optimal; We may
> unnecessarily use CPU time to determine load on engine that
> is idle most of time - or we may simply read load when we
> already know that the device is busy.
> 
> In some cases a device itself has counters to track its activity
> and possibility to raise interrupts when load goes above or below
> certain threshold.
> 
> This series adds support for watermark events to devfreq and
> introduces two example governors. The first patch adds two
> callbacks to the device profile (for setting watermark) and
> adds a new function call to devfreq that informs of crossed
> watermark.
> 
> The added governors demonstrate usage of the new API. The first
> governor (watermark simple) sets device to trigger low watermark
> event when load goes below 10% and high watermark interrupt when
> the load goes above 60%. Watermark active tries to keep load at
> certain level and it actively sets thresholds based on the
> frequency table in order get interrupts only when the load value
> would affect to the current frequency in re-estimation.

Hi Arto,


Please let me start with somewhat naive high-level question:
  What do you mean by watermark in this context?
Is it a product name of yours (interrupt-based PMU, I presume)?
Or does watermark have another semantics that I am not aware of?
Or do you really mean something like
  http://en.wikipedia.org/wiki/Digital_watermarking

Other itching points include:
- devfreq_watermark_event() was declared but has never been used.
    Who is supposed to call this function?
- Is enum watermark_type supposed to be used out of /drivers/devfreq/* ?
    Otherwise, please move it inside /drivers/devfreq/governor.h
    (I guess it is to be used inside corresponding governors only)
- Could you please watermark-specific interfaces (set_high/low_wmark) into
    its own public header file? (e.g., /include/linux/devfreq/governor_wmark.h)
  I think we can create another (governor_simpleondemand.h) in there as well
    in order to have threshold values configured by device drivers.
  Adding governor-specific configurations into devfreq.h seems not
    appropriate especially when we expect that we may need many different
    governors.

  OR..

  This seems that you can keep set_h/l_wmark functions exposed to
    drivers/devfreq/* only. Therefore, having /drivers/devfreq/governor_wmark.h
    should be sufficient as well, which should be more neat than the above.
  The callbacks are to be defined by the devfreq drivers, aren't they?

- The event name, "DEVFREQ_GOV_WMARK", defined in governor.h, seems to be
    more appropriate if it is named "DEVFREQ_GOV_INTERNAL" as we won't need
    to define event names for each govornor's internal needs.

  OR.. for more generality,
    we may define a macro like:

#define DEVFREQ_GOV_INTERNAL(value)	((0x1 << 31) | (value))
#define GET_DEVFREQ_GOV_INTERNAL(event)	((event) & ~(0x1 << 31))

- In general, I would love to see governors with minimal intervention on
    the framework core/main code, especially when it is not beneficial to
    other governors. Unlike cpufreq, we may contain many different types of
    devices in devfreq, which has the potential to accompany many different
    governors.


Cheers,
MyungJoo.

> 
> Arto Merilainen (1):
>   PM / devfreq: Add watermark active governor
> 
> Shridhar Rasal (2):
>   PM / devfreq: Add watermark events
>   PM / devfreq: Add watermark simple governor
> 
>  drivers/devfreq/Kconfig                 |  18 +++
>  drivers/devfreq/Makefile                |   2 +
>  drivers/devfreq/devfreq.c               |  19 +++
>  drivers/devfreq/governor.h              |   1 +
>  drivers/devfreq/governor_wmark_active.c | 276 ++++++++++++++++++++++++++++++++
>  drivers/devfreq/governor_wmark_simple.c | 245 ++++++++++++++++++++++++++++
>  include/linux/devfreq.h                 |  26 +++
>  7 files changed, 587 insertions(+)
>  create mode 100644 drivers/devfreq/governor_wmark_active.c
>  create mode 100644 drivers/devfreq/governor_wmark_simple.c
> 
> -- 
> 1.8.1.5
> 

Re: [RFC RESEND 0/3] Add watermark support to devfreq

[MyungJoo Ham]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 4484 bytes --]

> (Sorry for the spam. I am resending the series because I noted that
> some of the email addresses were mistyped)
> 
> Currently main mechanism to implement scaling using devfreq is
> polling and the device profile is free to set polling interval.
> However, in many cases this approach is not optimal; We may
> unnecessarily use CPU time to determine load on engine that
> is idle most of time - or we may simply read load when we
> already know that the device is busy.
> 
> In some cases a device itself has counters to track its activity
> and possibility to raise interrupts when load goes above or below
> certain threshold.
> 
> This series adds support for watermark events to devfreq and
> introduces two example governors. The first patch adds two
> callbacks to the device profile (for setting watermark) and
> adds a new function call to devfreq that informs of crossed
> watermark.
> 
> The added governors demonstrate usage of the new API. The first
> governor (watermark simple) sets device to trigger low watermark
> event when load goes below 10% and high watermark interrupt when
> the load goes above 60%. Watermark active tries to keep load at
> certain level and it actively sets thresholds based on the
> frequency table in order get interrupts only when the load value
> would affect to the current frequency in re-estimation.

Hi Arto,


Please let me start with somewhat naive high-level question:
  What do you mean by watermark in this context?
Is it a product name of yours (interrupt-based PMU, I presume)?
Or does watermark have another semantics that I am not aware of?
Or do you really mean something like
  http://en.wikipedia.org/wiki/Digital_watermarking

Other itching points include:
- devfreq_watermark_event() was declared but has never been used.
    Who is supposed to call this function?
- Is enum watermark_type supposed to be used out of /drivers/devfreq/* ?
    Otherwise, please move it inside /drivers/devfreq/governor.h
    (I guess it is to be used inside corresponding governors only)
- Could you please watermark-specific interfaces (set_high/low_wmark) into
    its own public header file? (e.g., /include/linux/devfreq/governor_wmark.h)
  I think we can create another (governor_simpleondemand.h) in there as well
    in order to have threshold values configured by device drivers.
  Adding governor-specific configurations into devfreq.h seems not
    appropriate especially when we expect that we may need many different
    governors.

  OR..

  This seems that you can keep set_h/l_wmark functions exposed to
    drivers/devfreq/* only. Therefore, having /drivers/devfreq/governor_wmark.h
    should be sufficient as well, which should be more neat than the above.
  The callbacks are to be defined by the devfreq drivers, aren't they?

- The event name, "DEVFREQ_GOV_WMARK", defined in governor.h, seems to be
    more appropriate if it is named "DEVFREQ_GOV_INTERNAL" as we won't need
    to define event names for each govornor's internal needs.

  OR.. for more generality,
    we may define a macro like:

#define DEVFREQ_GOV_INTERNAL(value)	((0x1 << 31) | (value))
#define GET_DEVFREQ_GOV_INTERNAL(event)	((event) & ~(0x1 << 31))

- In general, I would love to see governors with minimal intervention on
    the framework core/main code, especially when it is not beneficial to
    other governors. Unlike cpufreq, we may contain many different types of
    devices in devfreq, which has the potential to accompany many different
    governors.


Cheers,
MyungJoo.

> 
> Arto Merilainen (1):
>   PM / devfreq: Add watermark active governor
> 
> Shridhar Rasal (2):
>   PM / devfreq: Add watermark events
>   PM / devfreq: Add watermark simple governor
> 
>  drivers/devfreq/Kconfig                 |  18 +++
>  drivers/devfreq/Makefile                |   2 +
>  drivers/devfreq/devfreq.c               |  19 +++
>  drivers/devfreq/governor.h              |   1 +
>  drivers/devfreq/governor_wmark_active.c | 276 ++++++++++++++++++++++++++++++++
>  drivers/devfreq/governor_wmark_simple.c | 245 ++++++++++++++++++++++++++++
>  include/linux/devfreq.h                 |  26 +++
>  7 files changed, 587 insertions(+)
>  create mode 100644 drivers/devfreq/governor_wmark_active.c
>  create mode 100644 drivers/devfreq/governor_wmark_simple.c
> 
> -- 
> 1.8.1.5
> 
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [RFC RESEND 0/3] Add watermark support to devfreq

[Tomeu Vizoso]

On 8 December 2014 at 08:44, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>> (Sorry for the spam. I am resending the series because I noted that
>> some of the email addresses were mistyped)
>>
>> Currently main mechanism to implement scaling using devfreq is
>> polling and the device profile is free to set polling interval.
>> However, in many cases this approach is not optimal; We may
>> unnecessarily use CPU time to determine load on engine that
>> is idle most of time - or we may simply read load when we
>> already know that the device is busy.
>>
>> In some cases a device itself has counters to track its activity
>> and possibility to raise interrupts when load goes above or below
>> certain threshold.
>>
>> This series adds support for watermark events to devfreq and
>> introduces two example governors. The first patch adds two
>> callbacks to the device profile (for setting watermark) and
>> adds a new function call to devfreq that informs of crossed
>> watermark.
>>
>> The added governors demonstrate usage of the new API. The first
>> governor (watermark simple) sets device to trigger low watermark
>> event when load goes below 10% and high watermark interrupt when
>> the load goes above 60%. Watermark active tries to keep load at
>> certain level and it actively sets thresholds based on the
>> frequency table in order get interrupts only when the load value
>> would affect to the current frequency in re-estimation.
>
> Hi Arto,
>
>
> Please let me start with somewhat naive high-level question:
>   What do you mean by watermark in this context?
> Is it a product name of yours (interrupt-based PMU, I presume)?
> Or does watermark have another semantics that I am not aware of?
> Or do you really mean something like
>   http://en.wikipedia.org/wiki/Digital_watermarking

The watermark term is borrowed here from queues. The high and low
watermarks define the boundaries within the load has to be contained.
If the load is above or below the watermarks, a more suitable
frequency has to be found.

Regards,

Tomeu

> Other itching points include:
> - devfreq_watermark_event() was declared but has never been used.
>     Who is supposed to call this function?
> - Is enum watermark_type supposed to be used out of /drivers/devfreq/* ?
>     Otherwise, please move it inside /drivers/devfreq/governor.h
>     (I guess it is to be used inside corresponding governors only)
> - Could you please watermark-specific interfaces (set_high/low_wmark) into
>     its own public header file? (e.g., /include/linux/devfreq/governor_wmark.h)
>   I think we can create another (governor_simpleondemand.h) in there as well
>     in order to have threshold values configured by device drivers.
>   Adding governor-specific configurations into devfreq.h seems not
>     appropriate especially when we expect that we may need many different
>     governors.
>
>   OR..
>
>   This seems that you can keep set_h/l_wmark functions exposed to
>     drivers/devfreq/* only. Therefore, having /drivers/devfreq/governor_wmark.h
>     should be sufficient as well, which should be more neat than the above.
>   The callbacks are to be defined by the devfreq drivers, aren't they?
>
> - The event name, "DEVFREQ_GOV_WMARK", defined in governor.h, seems to be
>     more appropriate if it is named "DEVFREQ_GOV_INTERNAL" as we won't need
>     to define event names for each govornor's internal needs.
>
>   OR.. for more generality,
>     we may define a macro like:
>
> #define DEVFREQ_GOV_INTERNAL(value)     ((0x1 << 31) | (value))
> #define GET_DEVFREQ_GOV_INTERNAL(event) ((event) & ~(0x1 << 31))
>
> - In general, I would love to see governors with minimal intervention on
>     the framework core/main code, especially when it is not beneficial to
>     other governors. Unlike cpufreq, we may contain many different types of
>     devices in devfreq, which has the potential to accompany many different
>     governors.
>
>
> Cheers,
> MyungJoo.
>
>>
>> Arto Merilainen (1):
>>   PM / devfreq: Add watermark active governor
>>
>> Shridhar Rasal (2):
>>   PM / devfreq: Add watermark events
>>   PM / devfreq: Add watermark simple governor
>>
>>  drivers/devfreq/Kconfig                 |  18 +++
>>  drivers/devfreq/Makefile                |   2 +
>>  drivers/devfreq/devfreq.c               |  19 +++
>>  drivers/devfreq/governor.h              |   1 +
>>  drivers/devfreq/governor_wmark_active.c | 276 ++++++++++++++++++++++++++++++++
>>  drivers/devfreq/governor_wmark_simple.c | 245 ++++++++++++++++++++++++++++
>>  include/linux/devfreq.h                 |  26 +++
>>  7 files changed, 587 insertions(+)
>>  create mode 100644 drivers/devfreq/governor_wmark_active.c
>>  create mode 100644 drivers/devfreq/governor_wmark_simple.c
>>
>> --
>> 1.8.1.5
>>

Re: [RFC RESEND 0/3] Add watermark support to devfreq

[Arto Merilainen]

Hi MyungJoo,

Thank for your answer, see my answers inline.

On 12/08/2014 09:44 AM, MyungJoo Ham wrote:
> Please let me start with somewhat naive high-level question:
>    What do you mean by watermark in this context?

Sorry for poor choice of naming. "Load threshold interrupt" might be 
more appropriate. The idea of the series here is to add an ability to 
program hardware to give interrupts when the current load value goes 
above (or below) certain threshold(s). I will revisit the naming.

> Other itching points include:
> - devfreq_watermark_event() was declared but has never been used.
>      Who is supposed to call this function?

The device profile is responsible to call this function from an 
interrupt handler. I will document this.

> - Is enum watermark_type supposed to be used out of /drivers/devfreq/* ?

Yes, the idea is to use the enumerations for informing the type of the 
event. I.e. from interrupt handler we might call:
   devfreq_watermark_event(devfreq, ACTMON_INTR_BELOW_WMARK);

> - Could you please watermark-specific interfaces (set_high/low_wmark) into
>      its own public header file? (e.g., /include/linux/devfreq/governor_wmark.h)
>    I think we can create another (governor_simpleondemand.h) in there as well
>      in order to have threshold values configured by device drivers.
>    Adding governor-specific configurations into devfreq.h seems not
>      appropriate especially when we expect that we may need many different
>      governors.

I am not convinced that creating two separate interfaces is better than 
extending a single centralised interface. Let us first consider the 
compatibility...:

First, in trivial case the device driver does not implement the 
threshold feature. Obviously we cannot use threshold based governor for 
scaling but all existing polling based governors will work as earlier.

Second, let us assume that we have a device that supports threshold 
interrupts and the driver has implemented the required set_h/l_wmark 
interrupts and calls devfreq_watermark_event() when we get an interrupt. 
This device can therefore use wmark_simple and wmark_active governors. 
In addition to the wmark governors, we may choose to use a polling based 
governor. With current approach the polling governor simply ignores the 
events and leaves the thresholds "as is".

In short, I would not be concerned on compatibility. From governor side 
it should be ok to ignore an event. In addition, it is ok from governor 
initialization to fail in case the device driver does not implement all 
required features.


I am also afraid that initialisation makes two-communication-paths 
approach tricky. In code you likely could have..:
   devfreq_add_device(dev, &pdata->devfreq, "wmark_simple", &extra_fns);

.. but what happens if we wish to change the governor on the fly through 
sysfs? In worst case this is targeting to a governor that requires its 
own data field in initialisation.

>
>    OR..
>
>    This seems that you can keep set_h/l_wmark functions exposed to
>      drivers/devfreq/* only. Therefore, having /drivers/devfreq/governor_wmark.h
>      should be sufficient as well, which should be more neat than the above.
>    The callbacks are to be defined by the devfreq drivers, aren't they?

set_h/l_wmark callbacks should be implemented by the device profiles 
(the device drivers themselves) for setting the thresholds on hardware. 
These are not devfreq/governor private functions but something that 
should be implemented inside the device driver.

>
> - The event name, "DEVFREQ_GOV_WMARK", defined in governor.h, seems to be
>      more appropriate if it is named "DEVFREQ_GOV_INTERNAL" as we won't need
>      to define event names for each govornor's internal needs.
>
>    OR.. for more generality,
>      we may define a macro like:
>
> #define DEVFREQ_GOV_INTERNAL(value)	((0x1 << 31) | (value))
> #define GET_DEVFREQ_GOV_INTERNAL(event)	((event) & ~(0x1 << 31))

This should work.

>
> - In general, I would love to see governors with minimal intervention on
>      the framework core/main code, especially when it is not beneficial to
>      other governors. Unlike cpufreq, we may contain many different types of
>      devices in devfreq, which has the potential to accompany many different
>      governors.
>

I would like to see is that we could easily take advantage of hardware 
features and not let framework limit us too much. :-)

For example, Tomeu has been working on ACTMON driver that measures 
memory transfers (please refer to "Add support for Tegra Activity 
Monitor" series). This hardware block is capable of generating 
interrupts based on activity and hence by factoring the series properly, 
we could use the same governor from the series in different places 
easily. Without proper interfacing everyone ends up duplicating the code.

- Arto

>
> Cheers,
> MyungJoo.
>
>>
>> Arto Merilainen (1):
>>    PM / devfreq: Add watermark active governor
>>
>> Shridhar Rasal (2):
>>    PM / devfreq: Add watermark events
>>    PM / devfreq: Add watermark simple governor
>>
>>   drivers/devfreq/Kconfig                 |  18 +++
>>   drivers/devfreq/Makefile                |   2 +
>>   drivers/devfreq/devfreq.c               |  19 +++
>>   drivers/devfreq/governor.h              |   1 +
>>   drivers/devfreq/governor_wmark_active.c | 276 ++++++++++++++++++++++++++++++++
>>   drivers/devfreq/governor_wmark_simple.c | 245 ++++++++++++++++++++++++++++
>>   include/linux/devfreq.h                 |  26 +++
>>   7 files changed, 587 insertions(+)
>>   create mode 100644 drivers/devfreq/governor_wmark_active.c
>>   create mode 100644 drivers/devfreq/governor_wmark_simple.c
>>
>> --
>> 1.8.1.5
>>

Re: [RFC RESEND 0/3] Add watermark support to devfreq

[Alexandre Courbot]

On Tue, Dec 9, 2014 at 2:07 AM, Arto Merilainen <amerilainen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Hi MyungJoo,
>
> Thank for your answer, see my answers inline.
>
> On 12/08/2014 09:44 AM, MyungJoo Ham wrote:
>>
>> Please let me start with somewhat naive high-level question:
>>    What do you mean by watermark in this context?
>
>
> Sorry for poor choice of naming. "Load threshold interrupt" might be more
> appropriate. The idea of the series here is to add an ability to program
> hardware to give interrupts when the current load value goes above (or
> below) certain threshold(s). I will revisit the naming.

I personally like the watermark naming and think it is a pretty good
analogy to what this series is doing, but your call.

>
>> Other itching points include:
>> - devfreq_watermark_event() was declared but has never been used.
>>      Who is supposed to call this function?
>
>
> The device profile is responsible to call this function from an interrupt
> handler. I will document this.
>
>> - Is enum watermark_type supposed to be used out of /drivers/devfreq/* ?
>
>
> Yes, the idea is to use the enumerations for informing the type of the
> event. I.e. from interrupt handler we might call:
>   devfreq_watermark_event(devfreq, ACTMON_INTR_BELOW_WMARK);
>
>> - Could you please watermark-specific interfaces (set_high/low_wmark) into
>>      its own public header file? (e.g.,
>> /include/linux/devfreq/governor_wmark.h)
>>    I think we can create another (governor_simpleondemand.h) in there as
>> well
>>      in order to have threshold values configured by device drivers.
>>    Adding governor-specific configurations into devfreq.h seems not
>>      appropriate especially when we expect that we may need many different
>>      governors.

Considering that watermarking is a pretty common technique for the
kind of work that devfreq is attempting to cover (implementing
event-driven frequency scaling vs. polling-driven as current devfreq
does), doesn't it make sense to turn it into a core devfreq feature? I
don't think devfreq can be that easily extended and doing so might
make things messier than they are.

>
>
> I am not convinced that creating two separate interfaces is better than
> extending a single centralised interface. Let us first consider the
> compatibility...:
>
> First, in trivial case the device driver does not implement the threshold
> feature. Obviously we cannot use threshold based governor for scaling but
> all existing polling based governors will work as earlier.
>
> Second, let us assume that we have a device that supports threshold
> interrupts and the driver has implemented the required set_h/l_wmark
> interrupts and calls devfreq_watermark_event() when we get an interrupt.
> This device can therefore use wmark_simple and wmark_active governors. In
> addition to the wmark governors, we may choose to use a polling based
> governor. With current approach the polling governor simply ignores the
> events and leaves the thresholds "as is".
>
> In short, I would not be concerned on compatibility. From governor side it
> should be ok to ignore an event. In addition, it is ok from governor
> initialization to fail in case the device driver does not implement all
> required features.
>
>
> I am also afraid that initialisation makes two-communication-paths approach
> tricky. In code you likely could have..:
>   devfreq_add_device(dev, &pdata->devfreq, "wmark_simple", &extra_fns);
>
> .. but what happens if we wish to change the governor on the fly through
> sysfs? In worst case this is targeting to a governor that requires its own
> data field in initialisation.
>
>>
>>    OR..
>>
>>    This seems that you can keep set_h/l_wmark functions exposed to
>>      drivers/devfreq/* only. Therefore, having
>> /drivers/devfreq/governor_wmark.h
>>      should be sufficient as well, which should be more neat than the
>> above.
>>    The callbacks are to be defined by the devfreq drivers, aren't they?
>
>
> set_h/l_wmark callbacks should be implemented by the device profiles (the
> device drivers themselves) for setting the thresholds on hardware. These are
> not devfreq/governor private functions but something that should be
> implemented inside the device driver.
>
>>
>> - The event name, "DEVFREQ_GOV_WMARK", defined in governor.h, seems to be
>>      more appropriate if it is named "DEVFREQ_GOV_INTERNAL" as we won't
>> need
>>      to define event names for each govornor's internal needs.
>>
>>    OR.. for more generality,
>>      we may define a macro like:
>>
>> #define DEVFREQ_GOV_INTERNAL(value)     ((0x1 << 31) | (value))
>> #define GET_DEVFREQ_GOV_INTERNAL(event) ((event) & ~(0x1 << 31))
>
>
> This should work.
>
>>
>> - In general, I would love to see governors with minimal intervention on
>>      the framework core/main code, especially when it is not beneficial to
>>      other governors. Unlike cpufreq, we may contain many different types
>> of
>>      devices in devfreq, which has the potential to accompany many
>> different
>>      governors.
>>
>
> I would like to see is that we could easily take advantage of hardware
> features and not let framework limit us too much. :-)
>
> For example, Tomeu has been working on ACTMON driver that measures memory
> transfers (please refer to "Add support for Tegra Activity Monitor" series).
> This hardware block is capable of generating interrupts based on activity
> and hence by factoring the series properly, we could use the same governor
> from the series in different places easily. Without proper interfacing
> everyone ends up duplicating the code.

Indeed. Watermarking support would make the ACTMON driver simpler and
easier to integrate with all the governors that devfreq currently has.
Without core support for it, it could only interact properly with its
own, dedicated governor.

MyungJoo, the issue of having this feature in the core vs. having it
as a devfreq "extension" aside, do you agree with the core idea? An
informl ack for the idea would allow us to start leveraging this for
the ACTMON driver. With a concrete user it will make it easier for
everyone to understand how the pieces should fit together.

Re: [RFC RESEND 0/3] Add watermark support to devfreq

[Alexandre Courbot]

On Tue, Dec 9, 2014 at 2:07 AM, Arto Merilainen <amerilainen@nvidia.com> wrote:
> Hi MyungJoo,
>
> Thank for your answer, see my answers inline.
>
> On 12/08/2014 09:44 AM, MyungJoo Ham wrote:
>>
>> Please let me start with somewhat naive high-level question:
>>    What do you mean by watermark in this context?
>
>
> Sorry for poor choice of naming. "Load threshold interrupt" might be more
> appropriate. The idea of the series here is to add an ability to program
> hardware to give interrupts when the current load value goes above (or
> below) certain threshold(s). I will revisit the naming.

I personally like the watermark naming and think it is a pretty good
analogy to what this series is doing, but your call.

>
>> Other itching points include:
>> - devfreq_watermark_event() was declared but has never been used.
>>      Who is supposed to call this function?
>
>
> The device profile is responsible to call this function from an interrupt
> handler. I will document this.
>
>> - Is enum watermark_type supposed to be used out of /drivers/devfreq/* ?
>
>
> Yes, the idea is to use the enumerations for informing the type of the
> event. I.e. from interrupt handler we might call:
>   devfreq_watermark_event(devfreq, ACTMON_INTR_BELOW_WMARK);
>
>> - Could you please watermark-specific interfaces (set_high/low_wmark) into
>>      its own public header file? (e.g.,
>> /include/linux/devfreq/governor_wmark.h)
>>    I think we can create another (governor_simpleondemand.h) in there as
>> well
>>      in order to have threshold values configured by device drivers.
>>    Adding governor-specific configurations into devfreq.h seems not
>>      appropriate especially when we expect that we may need many different
>>      governors.

Considering that watermarking is a pretty common technique for the
kind of work that devfreq is attempting to cover (implementing
event-driven frequency scaling vs. polling-driven as current devfreq
does), doesn't it make sense to turn it into a core devfreq feature? I
don't think devfreq can be that easily extended and doing so might
make things messier than they are.

>
>
> I am not convinced that creating two separate interfaces is better than
> extending a single centralised interface. Let us first consider the
> compatibility...:
>
> First, in trivial case the device driver does not implement the threshold
> feature. Obviously we cannot use threshold based governor for scaling but
> all existing polling based governors will work as earlier.
>
> Second, let us assume that we have a device that supports threshold
> interrupts and the driver has implemented the required set_h/l_wmark
> interrupts and calls devfreq_watermark_event() when we get an interrupt.
> This device can therefore use wmark_simple and wmark_active governors. In
> addition to the wmark governors, we may choose to use a polling based
> governor. With current approach the polling governor simply ignores the
> events and leaves the thresholds "as is".
>
> In short, I would not be concerned on compatibility. From governor side it
> should be ok to ignore an event. In addition, it is ok from governor
> initialization to fail in case the device driver does not implement all
> required features.
>
>
> I am also afraid that initialisation makes two-communication-paths approach
> tricky. In code you likely could have..:
>   devfreq_add_device(dev, &pdata->devfreq, "wmark_simple", &extra_fns);
>
> .. but what happens if we wish to change the governor on the fly through
> sysfs? In worst case this is targeting to a governor that requires its own
> data field in initialisation.
>
>>
>>    OR..
>>
>>    This seems that you can keep set_h/l_wmark functions exposed to
>>      drivers/devfreq/* only. Therefore, having
>> /drivers/devfreq/governor_wmark.h
>>      should be sufficient as well, which should be more neat than the
>> above.
>>    The callbacks are to be defined by the devfreq drivers, aren't they?
>
>
> set_h/l_wmark callbacks should be implemented by the device profiles (the
> device drivers themselves) for setting the thresholds on hardware. These are
> not devfreq/governor private functions but something that should be
> implemented inside the device driver.
>
>>
>> - The event name, "DEVFREQ_GOV_WMARK", defined in governor.h, seems to be
>>      more appropriate if it is named "DEVFREQ_GOV_INTERNAL" as we won't
>> need
>>      to define event names for each govornor's internal needs.
>>
>>    OR.. for more generality,
>>      we may define a macro like:
>>
>> #define DEVFREQ_GOV_INTERNAL(value)     ((0x1 << 31) | (value))
>> #define GET_DEVFREQ_GOV_INTERNAL(event) ((event) & ~(0x1 << 31))
>
>
> This should work.
>
>>
>> - In general, I would love to see governors with minimal intervention on
>>      the framework core/main code, especially when it is not beneficial to
>>      other governors. Unlike cpufreq, we may contain many different types
>> of
>>      devices in devfreq, which has the potential to accompany many
>> different
>>      governors.
>>
>
> I would like to see is that we could easily take advantage of hardware
> features and not let framework limit us too much. :-)
>
> For example, Tomeu has been working on ACTMON driver that measures memory
> transfers (please refer to "Add support for Tegra Activity Monitor" series).
> This hardware block is capable of generating interrupts based on activity
> and hence by factoring the series properly, we could use the same governor
> from the series in different places easily. Without proper interfacing
> everyone ends up duplicating the code.

Indeed. Watermarking support would make the ACTMON driver simpler and
easier to integrate with all the governors that devfreq currently has.
Without core support for it, it could only interact properly with its
own, dedicated governor.

MyungJoo, the issue of having this feature in the core vs. having it
as a devfreq "extension" aside, do you agree with the core idea? An
informl ack for the idea would allow us to start leveraging this for
the ACTMON driver. With a concrete user it will make it easier for
everyone to understand how the pieces should fit together.

Re: [RFC RESEND 0/3] Add watermark support to devfreq

[Tomeu Vizoso]

On 5 December 2014 at 14:41, Arto Merilainen <amerilainen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> (Sorry for the spam. I am resending the series because I noted that
> some of the email addresses were mistyped)
>
> Currently main mechanism to implement scaling using devfreq is
> polling and the device profile is free to set polling interval.
> However, in many cases this approach is not optimal; We may
> unnecessarily use CPU time to determine load on engine that
> is idle most of time - or we may simply read load when we
> already know that the device is busy.
>
> In some cases a device itself has counters to track its activity
> and possibility to raise interrupts when load goes above or below
> certain threshold.

Hi Arto,

this is very interesting stuff, thanks for sending.

Regarding the question of whether the ACTMON driver should use this
generic governor, I think it comes down to whether we are able to
reach the best compromise between performance and power usage with it.

I see that the logic of the downstream ACTMON driver is much more
complex, as it takes into account the activity caused by the CPU
complex and gives it more weight than the total. It also sets
watermarks for the running average over the last 128 samples. The
frequency of the CPU is also taken into account.

Besides that, there's several factors and adjustments that I can only
think that have been determined through experimentation and
measurement and that in that case are very likely to be specific to a
given board and/or SoC.

It also takes into account consecutive watermark breaches and boosts
the frequency accordingly.

It would be great if you could ask within NVIDIA for the rationale
behind the way of calculating the target frequency downstream, as it
would allow us to better discern what can be made generic and what
belongs to the ACTMON driver itself.

It would be also good if we can find other SoCs that have similar
functionality, so we can make sure that what we end up adding to the
generic framework is of use to others.

Besides the above general points, two more specific comments:

* why not use the opp freq table from the DT (operating-points)
instead of freq_table?

* why having wmark_simple at all? And if we don't actually want it,
guess we can drop the event type arg?

Thanks,

Tomeu


> This series adds support for watermark events to devfreq and
> introduces two example governors. The first patch adds two
> callbacks to the device profile (for setting watermark) and
> adds a new function call to devfreq that informs of crossed
> watermark.
>
> The added governors demonstrate usage of the new API. The first
> governor (watermark simple) sets device to trigger low watermark
> event when load goes below 10% and high watermark interrupt when
> the load goes above 60%. Watermark active tries to keep load at
> certain level and it actively sets thresholds based on the
> frequency table in order get interrupts only when the load value
> would affect to the current frequency in re-estimation.
>
> Arto Merilainen (1):
>   PM / devfreq: Add watermark active governor
>
> Shridhar Rasal (2):
>   PM / devfreq: Add watermark events
>   PM / devfreq: Add watermark simple governor
>
>  drivers/devfreq/Kconfig                 |  18 +++
>  drivers/devfreq/Makefile                |   2 +
>  drivers/devfreq/devfreq.c               |  19 +++
>  drivers/devfreq/governor.h              |   1 +
>  drivers/devfreq/governor_wmark_active.c | 276 ++++++++++++++++++++++++++++++++
>  drivers/devfreq/governor_wmark_simple.c | 245 ++++++++++++++++++++++++++++
>  include/linux/devfreq.h                 |  26 +++
>  7 files changed, 587 insertions(+)
>  create mode 100644 drivers/devfreq/governor_wmark_active.c
>  create mode 100644 drivers/devfreq/governor_wmark_simple.c
>
> --
> 1.8.1.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC RESEND 0/3] Add watermark support to devfreq

[Tomeu Vizoso]

On 5 December 2014 at 14:41, Arto Merilainen <amerilainen@nvidia.com> wrote:
> (Sorry for the spam. I am resending the series because I noted that
> some of the email addresses were mistyped)
>
> Currently main mechanism to implement scaling using devfreq is
> polling and the device profile is free to set polling interval.
> However, in many cases this approach is not optimal; We may
> unnecessarily use CPU time to determine load on engine that
> is idle most of time - or we may simply read load when we
> already know that the device is busy.
>
> In some cases a device itself has counters to track its activity
> and possibility to raise interrupts when load goes above or below
> certain threshold.

Hi Arto,

this is very interesting stuff, thanks for sending.

Regarding the question of whether the ACTMON driver should use this
generic governor, I think it comes down to whether we are able to
reach the best compromise between performance and power usage with it.

I see that the logic of the downstream ACTMON driver is much more
complex, as it takes into account the activity caused by the CPU
complex and gives it more weight than the total. It also sets
watermarks for the running average over the last 128 samples. The
frequency of the CPU is also taken into account.

Besides that, there's several factors and adjustments that I can only
think that have been determined through experimentation and
measurement and that in that case are very likely to be specific to a
given board and/or SoC.

It also takes into account consecutive watermark breaches and boosts
the frequency accordingly.

It would be great if you could ask within NVIDIA for the rationale
behind the way of calculating the target frequency downstream, as it
would allow us to better discern what can be made generic and what
belongs to the ACTMON driver itself.

It would be also good if we can find other SoCs that have similar
functionality, so we can make sure that what we end up adding to the
generic framework is of use to others.

Besides the above general points, two more specific comments:

* why not use the opp freq table from the DT (operating-points)
instead of freq_table?

* why having wmark_simple at all? And if we don't actually want it,
guess we can drop the event type arg?

Thanks,

Tomeu


> This series adds support for watermark events to devfreq and
> introduces two example governors. The first patch adds two
> callbacks to the device profile (for setting watermark) and
> adds a new function call to devfreq that informs of crossed
> watermark.
>
> The added governors demonstrate usage of the new API. The first
> governor (watermark simple) sets device to trigger low watermark
> event when load goes below 10% and high watermark interrupt when
> the load goes above 60%. Watermark active tries to keep load at
> certain level and it actively sets thresholds based on the
> frequency table in order get interrupts only when the load value
> would affect to the current frequency in re-estimation.
>
> Arto Merilainen (1):
>   PM / devfreq: Add watermark active governor
>
> Shridhar Rasal (2):
>   PM / devfreq: Add watermark events
>   PM / devfreq: Add watermark simple governor
>
>  drivers/devfreq/Kconfig                 |  18 +++
>  drivers/devfreq/Makefile                |   2 +
>  drivers/devfreq/devfreq.c               |  19 +++
>  drivers/devfreq/governor.h              |   1 +
>  drivers/devfreq/governor_wmark_active.c | 276 ++++++++++++++++++++++++++++++++
>  drivers/devfreq/governor_wmark_simple.c | 245 ++++++++++++++++++++++++++++
>  include/linux/devfreq.h                 |  26 +++
>  7 files changed, 587 insertions(+)
>  create mode 100644 drivers/devfreq/governor_wmark_active.c
>  create mode 100644 drivers/devfreq/governor_wmark_simple.c
>
> --
> 1.8.1.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC RESEND 0/3] Add watermark support to devfreq

[Arto Merilainen]

(Sorry for the spam. I am resending the series because I noted that
some of the email addresses were mistyped)

Currently main mechanism to implement scaling using devfreq is
polling and the device profile is free to set polling interval.
However, in many cases this approach is not optimal; We may
unnecessarily use CPU time to determine load on engine that
is idle most of time - or we may simply read load when we
already know that the device is busy.

In some cases a device itself has counters to track its activity
and possibility to raise interrupts when load goes above or below
certain threshold.

This series adds support for watermark events to devfreq and
introduces two example governors. The first patch adds two
callbacks to the device profile (for setting watermark) and
adds a new function call to devfreq that informs of crossed
watermark.

The added governors demonstrate usage of the new API. The first
governor (watermark simple) sets device to trigger low watermark
event when load goes below 10% and high watermark interrupt when
the load goes above 60%. Watermark active tries to keep load at
certain level and it actively sets thresholds based on the
frequency table in order get interrupts only when the load value
would affect to the current frequency in re-estimation.

Arto Merilainen (1):
  PM / devfreq: Add watermark active governor

Shridhar Rasal (2):
  PM / devfreq: Add watermark events
  PM / devfreq: Add watermark simple governor

 drivers/devfreq/Kconfig                 |  18 +++
 drivers/devfreq/Makefile                |   2 +
 drivers/devfreq/devfreq.c               |  19 +++
 drivers/devfreq/governor.h              |   1 +
 drivers/devfreq/governor_wmark_active.c | 276 ++++++++++++++++++++++++++++++++
 drivers/devfreq/governor_wmark_simple.c | 245 ++++++++++++++++++++++++++++
 include/linux/devfreq.h                 |  26 +++
 7 files changed, 587 insertions(+)
 create mode 100644 drivers/devfreq/governor_wmark_active.c
 create mode 100644 drivers/devfreq/governor_wmark_simple.c

-- 
1.8.1.5

[RFC RESEND 0/3] Add watermark support to devfreq

[Arto Merilainen]

(Sorry for the spam. I am resending the series because I noted that
some of the email addresses were mistyped)

Currently main mechanism to implement scaling using devfreq is
polling and the device profile is free to set polling interval.
However, in many cases this approach is not optimal; We may
unnecessarily use CPU time to determine load on engine that
is idle most of time - or we may simply read load when we
already know that the device is busy.

In some cases a device itself has counters to track its activity
and possibility to raise interrupts when load goes above or below
certain threshold.

This series adds support for watermark events to devfreq and
introduces two example governors. The first patch adds two
callbacks to the device profile (for setting watermark) and
adds a new function call to devfreq that informs of crossed
watermark.

The added governors demonstrate usage of the new API. The first
governor (watermark simple) sets device to trigger low watermark
event when load goes below 10% and high watermark interrupt when
the load goes above 60%. Watermark active tries to keep load at
certain level and it actively sets thresholds based on the
frequency table in order get interrupts only when the load value
would affect to the current frequency in re-estimation.

Arto Merilainen (1):
  PM / devfreq: Add watermark active governor

Shridhar Rasal (2):
  PM / devfreq: Add watermark events
  PM / devfreq: Add watermark simple governor

 drivers/devfreq/Kconfig                 |  18 +++
 drivers/devfreq/Makefile                |   2 +
 drivers/devfreq/devfreq.c               |  19 +++
 drivers/devfreq/governor.h              |   1 +
 drivers/devfreq/governor_wmark_active.c | 276 ++++++++++++++++++++++++++++++++
 drivers/devfreq/governor_wmark_simple.c | 245 ++++++++++++++++++++++++++++
 include/linux/devfreq.h                 |  26 +++
 7 files changed, 587 insertions(+)
 create mode 100644 drivers/devfreq/governor_wmark_active.c
 create mode 100644 drivers/devfreq/governor_wmark_simple.c

-- 
1.8.1.5