From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54606) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SUhjO-0003tu-3i for qemu-devel@nongnu.org; Wed, 16 May 2012 13:09:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SUhjI-0006bM-RA for qemu-devel@nongnu.org; Wed, 16 May 2012 13:09:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:24024) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SUhjI-0006b0-Ir for qemu-devel@nongnu.org; Wed, 16 May 2012 13:09:40 -0400 Message-ID: <4FB3DE10.6090506@redhat.com> Date: Wed, 16 May 2012 20:04:16 +0300 From: Orit Wasserman MIME-Version: 1.0 References: <1337169582-28312-1-git-send-email-owasserm@redhat.com> <1337169582-28312-9-git-send-email-owasserm@redhat.com> <4FB3D9A7.2060704@redhat.com> In-Reply-To: <4FB3D9A7.2060704@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v10 8/9] Add set_cachesize command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: aliguori@us.ibm.com, quintela@redhat.com, stefanha@gmail.com, mdroth@linux.vnet.ibm.com, qemu-devel@nongnu.org, blauwirbel@gmail.com, Petter Svard , Benoit Hudzia , avi@redhat.com, pbonzini@redhat.com, Aidan Shribman On 05/16/2012 07:45 PM, Eric Blake wrote: > On 05/16/2012 05:59 AM, Orit Wasserman wrote: >> Change XBZRLE cache size in bytes (the size should be a power of 2). >> If XBZRLE cache size is too small there will be many cache miss. >> >> Signed-off-by: Benoit Hudzia >> Signed-off-by: Petter Svard >> Signed-off-by: Aidan Shribman >> Signed-off-by: Orit Wasserman > >> >> + >> +void xbzrle_cache_resize(int64_t order) >> +{ >> + cache_resize(XBZRLE.cache, pow(2, order)); > > '1 << order' is much more efficient than a call to pow(). ok > > >> +void qmp_migrate_set_cachesize(int64_t value, Error **errp) > >> + >> + /* power of 2 */ >> + if (value != 1 && !is_power_of_2(value)) { >> + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size", >> + "needs to be power of 2"); > > We already have QERR_PROPERTY_VALUE_NOT_POWER_OF_2, why aren't you using > that here? I will update it. > >> + return; >> + } >> + >> + s->xbzrle_cache_size = value; >> + xbzrle_cache_resize(log2(value)); > > log2() is rather expensive, ffs() from is more efficient at > converting a single bit into the appropriate order. ok > > >> ## >> +# @migrate_set_cachesize >> +# >> +# Set XBZRLE cache size >> +# >> +# @value: cache size in bytes >> +# >> +# Returns: nothing on success > > Document the error for a non-power-of-2 or for overflow. > > Document whether this command is safe for an ongoing migration, or > whether it must be called in advance of a migration. sure > >> +# >> +# Since: 1.1 > > 1.2. > > >> +static inline bool is_power_of_2(int64_t value) >> +{ >> + return !(value & (value - 1)); >> +} > > This says '0' is a power of 2, which is not true. Either fix the logic > to exclude 0, or fix the function name to state that you are really > checking that at most one bit is set. > > Also, if value is 0x8000000000000000, you are triggering unspecified > behavior per C99. Is it worth using uint64_t for defined behavior, or > do you need to take precautions regarding negative values? The input is int64 so I prefer to keep it this way. The calling function does the check for 0 , negative numbers and overflow but I can add those checks here too. > > >> +SQMP >> +migrate_set_cachesize >> +--------------------- >> + >> +Set cache size to be used by XBZRLE migration >> + >> +Arguments: >> + >> +- "value": cache size in bytes (json-int) > > Would it be any easier to take 'order' (log2 of the size) instead of the > actual cache size? That is, instead of calling "value":1048576, I would > rather type "value":20. Well the user is considering how much memory is going to be used and I though that it is simpler to use 1G than 30. But I guess the user is libvirt so it can be changed to order. > >> + >> +Example: >> + >> +-> { "execute": "migrate_set_cachesize", "arguments": { "value": 512 } } > > Isn't 512 bytes rather small? And given my argument about taking order > rather than bytes as being easier to use, don't you really mean 512 > megabytes (order 29) rather than 512 bytes (order 9)? > correct 512M not bytes ... Orit