Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <05aa0287-ebc4-b85c-4012-f16870caf0b8@amd.com>
Date:   Fri, 27 May 2022 07:52:31 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.4.1
Subject: Re: [PATCH v4 2/5] perf header: Parse non-cpu pmu capabilities
Content-Language: en-US
To:     "Liang, Kan" <kan.liang@linux.intel.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, Ravi Bangoria <ravi.bangoria@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>
 <adf43431-f828-75b2-359e-652c5fc96dc7@linux.intel.com>
From:   Ravi Bangoria <ravi.bangoria@amd.com>
In-Reply-To: <adf43431-f828-75b2-359e-652c5fc96dc7@linux.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UjlNRVNoRFhHTExSOHBHZCt4bmZxN2xLUEpKUlg0R2NSMElaa013NTdpd2Jy?=
 =?utf-8?B?d2VsK2JBNFFLeElmcVRtZkY5My9OUDAwTStYUW12TGQycS9Sa3NYeFVpMGhN?=
 =?utf-8?B?VUFrelR0ZEJGNVMwMkRWK05GVEk2d1loNVFrWXA5bjloM211RDRRK2JvOThW?=
 =?utf-8?B?Mklod3ZXYlozbUlYZWZiRnFia3FYWk5HdldKYjJSWk9EVm5IUHREWlZLZUtN?=
 =?utf-8?B?cEU1Z21xZmRIck5kUTFiWGE1eDNJdFVDY25pSUtmbEJ2R0Z0SzlqUjlobEMw?=
 =?utf-8?B?NVhyNDdOQ004R0IxbUxHOHR2OHZpVzlEbzFSMitJYUIzMG1aMmZpdDd0elpY?=
 =?utf-8?B?TGM5VVNxck5VUkp5bHpYWWRkeTZWMFRXbGhqalFrb0ZsNUp4N0JaemRqb1Bz?=
 =?utf-8?B?K2RzNENLVHdGbGplU1dia1RGQzMxTVNHLzVHMEcrUnl6WGh1aTBxL1R0OVRa?=
 =?utf-8?B?c2hPanFBa1RteUlsTmV3MmVTamlTUlU0R01tbEtVT0UvWTBQUENjOHVCQkxi?=
 =?utf-8?B?UGF5bjlQQ0xBMnc0RGRpdzdlYTZPcXd5NnJVcjZCUGxyQmtZcHoxczR1b1Z3?=
 =?utf-8?B?NGc2di8xdUY5NFlhYlBOS3YrS21lQW9rOTd6RFJBME8vMWJYRWZVNEpLcW9F?=
 =?utf-8?B?Mjh0RnNXK2gzSkl1a0dNc2FReEI4ekIySllqWFNVUmdTckNDR2FUNG1JMEQv?=
 =?utf-8?B?MllVK0hYeEcvYnA5UUE4VlJSc1A1dVkxQlltUUxMY2RKalZYYXJ2ZEdMV05h?=
 =?utf-8?B?N1JKc3FxcGhtVE9zQnp6dmoxaU1ISDd1K2xMbWRYb3JGMGZlbnc1ZXN1Y3lx?=
 =?utf-8?B?a0UrZGxaMFVYeTh6TGo2TDBQVHRZNHFoclZPY0JBTjBrMCtEUURzQzZ2M2ZT?=
 =?utf-8?B?KzZHbGoycXRFWFJIOHBNb3M2VXRjMWJNaHNnSWwzQk52Y2Ntb0lod3QzTHh3?=
 =?utf-8?B?eFJvdmlXT1FUZGh3dlMzelJBVzhVTXd0WWUzR1I3K0lnYldRQW9jOTEzZExu?=
 =?utf-8?B?NkdNWXhJWUwycVJuNjhjWmFJMWhMNVJSclcxbDNlMTlvNm1KTTY3QkYxT3E2?=
 =?utf-8?B?YnhHR28xL292UjVDUDFVdEQvUkluTW13UlU2Y0FvWEplUHlLM0x3Y3g4cjJ3?=
 =?utf-8?B?MGVKTW9CZmlSODNtVE9ycHJ6UFE3QmR0dC9IT3hrSjlWK01hWEFxc0tRbFBO?=
 =?utf-8?B?UXR1dGlTcE1LeGo1RjcwMlZBQklaUG5LR3JmdWY3L2JEU1NvWjNDYjhQVFNq?=
 =?utf-8?B?R1pCTU1menJydTI3R0lBbHRyWmFHcVF6UDNqRStZSU8zT1BYRmFxZzIraUpX?=
 =?utf-8?B?ajVCVFhNUEdwR1V4RnBtbHNiZlc1aGZ3Vis3eTBGekV2MWdOMnU5cVJUUHI5?=
 =?utf-8?B?R2JBR0RPWUEvd2ZHbFIvVDA3aUhFWExRWC9lL2xMcTJUaWJlcitnSDUrNWY4?=
 =?utf-8?B?MDdETWswZmQrdDlzUUpKVFF5L2xSODhZWWl5NUJBSGszaEdodmVMVTU0NUtM?=
 =?utf-8?B?aG9xYXQ5Qk5DZFZhelAwbmFMV3JXTDBXMktvQUcrQlk5TUVCVjVkOFRiNnIz?=
 =?utf-8?B?QXhMeUk2aklOZmJzdWtxQ29CTXNrdjNMTjdVUjlHQ0N4N2hBYlE0a2lvZXYx?=
 =?utf-8?B?Zno2ajJrVmhsQU9MN2Y5YzJITUw2bFRtWUV3RkxYV2xZS093YThoVGRuNXA3?=
 =?utf-8?B?UkxEckVzRktxSmRjUVVMQ3BKcGlCVHZTQzhkald3eXF1elNDckt4b3VGMVph?=
 =?utf-8?B?YWN6ZUFScndBcHNnSXRzb3hHc2tVTUp0Unl1WktJQzhNdW0yK2p2ZTgrZGY4?=
 =?utf-8?B?YmJYM01EQWVXQXhlSzBIbXh0aHF1SVU0YTFaT3BDa1dIQjl6NzE3VUFxR2dJ?=
 =?utf-8?B?WmVMRmtPZjRxNTJVZk5reWtlYUd6eENHMGZYdXdwRzlkQXNaZDN3cEpBWXFy?=
 =?utf-8?B?Z05hUmZ1Vy9ja1NyQVhoYWxZOHI5US9GbmErRm5lVHdZWHNQK3hyYmFEdjkw?=
 =?utf-8?B?ZWphcFlIY2hqN2s4TU9ydXdyVjBaOUtMUnpnRmcrUFhwK0lGVUVwMnkwYXZF?=
 =?utf-8?B?WHhCZkdnb3ZGRDNlTXczc2hNbis2UmNpc0Nvc3Ivck45b0dLbEVHMmVRK0Fs?=
 =?utf-8?B?OEZHMFQ5d1FBTkJkb1ExUWlJNVBHb21lakpPZ2t6K1RYR2FhdnlNTmhXTmR4?=
 =?utf-8?B?VU4wUE0ySTNuWEtqRHprdVFKTHNpeUhnM0dxaHFYdFJQVkNZdGxSRFFDazNZ?=
 =?utf-8?B?YmJjTXhUeUFHb0hEaUdRVG9MNFl6RkQvSSt2cHlUYi9sOGdFdTdDZWhQQTlQ?=
 =?utf-8?B?WFVTbzdnZjVUeHVLd3pEdWx3ejY5OVJqRkpnQXErbUlsMkJ3SzkzQT09?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ffcd9e2f-83c4-46ac-2e17-08da3f87cc1e
X-MS-Exchange-CrossTenant-AuthSource: PH7PR12MB6588.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 May 2022 02:22:50.7773
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: fUx9Bvo7N5GKwJSMSEf3Rj9X3x9R+3FtJJtl3nqrRH60R9klp53BVLWlWQB1DbhQ6QdyWdux6iHtGOpe8nVAsg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW5PR12MB5599
X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 26-May-22 9:25 PM, Liang, Kan wrote:
> 
> 
> 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.

Yeah I just realized that after replying. Anyway, thanks for clarifying.
Will change internal format of HEADER_HYBRID_CPU_PMU_CAPS (as well as
HEADER_CPU_PMU_CAPS), and replace HEADER_HYBRID_CPU_PMU_CAPS with
HEADER_PMU_CAPS.

Thanks,
Ravi