Re: [PATCH v5 3/8] x86/resctrl: Split the rdt_domain structure

[Reinette Chatre]

Hi Tony,

On 9/26/2023 11:46 AM, Tony Luck wrote:
> On Mon, Sep 25, 2023 at 04:25:15PM -0700, Reinette Chatre wrote:
>>> +struct rdt_domain {
>>> +	// First three fields must match struct rdt_mondomain below.
>>
>> Please avoid comments within declarations. Even so, could you please
>> elaborate what the above means? Why do the first three fields have to
>> match? I understand there is common code, for example, __rdt_find_domain()
>> that operated on the same members of the two structs but does that
>> require the members be in the same position in the struct?
>> I understand that a comment may be required if position in the struct
>> is important but I cannot see that it is.
> 
> [Just replying to this one point in your message to get guidance. I'll
> address all the rest in other replies]
> 
> I'm wrong about the first *three* fields ... but the first *two* fields
> (the "list" and the "id") do need to be at the same offsets in different
> structures if a common routine is going to be used to access those
> fields.
> 
> If the "id" were at offset 0x10 in the control version of the domain
> structure, and at offset 0x20 in the monitor version of the domain
> structure, there would be no hope for a common routine to access the
> "id" field when searching a list that could be either control or
> monitor domains.
> 
> I'm looking at making this far more explicit with a new patch between
> 0001 and 0002 that pulls the two fields into a common substructure that
> will be included in each of the control and monitor versions of the
> structure.
> 
> Patch included below.
> 
> But this seems like it is a lot of churn to avoid having separate
> functions to search control and monitor lists. Each a clone of
> the existing ~24 line rdt_find_domain() with just the type changed
> for the return value and the list travsersal.

Yes. Sorry, I did not realize this implication during the earlier
discussions.

> 
> What do you think?
> 

It sounds to me as though you are advocating for open coding 
rdt_find_ctrl_domain() and rdt_find_mon_domain()? That sounds good
to me.

Sorry for the noise.

Reinette