* [PATCH RESEND] perf/core: Fix installing cgroup event into cpu
@ 2018-01-23 4:13 linxiulei
2018-01-23 16:37 ` Peter Zijlstra
2018-01-26 2:02 ` kbuild test robot
0 siblings, 2 replies; 5+ messages in thread
From: linxiulei @ 2018-01-23 4:13 UTC (permalink / raw)
To: peterz, jolsa, mingo, acme, alexander.shishkin, linux-kernel,
tglx, eranian, torvalds, linux-perf-users, brendan.d.gregg
Cc: yang_oliver, jinli.zjl, leilei.lin
From: "leilei.lin" <leilei.lin@alibaba-inc.com>
Do not install cgroup event into the CPU context if the cgroup
is not running on this CPU
While there is no task of cgroup running specified CPU, current
kernel still install cgroup event into CPU context, that causes
another cgroup event can't be installed into this CPU.
Signed-off-by: leilei.lin <leilei.lin@alibaba-inc.com>
---
kernel/events/core.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 4df5b69..19c587b 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2284,6 +2284,7 @@ static int __perf_install_in_context(void *info)
struct perf_event_context *ctx = event->ctx;
struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
struct perf_event_context *task_ctx = cpuctx->task_ctx;
+ struct perf_cgroup *cgrp;
bool reprogram = true;
int ret = 0;
@@ -2311,6 +2312,19 @@ static int __perf_install_in_context(void *info)
raw_spin_lock(&task_ctx->lock);
}
+ if (event->cgrp) {
+ /*
+ * Only care about cgroup events.
+ *
+ * If only the task belongs to cgroup of this event,
+ * we will continue the installment
+ */
+ cgrp = perf_cgroup_from_task(current, ctx);
+ if (!cgroup_is_descendant(cgrp->css.cgroup,
+ event->cgrp->css.cgroup))
+ goto unlock;
+ }
+
if (reprogram) {
ctx_sched_out(ctx, cpuctx, EVENT_TIME);
add_event_to_ctx(event, ctx);
--
2.8.4.31.g9ed660f
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH RESEND] perf/core: Fix installing cgroup event into cpu
2018-01-23 4:13 [PATCH RESEND] perf/core: Fix installing cgroup event into cpu linxiulei
@ 2018-01-23 16:37 ` Peter Zijlstra
2018-01-24 7:29 ` Lin Xiulei
2018-01-26 2:02 ` kbuild test robot
1 sibling, 1 reply; 5+ messages in thread
From: Peter Zijlstra @ 2018-01-23 16:37 UTC (permalink / raw)
To: linxiulei
Cc: jolsa, mingo, acme, alexander.shishkin, linux-kernel, tglx,
eranian, torvalds, linux-perf-users, brendan.d.gregg,
yang_oliver, jinli.zjl, leilei.lin
On Tue, Jan 23, 2018 at 12:13:06PM +0800, linxiulei@gmail.com wrote:
> From: "leilei.lin" <leilei.lin@alibaba-inc.com>
>
> Do not install cgroup event into the CPU context if the cgroup
> is not running on this CPU
>
> While there is no task of cgroup running specified CPU, current
> kernel still install cgroup event into CPU context, that causes
> another cgroup event can't be installed into this CPU.
>
> Signed-off-by: leilei.lin <leilei.lin@alibaba-inc.com>
> ---
> kernel/events/core.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 4df5b69..19c587b 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -2284,6 +2284,7 @@ static int __perf_install_in_context(void *info)
> struct perf_event_context *ctx = event->ctx;
> struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
> struct perf_event_context *task_ctx = cpuctx->task_ctx;
> + struct perf_cgroup *cgrp;
> bool reprogram = true;
> int ret = 0;
>
> @@ -2311,6 +2312,19 @@ static int __perf_install_in_context(void *info)
> raw_spin_lock(&task_ctx->lock);
> }
>
> + if (event->cgrp) {
> + /*
> + * Only care about cgroup events.
> + *
> + * If only the task belongs to cgroup of this event,
> + * we will continue the installment
> + */
> + cgrp = perf_cgroup_from_task(current, ctx);
> + if (!cgroup_is_descendant(cgrp->css.cgroup,
> + event->cgrp->css.cgroup))
> + goto unlock;
> + }
> +
> if (reprogram) {
> ctx_sched_out(ctx, cpuctx, EVENT_TIME);
> add_event_to_ctx(event, ctx);
I think this is wrong. You're right in that we need not schedule it not,
but the above also completely fails to add it to the context, leading to
it never being scheduled ever.
At the very least we should do add_event_to_ctx() on it.
So the only thing you can do is pick 'reprogram' or not based on if the
current cgrp is related to the event->ctx.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RESEND] perf/core: Fix installing cgroup event into cpu
2018-01-23 16:37 ` Peter Zijlstra
@ 2018-01-24 7:29 ` Lin Xiulei
0 siblings, 0 replies; 5+ messages in thread
From: Lin Xiulei @ 2018-01-24 7:29 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Jiri Olsa, mingo, acme, alexander.shishkin, linux-kernel, tglx,
Stephane Eranian, torvalds, linux-perf-users, Brendan Gregg,
yang_oliver, jinli.zjl, leilei.lin
yes, you are right. In my cases, there also are some issues in
add_event_to_ctx(),
I am gonna fix it at v2
Thanks
2018-01-24 0:37 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
> On Tue, Jan 23, 2018 at 12:13:06PM +0800, linxiulei@gmail.com wrote:
>> From: "leilei.lin" <leilei.lin@alibaba-inc.com>
>>
>> Do not install cgroup event into the CPU context if the cgroup
>> is not running on this CPU
>>
>> While there is no task of cgroup running specified CPU, current
>> kernel still install cgroup event into CPU context, that causes
>> another cgroup event can't be installed into this CPU.
>>
>> Signed-off-by: leilei.lin <leilei.lin@alibaba-inc.com>
>> ---
>> kernel/events/core.c | 14 ++++++++++++++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index 4df5b69..19c587b 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -2284,6 +2284,7 @@ static int __perf_install_in_context(void *info)
>> struct perf_event_context *ctx = event->ctx;
>> struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
>> struct perf_event_context *task_ctx = cpuctx->task_ctx;
>> + struct perf_cgroup *cgrp;
>> bool reprogram = true;
>> int ret = 0;
>>
>> @@ -2311,6 +2312,19 @@ static int __perf_install_in_context(void *info)
>> raw_spin_lock(&task_ctx->lock);
>> }
>>
>> + if (event->cgrp) {
>> + /*
>> + * Only care about cgroup events.
>> + *
>> + * If only the task belongs to cgroup of this event,
>> + * we will continue the installment
>> + */
>> + cgrp = perf_cgroup_from_task(current, ctx);
>> + if (!cgroup_is_descendant(cgrp->css.cgroup,
>> + event->cgrp->css.cgroup))
>> + goto unlock;
>> + }
>> +
>> if (reprogram) {
>> ctx_sched_out(ctx, cpuctx, EVENT_TIME);
>> add_event_to_ctx(event, ctx);
>
> I think this is wrong. You're right in that we need not schedule it not,
> but the above also completely fails to add it to the context, leading to
> it never being scheduled ever.
>
> At the very least we should do add_event_to_ctx() on it.
>
> So the only thing you can do is pick 'reprogram' or not based on if the
> current cgrp is related to the event->ctx.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RESEND] perf/core: Fix installing cgroup event into cpu
2018-01-23 4:13 [PATCH RESEND] perf/core: Fix installing cgroup event into cpu linxiulei
2018-01-23 16:37 ` Peter Zijlstra
@ 2018-01-26 2:02 ` kbuild test robot
2018-01-29 3:37 ` Lin Xiulei
1 sibling, 1 reply; 5+ messages in thread
From: kbuild test robot @ 2018-01-26 2:02 UTC (permalink / raw)
To: linxiulei
Cc: kbuild-all, peterz, jolsa, mingo, acme, alexander.shishkin,
linux-kernel, tglx, eranian, torvalds, linux-perf-users,
brendan.d.gregg, yang_oliver, jinli.zjl, leilei.lin
[-- Attachment #1: Type: text/plain, Size: 3889 bytes --]
Hi leilei.lin,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tip/perf/core]
[also build test ERROR on v4.15-rc9 next-20180119]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/linxiulei-gmail-com/perf-core-Fix-installing-cgroup-event-into-cpu/20180126-083327
config: x86_64-randconfig-g0-01260853 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
kernel//events/core.c: In function '__perf_install_in_context':
kernel//events/core.c:2315:11: error: 'struct perf_event' has no member named 'cgrp'
if (event->cgrp) {
^
>> kernel//events/core.c:2322:3: error: implicit declaration of function 'perf_cgroup_from_task' [-Werror=implicit-function-declaration]
cgrp = perf_cgroup_from_task(current, ctx);
^
kernel//events/core.c:2322:8: warning: assignment makes pointer from integer without a cast
cgrp = perf_cgroup_from_task(current, ctx);
^
>> kernel//events/core.c:2323:33: error: dereferencing pointer to incomplete type
if (!cgroup_is_descendant(cgrp->css.cgroup,
^
kernel//events/core.c:2324:11: error: 'struct perf_event' has no member named 'cgrp'
event->cgrp->css.cgroup))
^
cc1: some warnings being treated as errors
vim +/perf_cgroup_from_task +2322 kernel//events/core.c
2274
2275 /*
2276 * Cross CPU call to install and enable a performance event
2277 *
2278 * Very similar to remote_function() + event_function() but cannot assume that
2279 * things like ctx->is_active and cpuctx->task_ctx are set.
2280 */
2281 static int __perf_install_in_context(void *info)
2282 {
2283 struct perf_event *event = info;
2284 struct perf_event_context *ctx = event->ctx;
2285 struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
2286 struct perf_event_context *task_ctx = cpuctx->task_ctx;
2287 struct perf_cgroup *cgrp;
2288 bool reprogram = true;
2289 int ret = 0;
2290
2291 raw_spin_lock(&cpuctx->ctx.lock);
2292 if (ctx->task) {
2293 raw_spin_lock(&ctx->lock);
2294 task_ctx = ctx;
2295
2296 reprogram = (ctx->task == current);
2297
2298 /*
2299 * If the task is running, it must be running on this CPU,
2300 * otherwise we cannot reprogram things.
2301 *
2302 * If its not running, we don't care, ctx->lock will
2303 * serialize against it becoming runnable.
2304 */
2305 if (task_curr(ctx->task) && !reprogram) {
2306 ret = -ESRCH;
2307 goto unlock;
2308 }
2309
2310 WARN_ON_ONCE(reprogram && cpuctx->task_ctx && cpuctx->task_ctx != ctx);
2311 } else if (task_ctx) {
2312 raw_spin_lock(&task_ctx->lock);
2313 }
2314
> 2315 if (event->cgrp) {
2316 /*
2317 * Only care about cgroup events.
2318 *
2319 * If only the task belongs to cgroup of this event,
2320 * we will continue the installment
2321 */
> 2322 cgrp = perf_cgroup_from_task(current, ctx);
> 2323 if (!cgroup_is_descendant(cgrp->css.cgroup,
2324 event->cgrp->css.cgroup))
2325 goto unlock;
2326 }
2327
2328 if (reprogram) {
2329 ctx_sched_out(ctx, cpuctx, EVENT_TIME);
2330 add_event_to_ctx(event, ctx);
2331 ctx_resched(cpuctx, task_ctx, get_event_type(event));
2332 } else {
2333 add_event_to_ctx(event, ctx);
2334 }
2335
2336 unlock:
2337 perf_ctx_unlock(cpuctx, task_ctx);
2338
2339 return ret;
2340 }
2341
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27148 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RESEND] perf/core: Fix installing cgroup event into cpu
2018-01-26 2:02 ` kbuild test robot
@ 2018-01-29 3:37 ` Lin Xiulei
0 siblings, 0 replies; 5+ messages in thread
From: Lin Xiulei @ 2018-01-29 3:37 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all, Peter Zijlstra, Jiri Olsa, mingo, acme,
alexander.shishkin, linux-kernel, tglx, Stephane Eranian,
torvalds, linux-perf-users, Brendan Gregg, yang_oliver,
jinli.zjl, leilei.lin
2018-01-26 10:02 GMT+08:00 kbuild test robot <lkp@intel.com>:
> Hi leilei.lin,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on tip/perf/core]
> [also build test ERROR on v4.15-rc9 next-20180119]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/linxiulei-gmail-com/perf-core-Fix-installing-cgroup-event-into-cpu/20180126-083327
> config: x86_64-randconfig-g0-01260853 (attached as .config)
> compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64
>
> All errors (new ones prefixed by >>):
>
> kernel//events/core.c: In function '__perf_install_in_context':
> kernel//events/core.c:2315:11: error: 'struct perf_event' has no member named 'cgrp'
> if (event->cgrp) {
> ^
>>> kernel//events/core.c:2322:3: error: implicit declaration of function 'perf_cgroup_from_task' [-Werror=implicit-function-declaration]
> cgrp = perf_cgroup_from_task(current, ctx);
> ^
> kernel//events/core.c:2322:8: warning: assignment makes pointer from integer without a cast
> cgrp = perf_cgroup_from_task(current, ctx);
> ^
>>> kernel//events/core.c:2323:33: error: dereferencing pointer to incomplete type
> if (!cgroup_is_descendant(cgrp->css.cgroup,
> ^
> kernel//events/core.c:2324:11: error: 'struct perf_event' has no member named 'cgrp'
> event->cgrp->css.cgroup))
> ^
> cc1: some warnings being treated as errors
>
> vim +/perf_cgroup_from_task +2322 kernel//events/core.c
>
> 2274
> 2275 /*
> 2276 * Cross CPU call to install and enable a performance event
> 2277 *
> 2278 * Very similar to remote_function() + event_function() but cannot assume that
> 2279 * things like ctx->is_active and cpuctx->task_ctx are set.
> 2280 */
> 2281 static int __perf_install_in_context(void *info)
> 2282 {
> 2283 struct perf_event *event = info;
> 2284 struct perf_event_context *ctx = event->ctx;
> 2285 struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
> 2286 struct perf_event_context *task_ctx = cpuctx->task_ctx;
> 2287 struct perf_cgroup *cgrp;
> 2288 bool reprogram = true;
> 2289 int ret = 0;
> 2290
> 2291 raw_spin_lock(&cpuctx->ctx.lock);
> 2292 if (ctx->task) {
> 2293 raw_spin_lock(&ctx->lock);
> 2294 task_ctx = ctx;
> 2295
> 2296 reprogram = (ctx->task == current);
> 2297
> 2298 /*
> 2299 * If the task is running, it must be running on this CPU,
> 2300 * otherwise we cannot reprogram things.
> 2301 *
> 2302 * If its not running, we don't care, ctx->lock will
> 2303 * serialize against it becoming runnable.
> 2304 */
> 2305 if (task_curr(ctx->task) && !reprogram) {
> 2306 ret = -ESRCH;
> 2307 goto unlock;
> 2308 }
> 2309
> 2310 WARN_ON_ONCE(reprogram && cpuctx->task_ctx && cpuctx->task_ctx != ctx);
> 2311 } else if (task_ctx) {
> 2312 raw_spin_lock(&task_ctx->lock);
> 2313 }
> 2314
>> 2315 if (event->cgrp) {
> 2316 /*
> 2317 * Only care about cgroup events.
> 2318 *
> 2319 * If only the task belongs to cgroup of this event,
> 2320 * we will continue the installment
> 2321 */
>> 2322 cgrp = perf_cgroup_from_task(current, ctx);
>> 2323 if (!cgroup_is_descendant(cgrp->css.cgroup,
> 2324 event->cgrp->css.cgroup))
> 2325 goto unlock;
> 2326 }
> 2327
> 2328 if (reprogram) {
> 2329 ctx_sched_out(ctx, cpuctx, EVENT_TIME);
> 2330 add_event_to_ctx(event, ctx);
> 2331 ctx_resched(cpuctx, task_ctx, get_event_type(event));
> 2332 } else {
> 2333 add_event_to_ctx(event, ctx);
> 2334 }
> 2335
> 2336 unlock:
> 2337 perf_ctx_unlock(cpuctx, task_ctx);
> 2338
> 2339 return ret;
> 2340 }
> 2341
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
@peter seems it has trouble in merging v1 patch, in fact it should've
merged the v3 patch. What am I supposed to do to fix it?
thanks
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-01-29 3:37 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-23 4:13 [PATCH RESEND] perf/core: Fix installing cgroup event into cpu linxiulei
2018-01-23 16:37 ` Peter Zijlstra
2018-01-24 7:29 ` Lin Xiulei
2018-01-26 2:02 ` kbuild test robot
2018-01-29 3:37 ` Lin Xiulei
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).