Re: [PATCH 31/34] drivers/perf: optimize m1_pmu_get_event_idx() by using find_bit() API

From: Yury Norov <yury.norov@gmail.com>
To: Marc Zyngier <maz@kernel.org>
Cc: linux-kernel@vger.kernel.org, Will Deacon <will@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	linux-arm-kernel@lists.infradead.org, Jan Kara <jack@suse.cz>,
	Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>,
	Matthew Wilcox <willy@infradead.org>,
	Rasmus Villemoes <linux@rasmusvillemoes.dk>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Maxim Kuvyrkov <maxim.kuvyrkov@linaro.org>,
	Alexey Klimov <klimov.linux@gmail.com>
Subject: Re: [PATCH 31/34] drivers/perf: optimize m1_pmu_get_event_idx() by using find_bit() API
Date: Sat, 18 Nov 2023 10:45:10 -0800	[thread overview]
Message-ID: <ZVkGNhZyA3S4m8Os@yury-ThinkPad> (raw)
In-Reply-To: <87cyw6ykes.wl-maz@kernel.org>

On Sat, Nov 18, 2023 at 06:40:43PM +0000, Marc Zyngier wrote:
> On Sat, 18 Nov 2023 15:51:02 +0000,
> Yury Norov <yury.norov@gmail.com> wrote:
> > 
> > The function searches used_mask for a bit in a for-loop bit by bit.
> > We can do it faster by using atomic find_and_set_bit().
> 
> Sure, let's do things fast. Correctness is overrated anyway.
> 
> > 
> > The comment to the function says that it searches for the first free
> > counter, but obviously for_each_set_bit() searches for the first set
> > counter.
> 
> No it doesn't. It iterates over the counters the event can count on.
> 
> > The following test_and_set_bit() tries to enable already set
> > bit, which is weird.
> 
> Maybe you could try to actually read the code?
> 
> > 
> > This patch, by using find_and_set_bit(), fixes this automatically.
> 
> This doesn't fix anything, but instead actively breaks the driver.
> 
> > 
> > Fixes: a639027a1be1 ("drivers/perf: Add Apple icestorm/firestorm CPU PMU driver")
> > Signed-off-by: Yury Norov <yury.norov@gmail.com>
> > ---
> >  drivers/perf/apple_m1_cpu_pmu.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/perf/apple_m1_cpu_pmu.c b/drivers/perf/apple_m1_cpu_pmu.c
> > index cd2de44b61b9..2d50670ffb01 100644
> > --- a/drivers/perf/apple_m1_cpu_pmu.c
> > +++ b/drivers/perf/apple_m1_cpu_pmu.c
> > @@ -447,12 +447,8 @@ static int m1_pmu_get_event_idx(struct pmu_hw_events *cpuc,
> >  	 * counting on the PMU at any given time, and by placing the
> >  	 * most constraining events first.
> >  	 */
> > -	for_each_set_bit(idx, &affinity, M1_PMU_NR_COUNTERS) {
> > -		if (!test_and_set_bit(idx, cpuc->used_mask))
> > -			return idx;
> > -	}
> > -
> > -	return -EAGAIN;
> > +	idx = find_and_set_bit(cpuc->used_mask, M1_PMU_NR_COUNTERS);
> > +	return idx < M1_PMU_NR_COUNTERS ? idx : -EAGAIN;
> 
> So now you're picking any possible counter, irrespective of the
> possible affinity of the event. This is great.

Ok, I'll drop the patch. Sorry.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel