Re: [PATCH v2 01/11] arch/x86: Start renaming the rdt files to more generic names

[Reinette Chatre]

Hi Babu,

On 10/9/2018 2:17 PM, Moger, Babu wrote:
> On 10/09/2018 11:39 AM, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 10/5/2018 1:55 PM, Moger, Babu wrote:
>>> New generation of AMD processors start support RDT(or QOS) features.
>>> With more than one vendors supporting these features, it seems more
>>> appropriate to rename these files.
>>>
>>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>>> ---
>>>  arch/x86/include/asm/{intel_rdt_sched.h => rdt_sched.h}     | 0
>>>  arch/x86/kernel/cpu/Makefile                                | 6 +++---
>>>  arch/x86/kernel/cpu/{intel_rdt.c => rdt.c}                  | 4 ++--
>>>  arch/x86/kernel/cpu/{intel_rdt.h => rdt.h}                  | 0
>>>  .../cpu/{intel_rdt_ctrlmondata.c => rdt_ctrlmondata.c}      | 2 +-
>>>  arch/x86/kernel/cpu/{intel_rdt_monitor.c => rdt_monitor.c}  | 2 +-
>>>  .../cpu/{intel_rdt_pseudo_lock.c => rdt_pseudo_lock.c}      | 6 +++---
>>>  ...ntel_rdt_pseudo_lock_event.h => rdt_pseudo_lock_event.h} | 2 +-
>>>  .../x86/kernel/cpu/{intel_rdt_rdtgroup.c => rdt_rdtgroup.c} | 4 ++--
>>>  arch/x86/kernel/process_32.c                                | 2 +-
>>>  arch/x86/kernel/process_64.c                                | 2 +-
>>>  11 files changed, 15 insertions(+), 15 deletions(-)
>>>  rename arch/x86/include/asm/{intel_rdt_sched.h => rdt_sched.h} (100%)
>>>  rename arch/x86/kernel/cpu/{intel_rdt.c => rdt.c} (99%)
>>>  rename arch/x86/kernel/cpu/{intel_rdt.h => rdt.h} (100%)
>>>  rename arch/x86/kernel/cpu/{intel_rdt_ctrlmondata.c => rdt_ctrlmondata.c} (99%)
>>>  rename arch/x86/kernel/cpu/{intel_rdt_monitor.c => rdt_monitor.c} (99%)
>>>  rename arch/x86/kernel/cpu/{intel_rdt_pseudo_lock.c => rdt_pseudo_lock.c} (99%)
>>>  rename arch/x86/kernel/cpu/{intel_rdt_pseudo_lock_event.h => rdt_pseudo_lock_event.h} (95%)
>>>  rename arch/x86/kernel/cpu/{intel_rdt_rdtgroup.c => rdt_rdtgroup.c} (99%)
>>
>> During the RFC it was agreed that "resctrl" will be the neutral name and
>> "intel_rdt", "amd_qos", or "arm mpam" would be the vendor specific names.
>>
>> It is ok to delay that renaming but I think any renaming done from this
>> point should respect this agreement.
>>
>> For example, if you want to rename intel_rdt.c then please rename it to
>> resctrl.c instead of just rdt.c which does not represent a generic name
>> as expressed as a goal in the subject of this patch.
> 
> I knew this was going to bit tricky. I can change all the places where I
> am touching the code to generic names(change from intel_rdt to "resctrl").

Yes, "intel_rdt" can be changed to the generic "resctrl" when it is not
vendor specific.

As far as all the code you touch is concerned it may be easier and cause
less confusion for now to just follow the current naming conventions as
you have done in patches 3 onwards and have it be included in the later
larger restructuring.

> Also lets change the "texts" which are visible to user to make it more
> generic.

Could you please elaborate what you mean with "texts" here? Are you
referring to the pr_info() found in intel_rdt_late_init()? Here it may
be good to also change to print "RESCTRL %s allocation
detected"/"RESCTRL %s monitoring detected" - the resource names printed
are already generic.

> But "rdt" has been used generously in multiple files(like rdt_resource,
> rdt_domain etc). Changing those definitions and functions will be messier.
> I will not worry about it now. Thoughts?

I agree. My comments were specific to the first two patches of this
series that started doing the renaming but not using the agreed upon
naming - especially since both those patches claim to transition to
generic names. Could just these two patches be modified to change
"intel_rdt" to "resctrl" instead of "intel_rdt" to "rdt" as it currently
does?

Reinette