Messages in this thread Patch in this message | | | Date | Tue, 21 Nov 2023 08:16:13 -0800 | From | Yury Norov <> | Subject | Re: [PATCH 07/34] perf/arm: optimize opencoded atomic find_bit() API |
| |
On Tue, Nov 21, 2023 at 03:53:44PM +0000, Will Deacon wrote: > On Sat, Nov 18, 2023 at 07:50:38AM -0800, Yury Norov wrote: > > Switch subsystem to use atomic find_bit() or atomic iterators as > > appropriate. > > > > Signed-off-by: Yury Norov <yury.norov@gmail.com> > > --- > > drivers/perf/arm-cci.c | 23 +++++------------------ > > drivers/perf/arm-ccn.c | 10 ++-------- > > drivers/perf/arm_dmc620_pmu.c | 9 ++------- > > drivers/perf/arm_pmuv3.c | 8 ++------ > > 4 files changed, 11 insertions(+), 39 deletions(-) > > > > diff --git a/drivers/perf/arm-cci.c b/drivers/perf/arm-cci.c > > index 61de861eaf91..70fbf9d09d37 100644 > > --- a/drivers/perf/arm-cci.c > > +++ b/drivers/perf/arm-cci.c > > @@ -320,12 +320,8 @@ static int cci400_get_event_idx(struct cci_pmu *cci_pmu, > > return CCI400_PMU_CYCLE_CNTR_IDX; > > } > > > > - for (idx = CCI400_PMU_CNTR0_IDX; idx <= CCI_PMU_CNTR_LAST(cci_pmu); ++idx) > > - if (!test_and_set_bit(idx, hw->used_mask)) > > - return idx; > > - > > - /* No counters available */ > > - return -EAGAIN; > > + idx = find_and_set_bit(hw->used_mask, CCI_PMU_CNTR_LAST(cci_pmu) + 1); > > CCI400_PMU_CNTR0_IDX is defined as 1, so isn't this wrong?
You're right. Will fix in v2 > [...] > > > diff --git a/drivers/perf/arm_dmc620_pmu.c b/drivers/perf/arm_dmc620_pmu.c > > index 30cea6859574..e41c84dabc3e 100644 > > --- a/drivers/perf/arm_dmc620_pmu.c > > +++ b/drivers/perf/arm_dmc620_pmu.c > > @@ -303,13 +303,8 @@ static int dmc620_get_event_idx(struct perf_event *event) > > end_idx = DMC620_PMU_MAX_COUNTERS; > > } > > > > - for (idx = start_idx; idx < end_idx; ++idx) { > > - if (!test_and_set_bit(idx, dmc620_pmu->used_mask)) > > - return idx; > > - } > > - > > - /* The counters are all in use. */ > > - return -EAGAIN; > > + idx = find_and_set_next_bit(dmc620_pmu->used_mask, end_idx, start_idx); > > It might just be me, but I'd find this a tonne easier to read if you swapped > the last two arguments around so that the offset came before the limit in > the new function.
I personally agree, but we already have find_next_*_bit(addr, nbits, offset) functions, and having atomic versions of the same with different order of arguments will make it even more messy...
Thanks, Yury
| |