Hi Tony, On 10/26/23 11:09, Luck, Tony wrote: >>> 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. > > Can you explain why you don't like the global? If there is a better name for it, > or a better comment for what it does, or you think the code that sets the value > could be clearer, then I'm happy to make changes there. My theory is always try to localize the changes and avoid global variables when there are other ways to do the same thing. It may not be strong argument. > > Which events are supported by a system is a static property. Figuring out once > at "init" time which event to use for mba_MBps seems a better choice than > re-checking for each of possibly hundreds of RMIDs every second. Even though > the check is cheap, it is utterly pointless. mbm_update happens here only to the active group (not on all the available rmids). Also, I am not clear about weather this is going fix your problem. You are setting the MSR limit based on total bandwidth. The MSR you are writing may only have the local socket effect. In cases where all the memory is allocated from remote socket then writing the MSR may not have any effect. Also you said you don't have the hardware to verify. Its always good to verify if is really fixing the problem. my 02 cents. Thanks Babu Moger
