[RFC please check] KVM: question about the commit "Use Little Endian for Dirty Bitmap"

[RFC please check] KVM: question about the commit "Use Little Endian for Dirty Bitmap"

[Takuya Yoshikawa]

Hi, Alex, Avi, Marcelo

I would like to ask you about the commit:
  c8240bd6f0b4b1b21ffd36dd44114d05c7afe0c0 "Use Little Endian for Dirty Bitmap"
I pasted a snipet below!

I am now confused by the Alex's comment to my recent patch:
  "change mark_page_dirty() to handle endian issues explicitly"
in which I open-coded the generic___set_le_bit().

So please explain me about the commit:
  1. is this really the thing you intended to do?
  2. including <asm-generic/bitops/le.h> directly is OK?
     -- I made a sample patch to avoid this, see below.

  3. or, I misunderstand something about Alex's comment?

Thanks,
  Takuya


=== snipet from commit c8240bd6f0b4b1b21ffd36dd44114d05c7afe0c0 ===
@@ -48,6 +48,7 @@
 #include <asm/io.h>
 #include <asm/uaccess.h>
 #include <asm/pgtable.h>
+#include <asm-generic/bitops/le.h>
 
 #ifdef KVM_COALESCED_MMIO_PAGE_OFFSET
 #include "coalesced_mmio.h"
@@ -1665,8 +1666,8 @@ void mark_page_dirty(struct kvm *kvm, gfn_t gfn)
 		unsigned long rel_gfn = gfn - memslot->base_gfn;
 
 		/* avoid RMW */
-		if (!test_bit(rel_gfn, memslot->dirty_bitmap))
-			set_bit(rel_gfn, memslot->dirty_bitmap);
+		if (!generic_test_le_bit(rel_gfn, memslot->dirty_bitmap))
+			generic___set_le_bit(rel_gfn, memslot->dirty_bitmap);
 	}
 }

===


=== not tested ===
[PATCH sample] KVM: avoid to include an asm-generic bitops header file directly

Including asm-generic bitops headers is kind of violation: there is no guarantee that
no one will change those functions we are using.

Signed-off-by: Takuya Yoshikawa <yoshikawatky-V2f5J/ArDKdQWUn/WH5d2uJTCWgwaB5pzw2xav70ESE@public.gmane.org>
---
 virt/kvm/kvm_main.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 3725605..f029760 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -51,7 +51,6 @@
 #include <asm/io.h>
 #include <asm/uaccess.h>
 #include <asm/pgtable.h>
-#include <asm-generic/bitops/le.h>
 
 #include "coalesced_mmio.h"
 
@@ -1179,6 +1178,18 @@ int kvm_clear_guest(struct kvm *kvm, gpa_t gpa, unsigned long len)
 }
 EXPORT_SYMBOL_GPL(kvm_clear_guest);
 
+/*
+ * __set_le_bit is defined for ppc only.
+ */
+static void kvm___set_le_bit(unsigned long nr, unsigned long *addr)
+{
+#ifdef __set_le_bit
+	__set_le_bit(nr, addr);
+#else
+	__set_bit(nr, addr);
+#endif
+}
+
 void mark_page_dirty(struct kvm *kvm, gfn_t gfn)
 {
 	struct kvm_memory_slot *memslot;
@@ -1188,9 +1199,7 @@ void mark_page_dirty(struct kvm *kvm, gfn_t gfn)
 	if (memslot && memslot->dirty_bitmap) {
 		unsigned long rel_gfn = gfn - memslot->base_gfn;
 
-		/* avoid RMW */
-		if (!generic_test_le_bit(rel_gfn, memslot->dirty_bitmap))
-			generic___set_le_bit(rel_gfn, memslot->dirty_bitmap);
+		kvm___set_le_bit(rel_gfn, memslot->dirty_bitmap);
 	}
 }
 
-- 
1.6.3.3

Re: [RFC please check] KVM: question about the commit "Use Little Endian for Dirty Bitmap"

[Takuya Yoshikawa]

(2010/04/21 15:07), Takuya Yoshikawa wrote:

> === not tested ===
> [PATCH sample] KVM: avoid to include an asm-generic bitops header file directly
>
> Including asm-generic bitops headers is kind of violation: there is no guarantee that
> no one will change those functions we are using.
>
> Signed-off-by: Takuya Yoshikawa<yoshikawatky-V2f5J/ArDKdQWUn/WH5d2uJTCWgwaB5pzw2xav70ESE@public.gmane.org>
Oh, I forgot to drop this line: this is made on my machine with not configured git.

Please ignore this Signed-off-by, the address is not valid.






> ---
>   virt/kvm/kvm_main.c |   17 +++++++++++++----
>   1 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 3725605..f029760 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -51,7 +51,6 @@
>   #include<asm/io.h>
>   #include<asm/uaccess.h>
>   #include<asm/pgtable.h>
> -#include<asm-generic/bitops/le.h>
>
>   #include "coalesced_mmio.h"
>
> @@ -1179,6 +1178,18 @@ int kvm_clear_guest(struct kvm *kvm, gpa_t gpa, unsigned long len)
>   }
>   EXPORT_SYMBOL_GPL(kvm_clear_guest);
>
> +/*
> + * __set_le_bit is defined for ppc only.
> + */
> +static void kvm___set_le_bit(unsigned long nr, unsigned long *addr)
> +{
> +#ifdef __set_le_bit
> +	__set_le_bit(nr, addr);
> +#else
> +	__set_bit(nr, addr);
> +#endif
> +}
> +
>   void mark_page_dirty(struct kvm *kvm, gfn_t gfn)
>   {
>   	struct kvm_memory_slot *memslot;
> @@ -1188,9 +1199,7 @@ void mark_page_dirty(struct kvm *kvm, gfn_t gfn)
>   	if (memslot&&  memslot->dirty_bitmap) {
>   		unsigned long rel_gfn = gfn - memslot->base_gfn;
>
> -		/* avoid RMW */
> -		if (!generic_test_le_bit(rel_gfn, memslot->dirty_bitmap))
> -			generic___set_le_bit(rel_gfn, memslot->dirty_bitmap);
> +		kvm___set_le_bit(rel_gfn, memslot->dirty_bitmap);
>   	}
>   }
>

Re: [RFC please check] KVM: question about the commit "Use Little Endian for Dirty Bitmap"

[Avi Kivity]

On 04/21/2010 09:07 AM, Takuya Yoshikawa wrote:
> Hi, Alex, Avi, Marcelo
>
> I would like to ask you about the commit:
>    c8240bd6f0b4b1b21ffd36dd44114d05c7afe0c0 "Use Little Endian for Dirty Bitmap"
> I pasted a snipet below!
>
> I am now confused by the Alex's comment to my recent patch:
>    "change mark_page_dirty() to handle endian issues explicitly"
> in which I open-coded the generic___set_le_bit().
>
> So please explain me about the commit:
>    1. is this really the thing you intended to do?
>    

I think so.

>    2. including<asm-generic/bitops/le.h>  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 <asm/bitops/le.h>, not sure how the 
include paths are set out.

>    3. or, I misunderstand something about Alex's comment?
>    

I missed the comment.  What was it?

-- 
error compiling committee.c: too many arguments to function

Re: [RFC please check] KVM: question about the commit "Use Little Endian for Dirty Bitmap"

[Alexander Graf]

On 21.04.2010, at 11:26, Avi Kivity wrote:

> On 04/21/2010 09:07 AM, Takuya Yoshikawa wrote:
>> Hi, Alex, Avi, Marcelo
>> 
>> I would like to ask you about the commit:
>>   c8240bd6f0b4b1b21ffd36dd44114d05c7afe0c0 "Use Little Endian for Dirty Bitmap"
>> I pasted a snipet below!
>> 
>> I am now confused by the Alex's comment to my recent patch:
>>   "change mark_page_dirty() to handle endian issues explicitly"
>> in which I open-coded the generic___set_le_bit().
>> 
>> So please explain me about the commit:
>>   1. is this really the thing you intended to do?
>>   
> 
> I think so.
> 
>>   2. including<asm-generic/bitops/le.h>  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 <asm/bitops/le.h>, 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.

Alex

Re: [RFC please check] KVM: question about the commit "Use Little Endian for Dirty Bitmap"

[Takuya Yoshikawa]

>>> So please explain me about the commit:
>>>    1. is this really the thing you intended to do?
>>>
>>
>> I think so.
>>
>>>    2. including<asm-generic/bitops/le.h>   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<asm/bitops/le.h>, 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
>