* [bug report] perf: Rewrite core context handling
@ 2022-11-15 12:46 Dan Carpenter
2022-11-16 6:02 ` Ravi Bangoria
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2022-11-15 12:46 UTC (permalink / raw)
To: peterz; +Cc: linux-perf-users
[ Resending two weeks of email. Crap. -dan ]
Hello Peter Zijlstra,
The patch bd2756811766: "perf: Rewrite core context handling" from
Oct 8, 2022, leads to the following Smatch static checker warning:
kernel/events/core.c:13190 inherit_event() warn: 'pmu_ctx' is an error pointer or valid
kernel/events/core.c
13146 static struct perf_event *
13147 inherit_event(struct perf_event *parent_event,
13148 struct task_struct *parent,
13149 struct perf_event_context *parent_ctx,
13150 struct task_struct *child,
13151 struct perf_event *group_leader,
13152 struct perf_event_context *child_ctx)
13153 {
13154 enum perf_event_state parent_state = parent_event->state;
13155 struct perf_event_pmu_context *pmu_ctx;
13156 struct perf_event *child_event;
13157 unsigned long flags;
13158
13159 /*
13160 * Instead of creating recursive hierarchies of events,
13161 * we link inherited events back to the original parent,
13162 * which has a filp for sure, which we use as the reference
13163 * count:
13164 */
13165 if (parent_event->parent)
13166 parent_event = parent_event->parent;
13167
13168 child_event = perf_event_alloc(&parent_event->attr,
13169 parent_event->cpu,
13170 child,
13171 group_leader, parent_event,
13172 NULL, NULL, -1);
13173 if (IS_ERR(child_event))
13174 return child_event;
^^^^^^^^^^^^^^^^^^^
inherit_event() has historically returned a mix of error pointers and
NULL. I am not clear on the rules.
13175
13176 pmu_ctx = find_get_pmu_context(child_event->pmu, child_ctx, child_event);
--> 13177 if (!pmu_ctx) {
The find_get_pmu_context() function only returns error pointers so this
is straight forward, but I don't know if this path should return an
error pointer or NULL or what. Probably easiest to just report it to
you. :)
13178 free_event(child_event);
13179 return NULL;
13180 }
13181 child_event->pmu_ctx = pmu_ctx;
13182
13183 /*
13184 * is_orphaned_event() and list_add_tail(&parent_event->child_list)
13185 * must be under the same lock in order to serialize against
13186 * perf_event_release_kernel(), such that either we must observe
13187 * is_orphaned_event() or they will observe us on the child_list.
13188 */
13189 mutex_lock(&parent_event->child_mutex);
13190 if (is_orphaned_event(parent_event) ||
13191 !atomic_long_inc_not_zero(&parent_event->refcount)) {
13192 mutex_unlock(&parent_event->child_mutex);
13193 /* task_ctx_data is freed with child_ctx */
13194 free_event(child_event);
13195 return NULL;
13196 }
13197
13198 get_ctx(child_ctx);
13199
13200 /*
13201 * Make the child state follow the state of the parent event,
13202 * not its attr.disabled bit. We hold the parent's mutex,
13203 * so we won't race with perf_event_{en, dis}able_family.
13204 */
13205 if (parent_state >= PERF_EVENT_STATE_INACTIVE)
13206 child_event->state = PERF_EVENT_STATE_INACTIVE;
13207 else
13208 child_event->state = PERF_EVENT_STATE_OFF;
13209
13210 if (parent_event->attr.freq) {
13211 u64 sample_period = parent_event->hw.sample_period;
13212 struct hw_perf_event *hwc = &child_event->hw;
13213
13214 hwc->sample_period = sample_period;
13215 hwc->last_period = sample_period;
13216
13217 local64_set(&hwc->period_left, sample_period);
13218 }
13219
13220 child_event->ctx = child_ctx;
13221 child_event->overflow_handler = parent_event->overflow_handler;
13222 child_event->overflow_handler_context
13223 = parent_event->overflow_handler_context;
13224
13225 /*
13226 * Precalculate sample_data sizes
13227 */
13228 perf_event__header_size(child_event);
13229 perf_event__id_header_size(child_event);
13230
13231 /*
13232 * Link it up in the child's context:
13233 */
13234 raw_spin_lock_irqsave(&child_ctx->lock, flags);
13235 add_event_to_ctx(child_event, child_ctx);
13236 child_event->attach_state |= PERF_ATTACH_CHILD;
13237 raw_spin_unlock_irqrestore(&child_ctx->lock, flags);
13238
13239 /*
13240 * Link this into the parent event's child list
13241 */
13242 list_add_tail(&child_event->child_list, &parent_event->child_list);
13243 mutex_unlock(&parent_event->child_mutex);
13244
13245 return child_event;
13246 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] perf: Rewrite core context handling
2022-11-15 12:46 [bug report] perf: Rewrite core context handling Dan Carpenter
@ 2022-11-16 6:02 ` Ravi Bangoria
2022-11-18 5:15 ` [PATCH] perf core: Return error pointer if inherit_event() fails to find pmu_ctx Ravi Bangoria
0 siblings, 1 reply; 3+ messages in thread
From: Ravi Bangoria @ 2022-11-16 6:02 UTC (permalink / raw)
To: Dan Carpenter; +Cc: peterz, linux-perf-users, cuigaosheng1, Ian Rogers
Hi Dan,
> 13168 child_event = perf_event_alloc(&parent_event->attr,
> 13169 parent_event->cpu,
> 13170 child,
> 13171 group_leader, parent_event,
> 13172 NULL, NULL, -1);
> 13173 if (IS_ERR(child_event))
> 13174 return child_event;
> ^^^^^^^^^^^^^^^^^^^
> inherit_event() has historically returned a mix of error pointers and
> NULL. I am not clear on the rules.
This probably needs to be fixed.
> 13176 pmu_ctx = find_get_pmu_context(child_event->pmu, child_ctx, child_event);
> --> 13177 if (!pmu_ctx) {
>
> The find_get_pmu_context() function only returns error pointers so this
> is straight forward, but I don't know if this path should return an
> error pointer or NULL or what. Probably easiest to just report it to
> you. :)
A fix for this one:
https://lore.kernel.org/r/20221114091833.1492575-1-cuigaosheng1@huawei.com
Thanks,
Ravi
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH] perf core: Return error pointer if inherit_event() fails to find pmu_ctx
2022-11-16 6:02 ` Ravi Bangoria
@ 2022-11-18 5:15 ` Ravi Bangoria
0 siblings, 0 replies; 3+ messages in thread
From: Ravi Bangoria @ 2022-11-18 5:15 UTC (permalink / raw)
To: peterz
Cc: ravi.bangoria, acme, mark.rutland, jolsa, namhyung, error27,
linux-perf-users, linux-kernel, sandipan.das, ananth.narayan,
santosh.shukla
inherit_event() returns NULL only when it finds orphaned events
otherwise it returns either valid child_event pointer or an error
pointer. Follow the same when it fails to find pmu_ctx.
Reported-by: Dan Carpenter <error27@gmail.com>
Fixes: bd2756811766 ("perf: Rewrite core context handling")
Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
---
kernel/events/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 5f262f91cd31..ce75288cdbae 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -13175,7 +13175,7 @@ inherit_event(struct perf_event *parent_event,
pmu_ctx = find_get_pmu_context(child_event->pmu, child_ctx, child_event);
if (IS_ERR(pmu_ctx)) {
free_event(child_event);
- return NULL;
+ return ERR_CAST(pmu_ctx);
}
child_event->pmu_ctx = pmu_ctx;
--
2.37.3
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-11-18 5:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-15 12:46 [bug report] perf: Rewrite core context handling Dan Carpenter
2022-11-16 6:02 ` Ravi Bangoria
2022-11-18 5:15 ` [PATCH] perf core: Return error pointer if inherit_event() fails to find pmu_ctx Ravi Bangoria
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).