Hi Tony,
On 10/25/23 16:06, Peter Newman wrote:
> Hi Tony,
>
> On Wed, Oct 25, 2023, 21:38 Tony Luck <tony.luck@xxxxxxxxx> wrote:
>>
>> On Wed, Oct 25, 2023 at 02:46:53PM +0200, Peter Newman wrote:
>>
>>>> +static struct mbm_state *get_mbm_data(struct rdt_domain *dom_mbm, int rmid)
>>>> +{
>>>> + if (is_mbm_local_enabled())
>>>> + return &dom_mbm->mbm_local[rmid];
>>>> +
>>>> + return &dom_mbm->mbm_total[rmid];
>>>> +}
>>>
>>> That looks very similar to the get_mbm_state() function I added to
>>> this same file recently:
>>>
>>> https://lore.kernel.org/all/20221220164132.443083-2-peternewman%40google.com
>>>
>>> I think the name you picked is misleadingly general. "local if
>>> available, otherwise total" seems to be a choice specific to the mbps
>>> controller. I think these functions should be reconciled a little
>>> better.
>>>
>>
>> Peter (and Babu, who made the same point about get_mbm_state().
>>
>> Do you want to see your function extended to do the "pick an MBM event?"
>
> What I meant was I think it would be enough to just give the function
> you added a name that's more specific to the Mbps controller use case.
> For example, get_mba_sc_mbm_state().
I actually liked this idea. Add a new function get_mba_sc_mbm_state. That
way we exactly know why this function is used. I see you already sent a v2
making the event global. Making it global may not be good idea. Can you
please update the patch and resend. Also please add the comment about why
you are adding that function.
--
Thanks
Babu Moger
