mbox series

[v3,0/2] dmaengine: idxd: IDXD pmu support

Message ID cover.1619033785.git.zanussi@kernel.org (mailing list archive)
Headers show
Series dmaengine: idxd: IDXD pmu support | expand

Message

Zanussi, Tom April 21, 2021, 9:04 p.m. UTC
Hi,

This is v3 of the IDXD pmu support patch, which addresses the comments
from Vinod:

 - Removed the default line for INTEL_IDXD_PERFMON making it default 'n'

 - Replaced #ifdef CONFIG_INTEL_IDXD_PERFMON with IS_ENABLED()

 - Split the patch into two separate patches, the perfmon
   implementation and the code that uses it in the IDXD driver.

 - Added a new file,
   Documentation/ABI/testing/sysfs-bus-event_source-devices-dsa that
   documents the new format and cpumask attributes, and added better
   comments for those in the code.

 - Changed 'dogrp' to 'do_group' in perfmon_collect_events()

 - Moved 'int idx' inside the loop in perfmon_validate_group() to the
   top of function.

 - In perfmon_pmu_read_counter(), return ioread64() directly and get
   rid of cntrdata.

I also fixed some erroneous code in perfmon_counter_overflow() that
because of my misreading of the spec caused unintended clearing of
wrong bits.  According to the spec you need to write 1 rather than 0
to an OVFSTATUS bit to clear it.

Thanks,

Tom

 -- original v2 text --

Hi,

This is v2 of the IDXD pmu support patch, which is the same as v1 but
removes a few assigned-but-unused variables reported by kernel test
robot <lkp@intel.com>.

 -- original v1 text --

Hi,

This patchset implements initial pmu support for the Intel DSA (Data
Streaming Accelerator [1]), which I'm hoping can go into 5.13.

I'm also hoping to supply a couple follow-on patches in the near
future, but I'm not yet sure how much sense they make, so I thought
I'd throw a couple ideas out there and maybe get some opinions before
going forward with them:

  - The perf userspace interface for this isn't exactly user-friedly,
    in that you currently need to specify numeric values for field
    values:

     # perf stat -e dsa0/filter_wq=0x1,filter_tc=0x1,filter_sz=0x7,
                    filter_eng=0x1,event=0x8,event_category=0x3/

    It would be nicer to be able to specify those values symbolically
    instead, and the way to do that seems to be via some JSON files in
    perf userspace.

  - Some of the DSA pmu support is patterned after existing uncore
    code, and there seems to be at least some opportunity to
    consolidate some of the things they both do into common code, such
    as the cpumask device attributes and related cpu hotplug support.
    At this point I'm not sure how much sense it makes to put any
    effort into that, but would be willing to try if there would be
    some interest in it, especially considering there will probably be
    future pmu support added that could benefit from it.

Thanks,

Tom

Tom Zanussi (2):
  dmaengine: idxd: Add IDXD performance monitor support
  dmaengine: idxd: Enable IDXD performance monitor support

 .../sysfs-bus-event_source-devices-dsa        |  30 +
 drivers/dma/Kconfig                           |  12 +
 drivers/dma/idxd/Makefile                     |   2 +
 drivers/dma/idxd/idxd.h                       |  45 ++
 drivers/dma/idxd/init.c                       |   9 +
 drivers/dma/idxd/irq.c                        |   5 +-
 drivers/dma/idxd/perfmon.c                    | 662 ++++++++++++++++++
 drivers/dma/idxd/perfmon.h                    | 119 ++++
 drivers/dma/idxd/registers.h                  | 108 +++
 include/linux/cpuhotplug.h                    |   1 +
 10 files changed, 989 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-event_source-devices-dsa
 create mode 100644 drivers/dma/idxd/perfmon.c
 create mode 100644 drivers/dma/idxd/perfmon.h

Comments

Vinod Koul April 23, 2021, 5:45 p.m. UTC | #1
On 21-04-21, 16:04, Tom Zanussi wrote:
> Hi,
> 
> This is v3 of the IDXD pmu support patch, which addresses the comments
> from Vinod:
> 
>  - Removed the default line for INTEL_IDXD_PERFMON making it default 'n'
> 
>  - Replaced #ifdef CONFIG_INTEL_IDXD_PERFMON with IS_ENABLED()
> 
>  - Split the patch into two separate patches, the perfmon
>    implementation and the code that uses it in the IDXD driver.
> 
>  - Added a new file,
>    Documentation/ABI/testing/sysfs-bus-event_source-devices-dsa that
>    documents the new format and cpumask attributes, and added better
>    comments for those in the code.
> 
>  - Changed 'dogrp' to 'do_group' in perfmon_collect_events()
> 
>  - Moved 'int idx' inside the loop in perfmon_validate_group() to the
>    top of function.
> 
>  - In perfmon_pmu_read_counter(), return ioread64() directly and get
>    rid of cntrdata.
> 
> I also fixed some erroneous code in perfmon_counter_overflow() that
> because of my misreading of the spec caused unintended clearing of
> wrong bits.  According to the spec you need to write 1 rather than 0
> to an OVFSTATUS bit to clear it.

Applied, thanks

This conflicted with Daves patches, I managed to resolve, pls check the
end result
Zanussi, Tom April 23, 2021, 6:15 p.m. UTC | #2
Hi Vinod,

On 4/23/2021 12:45 PM, Vinod Koul wrote:
> On 21-04-21, 16:04, Tom Zanussi wrote:
>> Hi,
>>
>> This is v3 of the IDXD pmu support patch, which addresses the comments
>> from Vinod:
>>
>>   - Removed the default line for INTEL_IDXD_PERFMON making it default 'n'
>>
>>   - Replaced #ifdef CONFIG_INTEL_IDXD_PERFMON with IS_ENABLED()
>>
>>   - Split the patch into two separate patches, the perfmon
>>     implementation and the code that uses it in the IDXD driver.
>>
>>   - Added a new file,
>>     Documentation/ABI/testing/sysfs-bus-event_source-devices-dsa that
>>     documents the new format and cpumask attributes, and added better
>>     comments for those in the code.
>>
>>   - Changed 'dogrp' to 'do_group' in perfmon_collect_events()
>>
>>   - Moved 'int idx' inside the loop in perfmon_validate_group() to the
>>     top of function.
>>
>>   - In perfmon_pmu_read_counter(), return ioread64() directly and get
>>     rid of cntrdata.
>>
>> I also fixed some erroneous code in perfmon_counter_overflow() that
>> because of my misreading of the spec caused unintended clearing of
>> wrong bits.  According to the spec you need to write 1 rather than 0
>> to an OVFSTATUS bit to clear it.
> 
> Applied, thanks
> 
> This conflicted with Daves patches, I managed to resolve, pls check the
> end result
> 

Thanks!

However, it looks like the new files in '[PATCH v3 1/2] dmaengine: idxd: Add
IDXD performance monitor support' didn't make it in, maybe didn't get 'added'
after resolving the conflicts...

+ create mode 100644 Documentation/ABI/testing/sysfs-bus-event_source-devices-dsa
+ create mode 100644 drivers/dma/idxd/perfmon.c
+ create mode 100644 drivers/dma/idxd/perfmon.h

Tom
Vinod Koul April 23, 2021, 6:23 p.m. UTC | #3
On 23-04-21, 13:15, Zanussi, Tom wrote:
 
> > Applied, thanks
> > 
> > This conflicted with Daves patches, I managed to resolve, pls check the
> > end result
> > 
> 
> Thanks!
> 
> However, it looks like the new files in '[PATCH v3 1/2] dmaengine: idxd: Add
> IDXD performance monitor support' didn't make it in, maybe didn't get 'added'
> after resolving the conflicts...

Right, i have added and updated the commit and pushed again

Thanks for letting me know