From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:60616) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SmqN5-0003M5-Ok for qemu-devel@nongnu.org; Thu, 05 Jul 2012 14:01:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SmqN3-00062A-Nr for qemu-devel@nongnu.org; Thu, 05 Jul 2012 14:01:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:18757) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SmqN3-00061X-GW for qemu-devel@nongnu.org; Thu, 05 Jul 2012 14:01:41 -0400 Message-ID: <4FF5D667.6070300@redhat.com> Date: Thu, 05 Jul 2012 21:01:11 +0300 From: Orit Wasserman MIME-Version: 1.0 References: <1341492709-13897-1-git-send-email-owasserm@redhat.com> <1341492709-13897-7-git-send-email-owasserm@redhat.com> <4FF59CD8.4080103@redhat.com> In-Reply-To: <4FF59CD8.4080103@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v15 6/9] Add xbzrle_encode_buffer and xbzrle_decode_buffer functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: peter.maydell@linaro.org, aliguori@us.ibm.com, quintela@redhat.com, stefanha@gmail.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com, blauwirbel@gmail.com, Petter Svard , Benoit Hudzia , avi@redhat.com, Aidan Shribman , pbonzini@redhat.com, chegu_vinod@hp.com On 07/05/2012 04:55 PM, Eric Blake wrote: > On 07/05/2012 06:51 AM, Orit Wasserman wrote: > > This commit message is a bit sparse. I'd document at least the fact > that our nzrun detection code in xbzrle_encode_buffer borrows > long-word-at-a-time NUL-detection tricks from strcmp(), as it is not an > intuitive trick known by all developers. > >> Signed-off-by: Benoit Hudzia >> Signed-off-by: Petter Svard >> Signed-off-by: Aidan Shribman >> Signed-off-by: Orit Wasserman > > I think I touched this one heavily enough that it warrants adding: > > Signed-off-by: Eric Blake > Of course Orit > Other than the commit message, I'm happy with this patch contents now. > Still some nits, though: > >> + >> + /* not aligned to sizeof(long) */ >> + res = (slen - i) % sizeof(long); > > Comment indentation is off. > >> + while (res && old_buf[i] == new_buf[i]) { >> + zrun_len++; >> + i++; >> + res--; >> + } >> + >> + if (!res) { > > A comment here might help: > > /* word at a time for speed */ > >> + while (i < slen && >> + (*(long *)(old_buf + i)) == (*(long *)(new_buf + i))) { > > On re-reading this, I'm worried whether it violates strict C99 aliasing > rules; I'm hoping the compiler doesn't mis-optimize it based on a > loophole of us using questionable semantics. In practice, I'm confident > that we are only doing aligned reads (otherwise I wouldn't have > suggested this line in the first place), which is why I'm personally > okay with it even if it technically violates C99. However, I'm open to > suggestions from anyone else on the list on how to better express the > notion of intentionally accessing a long at a time from an aligned > offset into a byte array, if that will help us avoid even theoretical > problems. This is a very interesting question .... Orit > >> + nzrun_len++; >> + res--; >> + } >> + >> + if (!res) { >> + /* truncation to 32-bit long okay */ > > Again, a longer comment might help: > > /* word at a time for speed, use of 32-bit long okay */ > >> + long mask = 0x0101010101010101ULL; >> + while (i < slen) { >> + xor = *(long *)(old_buf + i) ^ *(long *)(new_buf + i); > > Same potential strict aliasing problems as for zrun detection. >