Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <adf43431-f828-75b2-359e-652c5fc96dc7@linux.intel.com>
Date:   Thu, 26 May 2022 11:55:09 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v4 2/5] perf header: Parse non-cpu pmu capabilities
Content-Language: en-US
To:     Ravi Bangoria <ravi.bangoria@amd.com>
Cc:     acme@kernel.org, irogers@google.com, peterz@infradead.org,
        rrichter@amd.com, mingo@redhat.com, mark.rutland@arm.com,
        jolsa@kernel.org, namhyung@kernel.org, tglx@linutronix.de,
        bp@alien8.de, james.clark@arm.com, leo.yan@linaro.org,
        ak@linux.intel.com, eranian@google.com, like.xu.linux@gmail.com,
        x86@kernel.org, linux-perf-users@vger.kernel.org,
        linux-kernel@vger.kernel.org, sandipan.das@amd.com,
        ananth.narayan@amd.com, kim.phillips@amd.com,
        santosh.shukla@amd.com
References: <20220523033945.1612-1-ravi.bangoria@amd.com>
 <20220523033945.1612-3-ravi.bangoria@amd.com>
 <7c072dd2-de6f-e083-a400-17467d6fe6e8@linux.intel.com>
 <4f7239e2-c065-0800-f679-d4ed797fdecd@amd.com>
From:   "Liang, Kan" <kan.liang@linux.intel.com>
In-Reply-To: <4f7239e2-c065-0800-f679-d4ed797fdecd@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 5/26/2022 11:08 AM, Ravi Bangoria wrote:
> Hi Kan,
> 
> [...]
> 
>>> +static int write_pmu_caps(struct feat_fd *ff, struct evlist *evlist __maybe_unused)
>>> +{
>>> +    struct perf_pmu_caps *caps = NULL;
>>> +    struct perf_pmu *pmu = NULL;
>>> +    u32 nr_pmus = 0;
>>> +    int ret;
>>> +
>>> +    while ((pmu = perf_pmu__scan(pmu))) {
>>> +        if (!pmu->name || !strncmp(pmu->name, "cpu", 3) ||
>>> +            perf_pmu__caps_parse(pmu) <= 0)
>>> +            continue;
>>> +        nr_pmus++;
>>> +    }
>>> +
>>> +    ret = do_write(ff, &nr_pmus, sizeof(nr_pmus));
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    if (!nr_pmus)
>>> +        return 0;
>>> +
>>> +    while ((pmu = perf_pmu__scan(pmu))) {
>>> +        if (!pmu->name || !strncmp(pmu->name, "cpu", 3) || !pmu->nr_caps)
>>> +            continue;
>>> +
>>> +        ret = do_write_string(ff, pmu->name);
>>> +        if (ret < 0)
>>> +            return ret;
>>> +
>>> +        ret = do_write(ff, &pmu->nr_caps, sizeof(pmu->nr_caps));
>>> +        if (ret < 0)
>>> +            return ret;
>>> +
>>> +        list_for_each_entry(caps, &pmu->caps, list) {
>>> +            ret = do_write_string(ff, caps->name);
>>> +            if (ret < 0)
>>> +                return ret;
>>> +
>>> +            ret = do_write_string(ff, caps->value);
>>> +            if (ret < 0)
>>> +                return ret;
>>> +        }
>>> +    }
>>
>> The write_per_cpu_pmu_caps() also does a similar thing. Can we factor out a generic write_pmu_caps() which works for both cpu and non-cpu pmu capabilities?
> 
> I might be able to do this but..
> 
>> It seems the print_pmu_caps()/process_pmu_caps() can also does similar factor out.
> 
> not this, see below..
> 
>> Actually, more aggressively, why not use the HEADER_PMU_CAPS to replace the HEADER_HYBRID_CPU_PMU_CAPS? The HEADER_HYBRID_CPU_PMU_CAPS is the last header feature. It seems doable. We can always write/print the "cpu_" kind of PMU first to be compatible with the old tools.
> 
> There are some differences in how capabilities are stored in perf.data header
> as well as perf_env. In case of HEADER_CPU_PMU_CAPS or
> HEADER_HYBRID_CPU_PMU_CAPS, all capabilities are stored in a single string
> separated by NULL character. 

I think this is the format for the internal string, not the format of 
the perf.data header.

For the perf.data, here is the existing format for the 
HEADER_HYBRID_CPU_PMU_CAPS.

struct {
	u32 nr_pmu;
	struct {
		u32 nr_cpu_pmu_caps;
		{
			char	name[];
			char	value[];
		} [nr_cpu_pmu_caps];
		char pmu_name[];
	} [nr_pmu];
};

Here is your proposal.

+struct {
+	u32 nr_pmus;
+	struct {
+		char pmu_name[];
+		u32 nr_caps;
+		struct {
+			char name[];
+			char value[];
+		} [nr_caps];
+	} [nr_pmus];
+};

 From my understanding, they are the same. (It doesn't matter where we 
put the char pmu_name[];)

That's also why I think we should merge the HEADER_HYBRID_CPU_PMU_CAPS 
and HEADER_PMU_CAPS. I don't think it make senses to basically handle 
the same thing with different codes.


> Whereas, in case of HEADER_PMU_CAPS, they are
> stored as an array of strings. The reason for this difference is, searching
> in an array is far easier compared to searching in a NULL separated string.

I think the hybrid_cpc_node can be replaced by the env_pmu_caps.
Then you don't need to modify the perf_env__find_pmu_cap().

Thanks,
Kan

> So, I don't think I can extend HEADER_HYBRID_CPU_PMU_CAPS as HEADER_PMU_CAPS
> without adding complexity in perf_env__find_pmu_cap().
> 
> Thanks for the review,
> Ravi