by Javi Merino

[permalink] [raw]

Subject: Re: [RFC PATCH v6 1/9] tracing: Add array printing helpers

On Mon, Dec 08, 2014 at 04:04:52PM +0000, Dave P Martin wrote:
> On Mon, Dec 08, 2014 at 03:42:10PM +0000, Steven Rostedt wrote:
> > On Fri, 5 Dec 2014 19:04:12 +0000
> > "Javi Merino" <[email protected]> wrote:
>
> [...]
>
> > > +
> > > +DEFINE_PRINT_ARRAY(u8, unsigned int, "0x%x");
> > > +DEFINE_PRINT_ARRAY(u16, unsigned int, "0x%x");
> > > +DEFINE_PRINT_ARRAY(u32, unsigned int, "0x%x");
> > > +DEFINE_PRINT_ARRAY(u64, unsigned long long, "0x%llx");
> > > +
> >
> > I would really like to avoid adding a bunch of macros for each type.
> > Can't we have something like this:
> > ftrace_print_array(struct trace_seq *p, void *buf, int buf_len,
> > int size)
> > {
> > char *prefix = "";
> > void *ptr = buf;
> >
> > while (ptr < buf + buf_len) {
> > switch(size) {
> > case 8:
> > trace_seq_printf("%s0x%x", prefix,
> > *(unsigned char *)ptr);
>
> I think this should be *(u8 *) etc.

Done, see below.

> Otherwise, I don't have a problem with this approach. It's less
> ugly than my original.

It makes the lib traceevent patches uglier though ;)

> > break;
> > case 16:
> > trace_seq_printf("%s0x%x", prefix,
> > *(unsigned short *)ptr);
> > break;
> > case 32:
> > trace_seq_printf("%s0x%x", prefix,
> > *(unsigned int *)ptr);
> > break;
> > case 64:
> > trace_seq_printf("%s0x%llx", prefix,
> > *(unsigned long long *)ptr);
> > break;
> > default:
> > BUG();
> > }
> > prefix = ",";
> > ptr += size;
> > }
> >
> > }
> >
> > We probably could even make the "BUG()" into a build bug, with a little
> > work.
>
> That sounds possible.

The only way I can think of doing that is by moving the check to the
__print_array macro:

#define __print_array(array, count, el_size) \
({ \
BUILD_BUG_ON(el_size != 8 && el_size != 16 && el_size != 32 && el_size != 64); \
ftrace_print_array_seq(p, array, count, el_size); \
})

Is this what you have in mind?

> Javi?

What about this?

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 28672e87e910..d5bddb230ecd 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -44,6 +44,9 @@ const char *ftrace_print_bitmask_seq(struct trace_seq *p, void *bitmask_ptr,
const char *ftrace_print_hex_seq(struct trace_seq *p,
const unsigned char *buf, int len);

+const char *ftrace_print_array_seq(struct trace_seq *p,
+ const void *buf, int buf_len, size_t el_size);
+
struct trace_iterator;
struct trace_event;

diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index 26b4f2e13275..38c5f91f63da 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -263,6 +263,10 @@
#undef __print_hex
#define __print_hex(buf, buf_len) ftrace_print_hex_seq(p, buf, buf_len)

+#undef __print_array
+#define __print_array(array, count, el_size) \
+ ftrace_print_array_seq(p, array, count, el_size)
+
#undef DECLARE_EVENT_CLASS
#define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \
static notrace enum print_line_t \
@@ -676,6 +680,7 @@ static inline void ftrace_test_probe_##call(void) \
#undef __get_dynamic_array_len
#undef __get_str
#undef __get_bitmask
+#undef __print_array

#undef TP_printk
#define TP_printk(fmt, args...) "\"" fmt "\", " __stringify(args)
diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index c6977d5a9b12..b582261086e8 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -186,6 +186,48 @@ ftrace_print_hex_seq(struct trace_seq *p, const unsigned char *buf, int buf_len)
}
EXPORT_SYMBOL(ftrace_print_hex_seq);

+const char *
+ftrace_print_array_seq(struct trace_seq *p, const void *buf, int buf_len,
+ size_t el_size)
+{
+ const char *ret = trace_seq_buffer_ptr(p);
+ const char *prefix = "";
+ void *ptr = (void *)buf;
+
+ trace_seq_putc(p, '{');
+
+ while (ptr < buf + buf_len) {
+ switch(el_size) {
+ case 8:
+ trace_seq_printf(p, "%s0x%x", prefix,
+ *(u8 *)ptr);
+ break;
+ case 16:
+ trace_seq_printf(p, "%s0x%x", prefix,
+ *(u16 *)ptr);
+ break;
+ case 32:
+ trace_seq_printf(p, "%s0x%x", prefix,
+ *(u32 *)ptr);
+ break;
+ case 64:
+ trace_seq_printf(p, "%s0x%llx", prefix,
+ *(u64 *)ptr);
+ break;
+ default:
+ BUG();
+ }
+ prefix = ",";
+ ptr += el_size / 8;
+ }
+
+ trace_seq_putc(p, '}');
+ trace_seq_putc(p, 0);
+
+ return ret;
+}
+EXPORT_SYMBOL(ftrace_print_array_seq);
+
int ftrace_raw_output_prep(struct trace_iterator *iter,
struct trace_event *trace_event)
{
--
1.9.1

2014-12-23 15:13:57

by Eduardo Valentin

[permalink] [raw]

Subject: Re: [RFC PATCH v6 5/9] thermal: extend the cooling device API to include power information

Hi Javi

On Fri, Dec 05, 2014 at 07:04:16PM +0000, Javi Merino wrote:
> Add three optional callbacks to the cooling device interface to allow
> them to express power. In addition to the callbacks, add helpers to
> identify cooling devices that implement the power cooling device API.
>
> Cc: Zhang Rui <[email protected]>
> Cc: Eduardo Valentin <[email protected]>
> Signed-off-by: Javi Merino <[email protected]>
> ---
> Documentation/thermal/power_allocator.txt | 27 ++++++++++++++++++++++
> drivers/thermal/thermal_core.c | 38 +++++++++++++++++++++++++++++++
> include/linux/thermal.h | 12 ++++++++++
> 3 files changed, 77 insertions(+)
> create mode 100644 Documentation/thermal/power_allocator.txt
>
> diff --git a/Documentation/thermal/power_allocator.txt b/Documentation/thermal/power_allocator.txt
> new file mode 100644
> index 000000000000..d3bb79050c27
> --- /dev/null
> +++ b/Documentation/thermal/power_allocator.txt
> @@ -0,0 +1,27 @@
> +Cooling device power API
> +========================

Readers of this file need extra context here, IMO.

> +
> +Cooling devices controlled by this governor must supply the additional

What governor? the files says power allocator, and the title says,
cooling device power API...

> +"power" API in their `cooling_device_ops`. It consists on three ops:
> +

> +1. u32 get_actual_power(struct thermal_cooling_device *cdev);
> +@cdev: The `struct thermal_cooling_device` pointer
> +
> +`get_actual_power()` returns the power currently consumed by the
> +device in milliwatts.
> +
> +2. u32 state2power(struct thermal_cooling_device *cdev, unsigned long
> + state);
> +@cdev: The `struct thermal_cooling_device` pointer
> +@state: A cooling device state
> +
> +Convert cooling device state @state into power consumption in
> +milliwatts.
> +
> +3. unsigned long power2state(struct thermal_cooling_device *cdev,
> + u32 power);
> +@cdev: The `struct thermal_cooling_device` pointer
> +@power: power in milliwatts
> +
> +Calculate a cooling device state that would make the device consume at
> +most @power mW.

I believe it would be more helpful if you could provide extra context in
which the above functions are called, and for what.

> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 9021cb72a13a..c490f262ea7f 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -866,6 +866,44 @@ emul_temp_store(struct device *dev, struct device_attribute *attr,
> static DEVICE_ATTR(emul_temp, S_IWUSR, NULL, emul_temp_store);
> #endif/*CONFIG_THERMAL_EMULATION*/
>
> +/**
> + * power_actor_get_max_power() - get the maximum power that a cdev can consume
> + * @cdev: pointer to &thermal_cooling_device
> + *
> + * Calculate the maximum power consumption in milliwats that the
> + * cooling device can currently consume. If @cdev doesn't support the
> + * power_actor API, this function returns 0.
> + */
> +u32 power_actor_get_max_power(struct thermal_cooling_device *cdev)
> +{
> + if (!cdev_is_power_actor(cdev))
> + return 0;
> +
> + return cdev->ops->state2power(cdev, 0);
> +}
> +
> +/**
> + * power_actor_set_power() - limit the maximum power that a cooling device can consume
> + * @cdev: pointer to &thermal_cooling_device
> + * @power: the power in milliwatts
> + *
> + * Set the cooling device to consume at most @power milliwatts.
> + *
> + * Returns: 0 on success, -EINVAL if the cooling device does not
> + * implement the power actor API or -E* for other failures.
> + */
> +int power_actor_set_power(struct thermal_cooling_device *cdev, u32 power)
> +{
> + unsigned long state;
> +
> + if (!cdev_is_power_actor(cdev))
> + return -EINVAL;
> +
> + state = cdev->ops->power2state(cdev, power);
> +
> + return cdev->ops->set_cur_state(cdev, state);
> +}
> +
> static DEVICE_ATTR(type, 0444, type_show, NULL);
> static DEVICE_ATTR(temp, 0444, temp_show, NULL);
> static DEVICE_ATTR(mode, 0644, mode_show, mode_store);
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 2c14ab1f5c0d..1155457caf52 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -142,6 +142,9 @@ struct thermal_cooling_device_ops {
> int (*get_max_state) (struct thermal_cooling_device *, unsigned long *);
> int (*get_cur_state) (struct thermal_cooling_device *, unsigned long *);
> int (*set_cur_state) (struct thermal_cooling_device *, unsigned long);
> + u32 (*get_actual_power) (struct thermal_cooling_device *);
> + u32 (*state2power) (struct thermal_cooling_device *, unsigned long);
> + unsigned long (*power2state) (struct thermal_cooling_device *, u32);
> };
>
> struct thermal_cooling_device {
> @@ -322,6 +325,15 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
> }
>
> #endif
> +
> +static inline bool cdev_is_power_actor(struct thermal_cooling_device *cdev)
> +{

What would happen if one pass cdev == NULL?

> + return cdev->ops->get_actual_power && cdev->ops->state2power &&
> + cdev->ops->power2state;
> +}
> +
> +u32 power_actor_get_max_power(struct thermal_cooling_device *);
> +int power_actor_set_power(struct thermal_cooling_device *, u32);
> struct thermal_zone_device *thermal_zone_device_register(const char *, int, int,
> void *, struct thermal_zone_device_ops *,
> const struct thermal_zone_params *, int, int);

I am assuming the above two new functions are expected to be used also
outside thermal core, right? If yes, I'd suggest exporting them.

> --
> 1.9.1
>
>

Attachments:

(No filename) (5.41 kB)
signature.asc (473.00 B)
Digital signature Download all attachments