Messages in this thread | | | Date | Mon, 3 Oct 2022 09:15:52 -0400 | Subject | Re: [PATCH v3 01/15] perf/mem: Introduce PERF_MEM_LVLNUM_{EXTN_MEM|IO} | From | "Liang, Kan" <> |
| |
On 2022-10-01 2:37 a.m., Ravi Bangoria wrote: > On 30-Sep-22 7:47 PM, Liang, Kan wrote: >> >> >> On 2022-09-30 8:50 a.m., Ravi Bangoria wrote: >>> On 30-Sep-22 4:18 PM, kajoljain wrote: >>>> >>>> >>>> On 9/28/22 15:27, Ravi Bangoria wrote: >>>>> PERF_MEM_LVLNUM_EXTN_MEM which can be used to indicate accesses to >>>>> extension memory like CXL etc. PERF_MEM_LVL_IO can be used for IO >>>>> accesses but it can not distinguish between local and remote IO. >>>>> Introduce new field PERF_MEM_LVLNUM_IO which can be clubbed with >>>>> PERF_MEM_REMOTE_REMOTE to indicate Remote IO accesses. >>>>> >>>>> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com> >>>>> --- >>>>> include/uapi/linux/perf_event.h | 4 +++- >>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h >>>>> index e639c74cf5fb..4ae3c249f675 100644 >>>>> --- a/include/uapi/linux/perf_event.h >>>>> +++ b/include/uapi/linux/perf_event.h >>>>> @@ -1336,7 +1336,9 @@ union perf_mem_data_src { >>>>> #define PERF_MEM_LVLNUM_L2 0x02 /* L2 */ >>>>> #define PERF_MEM_LVLNUM_L3 0x03 /* L3 */ >>>>> #define PERF_MEM_LVLNUM_L4 0x04 /* L4 */ >>>>> -/* 5-0xa available */ >>>>> +/* 5-0x8 available */ >>>>> +#define PERF_MEM_LVLNUM_EXTN_MEM 0x09 /* Extension memory */ >>>> >>>> Hi Ravi, >>>> Here we are adding entry explicitly for accesses to Extension memory >>>> like CXL. In future if we want to extend it for cache or other accesses >>>> , we again need to add new entries. >>>> Can we rather add single entry say PERF_MEM_LVLNUM_EXTN and further can >>>> use reserved bits to specify memory/cache? >>> >>> Is everybody okay with this: >>> >>> #define PERF_MEM_LVLNUM_EXTN 0x09 /* CXL */ >> >> I think a generic name, PERF_MEM_LVLNUM_EXTN, only make sense, when it >> wants to include all the types of the Extension memory, e.g., CXL, PMEM, >> HBM, etc. Then we can set this bit and the corresponding CXL bits to >> understand the real source. Is it the case here? >> >> But if it's only for the CXL, I think it's better to use a dedicated >> name, PERF_MEM_LVLNUM_CXL. (as we did for PMEM, PERF_MEM_LVLNUM_PMEM). >> If so, I don't think we need the PERF_MEM_EXTN_CXL_ANY. > > Ok. For now, I think below is good enough? Later we can introduce new > variable to provide type of cxl device. > > > From 5deb2055e2b5b0a61403f2d5f4e5a784b14a65e3 Mon Sep 17 00:00:00 2001 > From: Ravi Bangoria <ravi.bangoria@amd.com> > Date: Sat, 1 Oct 2022 11:37:05 +0530 > Subject: [PATCH] perf/mem: Rename PERF_MEM_LVLNUM_EXTN_MEM to > PERF_MEM_LVLNUM_CXL > > PERF_MEM_LVLNUM_EXTN_MEM was introduced to cover CXL devices but it's > bit ambiguous name and also not generic enough to cover cxl.cache and > cxl.io devices. Rename it to PERF_MEM_LVLNUM_CXL to be more specific.
Looks good to me.
Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
Thanks, Kan
> > Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com> > --- > arch/x86/events/amd/ibs.c | 2 +- > include/uapi/linux/perf_event.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c > index 3271735f0070..4cb710efbdd9 100644 > --- a/arch/x86/events/amd/ibs.c > +++ b/arch/x86/events/amd/ibs.c > @@ -801,7 +801,7 @@ static void perf_ibs_get_mem_lvl(union ibs_op_data2 *op_data2, > /* Extension Memory */ > if (ibs_caps & IBS_CAPS_ZEN4 && > ibs_data_src == IBS_DATA_SRC_EXT_EXT_MEM) { > - data_src->mem_lvl_num = PERF_MEM_LVLNUM_EXTN_MEM; > + data_src->mem_lvl_num = PERF_MEM_LVLNUM_CXL; > if (op_data2->rmt_node) { > data_src->mem_remote = PERF_MEM_REMOTE_REMOTE; > /* IBS doesn't provide Remote socket detail */ > diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h > index 85be78e0e7f6..eb1090604d53 100644 > --- a/include/uapi/linux/perf_event.h > +++ b/include/uapi/linux/perf_event.h > @@ -1337,7 +1337,7 @@ union perf_mem_data_src { > #define PERF_MEM_LVLNUM_L3 0x03 /* L3 */ > #define PERF_MEM_LVLNUM_L4 0x04 /* L4 */ > /* 5-0x8 available */ > -#define PERF_MEM_LVLNUM_EXTN_MEM 0x09 /* Extension memory */ > +#define PERF_MEM_LVLNUM_CXL 0x09 /* CXL */ > #define PERF_MEM_LVLNUM_IO 0x0a /* I/O */ > #define PERF_MEM_LVLNUM_ANY_CACHE 0x0b /* Any cache */ > #define PERF_MEM_LVLNUM_LFB 0x0c /* LFB */
| |