* [PATCH v2 1/4] perf/core: check return value of perf_event_read IPI
2016-08-07 3:12 [PATCH v2 0/4] remove unnecessary IPI reading uncore events David Carrillo-Cisneros
@ 2016-08-07 3:12 ` David Carrillo-Cisneros
2016-08-10 11:04 ` Ingo Molnar
2016-08-07 3:12 ` [PATCH v2 2/4] perf/core: generalize event->group_flags David Carrillo-Cisneros
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: David Carrillo-Cisneros @ 2016-08-07 3:12 UTC (permalink / raw)
To: linux-kernel
Cc: x86, Ingo Molnar, Thomas Gleixner, Andi Kleen, Kan Liang,
Peter Zijlstra, Vegard Nossum, Paul Turner, Stephane Eranian,
David Carrillo-Cisneros
The call to smp_call_function_single in perf_event_read() may fail and,
when it does, its error value is the one to return.
Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
Reviewed-by: Stephane Eranian <eranian@google.com>
---
kernel/events/core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 9345028..96523fd 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3469,9 +3469,9 @@ static int perf_event_read(struct perf_event *event, bool group)
.group = group,
.ret = 0,
};
- smp_call_function_single(event->oncpu,
- __perf_event_read, &data, 1);
- ret = data.ret;
+ ret = smp_call_function_single(event->oncpu,
+ __perf_event_read, &data, 1);
+ ret = ret ? : data.ret;
} else if (event->state == PERF_EVENT_STATE_INACTIVE) {
struct perf_event_context *ctx = event->ctx;
unsigned long flags;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/4] perf/core: check return value of perf_event_read IPI
2016-08-07 3:12 ` [PATCH v2 1/4] perf/core: check return value of perf_event_read IPI David Carrillo-Cisneros
@ 2016-08-10 11:04 ` Ingo Molnar
2016-08-17 20:53 ` David Carrillo-Cisneros
0 siblings, 1 reply; 15+ messages in thread
From: Ingo Molnar @ 2016-08-10 11:04 UTC (permalink / raw)
To: David Carrillo-Cisneros
Cc: linux-kernel, x86, Ingo Molnar, Thomas Gleixner, Andi Kleen,
Kan Liang, Peter Zijlstra, Vegard Nossum, Paul Turner,
Stephane Eranian
* David Carrillo-Cisneros <davidcc@google.com> wrote:
> The call to smp_call_function_single in perf_event_read() may fail and,
> when it does, its error value is the one to return.
>
> Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
> Reviewed-by: Stephane Eranian <eranian@google.com>
> ---
> kernel/events/core.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 9345028..96523fd 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -3469,9 +3469,9 @@ static int perf_event_read(struct perf_event *event, bool group)
> .group = group,
> .ret = 0,
> };
> - smp_call_function_single(event->oncpu,
> - __perf_event_read, &data, 1);
> - ret = data.ret;
> + ret = smp_call_function_single(event->oncpu,
> + __perf_event_read, &data, 1);
> + ret = ret ? : data.ret;
> } else if (event->state == PERF_EVENT_STATE_INACTIVE) {
> struct perf_event_context *ctx = event->ctx;
> unsigned long flags;
Please also describe the symptoms of the bug - under what circumstances may the
smp-call fail and how does the user notice?
Thanks,
Ingo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/4] perf/core: check return value of perf_event_read IPI
2016-08-10 11:04 ` Ingo Molnar
@ 2016-08-17 20:53 ` David Carrillo-Cisneros
0 siblings, 0 replies; 15+ messages in thread
From: David Carrillo-Cisneros @ 2016-08-17 20:53 UTC (permalink / raw)
To: Ingo Molnar
Cc: linux-kernel, x86, Ingo Molnar, Thomas Gleixner, Andi Kleen,
Kan Liang, Peter Zijlstra, Vegard Nossum, Paul Turner,
Stephane Eranian
> > The call to smp_call_function_single in perf_event_read() may fail and,
> > when it does, its error value is the one to return.
> >
> Please also describe the symptoms of the bug - under what circumstances may the
> smp-call fail and how does the user notice?
>
> Thanks,
>
> Ingo
generic_exec_single only fails if cpu is invalid or not online, which
currently is not supposed to happen. Since perf_event_read's error is
not checked, I guess a WARN_ON_ONCE is needed. I'll post a rebased
version with this change.
Thanks,
David
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/4] perf/core: generalize event->group_flags
2016-08-07 3:12 [PATCH v2 0/4] remove unnecessary IPI reading uncore events David Carrillo-Cisneros
2016-08-07 3:12 ` [PATCH v2 1/4] perf/core: check return value of perf_event_read IPI David Carrillo-Cisneros
@ 2016-08-07 3:12 ` David Carrillo-Cisneros
2016-08-07 3:12 ` [PATCH v2 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG David Carrillo-Cisneros
` (2 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: David Carrillo-Cisneros @ 2016-08-07 3:12 UTC (permalink / raw)
To: linux-kernel
Cc: x86, Ingo Molnar, Thomas Gleixner, Andi Kleen, Kan Liang,
Peter Zijlstra, Vegard Nossum, Paul Turner, Stephane Eranian,
David Carrillo-Cisneros
Currently, PERF_GROUP_SOFTWARE is used in the group_flags field of a
group's leader to indicate that is_software_event(event) is true for all
events in a group. This is the only usage of event->group_flags.
This pattern of setting a group level flags when all events in the group
share a property is useful for the flag introduced in the next patch and
for future CQM/CMT flags. So this patches generalizes group_flags to work
as an aggregate of event level flags.
PERF_GROUP_SOFTWARE denotes an inmutable event's property. All other flags
that I intend to add are also determinable at event initialization.
To better convey the above, this patch renames event's group_flags to
group_caps and PERF_GROUP_SOFTWARE to PERF_EV_CAP_SOFTWARE.
Individual event flags are stored in the new event->event_caps. Since the
cap flags do not change after event initialization, there is no need to
serialize event_caps. This new field is used when events are added to a
context, similarly to how PERF_GROUP_SOFTWARE and is_software_event()
worked.
Lastly, for consistency, updates is_software_event() to rely in event_cap
instead of the context index.
Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
include/linux/perf_event.h | 18 +++++++++++++-----
kernel/events/core.c | 16 ++++++++--------
2 files changed, 21 insertions(+), 13 deletions(-)
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 7921f4f..fa5617f 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -497,9 +497,12 @@ typedef void (*perf_overflow_handler_t)(struct perf_event *,
struct perf_sample_data *,
struct pt_regs *regs);
-enum perf_group_flag {
- PERF_GROUP_SOFTWARE = 0x1,
-};
+/*
+ * Event capabilities. For event_caps and groups caps.
+ *
+ * PERF_EV_CAP_SOFTWARE: Is a software event.
+ */
+#define PERF_EV_CAP_SOFTWARE BIT(0)
#define SWEVENT_HLIST_BITS 8
#define SWEVENT_HLIST_SIZE (1 << SWEVENT_HLIST_BITS)
@@ -555,7 +558,12 @@ struct perf_event {
struct hlist_node hlist_entry;
struct list_head active_entry;
int nr_siblings;
- int group_flags;
+
+ /* Not serialized. Only written during event initialization. */
+ int event_caps;
+ /* The cumulative AND of all event_caps for events in this group. */
+ int group_caps;
+
struct perf_event *group_leader;
struct pmu *pmu;
void *pmu_private;
@@ -968,7 +976,7 @@ static inline bool is_sampling_event(struct perf_event *event)
*/
static inline int is_software_event(struct perf_event *event)
{
- return event->pmu->task_ctx_nr == perf_sw_context;
+ return event->event_caps & PERF_EV_CAP_SOFTWARE;
}
extern struct static_key perf_swevent_enabled[PERF_COUNT_SW_MAX];
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 96523fd..34049cc 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1405,8 +1405,7 @@ list_add_event(struct perf_event *event, struct perf_event_context *ctx)
if (event->group_leader == event) {
struct list_head *list;
- if (is_software_event(event))
- event->group_flags |= PERF_GROUP_SOFTWARE;
+ event->group_caps = event->event_caps;
list = ctx_group_list(event, ctx);
list_add_tail(&event->group_entry, list);
@@ -1561,9 +1560,7 @@ static void perf_group_attach(struct perf_event *event)
WARN_ON_ONCE(group_leader->ctx != event->ctx);
- if (group_leader->group_flags & PERF_GROUP_SOFTWARE &&
- !is_software_event(event))
- group_leader->group_flags &= ~PERF_GROUP_SOFTWARE;
+ group_leader->group_caps &= event->event_caps;
list_add_tail(&event->group_entry, &group_leader->sibling_list);
group_leader->nr_siblings++;
@@ -1669,7 +1666,7 @@ static void perf_group_detach(struct perf_event *event)
sibling->group_leader = sibling;
/* Inherit group flags from the previous leader */
- sibling->group_flags = event->group_flags;
+ sibling->group_caps = event->group_caps;
WARN_ON_ONCE(sibling->ctx != event->ctx);
}
@@ -2070,7 +2067,7 @@ static int group_can_go_on(struct perf_event *event,
/*
* Groups consisting entirely of software events can always go on.
*/
- if (event->group_flags & PERF_GROUP_SOFTWARE)
+ if (event->group_caps & PERF_EV_CAP_SOFTWARE)
return 1;
/*
* If an exclusive group is already on, no other hardware
@@ -9359,6 +9356,9 @@ SYSCALL_DEFINE5(perf_event_open,
goto err_alloc;
}
+ if (pmu->task_ctx_nr == perf_sw_context)
+ event->event_caps |= PERF_EV_CAP_SOFTWARE;
+
if (group_leader &&
(is_software_event(event) != is_software_event(group_leader))) {
if (is_software_event(event)) {
@@ -9372,7 +9372,7 @@ SYSCALL_DEFINE5(perf_event_open,
*/
pmu = group_leader->pmu;
} else if (is_software_event(group_leader) &&
- (group_leader->group_flags & PERF_GROUP_SOFTWARE)) {
+ (group_leader->group_caps & PERF_EV_CAP_SOFTWARE)) {
/*
* In case the group is a pure software group, and we
* try to add a hardware event, move the whole group to
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG
2016-08-07 3:12 [PATCH v2 0/4] remove unnecessary IPI reading uncore events David Carrillo-Cisneros
2016-08-07 3:12 ` [PATCH v2 1/4] perf/core: check return value of perf_event_read IPI David Carrillo-Cisneros
2016-08-07 3:12 ` [PATCH v2 2/4] perf/core: generalize event->group_flags David Carrillo-Cisneros
@ 2016-08-07 3:12 ` David Carrillo-Cisneros
2016-08-07 19:10 ` Nilay Vaish
2016-08-08 10:00 ` [PATCH v3 " David Carrillo-Cisneros
2016-08-07 3:12 ` [PATCH v2 4/4] perf/x86: use PMUEF_READ_CPU_PKG in uncore events David Carrillo-Cisneros
2016-08-10 18:30 ` [PATCH v2 0/4] remove unnecessary IPI reading " Nilay Vaish
4 siblings, 2 replies; 15+ messages in thread
From: David Carrillo-Cisneros @ 2016-08-07 3:12 UTC (permalink / raw)
To: linux-kernel
Cc: x86, Ingo Molnar, Thomas Gleixner, Andi Kleen, Kan Liang,
Peter Zijlstra, Vegard Nossum, Paul Turner, Stephane Eranian,
David Carrillo-Cisneros
Introduce the flag PMU_EV_CAP_READ_ACTIVE_PKG, useful for uncore events,
that allows a PMU to signal the generic perf code that an event is readable
in the current CPU if the event is active in a CPU in the same package as
the current CPU.
This is an optimization that avoids a unnecessary IPI for the common case
where uncore events are run and read in the same package but in
different CPUs.
As an example, the IPI removal speeds up perf_read in my Haswell system
as follows:
- For event UNC_C_LLC_LOOKUP: From 260 us to 31 us.
- For event RAPL's power/energy-cores/: From to 255 us to 27 us.
For the optimization to work, all events in the group must have it
(similarly to PERF_EV_CAP_SOFTWARE).
Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
include/linux/perf_event.h | 3 +++
kernel/events/core.c | 32 +++++++++++++++++++++++++-------
2 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index fa5617f..c8bb1b3 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -501,8 +501,11 @@ typedef void (*perf_overflow_handler_t)(struct perf_event *,
* Event capabilities. For event_caps and groups caps.
*
* PERF_EV_CAP_SOFTWARE: Is a software event.
+ * PERF_EV_CAP_READ_ACTIVE_PKG: A CPU event (or cgroup event) that can be read
+ * from any CPU in the package where it is active.
*/
#define PERF_EV_CAP_SOFTWARE BIT(0)
+#define PERF_EV_CAP_READ_ACTIVE_PKG BIT(1)
#define SWEVENT_HLIST_BITS 8
#define SWEVENT_HLIST_SIZE (1 << SWEVENT_HLIST_BITS)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 34049cc..77f1bd3 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3333,6 +3333,26 @@ struct perf_read_data {
int ret;
};
+static int find_cpu_to_read(struct perf_event *event)
+{
+ bool active = event->state == PERF_EVENT_STATE_ACTIVE;
+ int event_cpu = event->oncpu, local_cpu = smp_processor_id();
+ u16 local_pkg, event_pkg;
+
+ if (!active)
+ return -1;
+
+ if (event->group_caps & PERF_EV_CAP_READ_ACTIVE_PKG) {
+ event_pkg = topology_physical_package_id(event_cpu);
+ local_pkg = topology_physical_package_id(local_cpu);
+
+ if (event_pkg == local_pkg)
+ return local_cpu;
+ }
+
+ return event_cpu;
+}
+
/*
* Cross CPU call to read the hardware event
*/
@@ -3454,19 +3474,17 @@ u64 perf_event_read_local(struct perf_event *event)
static int perf_event_read(struct perf_event *event, bool group)
{
- int ret = 0;
+ int ret = 0, cpu_to_read;
- /*
- * If event is enabled and currently active on a CPU, update the
- * value in the event structure:
- */
- if (event->state == PERF_EVENT_STATE_ACTIVE) {
+ cpu_to_read = find_cpu_to_read(event);
+
+ if (cpu_to_read >= 0) {
struct perf_read_data data = {
.event = event,
.group = group,
.ret = 0,
};
- ret = smp_call_function_single(event->oncpu,
+ ret = smp_call_function_single(cpu_to_read,
__perf_event_read, &data, 1);
ret = ret ? : data.ret;
} else if (event->state == PERF_EVENT_STATE_INACTIVE) {
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG
2016-08-07 3:12 ` [PATCH v2 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG David Carrillo-Cisneros
@ 2016-08-07 19:10 ` Nilay Vaish
2016-08-07 20:10 ` David Carrillo-Cisneros
2016-08-08 10:00 ` [PATCH v3 " David Carrillo-Cisneros
1 sibling, 1 reply; 15+ messages in thread
From: Nilay Vaish @ 2016-08-07 19:10 UTC (permalink / raw)
To: David Carrillo-Cisneros, linux-kernel
Cc: x86, Ingo Molnar, Thomas Gleixner, Andi Kleen, Kan Liang,
Peter Zijlstra, Vegard Nossum, Paul Turner, Stephane Eranian
On 08/06/16 22:12, David Carrillo-Cisneros wrote:
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 34049cc..77f1bd3 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -3333,6 +3333,26 @@ struct perf_read_data {
> int ret;
> };
>
> +static int find_cpu_to_read(struct perf_event *event)
> +{
> + bool active = event->state == PERF_EVENT_STATE_ACTIVE;
> + int event_cpu = event->oncpu, local_cpu = smp_processor_id();
> + u16 local_pkg, event_pkg;
> +
> + if (!active)
> + return -1;
> +
> + if (event->group_caps & PERF_EV_CAP_READ_ACTIVE_PKG) {
> + event_pkg = topology_physical_package_id(event_cpu);
> + local_pkg = topology_physical_package_id(local_cpu);
> +
> + if (event_pkg == local_pkg)
> + return local_cpu;
> + }
> +
> + return event_cpu;
> +}
> +
> /*
> * Cross CPU call to read the hardware event
> */
> @@ -3454,19 +3474,17 @@ u64 perf_event_read_local(struct perf_event *event)
>
> static int perf_event_read(struct perf_event *event, bool group)
> {
> - int ret = 0;
> + int ret = 0, cpu_to_read;
>
> - /*
> - * If event is enabled and currently active on a CPU, update the
> - * value in the event structure:
> - */
> - if (event->state == PERF_EVENT_STATE_ACTIVE) {
> + cpu_to_read = find_cpu_to_read(event);
> +
> + if (cpu_to_read >= 0) {
> struct perf_read_data data = {
> .event = event,
> .group = group,
> .ret = 0,
> };
> - ret = smp_call_function_single(event->oncpu,
> + ret = smp_call_function_single(cpu_to_read,
> __perf_event_read, &data, 1);
> ret = ret ? : data.ret;
> } else if (event->state == PERF_EVENT_STATE_INACTIVE) {
>
I would like to suggest a small change to this patch. I think the check
on PERF_EVENT_STATE_ACTIVE should be retained in the perf_event_read()
function. The new function should assume that the event is active. I
find this more readable since the next check in function
perf_event_read() is on PERF_EVENT_STATE_INACTIVE.
Diff below:
+static int find_cpu_to_read(struct perf_event *event)
+{
+ int event_cpu = event->oncpu, local_cpu = smp_processor_id();
+ u16 local_pkg, event_pkg;
+
+ if (event->group_caps & PERF_EV_CAP_READ_ACTIVE_PKG) {
+ event_pkg = topology_physical_package_id(event_cpu);
+ local_pkg = topology_physical_package_id(local_cpu);
+
+ if (event_pkg == local_pkg)
+ return local_cpu;
+ }
+
+ return event_cpu;
+}
+
/*
* Cross CPU call to read the hardware event
*/
@@ -3477,17 +3493,15 @@ static int perf_event_read(struct perf_event
*event, bool group)
{
int ret = 0;
- /*
- * If event is enabled and currently active on a CPU, update the
- * value in the event structure:
- */
- if (event->state == PERF_EVENT_STATE_ACTIVE) {
+ if (event->state == PERF_EVENT_STATE_ACTIVE) {
+ int cpu_to_read = find_cpu_to_read(event);
+
struct perf_read_data data = {
.event = event,
.group = group,
.ret = 0,
};
- ret = smp_call_function_single(event->oncpu,
+ ret = smp_call_function_single(cpu_to_read,
__perf_event_read,
&data, 1);
ret = ret ? : data.ret;
} else if (event->state == PERF_EVENT_STATE_INACTIVE) {
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG
2016-08-07 19:10 ` Nilay Vaish
@ 2016-08-07 20:10 ` David Carrillo-Cisneros
2016-08-08 16:12 ` Nilay Vaish
0 siblings, 1 reply; 15+ messages in thread
From: David Carrillo-Cisneros @ 2016-08-07 20:10 UTC (permalink / raw)
To: Nilay Vaish
Cc: linux-kernel, x86, Ingo Molnar, Thomas Gleixner, Andi Kleen,
Kan Liang, Peter Zijlstra, Vegard Nossum, Paul Turner,
Stephane Eranian
Hi Nilay,
>> static int perf_event_read(struct perf_event *event, bool group)
>> {
>> - int ret = 0;
>> + int ret = 0, cpu_to_read;
>>
>> - /*
>> - * If event is enabled and currently active on a CPU, update the
>> - * value in the event structure:
>> - */
>> - if (event->state == PERF_EVENT_STATE_ACTIVE) {
>> + cpu_to_read = find_cpu_to_read(event);
>> +
>> + if (cpu_to_read >= 0) {
>> struct perf_read_data data = {
>> .event = event,
>> .group = group,
>> .ret = 0,
>> };
>> - ret = smp_call_function_single(event->oncpu,
>> + ret = smp_call_function_single(cpu_to_read,
>> __perf_event_read, &data,
>> 1);
>> ret = ret ? : data.ret;
>> } else if (event->state == PERF_EVENT_STATE_INACTIVE) {
>>
>
> I would like to suggest a small change to this patch. I think the check on
> PERF_EVENT_STATE_ACTIVE should be retained in the perf_event_read()
> function. The new function should assume that the event is active. I find
> this more readable since the next check in function perf_event_read() is on
> PERF_EVENT_STATE_INACTIVE.
Two oncoming flags that Intel CQM/CMT will use are meant to allow read
even if event is inactive. This makes sense in CQM/CMT because the hw
RMID is always reserved. I am ok with keeping the check for
STATE_ACTIVE until those flags are actually introduced, tough.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG
2016-08-07 20:10 ` David Carrillo-Cisneros
@ 2016-08-08 16:12 ` Nilay Vaish
2016-08-09 5:04 ` David Carrillo-Cisneros
2016-08-09 22:28 ` [PATCH " David Carrillo-Cisneros
0 siblings, 2 replies; 15+ messages in thread
From: Nilay Vaish @ 2016-08-08 16:12 UTC (permalink / raw)
To: David Carrillo-Cisneros
Cc: linux-kernel, x86, Ingo Molnar, Thomas Gleixner, Andi Kleen,
Kan Liang, Peter Zijlstra, Vegard Nossum, Paul Turner,
Stephane Eranian
On 7 August 2016 at 15:10, David Carrillo-Cisneros <davidcc@google.com> wrote:
> Hi Nilay,
>
>>> static int perf_event_read(struct perf_event *event, bool group)
>>> {
>>> - int ret = 0;
>>> + int ret = 0, cpu_to_read;
>>>
>>> - /*
>>> - * If event is enabled and currently active on a CPU, update the
>>> - * value in the event structure:
>>> - */
>>> - if (event->state == PERF_EVENT_STATE_ACTIVE) {
>>> + cpu_to_read = find_cpu_to_read(event);
>>> +
>>> + if (cpu_to_read >= 0) {
>>> struct perf_read_data data = {
>>> .event = event,
>>> .group = group,
>>> .ret = 0,
>>> };
>>> - ret = smp_call_function_single(event->oncpu,
>>> + ret = smp_call_function_single(cpu_to_read,
>>> __perf_event_read, &data,
>>> 1);
>>> ret = ret ? : data.ret;
>>> } else if (event->state == PERF_EVENT_STATE_INACTIVE) {
>>>
>>
>> I would like to suggest a small change to this patch. I think the check on
>> PERF_EVENT_STATE_ACTIVE should be retained in the perf_event_read()
>> function. The new function should assume that the event is active. I find
>> this more readable since the next check in function perf_event_read() is on
>> PERF_EVENT_STATE_INACTIVE.
>
> Two oncoming flags that Intel CQM/CMT will use are meant to allow read
> even if event is inactive. This makes sense in CQM/CMT because the hw
> RMID is always reserved. I am ok with keeping the check for
> STATE_ACTIVE until those flags are actually introduced, tough.
Hello David
Lets go with checking PERF_EVENT_STATE_ACTIVE in perf_event_read() for
the time being. With the new version of the patch that you posted, I
find that checking PERF_EVENT_STATE_ACTIVE in find_cpu_to_read() makes
you introduce another if statement for checking STATE_INACTIVE.
If your CQM/CMT patches later need the code structure you have now, I
would also support it. But as of now, I think, it is better to check
STATE_ACTIVE in perf_event_read().
Thanks
Nilay
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG
2016-08-08 16:12 ` Nilay Vaish
@ 2016-08-09 5:04 ` David Carrillo-Cisneros
2016-08-09 22:28 ` [PATCH " David Carrillo-Cisneros
1 sibling, 0 replies; 15+ messages in thread
From: David Carrillo-Cisneros @ 2016-08-09 5:04 UTC (permalink / raw)
To: Nilay Vaish
Cc: linux-kernel, x86, Ingo Molnar, Thomas Gleixner, Andi Kleen,
Kan Liang, Peter Zijlstra, Vegard Nossum, Paul Turner,
Stephane Eranian
Hi Nilay,
Sounds good, I will post an updated version.
Thanks,
David
On Mon, Aug 8, 2016 at 9:12 AM, Nilay Vaish <nilayvaish@gmail.com> wrote:
> On 7 August 2016 at 15:10, David Carrillo-Cisneros <davidcc@google.com> wrote:
>> Hi Nilay,
>>
>>>> static int perf_event_read(struct perf_event *event, bool group)
>>>> {
>>>> - int ret = 0;
>>>> + int ret = 0, cpu_to_read;
>>>>
>>>> - /*
>>>> - * If event is enabled and currently active on a CPU, update the
>>>> - * value in the event structure:
>>>> - */
>>>> - if (event->state == PERF_EVENT_STATE_ACTIVE) {
>>>> + cpu_to_read = find_cpu_to_read(event);
>>>> +
>>>> + if (cpu_to_read >= 0) {
>>>> struct perf_read_data data = {
>>>> .event = event,
>>>> .group = group,
>>>> .ret = 0,
>>>> };
>>>> - ret = smp_call_function_single(event->oncpu,
>>>> + ret = smp_call_function_single(cpu_to_read,
>>>> __perf_event_read, &data,
>>>> 1);
>>>> ret = ret ? : data.ret;
>>>> } else if (event->state == PERF_EVENT_STATE_INACTIVE) {
>>>>
>>>
>>> I would like to suggest a small change to this patch. I think the check on
>>> PERF_EVENT_STATE_ACTIVE should be retained in the perf_event_read()
>>> function. The new function should assume that the event is active. I find
>>> this more readable since the next check in function perf_event_read() is on
>>> PERF_EVENT_STATE_INACTIVE.
>>
>> Two oncoming flags that Intel CQM/CMT will use are meant to allow read
>> even if event is inactive. This makes sense in CQM/CMT because the hw
>> RMID is always reserved. I am ok with keeping the check for
>> STATE_ACTIVE until those flags are actually introduced, tough.
>
>
> Hello David
>
> Lets go with checking PERF_EVENT_STATE_ACTIVE in perf_event_read() for
> the time being. With the new version of the patch that you posted, I
> find that checking PERF_EVENT_STATE_ACTIVE in find_cpu_to_read() makes
> you introduce another if statement for checking STATE_INACTIVE.
>
> If your CQM/CMT patches later need the code structure you have now, I
> would also support it. But as of now, I think, it is better to check
> STATE_ACTIVE in perf_event_read().
>
>
> Thanks
> Nilay
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG
2016-08-08 16:12 ` Nilay Vaish
2016-08-09 5:04 ` David Carrillo-Cisneros
@ 2016-08-09 22:28 ` David Carrillo-Cisneros
2016-08-10 18:27 ` Nilay Vaish
1 sibling, 1 reply; 15+ messages in thread
From: David Carrillo-Cisneros @ 2016-08-09 22:28 UTC (permalink / raw)
To: linux-kernel
Cc: x86, Ingo Molnar, Thomas Gleixner, Andi Kleen, Kan Liang,
Peter Zijlstra, Vegard Nossum, Paul Turner, Stephane Eranian,
David Carrillo-Cisneros
Introduce the flag PMU_EV_CAP_READ_ACTIVE_PKG, useful for uncore events,
that allows a PMU to signal the generic perf code that an event is readable
in the current CPU if the event is active in a CPU in the same package as
the current CPU.
This is an optimization that avoids a unnecessary IPI for the common case
where uncore events are run and read in the same package but in
different CPUs.
As an example, the IPI removal speeds up perf_read in my Haswell system
as follows:
- For event UNC_C_LLC_LOOKUP: From 260 us to 31 us.
- For event RAPL's power/energy-cores/: From to 255 us to 27 us.
For the optimization to work, all events in the group must have it
(similarly to PERF_EV_CAP_SOFTWARE).
Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
include/linux/perf_event.h | 3 +++
kernel/events/core.c | 26 ++++++++++++++++++++++++--
2 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index fa5617f..c8bb1b3 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -501,8 +501,11 @@ typedef void (*perf_overflow_handler_t)(struct perf_event *,
* Event capabilities. For event_caps and groups caps.
*
* PERF_EV_CAP_SOFTWARE: Is a software event.
+ * PERF_EV_CAP_READ_ACTIVE_PKG: A CPU event (or cgroup event) that can be read
+ * from any CPU in the package where it is active.
*/
#define PERF_EV_CAP_SOFTWARE BIT(0)
+#define PERF_EV_CAP_READ_ACTIVE_PKG BIT(1)
#define SWEVENT_HLIST_BITS 8
#define SWEVENT_HLIST_SIZE (1 << SWEVENT_HLIST_BITS)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 34049cc..38ec68d 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3333,6 +3333,22 @@ struct perf_read_data {
int ret;
};
+static int find_cpu_to_read(struct perf_event *event, int local_cpu)
+{
+ int event_cpu = event->oncpu;
+ u16 local_pkg, event_pkg;
+
+ if (event->group_caps & PERF_EV_CAP_READ_ACTIVE_PKG) {
+ event_pkg = topology_physical_package_id(event_cpu);
+ local_pkg = topology_physical_package_id(local_cpu);
+
+ if (event_pkg == local_pkg)
+ return local_cpu;
+ }
+
+ return event_cpu;
+}
+
/*
* Cross CPU call to read the hardware event
*/
@@ -3454,7 +3470,7 @@ u64 perf_event_read_local(struct perf_event *event)
static int perf_event_read(struct perf_event *event, bool group)
{
- int ret = 0;
+ int ret = 0, cpu_to_read, local_cpu;
/*
* If event is enabled and currently active on a CPU, update the
@@ -3466,8 +3482,14 @@ static int perf_event_read(struct perf_event *event, bool group)
.group = group,
.ret = 0,
};
- ret = smp_call_function_single(event->oncpu,
+
+ local_cpu = get_cpu();
+ cpu_to_read = find_cpu_to_read(event, local_cpu);
+ put_cpu();
+
+ ret = smp_call_function_single(cpu_to_read,
__perf_event_read, &data, 1);
+
ret = ret ? : data.ret;
} else if (event->state == PERF_EVENT_STATE_INACTIVE) {
struct perf_event_context *ctx = event->ctx;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG
2016-08-09 22:28 ` [PATCH " David Carrillo-Cisneros
@ 2016-08-10 18:27 ` Nilay Vaish
0 siblings, 0 replies; 15+ messages in thread
From: Nilay Vaish @ 2016-08-10 18:27 UTC (permalink / raw)
To: David Carrillo-Cisneros
Cc: Linux Kernel list, x86, Ingo Molnar, Thomas Gleixner, Andi Kleen,
Kan Liang, Peter Zijlstra, Vegard Nossum, Paul Turner,
Stephane Eranian
On 9 August 2016 at 17:28, David Carrillo-Cisneros <davidcc@google.com> wrote:
> Introduce the flag PMU_EV_CAP_READ_ACTIVE_PKG, useful for uncore events,
> that allows a PMU to signal the generic perf code that an event is readable
> in the current CPU if the event is active in a CPU in the same package as
> the current CPU.
>
> This is an optimization that avoids a unnecessary IPI for the common case
> where uncore events are run and read in the same package but in
> different CPUs.
>
> As an example, the IPI removal speeds up perf_read in my Haswell system
> as follows:
> - For event UNC_C_LLC_LOOKUP: From 260 us to 31 us.
> - For event RAPL's power/energy-cores/: From to 255 us to 27 us.
>
> For the optimization to work, all events in the group must have it
> (similarly to PERF_EV_CAP_SOFTWARE).
>
> Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
> ---
> include/linux/perf_event.h | 3 +++
> kernel/events/core.c | 26 ++++++++++++++++++++++++--
> 2 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index fa5617f..c8bb1b3 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -501,8 +501,11 @@ typedef void (*perf_overflow_handler_t)(struct perf_event *,
> * Event capabilities. For event_caps and groups caps.
> *
> * PERF_EV_CAP_SOFTWARE: Is a software event.
> + * PERF_EV_CAP_READ_ACTIVE_PKG: A CPU event (or cgroup event) that can be read
> + * from any CPU in the package where it is active.
> */
> #define PERF_EV_CAP_SOFTWARE BIT(0)
> +#define PERF_EV_CAP_READ_ACTIVE_PKG BIT(1)
>
> #define SWEVENT_HLIST_BITS 8
> #define SWEVENT_HLIST_SIZE (1 << SWEVENT_HLIST_BITS)
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 34049cc..38ec68d 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -3333,6 +3333,22 @@ struct perf_read_data {
> int ret;
> };
>
> +static int find_cpu_to_read(struct perf_event *event, int local_cpu)
> +{
> + int event_cpu = event->oncpu;
> + u16 local_pkg, event_pkg;
> +
> + if (event->group_caps & PERF_EV_CAP_READ_ACTIVE_PKG) {
> + event_pkg = topology_physical_package_id(event_cpu);
> + local_pkg = topology_physical_package_id(local_cpu);
> +
> + if (event_pkg == local_pkg)
> + return local_cpu;
> + }
> +
> + return event_cpu;
> +}
> +
> /*
> * Cross CPU call to read the hardware event
> */
> @@ -3454,7 +3470,7 @@ u64 perf_event_read_local(struct perf_event *event)
>
> static int perf_event_read(struct perf_event *event, bool group)
> {
> - int ret = 0;
> + int ret = 0, cpu_to_read, local_cpu;
>
> /*
> * If event is enabled and currently active on a CPU, update the
> @@ -3466,8 +3482,14 @@ static int perf_event_read(struct perf_event *event, bool group)
> .group = group,
> .ret = 0,
> };
> - ret = smp_call_function_single(event->oncpu,
> +
> + local_cpu = get_cpu();
> + cpu_to_read = find_cpu_to_read(event, local_cpu);
> + put_cpu();
> +
> + ret = smp_call_function_single(cpu_to_read,
> __perf_event_read, &data, 1);
> +
> ret = ret ? : data.ret;
> } else if (event->state == PERF_EVENT_STATE_INACTIVE) {
> struct perf_event_context *ctx = event->ctx;
> --
> 2.8.0.rc3.226.g39d4020
>
David, thanks for making the changes. This patch looks good to me.
--
Nilay
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG
2016-08-07 3:12 ` [PATCH v2 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG David Carrillo-Cisneros
2016-08-07 19:10 ` Nilay Vaish
@ 2016-08-08 10:00 ` David Carrillo-Cisneros
1 sibling, 0 replies; 15+ messages in thread
From: David Carrillo-Cisneros @ 2016-08-08 10:00 UTC (permalink / raw)
To: linux-kernel
Cc: x86, Ingo Molnar, Thomas Gleixner, Andi Kleen, Kan Liang,
Peter Zijlstra, Vegard Nossum, Paul Turner, Stephane Eranian,
David Carrillo-Cisneros, xiaolong.ye
Introduce the flag PMU_EV_CAP_READ_ACTIVE_PKG, useful for uncore events,
that allows a PMU to signal the generic perf code that an event is readable
in the current CPU if the event is active in a CPU in the same package as
the current CPU.
This is an optimization that avoids a unnecessary IPI for the common case
where uncore events are run and read in the same package but in
different CPUs.
As an example, the IPI removal speeds up perf_read in my Haswell system
as follows:
- For event UNC_C_LLC_LOOKUP: From 260 us to 31 us.
- For event RAPL's power/energy-cores/: From to 255 us to 27 us.
For the optimization to work, all events in the group must have it
(similarly to PERF_EV_CAP_SOFTWARE).
Changes in v3:
- Fix "BUG: using smp_processor_id() in preemptible" reported by
Xiaolong Ye.
Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
include/linux/perf_event.h | 3 +++
kernel/events/core.c | 44 ++++++++++++++++++++++++++++++++++----------
2 files changed, 37 insertions(+), 10 deletions(-)
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index fa5617f..c8bb1b3 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -501,8 +501,11 @@ typedef void (*perf_overflow_handler_t)(struct perf_event *,
* Event capabilities. For event_caps and groups caps.
*
* PERF_EV_CAP_SOFTWARE: Is a software event.
+ * PERF_EV_CAP_READ_ACTIVE_PKG: A CPU event (or cgroup event) that can be read
+ * from any CPU in the package where it is active.
*/
#define PERF_EV_CAP_SOFTWARE BIT(0)
+#define PERF_EV_CAP_READ_ACTIVE_PKG BIT(1)
#define SWEVENT_HLIST_BITS 8
#define SWEVENT_HLIST_SIZE (1 << SWEVENT_HLIST_BITS)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 34049cc..2c22b30 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3333,6 +3333,26 @@ struct perf_read_data {
int ret;
};
+static int find_cpu_to_read(struct perf_event *event, int local_cpu)
+{
+ bool active = event->state == PERF_EVENT_STATE_ACTIVE;
+ int event_cpu = event->oncpu;
+ u16 local_pkg, event_pkg;
+
+ if (!active)
+ return -1;
+
+ if (event->group_caps & PERF_EV_CAP_READ_ACTIVE_PKG) {
+ event_pkg = topology_physical_package_id(event_cpu);
+ local_pkg = topology_physical_package_id(local_cpu);
+
+ if (event_pkg == local_pkg)
+ return local_cpu;
+ }
+
+ return event_cpu;
+}
+
/*
* Cross CPU call to read the hardware event
*/
@@ -3454,22 +3474,26 @@ u64 perf_event_read_local(struct perf_event *event)
static int perf_event_read(struct perf_event *event, bool group)
{
- int ret = 0;
+ int ret = 0, cpu_to_read, local_cpu;
- /*
- * If event is enabled and currently active on a CPU, update the
- * value in the event structure:
- */
- if (event->state == PERF_EVENT_STATE_ACTIVE) {
+ local_cpu = get_cpu();
+ cpu_to_read = find_cpu_to_read(event, local_cpu);
+
+ if (cpu_to_read >= 0) {
struct perf_read_data data = {
.event = event,
.group = group,
.ret = 0,
};
- ret = smp_call_function_single(event->oncpu,
+ ret = smp_call_function_single(cpu_to_read,
__perf_event_read, &data, 1);
- ret = ret ? : data.ret;
- } else if (event->state == PERF_EVENT_STATE_INACTIVE) {
+ put_cpu();
+
+ return ret ? : data.ret;
+ }
+ put_cpu();
+
+ if (event->state == PERF_EVENT_STATE_INACTIVE) {
struct perf_event_context *ctx = event->ctx;
unsigned long flags;
@@ -3490,7 +3514,7 @@ static int perf_event_read(struct perf_event *event, bool group)
raw_spin_unlock_irqrestore(&ctx->lock, flags);
}
- return ret;
+ return 0;
}
/*
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 4/4] perf/x86: use PMUEF_READ_CPU_PKG in uncore events
2016-08-07 3:12 [PATCH v2 0/4] remove unnecessary IPI reading uncore events David Carrillo-Cisneros
` (2 preceding siblings ...)
2016-08-07 3:12 ` [PATCH v2 3/4] perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG David Carrillo-Cisneros
@ 2016-08-07 3:12 ` David Carrillo-Cisneros
2016-08-10 18:30 ` [PATCH v2 0/4] remove unnecessary IPI reading " Nilay Vaish
4 siblings, 0 replies; 15+ messages in thread
From: David Carrillo-Cisneros @ 2016-08-07 3:12 UTC (permalink / raw)
To: linux-kernel
Cc: x86, Ingo Molnar, Thomas Gleixner, Andi Kleen, Kan Liang,
Peter Zijlstra, Vegard Nossum, Paul Turner, Stephane Eranian,
David Carrillo-Cisneros
Add flag to Intel's uncore and RAPL.
Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
Reviewed-by: Stephane Eranian <eranian@google.com>
---
arch/x86/events/intel/rapl.c | 2 ++
arch/x86/events/intel/uncore.c | 2 ++
arch/x86/events/intel/uncore_snb.c | 2 ++
3 files changed, 6 insertions(+)
diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c
index d0c58b3..9922526 100644
--- a/arch/x86/events/intel/rapl.c
+++ b/arch/x86/events/intel/rapl.c
@@ -357,6 +357,8 @@ static int rapl_pmu_event_init(struct perf_event *event)
if (event->cpu < 0)
return -EINVAL;
+ event->event_caps |= PERF_EV_CAP_READ_ACTIVE_PKG;
+
/*
* check event is known (determines counter)
*/
diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index dc965d2..dfb62ea 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -662,6 +662,8 @@ static int uncore_pmu_event_init(struct perf_event *event)
event->cpu = box->cpu;
event->pmu_private = box;
+ event->event_caps |= PERF_EV_CAP_READ_ACTIVE_PKG;
+
event->hw.idx = -1;
event->hw.last_tag = ~0ULL;
event->hw.extra_reg.idx = EXTRA_REG_NONE;
diff --git a/arch/x86/events/intel/uncore_snb.c b/arch/x86/events/intel/uncore_snb.c
index 96531d2..a966c00 100644
--- a/arch/x86/events/intel/uncore_snb.c
+++ b/arch/x86/events/intel/uncore_snb.c
@@ -315,6 +315,8 @@ static int snb_uncore_imc_event_init(struct perf_event *event)
event->cpu = box->cpu;
event->pmu_private = box;
+ event->event_caps |= PERF_EV_CAP_READ_ACTIVE_PKG;
+
event->hw.idx = -1;
event->hw.last_tag = ~0ULL;
event->hw.extra_reg.idx = EXTRA_REG_NONE;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 0/4] remove unnecessary IPI reading uncore events
2016-08-07 3:12 [PATCH v2 0/4] remove unnecessary IPI reading uncore events David Carrillo-Cisneros
` (3 preceding siblings ...)
2016-08-07 3:12 ` [PATCH v2 4/4] perf/x86: use PMUEF_READ_CPU_PKG in uncore events David Carrillo-Cisneros
@ 2016-08-10 18:30 ` Nilay Vaish
4 siblings, 0 replies; 15+ messages in thread
From: Nilay Vaish @ 2016-08-10 18:30 UTC (permalink / raw)
To: David Carrillo-Cisneros
Cc: Linux Kernel list, x86, Ingo Molnar, Thomas Gleixner, Andi Kleen,
Kan Liang, Peter Zijlstra, Vegard Nossum, Paul Turner,
Stephane Eranian
On 6 August 2016 at 22:12, David Carrillo-Cisneros <davidcc@google.com> wrote:
> This patch series adds a new flag to the struct perf_event
> (and a flag field to store it) to allow a PMU to tag a CPU or
> cgroup event as readable from any CPU in the same package and not
> just the CPU where the is currently active.
>
> This capability is used with uncore events to potentially avoid
> an unnecessary IPI when executing perf_event_read.
>
> A previous version of this change was introduced in the last Intel's
> CQM/CMT driver series (under review), but now we present it separately
> here since it is also useful for other uncore events.
>
> The next version of Intel CQM/CMT will add 3 new flags that use
> the pmu_event_flags field (added in patch 03 in this series).
>
> Patch 02 generalizes event->group_flags so that new flags can use it
> in a similar way that PERF_GROUP_SOFTWARE was used.
>
> Patches rebased at peterz/queue/perf/core
>
> Changes in v2:
> - Change logic to use event->group_flags instead of individually
> testing sibling in perf_event_read.
> - Remove erroneous read of inactive events.
>
> David Carrillo-Cisneros (4):
> perf/core: check return value of perf_event_read IPI
> perf/core: generalize event->group_flags
> perf/core: introduce PMU_EV_CAP_READ_ACTIVE_PKG
> perf/x86: use PMUEF_READ_CPU_PKG in uncore events
>
> arch/x86/events/intel/rapl.c | 2 ++
> arch/x86/events/intel/uncore.c | 2 ++
> arch/x86/events/intel/uncore_snb.c | 2 ++
> include/linux/perf_event.h | 21 +++++++++++----
> kernel/events/core.c | 52 +++++++++++++++++++++++++-------------
> 5 files changed, 57 insertions(+), 22 deletions(-)
>
> --
> 2.8.0.rc3.226.g39d4020
>
Reviewed-by: Nilay Vaish <nilayvaish@gmail.com>
^ permalink raw reply [flat|nested] 15+ messages in thread