On 29/08/2019 15:26, Vincent Guittot wrote: [...] >> Seeing how much stuff we already do in just computing the stats, do we >> really save that much by doing this? I'd expect it to be negligible with >> modern architectures and all of the OoO/voodoo, but maybe I need a >> refresher course. > > We are not only running on top/latest architecture >
I know, and I'm not going to argue for a mere division either. I think I made my point. [...]>>>>> + if (busiest->group_type == group_misfit_task) { >>>>> + /* Set imbalance to allow misfit task to be balanced. */ >>>>> + env->balance_type = migrate_misfit; >>>>> + env->imbalance = busiest->group_misfit_task_load; >>>> >>>> AFAICT we don't ever use this value, other than setting it to 0 in >>>> detach_tasks(), so what we actually set it to doesn't matter (as long as >>>> it's > 0). >>> >>> not yet. >>> it's only in patch 8/8 that we check if the tasks fits the cpu's >>> capacity during the detach_tasks >>> >> >> But that doesn't use env->imbalance, right? With that v3 patch it's just >> the task util's, so AFAICT my comment still stands. > > no, misfit case keeps using load and imbalance like the current > implementation in this patch. > The modifications on the way to handle misfit task are all in patch 8 > Right, my reply was a bit too terse. What I meant is that with patch 8 the value of env->imbalance is irrelevant when dealing with misfit tasks - we only check the task's utilization in detach_tasks(), we don't do any comparison of the task's signals with env->imbalance. Whether we set the imbalance to the load value and set it to 0 in detach_tasks() or set it to 1 and decrement it in detach_tasks() gives the same result. That's why I was saying it conceptually fits with the migrate_task logic, since we can set the imbalance to 1 (we only want to migrate one task).