[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2] schedule: move last_run_time to the credit scheduler privates
On 12.09.18 20:27, Dario Faggioli wrote: On Wed, 2018-09-12 at 17:47 +0300, Andrii Anisov wrote:From: Andrii Anisov <andrii_anisov@xxxxxxxx> The structure member last_run_time is used by credit scheduler only. So move it from a generic vcpu sctructure to the credit scheduler private vcpu definition. Signed-off-by: Andrii Anisov <andrii_anisov@xxxxxxxx> --- Changes in v2: - last_run_time type changed to s_time_t - scurr changed to svc - dropped stray blanks - pointers to const are used appropriatelyWell, the fact that the type is changing, and that you're also fixing/improving const-ness, should IMO at least be quickly mentioned in the changelog. Something like: "While there, also turn it into s_time_t, which is more appropriate. Also, properly const-ify one of the argument of __csched_vcpu_is_cache_hot()" OK. And I was right about to write that I would also like the changelog itself to contain the usual sentence: "No functional change intended" but then I saw this (sorry for not noticing it in v1):@@ -1869,6 +1874,7 @@ csched_schedule( /* Update credits of a non-idle VCPU. */ burn_credits(scurr, now); scurr->start_time -= now; In the first (internal) version of the patch this line was commented as "No matter what happens next with the current VCPU, we are safe to set its last run time here" Right you are. But while it is used for vcpu cache "temperature" measurement only, it is still OK. Scheduling parameters and statistics are tracked by other entities.+ scurr->last_run_time = now; } else { diff --git a/xen/common/schedule.c b/xen/common/schedule.c index 05281d6..3c299ca 100644 @@ -1556,7 +1556,6 @@ static void schedule(void) ((prev->pause_flags & VPF_blocked) ? RUNSTATE_blocked : (vcpu_runnable(prev) ? RUNSTATE_runnable : RUNSTATE_offline)), now); - prev->last_run_time = now;ASSERT(next->runstate.state != RUNSTATE_running);vcpu_runstate_change(next, RUNSTATE_running, now);Basically, right now last_run_time is updated only when, in csched_schedule() we chose a different vcpu than the one which was running, and it also was updated for the idle vcpu. With the patch, it looks to me that it is updated even when we continue to run the same vcpu, and is never updated for the idle vcpu. Yep, but it has no meaning for the idle vcpu. It never migrates. Now, considering how the info it contains is used currently, I'd say that not updating the field for the idle vcpu is not a problem. The fact that we _always_ update it for non-idle vcpus means we're changing what the field contains for running vcpus. Which again does not look to matter much right now. Do you agree with this analysis? Sure :) If yes, well, I think this is bearable. Or, at least, that the benefit of having the logic self-contained in Credit code, overweight this slight loss of coherency between the name of the field and its content (things remains well within Credit's standards wrt to time accounting! :-/). But I'd add a few words about all this either in the changelog, and probably also in a comment (even a very short one, like "last_run_time is only meaningful for non running tasks" close to where you update it, or something like that). OK. -- *Andrii Anisov* _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |