netdev - Re: [PATCH V5 1/1] bpf: control events stored in PERF_EVENT_ARRAY maps trace data output when perf sampling

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 21 Oct 2015 11:12:54 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Alexei Starovoitov <ast@...mgrid.com>
Cc:	Kaixu Xia <xiakaixu@...wei.com>, davem@...emloft.net,
	acme@...nel.org, mingo@...hat.com, masami.hiramatsu.pt@...achi.com,
	jolsa@...nel.org, daniel@...earbox.net, wangnan0@...wei.com,
	linux-kernel@...r.kernel.org, pi3orama@....com, hekuang@...wei.com,
	netdev@...r.kernel.org
Subject: Re: [PATCH V5 1/1] bpf: control events stored in PERF_EVENT_ARRAY
 maps trace data output when perf sampling

On Tue, Oct 20, 2015 at 03:53:02PM -0700, Alexei Starovoitov wrote:
> On 10/20/15 12:22 AM, Kaixu Xia wrote:
> >diff --git a/kernel/events/core.c b/kernel/events/core.c
> >index b11756f..5219635 100644
> >--- a/kernel/events/core.c
> >+++ b/kernel/events/core.c
> >@@ -6337,6 +6337,9 @@ static int __perf_event_overflow(struct perf_event *event,
> >  		irq_work_queue(&event->pending);
> >  	}
> >
> >+	if (unlikely(!atomic_read(&event->soft_enable)))
> >+		return 0;
> >+
> >  	if (event->overflow_handler)
> >  		event->overflow_handler(event, data, regs);
> >  	else
> 
> Peter,
> does this part look right or it should be moved right after
> if (unlikely(!is_sampling_event(event)))
>                 return 0;
> or even to other function?
> 
> It feels to me that it should be moved, since we probably don't
> want to active throttling, period adjust and event_limit for events
> that are in soft_disabled state.

Depends on what its meant to do. As long as you let the interrupt
happen, I think we should in fact do those things (maybe not the
event_limit), but period adjustment and esp. throttling are important
when the event is enabled.

If you want to actually disable the event: pmu->stop() will make it
stop, and you can restart using pmu->start().

And I suppose you can wrap that with a counter if you need nesting.

I'm not sure if any of that is a viable solution, because the patch
description is somewhat short on the problem statement.

As is, I'm not too charmed with the patch, but lacking a better
understanding of what exactly we're trying to achieve I'm struggling
with proposing alternatives.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html