On Tue, Jun 18, 2019 at 04:52:09PM +0000, Cosmin Marin wrote: > > > On 18/06/2019, 15:51, "Peter Xu" <pet...@redhat.com> wrote: > > On Tue, Jun 18, 2019 at 12:25:43PM +0000, Cosmin Marin wrote: > > Hi Peter, > > > > thanks for reviewing the patch. Indeed, I agree that it's > almost impossible to determine which solution it's better from the > scalability perspective. However, I feel that using per-vCPU timers is the > only way for ensuring correctness of the throttling ratio. > > The thing is that your patch actually contains two changes: > > 1. use N timers instead of one. > > 2. remove throttle_thread_scheduled check, so we do the throttle > always > > Here what I'm worried is that _maybe_ the 2nd item is the one that > really helped. > > C: The removal of *throttle_thread_scheduled* is a consequence of the > per-vCPU model only. In this model, each of the vCPUs schedules work just for > itself (as part of the timer's firing callback) - there's no global point of > control - therefore, the variable isn't helpful for scheduling anymore. > > Note that there is a side effect that we might queue more than one > work on one specific cpu if we queue it too fast, but it does not > block us from trying it out to identify which item (1 or 2 or both) > really helped here. Then if we think that (queuing too much) is an > issue then we can discuss on how to fix it since current patch will > have this problem as well. > > C: I believe that in the per-vCPU timer implementation we cannot queue > more than one piece of work because, here, the vCPU queues work for itself > and that happens only when the timer fires - so, the two "states" - > scheduling and sleeping - are mutually exclusive running from the same thread > context.
I think this is the place where I'm in question with - I don't think they are using the same context. IMO the timer will always be run in the main thread no matter you use per-cpu timer or not, however the sleeping part should be run on per-cpu. A simple way to verify it would be: break at cpu_throttle_timer_tick() to see which thread it is running in. > > > > It's a bit unclear to me how the throttling ratio inconsistency > can be fixed by using a single timer even avoiding the conditional timer > re-arming. Could you provide more details about the use of a single timer ? > > C: I feel like in this case it will sleep too much running into a > problem similar to the one solved by 90bb0c0; under heavy throttling more > than one work item may be scheduled. Right. So I feel like we need a solution that will avoid this problem but at the same time keep the proper accuracy of the throttling. Thanks, -- Peter Xu
