On Wed, 2017-06-28 at 16:19 +0100, George Dunlap wrote: > On Thu, Jun 8, 2017 at 1:08 PM, Dario Faggioli > wrote: > > > > +            /* > > +             * We are disabling the cap for this domain, which may > > have vCPUs > > +             * waiting for a replenishment, and we need to unpark > > them all. > > +             * Parked vcpus sit in the domain's parked_vcpus list, > > which would > > +             * require being manipulated with the budget_lock > > held. However, > > +             * we have already disabled budget accounting for all > > the vCPUs of > > +             * this domain in the loop above, and therefore, no > > vCPU will run > > +             * out of budget and need being added to the list. > > +             * > > +             * For this reason, it is safe, in this case, to just > > go ahead and > > +             * drain the list, without the need of taking the > > budget_lock. > > +             */ > > +            unpark_parked_vcpus(ops, &sdom->parked_vcpus); > > I think it is safe currently.  But is there any reason not to just > grab the lock anyway?  We don't expect cap adjustment actions to be > that common, and it would mean less chance of error in the future. > The code for doing that properly is a little more involved. That's because unpark_parked_vcpus() takes runqueue locks, and hence can't be called with the budget lock held anyway. We'd therefore need to do the "trick" of the temporary list, as we do already in repl_sdom_budget() and csched2_context_saved(). That's why I did not do that here... but, after all, if I actually do things that way, I can kill the comment, so probably the number of changed lines for this patch would be the same (if not smaller! :-D). I'll think about it (with a bias toward taking the lock, as you suggest). Thanks and Regards, Dario -- <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)