[v1] RE: [RFC PATCH 0/4] Updates for CXL Event Records

RE: [RFC PATCH 0/4] Updates for CXL Event Records

Posted by Shiju Jose 1 month, 1 week ago

>-----Original Message-----
>From: Alison Schofield <alison.schofield@intel.com>
>Sent: 16 October 2024 22:01
>To: Shiju Jose <shiju.jose@huawei.com>
>Cc: dave.jiang@intel.com; dan.j.williams@intel.com; Jonathan Cameron
><jonathan.cameron@huawei.com>; vishal.l.verma@intel.com;
>ira.weiny@intel.com; dave@stgolabs.net; linux-cxl@vger.kernel.org; linux-
>kernel@vger.kernel.org; Linuxarm <linuxarm@huawei.com>; tanxiaofei
><tanxiaofei@huawei.com>; Zengtao (B) <prime.zeng@hisilicon.com>
>Subject: Re: [RFC PATCH 0/4] Updates for CXL Event Records
>
>On Wed, Oct 16, 2024 at 05:33:45PM +0100, shiju.jose@huawei.com wrote:
>> From: Shiju Jose <shiju.jose@huawei.com>
>>
>> CXL spec rev 3.1 CXL Event Records has updated w.r.t CXL spec rev 3.0.
>> Add updates for the above spec changes in the CXL events records and
>> CXL trace events implementation.
>>
>> Note: Please apply following fix patch first if not present.
>> https://patchwork.kernel.org/project/cxl/patch/20241014143003.1170-1-s
>> hiju.jose@huawei.com/
>>
>> Shiju Jose (4):
>>   cxl/events: Updates for CXL Common Event Record Format
>>   cxl/events: Updates for CXL General Media Event Record
>>   cxl/events: Updates for CXL DRAM Event Record
>>   cxl/events: Updates for CXL Memory Module Event Record
>
>Thanks, this looks useful! I didn't review line by line but do have some feedback
>before for a v1:
Hi Alison,
Thanks for the feedbacks.

>
>- Suggest being more explicit in the commit msg(s). Something like:
>cxl/events: Update Common Event Record to CXL spec 3.1
Sure. Will add.

>
>- I was a bit surprised that this doesn't simply append new fields to the
>TP_printk() output. Is there some reason for that?
Will do. Thought print new fields before region_name and region_uuid. 

>
>- How about updating the mock of these events to include these new fields. I
>don't think this introduces any new formats, but I would certainly eyeball all 3:
>dmesg tp_printk, trace file, and monitor output because all 3 (sadly) present a
>bit differently.
>
I will update the CXL mock test for these new fields.
 
>-- Alison
>
>>
>>  drivers/cxl/core/trace.h | 201 +++++++++++++++++++++++++++++++++------
>>  include/cxl/event.h      |  20 +++-
>>  2 files changed, 190 insertions(+), 31 deletions(-)
>>
>> --
>> 2.34.1
>>
>
Thanks,
Shiju

Re: [RFC PATCH 0/4] Updates for CXL Event Records

Posted by Jonathan Cameron 1 month, 1 week ago

On Thu, 17 Oct 2024 10:39:58 +0100
Shiju Jose <shiju.jose@huawei.com> wrote:

> >-----Original Message-----
> >From: Alison Schofield <alison.schofield@intel.com>
> >Sent: 16 October 2024 22:01
> >To: Shiju Jose <shiju.jose@huawei.com>
> >Cc: dave.jiang@intel.com; dan.j.williams@intel.com; Jonathan Cameron
> ><jonathan.cameron@huawei.com>; vishal.l.verma@intel.com;
> >ira.weiny@intel.com; dave@stgolabs.net; linux-cxl@vger.kernel.org; linux-
> >kernel@vger.kernel.org; Linuxarm <linuxarm@huawei.com>; tanxiaofei
> ><tanxiaofei@huawei.com>; Zengtao (B) <prime.zeng@hisilicon.com>
> >Subject: Re: [RFC PATCH 0/4] Updates for CXL Event Records
> >
> >On Wed, Oct 16, 2024 at 05:33:45PM +0100, shiju.jose@huawei.com wrote:  
> >> From: Shiju Jose <shiju.jose@huawei.com>
> >>
> >> CXL spec rev 3.1 CXL Event Records has updated w.r.t CXL spec rev 3.0.
> >> Add updates for the above spec changes in the CXL events records and
> >> CXL trace events implementation.
> >>
> >> Note: Please apply following fix patch first if not present.
> >> https://patchwork.kernel.org/project/cxl/patch/20241014143003.1170-1-s
> >> hiju.jose@huawei.com/
> >>
> >> Shiju Jose (4):
> >>   cxl/events: Updates for CXL Common Event Record Format
> >>   cxl/events: Updates for CXL General Media Event Record
> >>   cxl/events: Updates for CXL DRAM Event Record
> >>   cxl/events: Updates for CXL Memory Module Event Record  
> >
> >Thanks, this looks useful! I didn't review line by line but do have some feedback
> >before for a v1:  
> Hi Alison,
> Thanks for the feedbacks.
> 
> >
> >- Suggest being more explicit in the commit msg(s). Something like:
> >cxl/events: Update Common Event Record to CXL spec 3.1  
> Sure. Will add.
> 
> >
> >- I was a bit surprised that this doesn't simply append new fields to the
> >TP_printk() output. Is there some reason for that?  
> Will do. Thought print new fields before region_name and region_uuid. 
> 
Worth calling out that the other change is that 3.1 added the option
of a spec defined format for the component ID (various PLDM defined
fields)

I don't think we need to maintain ABI compatibility to TP_printk so to me it
makes sense to print that formatted version if that is what is provide instead
of (rather than as well as) the component IDs.

For RAS Daemon etc, that can be figured out in userspace if needed
so no point in changing the trace point, but the print being the
subfields is nice to have.

Jonathan

> >
> >- How about updating the mock of these events to include these new fields. I
> >don't think this introduces any new formats, but I would certainly eyeball all 3:
> >dmesg tp_printk, trace file, and monitor output because all 3 (sadly) present a
> >bit differently.
> >  
> I will update the CXL mock test for these new fields.
>  
> >-- Alison
> >  
> >>
> >>  drivers/cxl/core/trace.h | 201 +++++++++++++++++++++++++++++++++------
> >>  include/cxl/event.h      |  20 +++-
> >>  2 files changed, 190 insertions(+), 31 deletions(-)
> >>
> >> --
> >> 2.34.1
> >>  
> >  
> Thanks,
> Shiju