On Mon, Oct 26, 2020 at 11:19:37PM +0900, Namhyung Kim wrote:
> This patch just added a warning before running it. I'd really want to
> fix the kernel if possible but don't have a good idea. Thoughts?
The easiest fix would be some multi threading in perf stat opening, then then
extra latencies could be mostly hidden. One thread per group would probably
be overkill, but just a few threads would lower the penalty significantly.
I think that would be better than this patch and it's likely not that much
more complicated, as this is already a lot of code.
> +{
> + const char *known_sw_pmu[] = {
> + "software", "tracepoint", "breakpoint", "kprobe", "uprobe", "msr"
That's a non scalable approach. New pmus get added regularly. It would be better to
indicate this in a generic way from the kernel.
> + pr_warning("WARNING: Event group has mixed hw/sw events.\n"
> + "This will slow down the perf_event_open syscall.\n"
> + "Consider putting a hw event as a leader.\n\n");
You really need to tell the user which group, otherwise it is hard to find
in a large command line.
-Andi
Andi Kleen <[email protected]> writes:
> On Mon, Oct 26, 2020 at 11:19:37PM +0900, Namhyung Kim wrote:
>> This patch just added a warning before running it. I'd really want to
>> fix the kernel if possible but don't have a good idea. Thoughts?
>
> The easiest fix would be some multi threading in perf stat opening, then then
> extra latencies could be mostly hidden. One thread per group would probably
> be overkill, but just a few threads would lower the penalty significantly.
>
> I think that would be better than this patch and it's likely not that much
> more complicated, as this is already a lot of code.
>
>> +{
>> + const char *known_sw_pmu[] = {
>> + "software", "tracepoint", "breakpoint", "kprobe", "uprobe", "msr"
>
> That's a non scalable approach. New pmus get added regularly. It would be better to
> indicate this in a generic way from the kernel.
That, and also, intel_pt is a software PMU and a few of its features
depend on intel_pt/.../ being a group leader.
Regards,
--
Alex
Hi Andi,
On Tue, Oct 27, 2020 at 12:21 AM Andi Kleen <[email protected]> wrote:
>
> On Mon, Oct 26, 2020 at 11:19:37PM +0900, Namhyung Kim wrote:
> > This patch just added a warning before running it. I'd really want to
> > fix the kernel if possible but don't have a good idea. Thoughts?
>
> The easiest fix would be some multi threading in perf stat opening, then then
> extra latencies could be mostly hidden. One thread per group would probably
> be overkill, but just a few threads would lower the penalty significantly.
Thanks for the suggestion. Yeah we could use threads to circumvent
the problem in userspace. But I think it'd better to solve it in the kernel.
Another problem I see is when there's a concurrent perf event in the
same context. Since it holds ctx->mutex during the synchronize_rcu
the other event should wait for it too. It'd be nice if it can release the
ctx->mutex before going to sleep unless we can remove it.
>
> I think that would be better than this patch and it's likely not that much
> more complicated, as this is already a lot of code.
>
> > +{
> > + const char *known_sw_pmu[] = {
> > + "software", "tracepoint", "breakpoint", "kprobe", "uprobe", "msr"
>
> That's a non scalable approach. New pmus get added regularly. It would be better to
> indicate this in a generic way from the kernel.
Maybe we can add a new attribute (task_ctx?) for that.
>
> > + pr_warning("WARNING: Event group has mixed hw/sw events.\n"
> > + "This will slow down the perf_event_open syscall.\n"
> > + "Consider putting a hw event as a leader.\n\n");
>
> You really need to tell the user which group, otherwise it is hard to find
> in a large command line.
OK
Thanks
Namhyung
Hi,
On Tue, Oct 27, 2020 at 12:49 AM Alexander Shishkin
<[email protected]> wrote:
>
> Andi Kleen <[email protected]> writes:
>
> > On Mon, Oct 26, 2020 at 11:19:37PM +0900, Namhyung Kim wrote:
> >> This patch just added a warning before running it. I'd really want to
> >> fix the kernel if possible but don't have a good idea. Thoughts?
> >
> > The easiest fix would be some multi threading in perf stat opening, then then
> > extra latencies could be mostly hidden. One thread per group would probably
> > be overkill, but just a few threads would lower the penalty significantly.
> >
> > I think that would be better than this patch and it's likely not that much
> > more complicated, as this is already a lot of code.
> >
> >> +{
> >> + const char *known_sw_pmu[] = {
> >> + "software", "tracepoint", "breakpoint", "kprobe", "uprobe", "msr"
> >
> > That's a non scalable approach. New pmus get added regularly. It would be better to
> > indicate this in a generic way from the kernel.
>
> That, and also, intel_pt is a software PMU and a few of its features
> depend on intel_pt/.../ being a group leader.
Thanks for the info, that's good to know.
So do you mean intel_pt requires other HW events in the same group?
Thanks
Namhyung