From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yoshiaki Tamura Subject: Re: [PATCH v2 3/6] Modifies wrapper functions for byte-based phys_ram_dirty bitmap to bit-based phys_ram_dirty bitmap. Date: Tue, 13 Apr 2010 17:01:40 +0900 Message-ID: <4BC424E4.7030609@lab.ntt.co.jp> References: <1270515084-24120-1-git-send-email-tamura.yoshiaki@lab.ntt.co.jp> <1270515084-24120-4-git-send-email-tamura.yoshiaki@lab.ntt.co.jp> <4BC2D562.7050507@redhat.com> <4BC2FCDF.2000909@lab.ntt.co.jp> <4BC2FF77.8020603@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, qemu-devel@nongnu.org, anthony@codemonkey.ws, aliguori@us.ibm.com, mtosatti@redhat.com, ohmura.kei@lab.ntt.co.jp To: Avi Kivity Return-path: Received: from tama500.ecl.ntt.co.jp ([129.60.39.148]:44742 "EHLO tama500.ecl.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750944Ab0DMICM (ORCPT ); Tue, 13 Apr 2010 04:02:12 -0400 In-Reply-To: <4BC2FF77.8020603@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Avi Kivity wrote: > On 04/12/2010 01:58 PM, Yoshiaki Tamura wrote: >>> Is it necessary to update migration and vga bitmaps? >>> >>> We can simply update the master bitmap, and update the migration and vga >>> bitmaps only when they need it. That can be done in a different patch. >> >> >> Let me explain the role of the master bitmap here. >> >> In the previous discussion, the master bitmap represented at least one >> dirty type is actually dirty. While implementing this approach, I >> though there is one downside that upon resetting the bitmap, it needs >> to check all dirty types whether they are already cleared to unset the >> master bitmap, and this might hurt the performance in general. > > The way I see it, the master bitmap is a buffer between the guest and > all the other client bitmaps (migration and vga). So, a bit can be set > in vga and cleared in master. Let's look at the following scenario: > > 1. guest touches address 0xa0000 (page 0xa0) > 2. qemu updates master bitmap bit 0xa0 > 3. vga timer fires > 4. vga syncs the dirty bitmap for addresses 0xa0000-0xc0000 > 4.1 vga_bitmap[0xa0-0xc0] |= master_bitmap[0xa0-0xc0] > 4.2 migration_bitmap[0xa0-0xc0] |= master_bitmap[0xa0-0xc0] > 4.3 master_bitmap[0xa0-0xc0] = 0 > 5. vga draws based on vga_bitmap > > the nice thing is that step 4.2 can be omitted if migration is not active. > > so, client bitmaps are always updated when the client requests them, > master bitmap is only a buffer. Thanks. I think I'm finally getting your point. At 4.2 and 4.3, is syncing to client necessary? Simply doing OR at get_dirty like in this patch not enough? >> In this patch, master bitmap represents all types are dirty, similar >> to existing 0xff. With this approach, resetting the master bitmap can >> be done without checking the other types. set_dirty_flags is actually >> taking the burden in this case though. Anyway, IIUC somebody would be >> unhappy depending on the role of the master bitmap. > > Yes, the problem is that set_dirty_flags is the common case and also > uses random access, we'd want to make it touch only a single bit. client > access is rare, and also sequential, and therefore efficient. I see. So set_dirty_flags would be like, 1. set master first regard less of dirty type. 2. if dirty type was CODE, set the client. >>> Note that we should only allocate the migration and vga bitmaps when >>> migration or vga is active. >> >> Right. May I do it in a different patch? > > Sure, more patches is usually better. > >> I think this is about optimization. In this patch, I focused on not >> breaking the existing function. > > Yes, that's the best approach. But we do need to see how all the > incremental steps end up. I totally agree. Thanks for helping. In summary, I'll prepare two patch sets. 1. v3 of this patch set. - Change FLAGS value to (1,2,4,8), and add IDX (0,1,2,3) - Use ffsl to convert FLAGS to IDX. - Add help function which takes IDX. - Change the behavior of set_dirty_flags as above. - Change dirty bitmap access to a loop. - Add brace after if () - Move some macros to qemu-common.h. 2. Allocate vga and migration dirty bitmap dynamically. Please modify or add items if I were missing.