Message-ID: <c07dfb04-4502-47d8-ad89-9f9c7898f246@amd.com>
Date:   Thu, 16 Mar 2023 16:11:09 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Reply-To: babu.moger@amd.com
Subject: Re: [PATCH v3 2/7] x86/resctrl: Remove few unnecessary rftype flags
Content-Language: en-US
To:     Reinette Chatre <reinette.chatre@intel.com>, corbet@lwn.net,
        tglx@linutronix.de, mingo@redhat.com, bp@alien8.de
Cc:     fenghua.yu@intel.com, dave.hansen@linux.intel.com, x86@kernel.org,
        hpa@zytor.com, paulmck@kernel.org, akpm@linux-foundation.org,
        quic_neeraju@quicinc.com, rdunlap@infradead.org,
        damien.lemoal@opensource.wdc.com, songmuchun@bytedance.com,
        peterz@infradead.org, jpoimboe@kernel.org, pbonzini@redhat.com,
        chang.seok.bae@intel.com, pawan.kumar.gupta@linux.intel.com,
        jmattson@google.com, daniel.sneddon@linux.intel.com,
        sandipan.das@amd.com, tony.luck@intel.com, james.morse@arm.com,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        bagasdotme@gmail.com, eranian@google.com,
        christophe.leroy@csgroup.eu, jarkko@kernel.org,
        adrian.hunter@intel.com, quic_jiles@quicinc.com,
        peternewman@google.com
References: <167778850105.1053859.14596357862185564029.stgit@bmoger-ubuntu>
 <167778867236.1053859.12920879751317268318.stgit@bmoger-ubuntu>
 <1a45fc82-9339-d086-2e70-27fd91906b8a@intel.com>
 <edcb0810-df39-669b-ebf2-5988a71525e9@amd.com>
 <4af7feb7-7663-7bab-33a4-7ffce092f55d@intel.com>
From:   "Moger, Babu" <babu.moger@amd.com>
In-Reply-To: <4af7feb7-7663-7bab-33a4-7ffce092f55d@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SmRhSEkzb1prS3dwcUVrOEtNb3h0ZUVidnVDcjdUYzBiOW9kNHlQK21CMi9i?=
 =?utf-8?B?a0tONFNndEhmVW1pVUtibGhIeFhaUmJJOU9aUUd1TnZRQ0ZJb1VCai9DbEUv?=
 =?utf-8?B?azBtMDZMOERPSDFnbThxQ2N5TmNzNUhXbkFXWjA0R0liZmVuMlRvUjhkczhT?=
 =?utf-8?B?RVplQjhmSXhRMnJIU0VMT1ZUcHVzdkQxRXpaMkNhVEVxV3lXYU11M0I5TVE3?=
 =?utf-8?B?MDdRSDBUekdNVDF2QXFPNGMxN2FOenhCNnJvVW5HZDlMbmpjREYxT3lLQU5z?=
 =?utf-8?B?M2NHbFN3QXB3ZjRpZFc3TXIxdGpFWUlhK284VFRDczR1M2pETUtrZUFycmFB?=
 =?utf-8?B?MDlCVytCMWRsTHVIaG4xVVdObE1nazNCbXdOV2g4aUZaRldIdmdHU2ppQ3BL?=
 =?utf-8?B?TUhYSytXcFJCc0g3LzEyaWNZN1BjbCs5UCtzRXJTNW83OHpTUWhqcms3U0pq?=
 =?utf-8?B?aGM1V3BucjlsaEFIbkdwaVYxUnMwOU9lcnFPblJjZmpyUnN2Lyt3UUY2T0ZN?=
 =?utf-8?B?U0FPSTdCalVvdFdrSWIzekgyUVFGYUxOVmUxY1FieGZlUFlTM1NkbVF3aEZG?=
 =?utf-8?B?Z3AwR3hYbUdWR0tFYnpIUWhrM1NmelBnem5OUG9XbjNrS3FQdmEya1BtUG5U?=
 =?utf-8?B?ZjZxWXVOeDloK1FkYkNjc1R1b0lqa3g2c3phWkFOa0FCbWRVdFVkK2NmdWll?=
 =?utf-8?B?THdBZEpFSWVhUXVBTmdReFN5cXA3N2FQa1dIYWdCMFdLMnhIakZSUmFXQUds?=
 =?utf-8?B?WVl3d2MwU0JxRUVEVHBKVi9lSlN2QzVMdDg2Ni8xc21IbFNJL2hGajhkcHRp?=
 =?utf-8?B?eUJXRG9RTWRlNWtEUFZVODNZbVdEVnJEUzZuSi80aWIyVEZzMjloalpFM1RS?=
 =?utf-8?B?TnpFRXpXRkdaalhmRUpUc0pFc2plTGlHWU5iamVSdVJpQmR1b2ZDTzM3VnBV?=
 =?utf-8?B?RThReWNzTUgzNEliWC96K3VrQUlmRzV1YkYvT0RyR3pzZ2xLcndpTmdRODk1?=
 =?utf-8?B?RkdESEFPdVVPVzE3bDVWeVlseVp6SEhBZW9KdFIrSDhQYmhHSi83Sjl5dG1k?=
 =?utf-8?B?VnFaM1JoMHArbkhyV0ZZa1ZGYUF4bGlrc3p2QmFFbGRLK2NIYmd5ZzZwVnln?=
 =?utf-8?B?QVhpME9jUmREZGM4MFdJRzIvNFBBOVVKSUJZMzh0SkVsczZERE1obHZ0S0xJ?=
 =?utf-8?B?aXRySDBVRy9rdXJaekM0OTd3cEpRK21RKy9VU0pINGtKbmtiT2dSTjVrTG4z?=
 =?utf-8?B?N2U0em5Rd1VHVnRDc3J1OG5Zc3hPT2tsMThrdWl3eHRKQTByMnphNUFzU1pY?=
 =?utf-8?B?UDJ1VEdVQUZINWJpWXlVbEZ1d1FXV0RkNUl0ZlJkV1NuOCtyWUpvR1p5dEs1?=
 =?utf-8?B?UXU3RmlsaDJ1emZFZnJOS1k5VTU4N2VoQjJrdm94UzM1cFQxbW01d0RtNXV3?=
 =?utf-8?B?VHRWZUoyZE9XMmpkaGNrcGVwMmgvV0tYcHdzMlJWQnFuN0ZQYjgwaXFwV013?=
 =?utf-8?B?L2w4dzhLek0xSkJoUE9hUlozc3VxWG56bjkvempQaVIvMTdZTFpNeGllbU1U?=
 =?utf-8?B?SVNGWFphOWxtZTJ1S205YitCQkltUW1LaC84cjhpdk14QmVtdzFxV2liTmd3?=
 =?utf-8?B?RE1IbElmZEgxeVFPcm81cXZHUWI5cmlmdllyUWYyZlVFazBIZlc2QWh1cmkv?=
 =?utf-8?B?dzJqNnhBcmt3T2RibDNFZ3hzaGJSQjEvL0YxRTNiak1IUEExRjZDRThWTndv?=
 =?utf-8?B?RmZoN1l3SzIwVERqVkM4NlZySksvZFJwczkxOHFGRzYwSDRQN2tta2VvZFdN?=
 =?utf-8?B?ZzltM0lPeE1PSk5WM01Zc0U2WW55ZFphc04xUVNGcS85UHp2MXI1dldyb3FJ?=
 =?utf-8?B?OWg1WUV6Uk1POFNtZHVJT0Q2WWFKb3IxZEh2dTArbHVGUitKLzFLN043TVJN?=
 =?utf-8?B?K2o1MHFqMk92ZnFkNXZxMVZ0RUVDWFNLV1dwZkdPN1pSTVRoYVJUT2dtY0RJ?=
 =?utf-8?B?Q3BvWXREWHhIL0F5bTRlQ2R0cEd3ZGlFTkMrNmhoUXFKVzd4ZEg4RUNnaSs2?=
 =?utf-8?B?ZEgrMkpyL1BLenBPTmVHZXRRdUlYZW5nTnZnWXhDWjVna3B5WGVEc3dnbkg0?=
 =?utf-8?Q?4JDeUi12KAUSg51hS+MaUUwOV?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 725c72ec-89ba-48eb-f5b8-08db2662fb1a
X-MS-Exchange-CrossTenant-AuthSource: MW3PR12MB4553.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Mar 2023 21:11:16.7367
 (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: SCPu/XYLsFrz2lWcebyKW7y/aA0VE5Lx6vYAl+K3kkFXIkZOEu5ZBz7Mi1F/yGd3
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB5745
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Reinette,

On 3/16/23 15:41, Reinette Chatre wrote:
> Hi Babu,
> 
> On 3/16/2023 1:31 PM, Moger, Babu wrote:
>> On 3/15/23 13:33, Reinette Chatre wrote:
>>> On 3/2/2023 12:24 PM, Babu Moger wrote:
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> index 15ea5b550fe9..3c86506e54c1 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> @@ -3163,7 +3163,7 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
>>>>  {
>>>>  	struct rdtgroup *prdtgrp, *rdtgrp;
>>>>  	struct kernfs_node *kn;
>>>> -	uint files = 0;
>>>> +	uint fflags = 0;
>>>
>>> Hoe does changing the variable name from "files" to "fflags" simplify
>>> the code?
>>
>> I should have said readability of the code. Its actually fflags we are
>> passing to rdtgroup_add_files. While changing flags below, I changed the
>> variable name to fflags.
> 
> You are correct in that it is the actual fflags parameter but what it
> reflects is which files will be created. I do not find that using "files"
> makes the code unreadable. 

Everything helps. I changed it because I was already changing few things
in this function. That is not the only change in this function. Here is
the main change in this function.

-	files = RFTYPE_BASE | BIT(RF_CTRLSHIFT + rtype);
-	ret = rdtgroup_add_files(kn, files);
+	if (rtype == RDTCTRL_GROUP)
+		fflags = RFTYPE_BASE | RFTYPE_CTRL;
+	else
+		fflags = RFTYPE_BASE | RFTYPE_MON;
+
+	ret = rdtgroup_add_files(kn, fflags);

In my opinion this is more clearer. Also I can delete some of these
un-necessary definitions below.

 #define RFTYPE_INFO			BIT(0)
 #define RFTYPE_BASE			BIT(1)
-#define RF_CTRLSHIFT			4
-#define RF_MONSHIFT			5
-#define RF_TOPSHIFT			6
-#define RFTYPE_CTRL			BIT(RF_CTRLSHIFT)
-#define RFTYPE_MON			BIT(RF_MONSHIFT)
-#define RFTYPE_TOP			BIT(RF_TOPSHIFT)
+#define RFTYPE_CTRL			BIT(4)
+#define RFTYPE_MON			BIT(5)
+#define RFTYPE_TOP			BIT(6)

Thanks
Babu