Re: [PATCH v2 2/2] perf/dwc_pcie: Skip sibling walks for new group leaders

On 8/12/26 12:07 AM, Zhanpeng Zhang wrote:
> for_each_sibling_event() requires the event context lock to be held.
> perf_try_init_event() only takes that lock when initializing a group
> member.

newline here for a new paragraph.

> The DWC PCIe PMU walks the sibling list twice for every event, including a
> new group leader whose event_init() method runs without the lock. Although
> the leader cannot have siblings yet, the lock assertion is evaluated before
> the empty list is walked and produces two lockdep warnings.
> 
> Skip both sibling walks while initializing a group leader. Group member
> validation is unchanged and continues to run with the event context mutex
> held.
> 
> Fixes: af9597adc2f1 ("drivers/perf: add DesignWare PCIe PMU driver")
> Signed-off-by: Zhanpeng Zhang <[email protected]>

looks good to me.

Reviewed-by: Yicong Yang <[email protected]>

> ---
> A system-wide DWC PCIe PMU perf stat completed successfully with the same
> debug configuration, without lockdep reports or kernel taint.
> 
>  drivers/perf/dwc_pcie_pmu.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/perf/dwc_pcie_pmu.c b/drivers/perf/dwc_pcie_pmu.c
> index 7b2e28aade73..a6667ac19a99 100644
> --- a/drivers/perf/dwc_pcie_pmu.c
> +++ b/drivers/perf/dwc_pcie_pmu.c
> @@ -417,6 +417,10 @@ static int dwc_pcie_pmu_validate_group(struct perf_event *event)
>  		if (dwc_pcie_pmu_validate_add_lane_event(leader, val_lane_events))
>  			return -ENOSPC;
>  
> +	/* A new group leader cannot have any siblings yet. */
> +	if (event == leader)
> +		return 0;
> +
>  	for_each_sibling_event(sibling, leader) {
>  		type = DWC_PCIE_EVENT_TYPE(sibling);
>  		if (type == DWC_PCIE_TIME_BASE_EVENT) {
> @@ -452,9 +456,11 @@ static int dwc_pcie_pmu_event_init(struct perf_event *event)
>  	if (event->cpu < 0 || event->attach_state & PERF_ATTACH_TASK)
>  		return -EINVAL;
>  
> -	for_each_sibling_event(sibling, event->group_leader) {
> -		if (sibling->pmu != event->pmu && !is_software_event(sibling))
> -			return -EINVAL;
> +	if (event != event->group_leader) {
> +		for_each_sibling_event(sibling, event->group_leader) {
> +			if (sibling->pmu != event->pmu && !is_software_event(sibling))
> +				return -EINVAL;
> +		}

personally feel better to make this check into dwc_pcie_pmu_validate_group().

thanks.