Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 18 Oct 2019 09:44:03 -0700
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Georgi Djakov <georgi.djakov@linaro.org>
Cc:     linux-pm@vger.kernel.org, rostedt@goodmis.org, mingo@redhat.com,
        vincent.guittot@linaro.org, daidavid1@codeaurora.org,
        okukatla@codeaurora.org, evgreen@chromium.org, mka@chromium.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] interconnect: Add basic tracepoints
Message-ID: <20191018164403.GB1669@tuxbook-pro>
References: <20191018140224.15087-1-georgi.djakov@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191018140224.15087-1-georgi.djakov@linaro.org>
User-Agent: Mutt/1.12.1 (2019-06-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri 18 Oct 07:02 PDT 2019, Georgi Djakov wrote:

> The tracepoints can help with understanding the system behavior of a
> given interconnect path when the consumer drivers change their bandwidth
> demands. This might be interesting when we want to monitor the requested
> interconnect bandwidth for each client driver. The paths may share the
> same nodes and this will help to understand "who and when is requesting
> what". All this is useful for subsystem drivers developers and may also
> provide hints when optimizing the power and performance profile of the
> system.
> 

This is very useful, thanks for writing it up.

> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
[..]
> @@ -449,6 +452,9 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
>  
>  		/* aggregate requests for this node */
>  		aggregate_requests(node);
> +
> +		trace_icc_set_bw(node, dev_name(path->reqs[i].dev),
> +				 avg_bw, peak_bw);

When I've been debugging interconnect things I've added a
kstrdup_const() of "name" in of_icc_get() and then included that here.

I find including the path name quite useful for devices with multiple
paths.

>  	}
>  
>  	ret = apply_constraints(path);
> @@ -461,6 +467,9 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
>  			path->reqs[i].avg_bw = old_avg;
>  			path->reqs[i].peak_bw = old_peak;
>  			aggregate_requests(node);
> +
> +			trace_icc_set_bw(node, dev_name(path->reqs[i].dev),
> +					 old_avg, old_peak);
>  		}
>  		apply_constraints(path);

And analog to e.g. the clock traces I would suggest that you trace
device, path and "ret" here.

Regards,
Bjorn