On Thu, Oct 26, 2023 at 12:19:14PM -0500, Moger, Babu wrote: > 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. A good theory. I do this too. But it seems I'm more likely to go with global variables if the cost of avoiding them is high. But "cost" is a very subjective thing. > > 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). mbaMBps needs to get data from all active RMIDs to provide input to the feedback loop. That might be a lot of RMIDs if many jobs are being monitored independently (which I believe is a common mode of operation). > 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. Intel MBA controls operate on all memory operations that miss the L3 cache (whether they are going to a local memory controller, or across a UPI link to a memory controller on another socket). > 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. I don't have hardare that enforces this. But Linux does have a boot option clearcpuid=cqm_mbm_local to tell Linux that the system doesn't provide a local counter. I've been using that for all my testing. -Tony
