Messages in this thread | | | Date | Fri, 28 Jun 2013 23:14:45 +0530 | From | Srikar Dronamraju <> | Subject | Re: [PATCH 5/8] sched: Favour moving tasks towards the preferred node |
| |
> > > > > > - if (migrate_improves_locality(p, env)) > > > + if (migrate_improves_locality(p, env)) { > > > +#ifdef CONFIG_SCHEDSTATS > > > + schedstat_inc(env->sd, lb_hot_gained[env->idle]); > > > + schedstat_inc(p, se.statistics.nr_forced_migrations); > > > +#endif > > > return 1; > > > + } > > > > > > > In this case, we account even cache cold threads as _cache hot_ in > > schedstats. > > > > We need the task_hot() call to determine if task is cache hot or not. > > So the migrate_improves_locality(), I think should be called within the > > tsk_cache_hot check. > > > > Do you have issues with the above snippet that I posted earlier? > > > > The migrate_improves_locality call had already happened so it cannot be > true after the tsk_cache_hot check is made so I was confused. If the call is > moved within task cache hot then it changes the intent of the patch because
Yes, I was suggesting moving it inside.
> cache hotness then trumps memory locality which is not intended. Memory > locality is expected to trump cache hotness. >
But whether the memory locality trumps or the cache hotness, the result would still be the same but a little concise code.
> How about this? > > tsk_cache_hot = task_hot(p, env->src_rq->clock_task, env->sd); > > if (migrate_improves_locality(p, env)) { > #ifdef CONFIG_SCHEDSTATS > if (tsk_cache_hot) { > schedstat_inc(env->sd, lb_hot_gained[env->idle]); > schedstat_inc(p, se.statistics.nr_forced_migrations); > } > #endif > return 1; > } > > if (!tsk_cache_hot || > env->sd->nr_balance_failed > env->sd->cache_nice_tries) { > #ifdef CONFIG_SCHEDSTATS > if (tsk_cache_hot) { > schedstat_inc(env->sd, lb_hot_gained[env->idle]); > schedstat_inc(p, se.statistics.nr_forced_migrations); > } > #endif > return 1; > }
Yes, this looks fine to me. >
-- Thanks and Regards Srikar Dronamraju
| |