From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54909) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZLDre-0006K7-TM for qemu-devel@nongnu.org; Fri, 31 Jul 2015 13:13:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZLDrY-0005dZ-NQ for qemu-devel@nongnu.org; Fri, 31 Jul 2015 13:12:58 -0400 Received: from e32.co.us.ibm.com ([32.97.110.150]:60884) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZLDrY-0005d4-Gu for qemu-devel@nongnu.org; Fri, 31 Jul 2015 13:12:52 -0400 Received: from /spool/local by e32.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 31 Jul 2015 11:12:50 -0600 Received: from b03cxnp08027.gho.boulder.ibm.com (b03cxnp08027.gho.boulder.ibm.com [9.17.130.19]) by d03dlp01.boulder.ibm.com (Postfix) with ESMTP id 43C561FF0042 for ; Fri, 31 Jul 2015 11:03:59 -0600 (MDT) Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by b03cxnp08027.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t6VHCnhu40173726 for ; Fri, 31 Jul 2015 10:12:49 -0700 Received: from d03av02.boulder.ibm.com (localhost [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t6VHCmCS004172 for ; Fri, 31 Jul 2015 11:12:48 -0600 Message-ID: <55BBAC8F.6030604@linux.vnet.ibm.com> Date: Fri, 31 Jul 2015 13:12:47 -0400 From: "Jason J. Herne" MIME-Version: 1.0 References: <1435855010-30882-1-git-send-email-jjherne@linux.vnet.ibm.com> <1435855010-30882-2-git-send-email-jjherne@linux.vnet.ibm.com> <55956A2E.4020806@redhat.com> <55A3CEAF.6030504@linux.vnet.ibm.com> <55A3D5D8.7070902@redhat.com> <55A654D6.5000906@linux.vnet.ibm.com> <55A657ED.3070407@redhat.com> <55A7BDF4.4020509@linux.vnet.ibm.com> <55B0BAE6.3040504@redhat.com> In-Reply-To: <55B0BAE6.3040504@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 1/5] cpu: Provide vcpu throttling interface Reply-To: jjherne@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , afaerber@suse.de, amit.shah@redhat.com, dgilbert@redhat.com, borntraeger@de.ibm.com, quintela@redhat.com, qemu-devel@nongnu.org On 07/23/2015 05:59 AM, Paolo Bonzini wrote: > > > On 16/07/2015 16:21, Jason J. Herne wrote: >> 1. Using atomic operations to manage throttle_percentage. I'm not sure >> where atomics are applicable here. If this is still a concern hopefully >> someone can explain. > > I would use atomic_read/atomic_set in cpu_throttle_set, > cpu_throttle_stop, cpu_throttle_active, cpu_throttle_get_percentage. > In addition, the function naming seems to be a bit inconsistent: please > rename cpu_throttle_set to cpu_throttle_set_percentage. > > Second, here: > >>> +static void cpu_throttle_thread(void *opaque) >>> +{ >>> + double pct = (double)throttle_percentage/100; > > Please use cpu_throttle_get_percentage(), and > >>> + double throttle_ratio = pct / (1 - pct); >>> + long sleeptime_ms = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE); > > ... move these computations below the if. > > I'm also not sure about throttle_ratio, why is it needed? If pct >= 0.5 you > end up with throttle_ratio >= 1, i.e. no way for the CPU to do any work. This > would definitely cause a problem with callbacks piling up. > Throttle ratio is relative to CPU_THROTTLE_TIMESLICE. Take a look at how throttle_ratio is used in the calculation: long sleeptime_ms = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE); A value of 1 means we sleep the same amount of time that we execute. >>> + if (!throttle_percentage) { >>> + return; >>> + } >>> + >>> + qemu_mutex_unlock_iothread(); >>> + g_usleep(sleeptime_ms * 1000); /* Convert ms to us for usleep call */ >>> + qemu_mutex_lock_iothread(); >>> +} >>> + > >> 2. Callback stacking. And it seems like we are convinced that it is not >> a big issue. Anyone disagree? > > I think it's not a big issue to have many timers, but it is a big issue to have many callbacks. What I suggested is this: > > if (!atomic_xchg(&cpu->throttle_thread_scheduled, 1)) { > async_run_on_cpu(cpu, cpu_throttle_thread, NULL); > } > > and in the callback: > > atomic_set(&cpu->throttle_thread_scheduled, 0); > g_usleep(...); > > Paolo > > -- -- Jason J. Herne (jjherne@linux.vnet.ibm.com)