From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KQNZn-0007v6-US for qemu-devel@nongnu.org; Tue, 05 Aug 2008 10:31:52 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KQNZm-0007uU-Sy for qemu-devel@nongnu.org; Tue, 05 Aug 2008 10:31:51 -0400 Received: from [199.232.76.173] (port=49605 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KQNZm-0007uL-Jx for qemu-devel@nongnu.org; Tue, 05 Aug 2008 10:31:50 -0400 Received: from ns.suse.de ([195.135.220.2]:35991 helo=mx1.suse.de) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1KQNL9-0003Go-Qn for qemu-devel@nongnu.org; Tue, 05 Aug 2008 10:16:44 -0400 Message-ID: <48986078.6060701@suse.de> Date: Tue, 05 Aug 2008 16:15:20 +0200 From: Kevin Wolf MIME-Version: 1.0 Subject: Re: [Qemu-devel] [patch 1/5][v2] Extract code from get_cluster_offset() References: <20080729141352.573798859@bull.net> <20080729141447.164763280@bull.net> In-Reply-To: <20080729141447.164763280@bull.net> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laurent Vivier Cc: qemu-devel@nongnu.org Laurent Vivier schrieb: > Extract code from get_cluster_offset() into new functions: > > - seek_l2_table() > > Search an l2 offset in the l2_cache table. > > - l2_load() > > Read the l2 entry from disk > > - l2_allocate() > > Allocate a new l2 entry. > > Signed-off-by: Laurent Vivier Acked-by: Kevin Wolf However, unrelated to your patch, I noticed that you touch code which could use more comments in some places. (Oh btw, why do you explain the new functions in the patch header but not in the code itself? But okay, these are more or less obvious.) > +static uint64_t *seek_l2_table(BDRVQcowState *s, uint64_t l2_offset) > +{ > + int i,j; > + > + for(i = 0; i < L2_CACHE_SIZE; i++) { > + if (l2_offset == s->l2_cache_offsets[i]) { > + /* increment the hit count */ > + if (++s->l2_cache_counts[i] == 0xffffffff) { > + for(j = 0; j < L2_CACHE_SIZE; j++) { > + s->l2_cache_counts[j] >>= 1; > + } > + } This if block is one of them. While it's quite obvious that some counter is incremented, the action to avoid an overflow isn't. Why don't we do something like if (count < 0xffffffff) count++? And l2_cache_counts is a bad name anyway, IMHO. I first thought of something like a reference counter (and then it would be definitely worth a comment; now it is debatable). l2_cache_hits could be a better name. > + if (!(l2_offset & QCOW_OFLAG_COPIED) && allocate) { > + free_clusters(bs, l2_offset, s->l2_size * sizeof(uint64_t)); > + ret = l2_allocate(bs, l1_index, &l2_table, &l2_offset); > + if (ret == 0) > + return 0; The assumption is that allocate != 0 means there will be a write operation and the cluster has to be copied, right? You certainly can figure that out from the code, but it would be nice to have a comment saying exactly this and that free_clusters is used to decrease the refcount of the copied cluster. Agreed, having written this, both are not really that bad. But it were the places where I needed a bit more time to understand what the code is doing and if it's right, so I thought I'd mention them. Kevin