From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38191) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XhCa9-0005Zt-Dk for qemu-devel@nongnu.org; Thu, 23 Oct 2014 03:13:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XhCa5-0004BL-7Z for qemu-devel@nongnu.org; Thu, 23 Oct 2014 03:13:13 -0400 Message-ID: <5448AA75.4000902@redhat.com> Date: Thu, 23 Oct 2014 09:12:53 +0200 From: Max Reitz MIME-Version: 1.0 References: <201410211604116199416@sangfor.com> <5446265E.306@redhat.com> <54482D38.5080904@redhat.com> <20141023070325.GA3522@noname.redhat.com> In-Reply-To: <20141023070325.GA3522@noname.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] snapshot: use local variable to bdrv_pwrite_sync L1 table List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , Eric Blake Cc: qemu-trivial , Zhang Haoyu , qemu-devel , Stefan Hajnoczi On 2014-10-23 at 09:03, Kevin Wolf wrote: > Am 23.10.2014 um 00:18 hat Eric Blake geschrieben: >> On 10/21/2014 03:24 AM, Max Reitz wrote: >>> On 2014-10-21 at 10:04, Zhang Haoyu wrote: >>>> Use local variable to bdrv_pwrite_sync L1 table, >>>> needless to make conversion of cached L1 table between >>>> big-endian and host style. >>>> >>>> Signed-off-by: Zhang Haoyu >>>> --- >>>> block/qcow2-refcount.c | 22 +++++++--------------- >>>> 1 file changed, 7 insertions(+), 15 deletions(-) >>>> >> I know we're up to v5 and that Max already took it into his branch, but... >> >> >>>> l1_size2 = l1_size * sizeof(uint64_t); >>>> + l1_table = g_try_malloc0(align_offset(l1_size2, 512)); >>> I wanted to propose using qemu_try_blockalign(), but since it'd require >>> a memset() afterwards, it gets rather ugly. >> Not after this recent patch: >> >> https://lists.gnu.org/archive/html/qemu-devel/2014-10/msg02499.html > We can switch to qemu_try_blockalign0() in a follow-up patch. > > But this is far from being a fast path, so there is very little to gain > anyway. We don't need the 0 variant anyway. In v5, the one I applied, it's just qemu_try_blockalign(). The size does not have to be aligned to BDRV_SECTOR_SIZE, since any access index is below l1_size and only bdrv_pread() and bdrv_pwrite() are used, so we can first omit the align_offset(). Second, all the rest (0 .. l1_size - 1) is overwritten either by memcpy() or bdrv_pread(). Therefore, no 0-ing required and qemu_try_blockalign() was fine. There are things we can do in follow-up patches to optimize v5 (getting read of the memcpy()s), but as Kevin said (and myself before that in reply to v5), this function is not performance-critical, so there's no real point in doing so (other than "Clearly unoptimized code makes my fingers twitch"). Max