[PATCH 0/3] perf: note task_ctx_nr < 0 and sw event merging behavior

[PATCH 0/3] perf: note task_ctx_nr < 0 and sw event merging behavior

[Cody P Schafer]

These patches only add/adjust comments.

Cody P Schafer (3):
  perf: comment on usage of perf_invalid_context
  perf: clarify comment regarding event merging
  perf: clarify comment regarding perf_pmu_contexts

 include/linux/sched.h | 4 ++++
 kernel/events/core.c  | 8 +++++---
 2 files changed, 9 insertions(+), 3 deletions(-)

-- 
1.8.5.2

[PATCH 1/3] perf: comment on usage of perf_invalid_context

[Cody P Schafer]

Context numbers less than 0 are treated specially within the events
code, add a comment to document this.

Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
---
 include/linux/sched.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 53f97eb..f574820 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1033,6 +1033,10 @@ struct sched_rt_entity {
 struct rcu_node;
 
 enum perf_event_task_context {
+	/*
+	 * When <0, allocate a pmu local pmu->pmu_cpu_context (instead
+	 * of sharing among pmus in the same context) and forbid task tracking.
+	 */
 	perf_invalid_context = -1,
 	perf_hw_context = 0,
 	perf_sw_context,
-- 
1.8.5.2

Re: [PATCH 1/3] perf: comment on usage of perf_invalid_context

[Peter Zijlstra]

On Thu, Jan 09, 2014 at 03:51:30PM -0800, Cody P Schafer wrote:
> Context numbers less than 0 are treated specially within the events
> code, add a comment to document this.
> 
> Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
> ---
>  include/linux/sched.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 53f97eb..f574820 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1033,6 +1033,10 @@ struct sched_rt_entity {
>  struct rcu_node;
>  
>  enum perf_event_task_context {
> +	/*
> +	 * When <0, allocate a pmu local pmu->pmu_cpu_context (instead
> +	 * of sharing among pmus in the same context) and forbid task tracking.
> +	 */

Please explain things in terms of the existing enum names; we don't want
people to start using randon negative values instead of
'perf_invalid_context'.

>  	perf_invalid_context = -1,
>  	perf_hw_context = 0,
>  	perf_sw_context,
> -- 
> 1.8.5.2
> 

[PATCH 2/3] perf: clarify comment regarding event merging

[Cody P Schafer]

There are actually 2 things about software events that allow us to
merge them: they never fail to schedule _and_ they have transaction
handlers we can (and do, when they are added to !sw groups) ignore. Note
both of these in the comment on adding sw events to !sw groups.

Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
---
 kernel/events/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index f574401..e9f60d0 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7065,7 +7065,8 @@ SYSCALL_DEFINE5(perf_event_open,
 			 *
 			 * Allow the addition of software events to !software
 			 * groups, this is safe because software events never
-			 * fail to schedule.
+			 * fail to schedule and have ignorable transaction
+			 * handlers ({start,cancel,commit}_txn).
 			 */
 			pmu = group_leader->pmu;
 		} else if (is_software_event(group_leader) &&
-- 
1.8.5.2

Re: [PATCH 2/3] perf: clarify comment regarding event merging

[Peter Zijlstra]

On Thu, Jan 09, 2014 at 03:51:31PM -0800, Cody P Schafer wrote:
> There are actually 2 things about software events that allow us to
> merge them: they never fail to schedule _and_ they have transaction
> handlers we can (and do, when they are added to !sw groups) ignore. Note
> both of these in the comment on adding sw events to !sw groups.

The latter is a direct consequence of the former. Since they can always
be scheduled, they don't need any schedulability testing, and therefore
the transaction stuff is useless.

Re: [PATCH 2/3] perf: clarify comment regarding event merging

[Cody P Schafer]

On 01/10/2014 01:36 AM, Peter Zijlstra wrote:
> On Thu, Jan 09, 2014 at 03:51:31PM -0800, Cody P Schafer wrote:
>> There are actually 2 things about software events that allow us to
>> merge them: they never fail to schedule _and_ they have transaction
>> handlers we can (and do, when they are added to !sw groups) ignore. Note
>> both of these in the comment on adding sw events to !sw groups.
>
> The latter is a direct consequence of the former. Since they can always
> be scheduled, they don't need any schedulability testing, and therefore
> the transaction stuff is useless.

Right. I guess what I was getting at were the 2 types of "schedulability":
	1. individual event schedulability (ie: "did add() return an error?")
	2. txn schedulability (ie: "did commit_txn() return an error?")

I'm in the process of adding a pmu which guarantees #1, but not #2 (it 
essentially provides access to some always-running counters which can be 
atomically copied in groups). As a result, I'm teasing apart some of the 
special casing done for sw events.

This will probably make a bit more sense with some better terminology on 
my part and some actual code. I'll update and resend later.

[PATCH 3/3] perf: clarify comment regarding perf_pmu_contexts

[Cody P Schafer]

Again, note that the behavior of task_ctx_nr < 0 is an exception.

Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
---
 kernel/events/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index e9f60d0..159ef12 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6276,8 +6276,9 @@ static int perf_event_idx_default(struct perf_event *event)
 }
 
 /*
- * Ensures all contexts with the same task_ctx_nr have the same
- * pmu_cpu_context too.
+ * Ensures all contexts with the same task_ctx_nr (where that task_ctx_nr
+ * is >=0) have the same pmu_cpu_context too. Contexts with with negative (<0)
+ * task_ctx_nr do not share pmu_cpu_contexts.
  */
 static void *find_pmu_context(int ctxn)
 {
-- 
1.8.5.2

Re: [PATCH 3/3] perf: clarify comment regarding perf_pmu_contexts

[Peter Zijlstra]

On Thu, Jan 09, 2014 at 03:51:32PM -0800, Cody P Schafer wrote:
> Again, note that the behavior of task_ctx_nr < 0 is an exception.
> 
> Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
> ---
>  kernel/events/core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index e9f60d0..159ef12 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -6276,8 +6276,9 @@ static int perf_event_idx_default(struct perf_event *event)
>  }
>  
>  /*
> - * Ensures all contexts with the same task_ctx_nr have the same
> - * pmu_cpu_context too.
> + * Ensures all contexts with the same task_ctx_nr (where that task_ctx_nr
> + * is >=0) have the same pmu_cpu_context too. Contexts with with negative (<0)
> + * task_ctx_nr do not share pmu_cpu_contexts.
>   */

Please stay within the enum name space; the <0 really is an
implementation detail, mostly done because testing for sign is often
cheaper than an immediate comparison.