Re: [PATCH v8] PCI: hotplug: Add a generic RAS tracepoint for hotplug event

[Shuai Xue <xueshuai@linux.alibaba.com>]

在 2025/5/20 18:07, Ilpo Järvinen 写道:
> On Tue, 20 May 2025, Shuai Xue wrote:
> 
>> Hi, Ilpo,
>>
>> 在 2025/5/20 01:10, Ilpo Järvinen 写道:
>>> On Mon, 12 May 2025, Shuai Xue wrote:
>>>
>>>> Hotplug events are critical indicators for analyzing hardware health,
>>>> particularly in AI supercomputers where surprise link downs can
>>>> significantly impact system performance and reliability.
>>>>
>>>> To this end, define a new TRACING_SYSTEM named pci, add a generic RAS
>>>> tracepoint for hotplug event to help healthy check, and generate
>>>> tracepoints for pcie hotplug event. Add enum pci_hotplug_event in
>>>> include/uapi/linux/pci.h so applications like rasdaemon can register
>>>> tracepoint event handlers for it.
>>>>
>>>> The output like below:
>>>>
>>>> $ echo 1 > /sys/kernel/debug/tracing/events/pci/pci_hp_event/enable
>>>> $ cat /sys/kernel/debug/tracing/trace_pipe
>>>>       <...>-206     [001] .....    40.373870: pci_hp_event: 0000:00:02.0
>>>> slot:10, event:Link Down
>>>>
>>>>       <...>-206     [001] .....    40.374871: pci_hp_event: 0000:00:02.0
>>>> slot:10, event:Card not present
>>>>
>>>> Suggested-by: Lukas Wunner <lukas@wunner.de>
>>>> Suggested-by: Steven Rostedt <rostedt@goodmis.org>
>>>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>>>> Reviewed-by: Lukas Wunner <lukas@wunner.de>
>>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>> ---
>>>> changes since v7:
>>>> - replace the TRACE_INCLUDE_PATH to avoid macro conflict per Steven
>>>> - pick up Reviewed-by from Lukas Wunner
>>>> ---
>>>>    drivers/pci/hotplug/Makefile      |  3 ++
>>>>    drivers/pci/hotplug/pciehp_ctrl.c | 33 ++++++++++++---
>>>>    drivers/pci/hotplug/trace.h       | 68 +++++++++++++++++++++++++++++++
>>>>    include/uapi/linux/pci.h          |  7 ++++
>>>>    4 files changed, 105 insertions(+), 6 deletions(-)
>>>>    create mode 100644 drivers/pci/hotplug/trace.h
>>>>
>>>> diff --git a/drivers/pci/hotplug/Makefile b/drivers/pci/hotplug/Makefile
>>>> index 40aaf31fe338..a1a9d1e98962 100644
>>>> --- a/drivers/pci/hotplug/Makefile
>>>> +++ b/drivers/pci/hotplug/Makefile
>>>> @@ -3,6 +3,9 @@
>>>>    # Makefile for the Linux kernel pci hotplug controller drivers.
>>>>    #
>>>>    +# define_trace.h needs to know how to find our header
>>>> +CFLAGS_pciehp_ctrl.o				:= -I$(src)
>>>> +
>>>>    obj-$(CONFIG_HOTPLUG_PCI)		+= pci_hotplug.o
>>>>    obj-$(CONFIG_HOTPLUG_PCI_COMPAQ)	+= cpqphp.o
>>>>    obj-$(CONFIG_HOTPLUG_PCI_IBM)		+= ibmphp.o
>>>> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c
>>>> b/drivers/pci/hotplug/pciehp_ctrl.c
>>>> index d603a7aa7483..f9beb4d3a9b8 100644
>>>> --- a/drivers/pci/hotplug/pciehp_ctrl.c
>>>> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
>>>> @@ -23,6 +23,9 @@
>>>>    #include "../pci.h"
>>>>    #include "pciehp.h"
>>>>    +#define CREATE_TRACE_POINTS
>>>> +#include "trace.h"
>>>> +
>>>>    /* The following routines constitute the bulk of the
>>>>       hotplug controller logic
>>>>     */
>>>> @@ -244,12 +247,20 @@ void pciehp_handle_presence_or_link_change(struct
>>>> controller *ctrl, u32 events)
>>>>    	case ON_STATE:
>>>>    		ctrl->state = POWEROFF_STATE;
>>>>    		mutex_unlock(&ctrl->state_lock);
>>>> -		if (events & PCI_EXP_SLTSTA_DLLSC)
>>>> +		if (events & PCI_EXP_SLTSTA_DLLSC) {
>>>>    			ctrl_info(ctrl, "Slot(%s): Link Down\n",
>>>>    				  slot_name(ctrl));
>>>> -		if (events & PCI_EXP_SLTSTA_PDC)
>>>> +			trace_pci_hp_event(pci_name(ctrl->pcie->port),
>>>> +					   slot_name(ctrl),
>>>> +					   PCI_HOTPLUG_LINK_DOWN);
>>>> +		}
>>>> +		if (events & PCI_EXP_SLTSTA_PDC) {
>>>>    			ctrl_info(ctrl, "Slot(%s): Card not present\n",
>>>>    				  slot_name(ctrl));
>>>> +			trace_pci_hp_event(pci_name(ctrl->pcie->port),
>>>> +					   slot_name(ctrl),
>>>> +					   PCI_HOTPLUG_CARD_NOT_PRESENT);
>>>> +		}
>>>>    		pciehp_disable_slot(ctrl, SURPRISE_REMOVAL);
>>>>    		break;
>>>>    	default:
>>>> @@ -269,6 +280,9 @@ void pciehp_handle_presence_or_link_change(struct
>>>> controller *ctrl, u32 events)
>>>>    					      INDICATOR_NOOP);
>>>>    			ctrl_info(ctrl, "Slot(%s): Card not present\n",
>>>>    				  slot_name(ctrl));
>>>> +			trace_pci_hp_event(pci_name(ctrl->pcie->port),
>>>> +					   slot_name(ctrl),
>>>> +					   PCI_HOTPLUG_CARD_NOT_PRESENT);
>>>>    		}
>>>>    		mutex_unlock(&ctrl->state_lock);
>>>>    		return;
>>>> @@ -281,12 +295,19 @@ void pciehp_handle_presence_or_link_change(struct
>>>> controller *ctrl, u32 events)
>>>>    	case OFF_STATE:
>>>>    		ctrl->state = POWERON_STATE;
>>>>    		mutex_unlock(&ctrl->state_lock);
>>>> -		if (present)
>>>> +		if (present) {
>>>>    			ctrl_info(ctrl, "Slot(%s): Card present\n",
>>>>    				  slot_name(ctrl));
>>>> -		if (link_active)
>>>> -			ctrl_info(ctrl, "Slot(%s): Link Up\n",
>>>> -				  slot_name(ctrl));
>>>> +			trace_pci_hp_event(pci_name(ctrl->pcie->port),
>>>> +					   slot_name(ctrl),
>>>> +					   PCI_HOTPLUG_CARD_PRESENT);
>>>> +		}
>>>> +		if (link_active) {
>>>> +			ctrl_info(ctrl, "Slot(%s): Link Up\n",
>>>> slot_name(ctrl));
>>>> +			trace_pci_hp_event(pci_name(ctrl->pcie->port),
>>>> +					   slot_name(ctrl),
>>>> +					   PCI_HOTPLUG_LINK_UP);
>>>> +		}
>>>>    		ctrl->request_result = pciehp_enable_slot(ctrl);
>>>>    		break;
>>>>    	default:
>>>> diff --git a/drivers/pci/hotplug/trace.h b/drivers/pci/hotplug/trace.h
>>>> new file mode 100644
>>>> index 000000000000..21329c198019
>>>> --- /dev/null
>>>> +++ b/drivers/pci/hotplug/trace.h
>>>> @@ -0,0 +1,68 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +#if !defined(_TRACE_HW_EVENT_PCI_HP_H) ||
>>>> defined(TRACE_HEADER_MULTI_READ)
>>>> +#define _TRACE_HW_EVENT_PCI_HP_H
>>>> +
>>>> +#include <linux/tracepoint.h>
>>>> +
>>>> +#undef TRACE_SYSTEM
>>>> +#define TRACE_SYSTEM pci
>>>> +
>>>> +#define PCI_HOTPLUG_EVENT					\
>>>> +	EM(PCI_HOTPLUG_LINK_UP,			"Link Up")	\
>>>> +	EM(PCI_HOTPLUG_LINK_DOWN,		"Link Down")	\
>>>> +	EM(PCI_HOTPLUG_CARD_PRESENT,		"Card present")	\
>>>> +	EMe(PCI_HOTPLUG_CARD_NOT_PRESENT,	"Card not present")
>>>
>>> Hi,
>>>
>>> While I was thinking of adding tracing into PCIe BW controller (bwctrl),
>>> I ended up thinking that perhaps it would make more sense to have PCIe
>>> Link related tracepoints which would cover both hotplug and bwctrl so that
>>> also Link Speed changes would be reported through the same trace event.
>>>
>>> Downgraded speed may indicate there's something wrong with the card and
>>> the Link Speed does have performance impact too for those who are pushing
>>> BW boundaries such as AI systems.
>>
>> Agreed!
>>
>>>
>>> So my suggestion is:
>>>
>>> - Add "Link Speed changed" to the event types.
>>> - Add Link Speed and Width into the event format (and probably also Flit
>>>     mode as PCIe gen6 is coming).
>>
>>
>> How about bellow event format:
>>
>> +	TP_STRUCT__entry(
>> +		__string(	port_name,	port_name	)
>> +		__field(	unsigned char,	cur_bus_speed	)
>> +		__field(	unsigned char,	max_bus_speed	)
> 
> Add also the Link Width.

Got it.
> 
>> +		__field(	unsigned char,	flit_mode	)
>> +	),
>>
>> And add the event to pcie_update_link_speed():
>>
>> @@ -796,6 +799,10 @@ void pcie_update_link_speed(struct pci_bus *bus)
>>          pcie_capability_read_word(bridge, PCI_EXP_LNKSTA, &linksta);
>>          pcie_capability_read_word(bridge, PCI_EXP_LNKSTA2, &linksta2);
>>          __pcie_update_link_speed(bus, linksta, linksta2);
>> +
>> +       trace_pci_link_event(pci_name(bridge), bus->cur_bus_speed,
>> +                                          bus->max_bus_speed,
> 
> I don't think outputting the internal values of enum pci_bus_speed is a
> good idea. Maybe these could be printed as a string (with
> pci_speed_string()) or encoded with trace interface specific values.

I see, a human readable string is better.
> 
> Perhaps it would make sense to check if the speed really changed before
> sending that event, but there are good sides in both approaches as I
> know some platforms assert LBMS more than once during a single Link Speed
> change.
> 
>> +                                          PCI_HOTPLUG_LINK_SPEED_CHANGED);
>>
>> But I don't find link speed changed in hotplug driver
> 
> pciehp_check_link_status() calls __pcie_update_link_speed().

Thanks.

> 
>> , and the format of "Link Speed changed" is a bit different from
>> "pci_hp_event".
> 
> The difference is only because when the Link is down, there's no Link
> Speed (obviously). Whenever a new device is hotplugged and it comes up,
> there's also Link Speed for it which can be included into the trace event.
> 
> I think the trace event should have some special value for the fields that
> are N/A due to Link being off. While it would be possible to create
> separate events for speed changes and hotplug, I don't see any pros in
> that approach over just having the N/A fields marked as such when the Link
> is Down.
> 
> Perhaps it would even make sense to add PCIE_SPEED_LINK_DOWN into
> bus->cur_bus_speed when hotplug finds the card is gone (I'm not entirely
> sure how bwctrl or pcie_cooling driver would cope with that though, they
> might need minor tweaking to support it, and there are a few other drivers
> that use that field).
> 
>> Do we really need a PCI_HOTPLUG_EVENT? May PCI_LINK_EVENT is more
>> appropriate?
> 
> Ah, right, I forgot to mention it would make sense to rename it to
> PCI_LINK_EVENT.
> 

Got it.

Thanks.
Shuai