Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Message-ID: <8f6c7b40-5218-4427-865d-55e5f09c594f@intel.com>
Date:   Thu, 7 Dec 2023 10:02:01 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 08/26] selftests/resctrl: Split measure_cache_vals()
Content-Language: en-US
To:     =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
CC:     <linux-kselftest@vger.kernel.org>, Shuah Khan <shuah@kernel.org>,
        "Shaopeng Tan" <tan.shaopeng@jp.fujitsu.com>,
        =?UTF-8?Q?Maciej_Wiecz=C3=B3r-Retman?= 
        <maciej.wieczor-retman@intel.com>,
        Fenghua Yu <fenghua.yu@intel.com>,
        LKML <linux-kernel@vger.kernel.org>
References: <20231120111340.7805-1-ilpo.jarvinen@linux.intel.com>
 <20231120111340.7805-9-ilpo.jarvinen@linux.intel.com>
 <c303ba1b-d7bd-47cf-9e81-8ea0c60b973c@intel.com>
 <e87d8ba-141a-5779-fc6-27e4735fc1bf@linux.intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <e87d8ba-141a-5779-fc6-27e4735fc1bf@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?aU94bFNOT3VZeFhlWFhQRFR2QVdZbGNPNEZhZGRuQWJXOEtNRXEzeVNJTWtS?=
 =?utf-8?B?a0RyaGRvZlo5WFZoWW9wV3ZhK3ZMKzBJRjFIZFRLMys1eWE3UHlQK1VrMzFa?=
 =?utf-8?B?UnBxYWlod29qRDlQNTFaQ1JmejBST1FtQUJTMFVKQTJVdEFpYXpXNVRBSUhY?=
 =?utf-8?B?VFo3SStiRitJcU4weE03Sk05MlZrOFpjQ1hEMUo5eDZGVlJvLzczeUNiSkx2?=
 =?utf-8?B?WkhKQkI2Y0pBMGtyYjltd1ByeDNyS1VhaW12WDdiekIxZkNQWHMvdlNHU2JF?=
 =?utf-8?B?VDF1eVpIQ1ZFRWZ1bXUrV0ZlYk52b21RMlUxem9Bak9IUGI0V0pMM3VERGp6?=
 =?utf-8?B?eGxkRHlOek4rcFJTUzBveXp6UWRwWDNqbzFyN1c3T2xsYi9CdFNyMUhFVko1?=
 =?utf-8?B?YW1XRXhRZ2oyNEtwWjNzWkczbUNXMERhUForWWh6ZDh0VUNRV2dlMDg2UTg4?=
 =?utf-8?B?M1Y5UGxiVFAxcnRURjdMTmNBQnlNdFhxSVdpaTNhTkxJc2JDT1VIVFVkT0ZQ?=
 =?utf-8?B?ZzMzUjFkWDFDeWI2ZVJON2FYL0prbXlGMGQvNEVtM1FqbEZERmJPRVhRZWZp?=
 =?utf-8?B?dWE5RFMrTlBmU3pLcnJlaDdHR3o1Q1E4YVRWQ0N4emNDNGppTTIrOGR4dHpW?=
 =?utf-8?B?VEY5SGgraTFUUUg2TGFCeUoyekhyVDhneXZadUJkMCtpWUlIV3lIam84WWVw?=
 =?utf-8?B?RnBhc2NYejNZYzlucEpiQXhDNjd3S1hUc216QktqTkpEelRtZE5CTWxQbXJX?=
 =?utf-8?B?U0N4a0REZUpqajRMcmJxR3Uzb00zL2xkSlBWQ3g0Y3pQSUJkd2hZOXJHa3BS?=
 =?utf-8?B?YktVL3BZN0theG9rcW43eFpnMW8zOTZteVdzaDA5WCs2dXJqelUyVTcvSWRw?=
 =?utf-8?B?WnY5YXRlQUNGMUZERE5WbmdtZXpiL3A3OVNvOUJKelU0TmUwMDd3WWtaQjg3?=
 =?utf-8?B?NEVhU0lJUVJyOGdLM3Z4b3FPbVdqdk9GdW55a256S240ZkpCeHd4dWxEOWg0?=
 =?utf-8?B?dnR0eTREOEJSazgyMVhnK0ptang5WHVJYmVxbm9namR1dTBoTHE4NFRIZ2k2?=
 =?utf-8?B?VVAyVFRSRWdPT1hFbDhOS3BCVmZCa1lyUFE3cjY5RjQ4RXVCb1U2b2IwTzJQ?=
 =?utf-8?B?SWJqWGZJeFp5L3pYb0Y2bWRVVlRGVkNUOHpYMk5CeDVNc1N3Uzl6NG9WVDNh?=
 =?utf-8?B?WTBnaEVmcFFJQW5rMkpUVEh4VjZmeXZlTWlQeHlzREtROHdSVjJzRHlpQmxH?=
 =?utf-8?B?REcvWGFjWXhuMTFzZi9tcnpaSTlzSWZtTkdqZG1pczdxNWpYa2dXQUVTczVJ?=
 =?utf-8?B?ai9kYWFTUUd6ZWlUd3lhRFMyOUFWS2xDZmQvbUhXRUlKdllSeVNPcnRPQmNn?=
 =?utf-8?B?cGZpdnBWYzdPZVdYT0V3Wmd6c2VZNGtiTDlRM2pPNk1nSml2ZXpISG45aHg1?=
 =?utf-8?B?dldaWG1sNmFVRGlnV1BBZlNha3JrLzRQWTBWWllOUVRoR0dMTE1CZStEWGtH?=
 =?utf-8?B?QXJmemFhaC9GVWdtYTZQdGl1T2R3b1RzaEMvREJEZVk1RnVNaU1uU2E5K3Zu?=
 =?utf-8?B?bEcxZFFiV2NOVGFkbDlSaDRNK25ZbkFWZkVzdm53K0plL0tiTlRSbFdiNTUx?=
 =?utf-8?B?TUdsRk1KM1VTQkJMTzh4UC9VY3IxSTUzTlBqTkQ0WmdpNkp0dFNQdjR3VzJ0?=
 =?utf-8?B?Qm5qRCt1bnlyR1lXbURWTjFyTkFkM2o0aGszWGs4ZWNnNFg5TVRSM0hjWUd1?=
 =?utf-8?B?bURjbVZXenVUMG40aVJZTHhQaGo5eUxLUzIwL1RUNVJXZFdJUTZxZ2VLQ1BW?=
 =?utf-8?B?bGtYcjFHWmxVOG1uTnFzR3M5dndTamY2TDdPQ1I5MkszRENaS3J0S3JzT3Fo?=
 =?utf-8?B?eUJlZzNqVEUxcTZtbjRIZ0JzcWcxUFBmcndndEpldmUxL1E1dDVvdEtlL0d4?=
 =?utf-8?B?WHVXN2ZyOStZWkZRZzg5ZGJhbWZnWm1hT2F1NklrZ0hkcWxLS3c5VzVQNEdU?=
 =?utf-8?B?R2FZYkJxeWZIdURoNkVzVjhFMkNQSGxrRUFhWlc4RGovUGVWamdya0xWeURO?=
 =?utf-8?B?YzhOUFE4VjNEMksrUUpZSW9YcmpEQnN6ZWFBdVltK1hreWxiSGFRcHJLVGJ6?=
 =?utf-8?B?Ti9KaytPazVpWE1rU0d4NnIzSWhtVWJycGZ6U1paY1lPNzhDTDVhVm5YZVhD?=
 =?utf-8?B?S1E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6f7d6b79-a8df-4955-b7f6-08dbf74e9e2c
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Dec 2023 18:02:03.8960
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 3xt54KaOwX7IVdFlL4gyAgK4hJagHdm21r+Ahahu5gF8xEEhHT0e/9nVjY7pJJXb8YWhY+5tPC9Mwjp6ihwmcZqWeZCHD95lNQWEJZRugp0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB7247
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Thu, 07 Dec 2023 10:02:24 -0800 (PST)

Hi Ilpo,

On 12/7/2023 6:32 AM, Ilpo Järvinen wrote:
> On Tue, 28 Nov 2023, Reinette Chatre wrote:
>> On 11/20/2023 3:13 AM, Ilpo Järvinen wrote:

...
>>> -	/*
>>> -	 * Measure llc occupancy from resctrl.
>>> -	 */
>>> -	if (!strncmp(param->resctrl_val, CMT_STR, sizeof(CMT_STR))) {
>>> -		ret = get_llc_occu_resctrl(&llc_occu_resc);
>>> -		if (ret < 0)
>>> -			return ret;
>>> -		llc_value = llc_occu_resc;
>>> -	}
>>> -	ret = print_results_cache(param->filename, bm_pid, llc_value);
>>> -	if (ret)
>>> +	ret = print_results_cache(filename, bm_pid, llc_perf_miss);
>>> +	return ret;
>>> +}
>>
>> Perhaps print_results_cache() can be made to return negative error
>> and this just be "return print_results_cache(...)" and the function
>> comment be accurate?
> 
> I think, I'll just change all "return errno;" to "return -1" before this,
> however, one open question which impacts whether this is actually Fixes 
> class issue:
> 
> It seems that perror()'s manpage doesn't answer one important question, 
> whether it ifself can alter errno or not. The resctrl selftest code 
> assumes it doesn't but some evidence I came across says otherwise so doing 
> return errno; after calling perror() might not even be valid at all.
> 
> So I'm tempted to create an additional Fixes patch about the return change 
> into the front of the series.
> 

I would not trust errno to contain code of earlier calls after a call to perror().
If errno is needed I think it should be saved before calling perror(). Looking
at perror() at [1] I do not see an effort to restore errno before it returns,
and looking at the implementation of perror() there appears to be many
opportunities for errno to change.

Reinette

[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=stdio-common/perror.c;h=51e621e332a5e2aa76ecefb3bcf325efb43b345f;hb=HEAD#l47