Messages in this thread | | | Date | Tue, 29 Apr 2014 09:22:00 +0530 | From | Preeti U Murthy <> | Subject | Re: [PATCH 1/3] sched, balancing: Update rq->max_idle_balance_cost whenever newidle balance is attempted |
| |
On 04/28/2014 11:34 PM, Jason Low wrote: > On Sun, 2014-04-27 at 14:01 +0530, Preeti Murthy wrote: >> Hi Jason, Peter, >> >> The below patch looks good to me except for one point. >> >> In idle_balance() the below code snippet does not look right: >> >> - if (pulled_task || time_after(jiffies, this_rq->next_balance)) { >> - /* >> - * We are going idle. next_balance may be set based on >> - * a busy processor. So reset next_balance. >> - */ >> +out: >> + /* Move the next balance forward */ >> + if (time_after(this_rq->next_balance, next_balance)) >> this_rq->next_balance = next_balance; >> - } >> >> By not checking this_rq->next_balance against jiffies, >> we might end up not updating this parameter when it >> has expired. >> >> So shouldn't it be: >> >> if (time_after(jiffies, this_rq->next_balance) || >> time_after(this_rq->next_balance, next_balance)) >> this_rq->next_balance = next_balance; > > Hi Preeti, > > If jiffies is after this_rq->next_balance, doesn't that mean that it's > actually due for a periodic balance and we wouldn't need to modify it? > In rebalance_domains(), we do load_balance if time_after_eq(jiffies, > sd->last_balance + interval).
Right. So I missed the point that we don't really have a problem with the rq->next_balance being expired. It will anyway ensure that in the next call to rebalance_domains() load balancing will be done and that is all we want. Thanks for pointing it out.
Regards Preeti U Murthy > >> >> Besides this: >> Reviewed-by: Preeti U Murthy <preeti@linux.vnet.ibm.com> > > Thanks for the review. >
| |