From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takuya Yoshikawa Subject: Re: [RFC please check] KVM: question about the commit "Use Little Endian for Dirty Bitmap" Date: Thu, 22 Apr 2010 13:12:17 +0900 Message-ID: <4BCFCCA1.3020000@oss.ntt.co.jp> References: <20100421150720.16516cb7.yoshikawa.takuya@oss.ntt.co.jp> <4BCEC4AD.6060305@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Avi Kivity , mtosatti-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, kvm-ppc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Alexander Graf Return-path: In-Reply-To: Sender: kvm-ppc-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: kvm.vger.kernel.org >>> So please explain me about the commit: >>> 1. is this really the thing you intended to do? >>> >> >> I think so. >> >>> 2. including directly is OK? >>> -- I made a sample patch to avoid this, see below. >>> >> >> I don't see a problem with it, it is also included from other places. >> >> It might be possible to change it to, not sure how the include paths are set out. > > That's the great thing about being in-kernel :-). > >> >>> 3. or, I misunderstand something about Alex's comment? >>> >> >> I missed the comment. What was it? > > What comment? The reason was that longs and big endian don't match and I wanted to make it LE. This seemed like the right function to use, as other code in Linux uses it too. > This one! === > +static int __mark_page_dirty(unsigned long nr, > > + unsigned long *dirty_bitmap) > > +{ > > +#ifdef __BIG_ENDIAN > > + nr = nr ^ BITOP_LE_SWIZZLE; Why an XOR here? Also is this LE set_bit new? I didn't see it when I did the patch back then :). === I confused that "LE set_bit" is about the current generic___set_le_bit(). So I felt that "then, who introduced this set le bit actually?" Now, it's OK about it, thanks :). But I still feel that __set_le_bit_user() will be too KVM specific helper. Then, I have one idea: while looking around the bitops headers, I noticed that ppc bitops.h has exact copy of the *_le_bit definitions which are in the asm-generic le.h -- just for its internal use. So I'll check whether we can include le.h in ppc bitops.h for cleanup and during that work, define the "(nr) ^ BITOP_LE_SWIZZLE" part as separate macro like le_bit_offset. I don't know this will be accepted but this sounds little bit more generic: and we can reduce chances that we have to ask other maintainers to merge helpers. > Alex >