| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 13 Jan 2010 18:22:54 +0100
From: Stephane Eranian <eranian@...gle.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: linux-kernel@...r.kernel.org, mingo@...e.hu, paulus@...ba.org,
davem@...emloft.net, perfmon2-devel@...ts.sf.net,
Frédéric Weisbecker <fweisbec@...il.com>
Subject: Re: [PATCH] perf_events: improve x86 event scheduling
Ok,
Something like that should problably do it:
static void event_sched_out(struct perf_event *event, int cpu)
{
event->state = PERF_EVENT_STATE_INACTIVE;
event->oncpu = -1;
}
hw_perf_group_sched_in()
{
....
n = 1;
list_for_each_entry(sub, &leader->sibling_list, group_entry) {
if (sub->state > PERF_EVENT_STATE_OFF) {
ret = event_sched_in(sub, cpu);
if (ret)
goto undo;
++n;
}
}
/*
* copy new assignment, now we know it is possible
* will be used by hw_perf_enable()
*/
memcpy(cpuc->assign, assign, n*sizeof(int));
cpuc->n_events = n;
cpuc->n_added = n - n0;
ctx->nr_active += n;
/*
* 1 means successful and events are active
* This is not quite true because we defer
* actual activation until hw_perf_enable() but
* this way we* ensure caller won't try to enable
* individual events
*/
return 1;
undo:
event_sched_out(leader, cpu);
n0 = n;
n = 1;
list_for_each_entry(sub, &leader->sibling_list, group_entry) {
if (sub->state == PERF_EVENT_STATE_ACTIVE) {
event_sched_out(sub, cpu);
if (++n == n0)
break;
}
}
return ret;
}
Looking at the generic and PPC code, there are a few points which
are still unclear to me:
1. I believe that the check on attr_exclusive as is done in
perf_event.c:event_sched_in()
is missing from my patch and the PPC equivalent code.
if (event->attr.exclusive)
cpuctx->exclusive = 1;
So is the smp_wmb() and the accounting in cpuctx->active_oncpu.
2. the management of event->tstamp_stopped, tstamp_running
Looks like we may have to undo the update to stamp_running or
defer the update
until we know for sure, event_sched_in() succeeded for all events,
incl. software
events.
On Wed, Jan 13, 2010 at 5:52 PM, Stephane Eranian <eranian@...gle.com> wrote:
> On Wed, Jan 13, 2010 at 5:29 PM, Peter Zijlstra <peterz@...radead.org> wrote:
>> On Wed, 2010-01-13 at 10:54 +0100, Stephane Eranian wrote:
>>
>>> > One concern I do have is the loss of error checking on
>>> > event_sched_in()'s event->pmu->enable(), that could be another
>>> > 'hardware' PMU like breakpoints, in which case it could fail.
>>> >
>>> Well, x86_pmu_enable() does return an error code, so it is up
>>> to the upper layer to handle the error gracefully and in particular
>>> in perf_ctx_adjust_freq().
>>
>>> +static void event_sched_in(struct perf_event *event, int cpu)
>>> +{
>>> + event->state = PERF_EVENT_STATE_ACTIVE;
>>> + event->oncpu = cpu;
>>> + event->tstamp_running += event->ctx->time - event->tstamp_stopped;
>>> + if (is_software_event(event))
>>> + event->pmu->enable(event);
>>> +}
>>> +
>>> +/*
>>> + * Called to enable a whole group of events.
>>> + * Returns 1 if the group was enabled, or -EAGAIN if it could not be.
>>> + * Assumes the caller has disabled interrupts and has
>>> + * frozen the PMU with hw_perf_save_disable.
>>> + *
>>> + * called with PMU disabled. If successful and return value 1,
>>> + * then guaranteed to call perf_enable() and hw_perf_enable()
>>> + */
>>> +int hw_perf_group_sched_in(struct perf_event *leader,
>>> + struct perf_cpu_context *cpuctx,
>>> + struct perf_event_context *ctx, int cpu)
>>> +{
>>> + struct cpu_hw_events *cpuc = &per_cpu(cpu_hw_events, cpu);
>>> + struct perf_event *sub;
>>> + int assign[X86_PMC_IDX_MAX];
>>> + int n, n0, ret;
>>> +
>>> + n0 = cpuc->n_events;
>>> +
>>> + n = collect_events(cpuc, leader, true);
>>> + if (n < 0)
>>> + return n;
>>> +
>>> + ret = x86_schedule_events(cpuc, n, assign);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /*
>>> + * copy new assignment, now we know it is possible
>>> + * will be used by hw_perf_enable()
>>> + */
>>> + memcpy(cpuc->assign, assign, n*sizeof(int));
>>> +
>>> + cpuc->n_events = n;
>>> + cpuc->n_added = n - n0;
>>> +
>>> + n = 1;
>>> + event_sched_in(leader, cpu);
>>> + list_for_each_entry(sub, &leader->sibling_list, group_entry) {
>>> + if (sub->state != PERF_EVENT_STATE_OFF) {
>>> + event_sched_in(sub, cpu);
>>> + ++n;
>>> + }
>>> + }
>>> + ctx->nr_active += n;
>>> +
>>> + /*
>>> + * 1 means successful and events are active
>>> + * This is not quite true because we defer
>>> + * actual activation until hw_perf_enable() but
>>> + * this way we* ensure caller won't try to enable
>>> + * individual events
>>> + */
>>> + return 1;
>>> +}
>>
>> That most certainly looses error codes for all !is_x86_event() events in
>> the group.
>>
>> So you either need to deal with that event_sched_in() failing, or
>> guarantee that it always succeeds (forcing software events only for
>> example).
>>
> True, but that one can be fixed because it is only called from
> hw_perf_group_sched_in() which returns an error code.
>
> The same code would have to be fixed on PPC as well.
>
>>
>
>
>
> --
> Stephane Eranian | EMEA Software Engineering
> Google France | 38 avenue de l'Opéra | 75002 Paris
> Tel : +33 (0) 1 42 68 53 00
> This email may be confidential or privileged. If you received this
> communication by mistake, please
> don't forward it to anyone else, please erase all copies and
> attachments, and please let me know that
> it went to the wrong person. Thanks
>
--
Stephane Eranian | EMEA Software Engineering
Google France | 38 avenue de l'Opéra | 75002 Paris
Tel : +33 (0) 1 42 68 53 00
This email may be confidential or privileged. If you received this
communication by mistake, please
don't forward it to anyone else, please erase all copies and
attachments, and please let me know that
it went to the wrong person. Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists