* [PATCH] drivers: hv: use Linux version in guest ID
From: Bjørn Mork @ 2012-07-20 8:03 UTC (permalink / raw)
To: KY Srinivasan
Cc: Greg KH, Paolo Bonzini, devel, linux-kernel, virtualization,
Bjørn Mork
In-Reply-To: <87fw8mlv7e.fsf@nemi.mork.no>
Use OS version number in the guest ID as recommended by Microsoft
Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
So how about something like this? Yes, I know including
linux/version.h often is unwanted, but the spec does
recommend using the actual OS version in the guest ID
Yes, this will be offensive again when Linux is at
version 181.0, but we might have found a better solution by
then :-)
drivers/hv/hyperv_vmbus.h | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index b9426a6..cc0c3f0 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -29,6 +29,7 @@
#include <asm/sync_bitops.h>
#include <linux/atomic.h>
#include <linux/hyperv.h>
+#include <linux/version.h>
/*
* The below CPUID leaves are present if VersionAndFeatures.HypervisorPresent
@@ -410,10 +411,21 @@ enum {
#define HV_PRESENT_BIT 0x80000000
-#define HV_LINUX_GUEST_ID_LO 0x00000000
-#define HV_LINUX_GUEST_ID_HI 0xB16B00B5
-#define HV_LINUX_GUEST_ID (((u64)HV_LINUX_GUEST_ID_HI << 32) | \
- HV_LINUX_GUEST_ID_LO)
+/* Linux vendor ID allocated by Microsoft */
+#define HV_LINUX_GUEST_ID_VENDOR 0xB16B
+
+/* Creating a guest ID conforming to the encoding recommended by
+ * Microsoft, with variant and build fixed to 0
+ *
+ * 63:48 - Vendor ID
+ * 47:40 - OS Variant
+ * 39:32 - Major Version
+ * 31:24 - Minor Version
+ * 23:16 - Service Version
+ * 15:0 - Build Number
+ */
+#define HV_LINUX_GUEST_ID ((u64)HV_LINUX_GUEST_ID_VENDOR << 48 | \
+ (u64)LINUX_VERSION_CODE << 16)
#define HV_CPU_POWER_MANAGEMENT (1 << 0)
#define HV_RECOMMENDATIONS_MAX 4
--
1.7.10.4
^ permalink raw reply related
* Re: 0xB16B00B5? Really? (was Re: Move hyperv out of the drivers/staging/ directory)
From: Bjørn Mork @ 2012-07-20 7:59 UTC (permalink / raw)
To: KY Srinivasan
Cc: Greg KH (gregkh@linuxfoundation.org), Paolo Bonzini,
devel@linuxdriverproject.org, linux-kernel@vger.kernel.org,
virtualization@lists.osdl.org
In-Reply-To: <426367E2313C2449837CD2DE46E7EAF9235476BC@SN2PRD0310MB382.namprd03.prod.outlook.com>
KY Srinivasan <kys@microsoft.com> writes:
> Here is the link that describes how the guest ID should be composed:
>
> http://msdn.microsoft.com/en-us/library/windows/hardware/ff542653%28v=vs.85%29.aspx
The unwanted value does not conform to those rules: Linux is not yet at
major version 0xB5.
I propose that you change this into a macro actually taking the OS
major/minor version into account, keeping the 0xB16B vendor ID (which
seems safest as the vendor ID registration process seems a bit unclear)
Bjørn
^ permalink raw reply
* RE: 0xB16B00B5? Really? (was Re: Move hyperv out of the drivers/staging/ directory)
From: KY Srinivasan @ 2012-07-20 0:41 UTC (permalink / raw)
To: Greg KH (gregkh@linuxfoundation.org)
Cc: Paolo Bonzini, devel@linuxdriverproject.org,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <20120719232832.GA6317@kroah.com>
> -----Original Message-----
> From: Greg KH (gregkh@linuxfoundation.org)
> [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, July 19, 2012 7:29 PM
> To: KY Srinivasan
> Cc: Paolo Bonzini; devel@linuxdriverproject.org; linux-kernel@vger.kernel.org;
> virtualization@lists.osdl.org
> Subject: Re: 0xB16B00B5? Really? (was Re: Move hyperv out of the
> drivers/staging/ directory)
>
> On Thu, Jul 19, 2012 at 10:30:38PM +0000, KY Srinivasan wrote:
> > > > As you know, this ID has been in use for a long time now. While the
> hypervisor
> > > > does not interpret the guest ID that is registered, I am not sure what
> > > dependencies
> > > > there might be on this value.
> > >
> > > Could you please go find out the answer to this?
> >
> > That is easier said than done. I have sent emails out asking this very question
> and I have
> > not received a definitive answer yet. Not knowing if and when I can get a
> definitive
> > answer here, I chose the least risky approach in my patch.
>
> What happens if you test with different values?
Nothing and that is not the issue. Current MSFT hypervisors don't interpret this
ID value while future versions might. However, this ID can be retrieved by the parent
partition and can be used by the management stack today (that is what I am told).
>
> > > If, as you originally stated, there is a range of values we can use,
> > > then we should probably use another one, right?
> >
> > On the Windows side this ID namespace is managed well.
>
> It is? How is this managed? What does this tell the hypervisor? What
> changes with the different values?
>
> > However on the Linux side, we have really had this current ID in use
> > for almost five years now. I am not aware of any pool of IDs available
> > for Linux usage except that Linux IDs be distinct from the guest IDs
> > in use by MSFT operating systems. If I were to change the guest ID, I
> > would probably want to comply with the MSFT guidance on constructing
> > these IDs (although not all fields may be relevant for Linux).
>
> What are those rules?
Here is the link that describes how the guest ID should be composed:
http://msdn.microsoft.com/en-us/library/windows/hardware/ff542653%28v=vs.85%29.aspx
Regards,
K. Y
^ permalink raw reply
* RE: 0xB16B00B5? Really? (was Re: Move hyperv out of the drivers/staging/ directory)
From: KY Srinivasan @ 2012-07-20 0:37 UTC (permalink / raw)
To: Anthony Liguori
Cc: Greg KH (gregkh@linuxfoundation.org), Paolo Bonzini,
devel@linuxdriverproject.org, linux-kernel@vger.kernel.org,
virtualization@lists.osdl.org
In-Reply-To: <500895AC.7020303@codemonkey.ws>
> -----Original Message-----
> From: Anthony Liguori [mailto:anthony@codemonkey.ws]
> Sent: Thursday, July 19, 2012 7:18 PM
> To: KY Srinivasan
> Cc: Greg KH (gregkh@linuxfoundation.org); Paolo Bonzini;
> devel@linuxdriverproject.org; linux-kernel@vger.kernel.org;
> virtualization@lists.osdl.org
> Subject: Re: 0xB16B00B5? Really? (was Re: Move hyperv out of the
> drivers/staging/ directory)
>
> On 07/19/2012 05:30 PM, KY Srinivasan wrote:
> >
> >
> >> -----Original Message-----
> >> From: Greg KH (gregkh@linuxfoundation.org)
> >> [mailto:gregkh@linuxfoundation.org]
> >> Sent: Thursday, July 19, 2012 6:02 PM
> >> To: KY Srinivasan
> >> Cc: Paolo Bonzini; devel@linuxdriverproject.org; linux-
> kernel@vger.kernel.org;
> >> virtualization@lists.osdl.org
> >> Subject: Re: 0xB16B00B5? Really? (was Re: Move hyperv out of the
> >> drivers/staging/ directory)
> >>
> >> On Thu, Jul 19, 2012 at 09:22:53PM +0000, KY Srinivasan wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Greg KH (gregkh@linuxfoundation.org)
> >>>> [mailto:gregkh@linuxfoundation.org]
> >>>> Sent: Thursday, July 19, 2012 5:07 PM
> >>>> To: KY Srinivasan
> >>>> Cc: Paolo Bonzini; devel@linuxdriverproject.org; linux-
> >> kernel@vger.kernel.org;
> >>>> virtualization@lists.osdl.org
> >>>> Subject: Re: 0xB16B00B5? Really? (was Re: Move hyperv out of the
> >>>> drivers/staging/ directory)
> >>>>
> >>>> On Thu, Jul 19, 2012 at 02:11:47AM +0000, KY Srinivasan wrote:
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of
> Paolo
> >>>>>> Bonzini
> >>>>>> Sent: Friday, July 13, 2012 6:23 AM
> >>>>>> To: KY Srinivasan
> >>>>>> Cc: Greg KH; devel@linuxdriverproject.org; linux-
> kernel@vger.kernel.org;
> >>>>>> virtualization@lists.osdl.org
> >>>>>> Subject: 0xB16B00B5? Really? (was Re: Move hyperv out of the
> >>>> drivers/staging/
> >>>>>> directory)
> >>>>>>
> >>>>>> Il 04/10/2011 21:34, Greg KH ha scritto:
> >>>>>>> diff --git a/drivers/staging/hv/hyperv_vmbus.h
> >>>> b/drivers/hv/hyperv_vmbus.h
> >>>>>>> similarity index 99%
> >>>>>>> rename from drivers/staging/hv/hyperv_vmbus.h
> >>>>>>> rename to drivers/hv/hyperv_vmbus.h
> >>>>>>> index 3d2d836..8261cb6 100644
> >>>>>>> --- a/drivers/staging/hv/hyperv_vmbus.h
> >>>>>>> +++ b/drivers/hv/hyperv_vmbus.h
> >>>>>>> @@ -28,8 +28,7 @@
> >>>>>>> #include<linux/list.h>
> >>>>>>> #include<asm/sync_bitops.h>
> >>>>>>> #include<linux/atomic.h>
> >>>>>>> -
> >>>>>>> -#include "hyperv.h"
> >>>>>>> +#include<linux/hyperv.h>
> >>>>>>>
> >>>>>>> /*
> >>>>>>> * The below CPUID leaves are present if
> >>>>>> VersionAndFeatures.HypervisorPresent
> >>>>>>
> >>>>>> git's rename detection snips away this gem:
> >>>>>>
> >>>>>> +#define HV_LINUX_GUEST_ID_LO 0x00000000
> >>>>>> +#define HV_LINUX_GUEST_ID_HI 0xB16B00B5
> >>>>>> +#define HV_LINUX_GUEST_ID
> (((u64)HV_LINUX_GUEST_ID_HI
> >>>>>> << 32) | \
> >>>>>> + HV_LINUX_GUEST_ID_LO)
> >>>>>>
> >>>>>> Somone was trying to be funny, I guess.
> >>>>>>
> >>>>>> KY, I suppose you have access to Hyper-V code or can ask someone who
> >>>> does.
> >>>>>> Is this signature actually used in the Hyper-V host code?
> >>>>>
> >>>>> Paolo,
> >>>>>
> >>>>> As I noted earlier, this is just a guest ID that needs to be registered with
> the
> >>>>> hypervisor. Thanks for reporting this issue and on behalf of Microsoft, I
> >> would
> >>>>> like to apologize for this offensive string. I have submitted a patch to fix
> this
> >>>> issue.
> >>>>
> >>>> You only changed it to be in decimal, you did not change the id at all.
> >>>> Is there some reason why you can not change it? You said there was a
> >>>> reserved range of ids that could be used, perhaps just pick another one?
> >>>> What is the valid range that can be used here?
> >>>
> >>> Greg,
> >>>
> >>> As you know, this ID has been in use for a long time now. While the
> hypervisor
> >>> does not interpret the guest ID that is registered, I am not sure what
> >> dependencies
> >>> there might be on this value.
> >>
> >> Could you please go find out the answer to this?
> >
> > That is easier said than done. I have sent emails out asking this very question
> and I have
> > not received a definitive answer yet. Not knowing if and when I can get a
> definitive
> > answer here, I chose the least risky approach in my patch.
> >>
> >> If, as you originally stated, there is a range of values we can use,
> >> then we should probably use another one, right?
> >
> > On the Windows side this ID namespace is managed well. However on the Linux
> > side, we have really had this current ID in use for almost five years now. I am
> not
> > aware of any pool of IDs available for Linux usage except that Linux IDs be
> distinct from
> > the guest IDs in use by MSFT operating systems. If I were to change the guest
> ID, I would
> > probably want to comply with the MSFT guidance on constructing these IDs
> (although not
> > all fields may be relevant for Linux).
>
> Presumably, Hyper-V can deal with unexpected values here, no? Otherwise, it
> wouldn't be future proof against new types of guests.
Anthony,
As I noted earlier, the current versions of MSFT hypervisors don't interpret the guest ID
value although future versions might. However, this ID may be retrieved from the
parent partition and can be used by the management stack (today).
>
> So worst case scenario, Hyper-V disables optimizations on Linux guests that
> report then new ID until they patch Hyper-V to know about the new ID.
>
> That seems like a reasonable trade off to me. I'm sure there's sufficient
> incentive to patch Hyper-V for this at Microsoft...
This is something I want to do once I am sure that nothing will break.
Regards,
K. Y
^ permalink raw reply
* Re: 0xB16B00B5? Really? (was Re: Move hyperv out of the drivers/staging/ directory)
From: Greg KH (gregkh@linuxfoundation.org) @ 2012-07-19 23:28 UTC (permalink / raw)
To: KY Srinivasan
Cc: Paolo Bonzini, devel@linuxdriverproject.org,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <426367E2313C2449837CD2DE46E7EAF923547620@SN2PRD0310MB382.namprd03.prod.outlook.com>
On Thu, Jul 19, 2012 at 10:30:38PM +0000, KY Srinivasan wrote:
> > > As you know, this ID has been in use for a long time now. While the hypervisor
> > > does not interpret the guest ID that is registered, I am not sure what
> > dependencies
> > > there might be on this value.
> >
> > Could you please go find out the answer to this?
>
> That is easier said than done. I have sent emails out asking this very question and I have
> not received a definitive answer yet. Not knowing if and when I can get a definitive
> answer here, I chose the least risky approach in my patch.
What happens if you test with different values?
> > If, as you originally stated, there is a range of values we can use,
> > then we should probably use another one, right?
>
> On the Windows side this ID namespace is managed well.
It is? How is this managed? What does this tell the hypervisor? What
changes with the different values?
> However on the Linux side, we have really had this current ID in use
> for almost five years now. I am not aware of any pool of IDs available
> for Linux usage except that Linux IDs be distinct from the guest IDs
> in use by MSFT operating systems. If I were to change the guest ID, I
> would probably want to comply with the MSFT guidance on constructing
> these IDs (although not all fields may be relevant for Linux).
What are those rules?
thanks,
greg k-h
^ permalink raw reply
* Re: 0xB16B00B5? Really? (was Re: Move hyperv out of the drivers/staging/ directory)
From: Anthony Liguori @ 2012-07-19 23:18 UTC (permalink / raw)
To: KY Srinivasan
Cc: Greg KH (gregkh@linuxfoundation.org), Paolo Bonzini,
devel@linuxdriverproject.org, linux-kernel@vger.kernel.org,
virtualization@lists.osdl.org
In-Reply-To: <426367E2313C2449837CD2DE46E7EAF923547620@SN2PRD0310MB382.namprd03.prod.outlook.com>
On 07/19/2012 05:30 PM, KY Srinivasan wrote:
>
>
>> -----Original Message-----
>> From: Greg KH (gregkh@linuxfoundation.org)
>> [mailto:gregkh@linuxfoundation.org]
>> Sent: Thursday, July 19, 2012 6:02 PM
>> To: KY Srinivasan
>> Cc: Paolo Bonzini; devel@linuxdriverproject.org; linux-kernel@vger.kernel.org;
>> virtualization@lists.osdl.org
>> Subject: Re: 0xB16B00B5? Really? (was Re: Move hyperv out of the
>> drivers/staging/ directory)
>>
>> On Thu, Jul 19, 2012 at 09:22:53PM +0000, KY Srinivasan wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Greg KH (gregkh@linuxfoundation.org)
>>>> [mailto:gregkh@linuxfoundation.org]
>>>> Sent: Thursday, July 19, 2012 5:07 PM
>>>> To: KY Srinivasan
>>>> Cc: Paolo Bonzini; devel@linuxdriverproject.org; linux-
>> kernel@vger.kernel.org;
>>>> virtualization@lists.osdl.org
>>>> Subject: Re: 0xB16B00B5? Really? (was Re: Move hyperv out of the
>>>> drivers/staging/ directory)
>>>>
>>>> On Thu, Jul 19, 2012 at 02:11:47AM +0000, KY Srinivasan wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
>>>>>> Bonzini
>>>>>> Sent: Friday, July 13, 2012 6:23 AM
>>>>>> To: KY Srinivasan
>>>>>> Cc: Greg KH; devel@linuxdriverproject.org; linux-kernel@vger.kernel.org;
>>>>>> virtualization@lists.osdl.org
>>>>>> Subject: 0xB16B00B5? Really? (was Re: Move hyperv out of the
>>>> drivers/staging/
>>>>>> directory)
>>>>>>
>>>>>> Il 04/10/2011 21:34, Greg KH ha scritto:
>>>>>>> diff --git a/drivers/staging/hv/hyperv_vmbus.h
>>>> b/drivers/hv/hyperv_vmbus.h
>>>>>>> similarity index 99%
>>>>>>> rename from drivers/staging/hv/hyperv_vmbus.h
>>>>>>> rename to drivers/hv/hyperv_vmbus.h
>>>>>>> index 3d2d836..8261cb6 100644
>>>>>>> --- a/drivers/staging/hv/hyperv_vmbus.h
>>>>>>> +++ b/drivers/hv/hyperv_vmbus.h
>>>>>>> @@ -28,8 +28,7 @@
>>>>>>> #include<linux/list.h>
>>>>>>> #include<asm/sync_bitops.h>
>>>>>>> #include<linux/atomic.h>
>>>>>>> -
>>>>>>> -#include "hyperv.h"
>>>>>>> +#include<linux/hyperv.h>
>>>>>>>
>>>>>>> /*
>>>>>>> * The below CPUID leaves are present if
>>>>>> VersionAndFeatures.HypervisorPresent
>>>>>>
>>>>>> git's rename detection snips away this gem:
>>>>>>
>>>>>> +#define HV_LINUX_GUEST_ID_LO 0x00000000
>>>>>> +#define HV_LINUX_GUEST_ID_HI 0xB16B00B5
>>>>>> +#define HV_LINUX_GUEST_ID (((u64)HV_LINUX_GUEST_ID_HI
>>>>>> << 32) | \
>>>>>> + HV_LINUX_GUEST_ID_LO)
>>>>>>
>>>>>> Somone was trying to be funny, I guess.
>>>>>>
>>>>>> KY, I suppose you have access to Hyper-V code or can ask someone who
>>>> does.
>>>>>> Is this signature actually used in the Hyper-V host code?
>>>>>
>>>>> Paolo,
>>>>>
>>>>> As I noted earlier, this is just a guest ID that needs to be registered with the
>>>>> hypervisor. Thanks for reporting this issue and on behalf of Microsoft, I
>> would
>>>>> like to apologize for this offensive string. I have submitted a patch to fix this
>>>> issue.
>>>>
>>>> You only changed it to be in decimal, you did not change the id at all.
>>>> Is there some reason why you can not change it? You said there was a
>>>> reserved range of ids that could be used, perhaps just pick another one?
>>>> What is the valid range that can be used here?
>>>
>>> Greg,
>>>
>>> As you know, this ID has been in use for a long time now. While the hypervisor
>>> does not interpret the guest ID that is registered, I am not sure what
>> dependencies
>>> there might be on this value.
>>
>> Could you please go find out the answer to this?
>
> That is easier said than done. I have sent emails out asking this very question and I have
> not received a definitive answer yet. Not knowing if and when I can get a definitive
> answer here, I chose the least risky approach in my patch.
>>
>> If, as you originally stated, there is a range of values we can use,
>> then we should probably use another one, right?
>
> On the Windows side this ID namespace is managed well. However on the Linux
> side, we have really had this current ID in use for almost five years now. I am not
> aware of any pool of IDs available for Linux usage except that Linux IDs be distinct from
> the guest IDs in use by MSFT operating systems. If I were to change the guest ID, I would
> probably want to comply with the MSFT guidance on constructing these IDs (although not
> all fields may be relevant for Linux).
Presumably, Hyper-V can deal with unexpected values here, no? Otherwise, it
wouldn't be future proof against new types of guests.
So worst case scenario, Hyper-V disables optimizations on Linux guests that
report then new ID until they patch Hyper-V to know about the new ID.
That seems like a reasonable trade off to me. I'm sure there's sufficient
incentive to patch Hyper-V for this at Microsoft...
Regards,
Anthony Liguori
>
> Regards,
>
> K. Y
>
>
>
^ permalink raw reply
* RE: 0xB16B00B5? Really? (was Re: Move hyperv out of the drivers/staging/ directory)
From: KY Srinivasan @ 2012-07-19 22:30 UTC (permalink / raw)
To: Greg KH (gregkh@linuxfoundation.org)
Cc: Paolo Bonzini, devel@linuxdriverproject.org,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <20120719220142.GA1513@kroah.com>
> -----Original Message-----
> From: Greg KH (gregkh@linuxfoundation.org)
> [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, July 19, 2012 6:02 PM
> To: KY Srinivasan
> Cc: Paolo Bonzini; devel@linuxdriverproject.org; linux-kernel@vger.kernel.org;
> virtualization@lists.osdl.org
> Subject: Re: 0xB16B00B5? Really? (was Re: Move hyperv out of the
> drivers/staging/ directory)
>
> On Thu, Jul 19, 2012 at 09:22:53PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH (gregkh@linuxfoundation.org)
> > > [mailto:gregkh@linuxfoundation.org]
> > > Sent: Thursday, July 19, 2012 5:07 PM
> > > To: KY Srinivasan
> > > Cc: Paolo Bonzini; devel@linuxdriverproject.org; linux-
> kernel@vger.kernel.org;
> > > virtualization@lists.osdl.org
> > > Subject: Re: 0xB16B00B5? Really? (was Re: Move hyperv out of the
> > > drivers/staging/ directory)
> > >
> > > On Thu, Jul 19, 2012 at 02:11:47AM +0000, KY Srinivasan wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
> > > > > Bonzini
> > > > > Sent: Friday, July 13, 2012 6:23 AM
> > > > > To: KY Srinivasan
> > > > > Cc: Greg KH; devel@linuxdriverproject.org; linux-kernel@vger.kernel.org;
> > > > > virtualization@lists.osdl.org
> > > > > Subject: 0xB16B00B5? Really? (was Re: Move hyperv out of the
> > > drivers/staging/
> > > > > directory)
> > > > >
> > > > > Il 04/10/2011 21:34, Greg KH ha scritto:
> > > > > > diff --git a/drivers/staging/hv/hyperv_vmbus.h
> > > b/drivers/hv/hyperv_vmbus.h
> > > > > > similarity index 99%
> > > > > > rename from drivers/staging/hv/hyperv_vmbus.h
> > > > > > rename to drivers/hv/hyperv_vmbus.h
> > > > > > index 3d2d836..8261cb6 100644
> > > > > > --- a/drivers/staging/hv/hyperv_vmbus.h
> > > > > > +++ b/drivers/hv/hyperv_vmbus.h
> > > > > > @@ -28,8 +28,7 @@
> > > > > > #include <linux/list.h>
> > > > > > #include <asm/sync_bitops.h>
> > > > > > #include <linux/atomic.h>
> > > > > > -
> > > > > > -#include "hyperv.h"
> > > > > > +#include <linux/hyperv.h>
> > > > > >
> > > > > > /*
> > > > > > * The below CPUID leaves are present if
> > > > > VersionAndFeatures.HypervisorPresent
> > > > >
> > > > > git's rename detection snips away this gem:
> > > > >
> > > > > +#define HV_LINUX_GUEST_ID_LO 0x00000000
> > > > > +#define HV_LINUX_GUEST_ID_HI 0xB16B00B5
> > > > > +#define HV_LINUX_GUEST_ID (((u64)HV_LINUX_GUEST_ID_HI
> > > > > << 32) | \
> > > > > + HV_LINUX_GUEST_ID_LO)
> > > > >
> > > > > Somone was trying to be funny, I guess.
> > > > >
> > > > > KY, I suppose you have access to Hyper-V code or can ask someone who
> > > does.
> > > > > Is this signature actually used in the Hyper-V host code?
> > > >
> > > > Paolo,
> > > >
> > > > As I noted earlier, this is just a guest ID that needs to be registered with the
> > > > hypervisor. Thanks for reporting this issue and on behalf of Microsoft, I
> would
> > > > like to apologize for this offensive string. I have submitted a patch to fix this
> > > issue.
> > >
> > > You only changed it to be in decimal, you did not change the id at all.
> > > Is there some reason why you can not change it? You said there was a
> > > reserved range of ids that could be used, perhaps just pick another one?
> > > What is the valid range that can be used here?
> >
> > Greg,
> >
> > As you know, this ID has been in use for a long time now. While the hypervisor
> > does not interpret the guest ID that is registered, I am not sure what
> dependencies
> > there might be on this value.
>
> Could you please go find out the answer to this?
That is easier said than done. I have sent emails out asking this very question and I have
not received a definitive answer yet. Not knowing if and when I can get a definitive
answer here, I chose the least risky approach in my patch.
>
> If, as you originally stated, there is a range of values we can use,
> then we should probably use another one, right?
On the Windows side this ID namespace is managed well. However on the Linux
side, we have really had this current ID in use for almost five years now. I am not
aware of any pool of IDs available for Linux usage except that Linux IDs be distinct from
the guest IDs in use by MSFT operating systems. If I were to change the guest ID, I would
probably want to comply with the MSFT guidance on constructing these IDs (although not
all fields may be relevant for Linux).
Regards,
K. Y
^ permalink raw reply
* Re: 0xB16B00B5? Really? (was Re: Move hyperv out of the drivers/staging/ directory)
From: Greg KH (gregkh@linuxfoundation.org) @ 2012-07-19 22:01 UTC (permalink / raw)
To: KY Srinivasan
Cc: Paolo Bonzini, devel@linuxdriverproject.org,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <426367E2313C2449837CD2DE46E7EAF9235475EE@SN2PRD0310MB382.namprd03.prod.outlook.com>
On Thu, Jul 19, 2012 at 09:22:53PM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Greg KH (gregkh@linuxfoundation.org)
> > [mailto:gregkh@linuxfoundation.org]
> > Sent: Thursday, July 19, 2012 5:07 PM
> > To: KY Srinivasan
> > Cc: Paolo Bonzini; devel@linuxdriverproject.org; linux-kernel@vger.kernel.org;
> > virtualization@lists.osdl.org
> > Subject: Re: 0xB16B00B5? Really? (was Re: Move hyperv out of the
> > drivers/staging/ directory)
> >
> > On Thu, Jul 19, 2012 at 02:11:47AM +0000, KY Srinivasan wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
> > > > Bonzini
> > > > Sent: Friday, July 13, 2012 6:23 AM
> > > > To: KY Srinivasan
> > > > Cc: Greg KH; devel@linuxdriverproject.org; linux-kernel@vger.kernel.org;
> > > > virtualization@lists.osdl.org
> > > > Subject: 0xB16B00B5? Really? (was Re: Move hyperv out of the
> > drivers/staging/
> > > > directory)
> > > >
> > > > Il 04/10/2011 21:34, Greg KH ha scritto:
> > > > > diff --git a/drivers/staging/hv/hyperv_vmbus.h
> > b/drivers/hv/hyperv_vmbus.h
> > > > > similarity index 99%
> > > > > rename from drivers/staging/hv/hyperv_vmbus.h
> > > > > rename to drivers/hv/hyperv_vmbus.h
> > > > > index 3d2d836..8261cb6 100644
> > > > > --- a/drivers/staging/hv/hyperv_vmbus.h
> > > > > +++ b/drivers/hv/hyperv_vmbus.h
> > > > > @@ -28,8 +28,7 @@
> > > > > #include <linux/list.h>
> > > > > #include <asm/sync_bitops.h>
> > > > > #include <linux/atomic.h>
> > > > > -
> > > > > -#include "hyperv.h"
> > > > > +#include <linux/hyperv.h>
> > > > >
> > > > > /*
> > > > > * The below CPUID leaves are present if
> > > > VersionAndFeatures.HypervisorPresent
> > > >
> > > > git's rename detection snips away this gem:
> > > >
> > > > +#define HV_LINUX_GUEST_ID_LO 0x00000000
> > > > +#define HV_LINUX_GUEST_ID_HI 0xB16B00B5
> > > > +#define HV_LINUX_GUEST_ID (((u64)HV_LINUX_GUEST_ID_HI
> > > > << 32) | \
> > > > + HV_LINUX_GUEST_ID_LO)
> > > >
> > > > Somone was trying to be funny, I guess.
> > > >
> > > > KY, I suppose you have access to Hyper-V code or can ask someone who
> > does.
> > > > Is this signature actually used in the Hyper-V host code?
> > >
> > > Paolo,
> > >
> > > As I noted earlier, this is just a guest ID that needs to be registered with the
> > > hypervisor. Thanks for reporting this issue and on behalf of Microsoft, I would
> > > like to apologize for this offensive string. I have submitted a patch to fix this
> > issue.
> >
> > You only changed it to be in decimal, you did not change the id at all.
> > Is there some reason why you can not change it? You said there was a
> > reserved range of ids that could be used, perhaps just pick another one?
> > What is the valid range that can be used here?
>
> Greg,
>
> As you know, this ID has been in use for a long time now. While the hypervisor
> does not interpret the guest ID that is registered, I am not sure what dependencies
> there might be on this value.
Could you please go find out the answer to this?
If, as you originally stated, there is a range of values we can use,
then we should probably use another one, right?
thanks,
greg k-h
^ permalink raw reply
* RE: 0xB16B00B5? Really? (was Re: Move hyperv out of the drivers/staging/ directory)
From: KY Srinivasan @ 2012-07-19 21:22 UTC (permalink / raw)
To: Greg KH (gregkh@linuxfoundation.org)
Cc: Paolo Bonzini, devel@linuxdriverproject.org,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <20120719210709.GA32031@kroah.com>
> -----Original Message-----
> From: Greg KH (gregkh@linuxfoundation.org)
> [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, July 19, 2012 5:07 PM
> To: KY Srinivasan
> Cc: Paolo Bonzini; devel@linuxdriverproject.org; linux-kernel@vger.kernel.org;
> virtualization@lists.osdl.org
> Subject: Re: 0xB16B00B5? Really? (was Re: Move hyperv out of the
> drivers/staging/ directory)
>
> On Thu, Jul 19, 2012 at 02:11:47AM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
> > > Bonzini
> > > Sent: Friday, July 13, 2012 6:23 AM
> > > To: KY Srinivasan
> > > Cc: Greg KH; devel@linuxdriverproject.org; linux-kernel@vger.kernel.org;
> > > virtualization@lists.osdl.org
> > > Subject: 0xB16B00B5? Really? (was Re: Move hyperv out of the
> drivers/staging/
> > > directory)
> > >
> > > Il 04/10/2011 21:34, Greg KH ha scritto:
> > > > diff --git a/drivers/staging/hv/hyperv_vmbus.h
> b/drivers/hv/hyperv_vmbus.h
> > > > similarity index 99%
> > > > rename from drivers/staging/hv/hyperv_vmbus.h
> > > > rename to drivers/hv/hyperv_vmbus.h
> > > > index 3d2d836..8261cb6 100644
> > > > --- a/drivers/staging/hv/hyperv_vmbus.h
> > > > +++ b/drivers/hv/hyperv_vmbus.h
> > > > @@ -28,8 +28,7 @@
> > > > #include <linux/list.h>
> > > > #include <asm/sync_bitops.h>
> > > > #include <linux/atomic.h>
> > > > -
> > > > -#include "hyperv.h"
> > > > +#include <linux/hyperv.h>
> > > >
> > > > /*
> > > > * The below CPUID leaves are present if
> > > VersionAndFeatures.HypervisorPresent
> > >
> > > git's rename detection snips away this gem:
> > >
> > > +#define HV_LINUX_GUEST_ID_LO 0x00000000
> > > +#define HV_LINUX_GUEST_ID_HI 0xB16B00B5
> > > +#define HV_LINUX_GUEST_ID (((u64)HV_LINUX_GUEST_ID_HI
> > > << 32) | \
> > > + HV_LINUX_GUEST_ID_LO)
> > >
> > > Somone was trying to be funny, I guess.
> > >
> > > KY, I suppose you have access to Hyper-V code or can ask someone who
> does.
> > > Is this signature actually used in the Hyper-V host code?
> >
> > Paolo,
> >
> > As I noted earlier, this is just a guest ID that needs to be registered with the
> > hypervisor. Thanks for reporting this issue and on behalf of Microsoft, I would
> > like to apologize for this offensive string. I have submitted a patch to fix this
> issue.
>
> You only changed it to be in decimal, you did not change the id at all.
> Is there some reason why you can not change it? You said there was a
> reserved range of ids that could be used, perhaps just pick another one?
> What is the valid range that can be used here?
Greg,
As you know, this ID has been in use for a long time now. While the hypervisor
does not interpret the guest ID that is registered, I am not sure what dependencies
there might be on this value. That is the reason I am reluctant to change the value.
Since the hex representation of this constant was what was offensive, changing it to
a decimal number would not break anything while simultaneously addressing the issue
Paolo brought up.
Regards,
K. Y
^ permalink raw reply
* Re: 0xB16B00B5? Really? (was Re: Move hyperv out of the drivers/staging/ directory)
From: Greg KH (gregkh@linuxfoundation.org) @ 2012-07-19 21:07 UTC (permalink / raw)
To: KY Srinivasan
Cc: Paolo Bonzini, devel@linuxdriverproject.org,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <426367E2313C2449837CD2DE46E7EAF923547292@SN2PRD0310MB382.namprd03.prod.outlook.com>
On Thu, Jul 19, 2012 at 02:11:47AM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
> > Bonzini
> > Sent: Friday, July 13, 2012 6:23 AM
> > To: KY Srinivasan
> > Cc: Greg KH; devel@linuxdriverproject.org; linux-kernel@vger.kernel.org;
> > virtualization@lists.osdl.org
> > Subject: 0xB16B00B5? Really? (was Re: Move hyperv out of the drivers/staging/
> > directory)
> >
> > Il 04/10/2011 21:34, Greg KH ha scritto:
> > > diff --git a/drivers/staging/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> > > similarity index 99%
> > > rename from drivers/staging/hv/hyperv_vmbus.h
> > > rename to drivers/hv/hyperv_vmbus.h
> > > index 3d2d836..8261cb6 100644
> > > --- a/drivers/staging/hv/hyperv_vmbus.h
> > > +++ b/drivers/hv/hyperv_vmbus.h
> > > @@ -28,8 +28,7 @@
> > > #include <linux/list.h>
> > > #include <asm/sync_bitops.h>
> > > #include <linux/atomic.h>
> > > -
> > > -#include "hyperv.h"
> > > +#include <linux/hyperv.h>
> > >
> > > /*
> > > * The below CPUID leaves are present if
> > VersionAndFeatures.HypervisorPresent
> >
> > git's rename detection snips away this gem:
> >
> > +#define HV_LINUX_GUEST_ID_LO 0x00000000
> > +#define HV_LINUX_GUEST_ID_HI 0xB16B00B5
> > +#define HV_LINUX_GUEST_ID (((u64)HV_LINUX_GUEST_ID_HI
> > << 32) | \
> > + HV_LINUX_GUEST_ID_LO)
> >
> > Somone was trying to be funny, I guess.
> >
> > KY, I suppose you have access to Hyper-V code or can ask someone who does.
> > Is this signature actually used in the Hyper-V host code?
>
> Paolo,
>
> As I noted earlier, this is just a guest ID that needs to be registered with the
> hypervisor. Thanks for reporting this issue and on behalf of Microsoft, I would
> like to apologize for this offensive string. I have submitted a patch to fix this issue.
You only changed it to be in decimal, you did not change the id at all.
Is there some reason why you can not change it? You said there was a
reserved range of ids that could be used, perhaps just pick another one?
What is the valid range that can be used here?
thanks,
greg k-h
^ permalink raw reply
* RE: 0xB16B00B5? Really? (was Re: Move hyperv out of the drivers/staging/ directory)
From: KY Srinivasan @ 2012-07-19 15:53 UTC (permalink / raw)
To: Borislav Petkov
Cc: Paolo Bonzini, virtualization@lists.osdl.org,
devel@linuxdriverproject.org, linux-kernel@vger.kernel.org,
Greg KH (gregkh@linuxfoundation.org)
In-Reply-To: <20120719153014.GA13235@x1.osrc.amd.com>
> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Thursday, July 19, 2012 11:30 AM
> To: KY Srinivasan
> Cc: Paolo Bonzini; Greg KH (gregkh@linuxfoundation.org);
> devel@linuxdriverproject.org; linux-kernel@vger.kernel.org;
> virtualization@lists.osdl.org
> Subject: Re: 0xB16B00B5? Really? (was Re: Move hyperv out of the
> drivers/staging/ directory)
>
> On Thu, Jul 19, 2012 at 02:11:47AM +0000, KY Srinivasan wrote:
> > As I noted earlier, this is just a guest ID that needs to be
> > registered with the hypervisor. Thanks for reporting this issue and
> > on behalf of Microsoft, I would like to apologize for this offensive
> > string. I have submitted a patch to fix this issue.
>
> Ok, quick question:
>
> http://msdn.microsoft.com/en-
> us/library/windows/hardware/ff542653%28v=vs.85%29.aspx
>
> says that that guest ID encodes a bunch of version numbers and IDs
> pertaining to the guest OS. And in that case, I would've thought
> you'd encode kernel versions or whatever like it is done with
> LINUX_VERSION_CODE, for example.
>
> How does this define fit with that scheme or was it chosen completely
> arbitrarily?
From what I know, this constant was chosen I think, completely arbitrarily.
Currently, the hypervisor does not interpret this, however the host partition
can retrieve it.
Regards,
K. Y
^ permalink raw reply
* Re: 0xB16B00B5? Really? (was Re: Move hyperv out of the drivers/staging/ directory)
From: Borislav Petkov @ 2012-07-19 15:30 UTC (permalink / raw)
To: KY Srinivasan
Cc: Paolo Bonzini, Greg KH (gregkh@linuxfoundation.org),
devel@linuxdriverproject.org, linux-kernel@vger.kernel.org,
virtualization@lists.osdl.org
In-Reply-To: <426367E2313C2449837CD2DE46E7EAF923547292@SN2PRD0310MB382.namprd03.prod.outlook.com>
On Thu, Jul 19, 2012 at 02:11:47AM +0000, KY Srinivasan wrote:
> As I noted earlier, this is just a guest ID that needs to be
> registered with the hypervisor. Thanks for reporting this issue and
> on behalf of Microsoft, I would like to apologize for this offensive
> string. I have submitted a patch to fix this issue.
Ok, quick question:
http://msdn.microsoft.com/en-us/library/windows/hardware/ff542653%28v=vs.85%29.aspx
says that that guest ID encodes a bunch of version numbers and IDs
pertaining to the guest OS. And in that case, I would've thought
you'd encode kernel versions or whatever like it is done with
LINUX_VERSION_CODE, for example.
How does this define fit with that scheme or was it chosen completely
arbitrarily?
Thanks.
--
Regards/Gruss,
Boris.
^ permalink raw reply
* Re: [PATCH RESEND 5/5] vhost-blk: Add vhost-blk support
From: Michael S. Tsirkin @ 2012-07-19 13:09 UTC (permalink / raw)
To: Anthony Liguori; +Cc: linux-kernel, kvm, virtualization
In-Reply-To: <87mx2vrjdl.fsf@codemonkey.ws>
On Thu, Jul 19, 2012 at 08:05:42AM -0500, Anthony Liguori wrote:
> Of course, the million dollar question is why would using AIO in the
> kernel be faster than using AIO in userspace?
Actually for me a more important question is how does it compare
with virtio-blk dataplane?
--
MST
^ permalink raw reply
* Re: [PATCH RESEND 5/5] vhost-blk: Add vhost-blk support
From: Michael S. Tsirkin @ 2012-07-19 13:09 UTC (permalink / raw)
To: Anthony Liguori; +Cc: linux-kernel, kvm, virtualization
In-Reply-To: <87mx2vrjdl.fsf@codemonkey.ws>
On Thu, Jul 19, 2012 at 08:05:42AM -0500, Anthony Liguori wrote:
> Asias He <asias@redhat.com> writes:
>
> > vhost-blk is a in kernel virito-blk device accelerator.
> >
> > This patch is based on Liu Yuan's implementation with various
> > improvements and bug fixes. Notably, this patch makes guest notify and
> > host completion processing in parallel which gives about 60% performance
> > improvement compared to Liu Yuan's implementation.
> >
> > Performance evaluation:
> > -----------------------------
> > The comparison is between kvm tool with usersapce implementation and kvm
> > tool with vhost-blk.
> >
> > 1) Fio with libaio ioengine on Fusion IO device
> > With bio-based IO path, sequential read/write, random read/write
> > IOPS boost : 8.4%, 15.3%, 10.4%, 14.6%
> > Latency improvement: 8.5%, 15.4%, 10.4%, 15.1%
> >
> > 2) Fio with vsync ioengine on Fusion IO device
> > With bio-based IO path, sequential read/write, random read/write
> > IOPS boost : 10.5%, 4.8%, 5.2%, 5.6%
> > Latency improvement: 11.4%, 5.0%, 5.2%, 5.8%
> >
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: linux-kernel@vger.kernel.org
> > Cc: kvm@vger.kernel.org
> > Cc: virtualization@lists.linux-foundation.org
> > Signed-off-by: Asias He <asias@redhat.com>
> > ---
> > drivers/vhost/Kconfig | 10 +
> > drivers/vhost/Makefile | 2 +
> > drivers/vhost/blk.c | 600 ++++++++++++++++++++++++++++++++++++++++++++++++
> > drivers/vhost/vhost.h | 5 +
> > include/linux/vhost.h | 3 +
> > 5 files changed, 620 insertions(+)
> > create mode 100644 drivers/vhost/blk.c
> >
> > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > index c387067..fa071a8 100644
> > --- a/drivers/vhost/Kconfig
> > +++ b/drivers/vhost/Kconfig
> > @@ -16,4 +16,14 @@ config VHOST_NET
> >
> > To compile this driver as a module, choose M here: the module will
> > be called vhost_net.
> > +config VHOST_BLK
> > + tristate "Host kernel accelerator for virtio blk (EXPERIMENTAL)"
> > + depends on VHOST && BLOCK && AIO && EVENTFD && EXPERIMENTAL
> > + ---help---
> > + This kernel module can be loaded in host kernel to accelerate
> > + guest block with virtio_blk. Not to be confused with virtio_blk
> > + module itself which needs to be loaded in guest kernel.
> > +
> > + To compile this driver as a module, choose M here: the module will
> > + be called vhost_blk.
> >
> > diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile
> > index cd36885..aa461d5 100644
> > --- a/drivers/vhost/Makefile
> > +++ b/drivers/vhost/Makefile
> > @@ -1,4 +1,6 @@
> > obj-$(CONFIG_VHOST) += vhost.o
> > obj-$(CONFIG_VHOST_NET) += vhost_net.o
> > +obj-$(CONFIG_VHOST_BLK) += vhost_blk.o
> >
> > vhost_net-y := net.o
> > +vhost_blk-y := blk.o
> > diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
> > new file mode 100644
> > index 0000000..6a94894
> > --- /dev/null
> > +++ b/drivers/vhost/blk.c
> > @@ -0,0 +1,600 @@
> > +/*
> > + * Copyright (C) 2011 Taobao, Inc.
> > + * Author: Liu Yuan <tailai.ly@taobao.com>
> > + *
> > + * Copyright (C) 2012 Red Hat, Inc.
> > + * Author: Asias He <asias@redhat.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2.
> > + *
> > + * virtio-blk server in host kernel.
> > + */
> > +
> > +#include <linux/miscdevice.h>
> > +#include <linux/module.h>
> > +#include <linux/vhost.h>
> > +#include <linux/virtio_blk.h>
> > +#include <linux/eventfd.h>
> > +#include <linux/mutex.h>
> > +#include <linux/file.h>
> > +#include <linux/mmu_context.h>
> > +#include <linux/anon_inodes.h>
> > +#include <linux/kthread.h>
> > +#include <linux/blkdev.h>
> > +
> > +#include "vhost.h"
> > +
> > +#define BLK_HDR 0
> > +
> > +enum {
> > + VHOST_BLK_VQ_REQ = 0,
> > + VHOST_BLK_VQ_MAX = 1,
> > +};
> > +
> > +struct vhost_blk_req {
> > + u16 head;
> > + u8 *status;
> > +};
> > +
> > +struct vhost_blk {
> > + struct task_struct *worker_host_kick;
> > + struct task_struct *worker;
> > + struct vhost_blk_req *reqs;
> > + struct vhost_virtqueue vq;
> > + struct eventfd_ctx *ectx;
> > + struct io_event *ioevent;
> > + struct kioctx *ioctx;
> > + struct vhost_dev dev;
> > + struct file *efile;
> > + u64 ioevent_nr;
> > + bool stop;
> > +};
> > +
> > +static inline int vhost_blk_read_events(struct vhost_blk *blk, long nr)
> > +{
> > + mm_segment_t old_fs = get_fs();
> > + int ret;
> > +
> > + set_fs(KERNEL_DS);
> > + ret = read_events(blk->ioctx, nr, nr, blk->ioevent, NULL);
> > + set_fs(old_fs);
> > +
> > + return ret;
> > +}
> > +
> > +static int vhost_blk_setup(struct vhost_blk *blk)
> > +{
> > + struct kioctx *ctx;
> > +
> > + if (blk->ioctx)
> > + return 0;
> > +
> > + blk->ioevent_nr = blk->vq.num;
> > + ctx = ioctx_alloc(blk->ioevent_nr);
> > + if (IS_ERR(ctx)) {
> > + pr_err("Failed to ioctx_alloc");
> > + return PTR_ERR(ctx);
> > + }
>
> Not that it's very likely that ioctx_alloc will fail in practice.
> There's a fixed number of events that can be allocated that's currently
> 0x10000. If you have a ring queue size of 1024 (which is normal) then
> that limits you to 64 vhost-blk devices.
>
> Realistically, I don't think you can only do aio with vhost-blk because
> of this (and many other) limitations. It's necessary to be able to fall
> back to a thread pool because AIO cannot be relied upon.
>
> > + put_ioctx(ctx);
> > + blk->ioctx = ctx;
> > +
> > + blk->ioevent = kmalloc(sizeof(struct io_event) * blk->ioevent_nr,
> > + GFP_KERNEL);
> > + if (!blk->ioevent) {
> > + pr_err("Failed to allocate memory for io_events");
> > + return -ENOMEM;
> > + }
> > +
> > + blk->reqs = kmalloc(sizeof(struct vhost_blk_req) * blk->ioevent_nr,
> > + GFP_KERNEL);
> > + if (!blk->reqs) {
> > + pr_err("Failed to allocate memory for vhost_blk_req");
> > + return -ENOMEM;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static inline int vhost_blk_set_status(struct vhost_blk *blk, u8 *statusp,
> > + u8 status)
> > +{
> > + if (copy_to_user(statusp, &status, sizeof(status))) {
> > + vq_err(&blk->vq, "Failed to write status\n");
> > + vhost_discard_vq_desc(&blk->vq, 1);
> > + return -EFAULT;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void vhost_blk_enable_vq(struct vhost_blk *blk,
> > + struct vhost_virtqueue *vq)
> > +{
> > + wake_up_process(blk->worker_host_kick);
> > +}
> > +
> > +static int vhost_blk_io_submit(struct vhost_blk *blk, struct file *file,
> > + struct vhost_blk_req *req,
> > + struct iovec *iov, u64 nr_vecs, loff_t offset,
> > + int opcode)
> > +{
> > + struct kioctx *ioctx = blk->ioctx;
> > + mm_segment_t oldfs = get_fs();
> > + struct kiocb_batch batch;
> > + struct blk_plug plug;
> > + struct kiocb *iocb;
> > + int ret;
> > +
> > + if (!try_get_ioctx(ioctx)) {
> > + pr_info("Failed to get ioctx");
> > + return -EAGAIN;
> > + }
> > +
> > + atomic_long_inc_not_zero(&file->f_count);
> > + eventfd_ctx_get(blk->ectx);
> > +
> > + /* TODO: batch to 1 is not good! */
> > + kiocb_batch_init(&batch, 1);
> > + blk_start_plug(&plug);
> > +
> > + iocb = aio_get_req(ioctx, &batch);
> > + if (unlikely(!iocb)) {
> > + ret = -EAGAIN;
> > + goto out;
> > + }
> > +
> > + iocb->ki_filp = file;
> > + iocb->ki_pos = offset;
> > + iocb->ki_buf = (void *)iov;
> > + iocb->ki_left = nr_vecs;
> > + iocb->ki_nbytes = nr_vecs;
> > + iocb->ki_opcode = opcode;
> > + iocb->ki_obj.user = req;
> > + iocb->ki_eventfd = blk->ectx;
> > +
> > + set_fs(KERNEL_DS);
> > + ret = aio_setup_iocb(iocb, false);
> > + set_fs(oldfs);
> > + if (unlikely(ret))
> > + goto out_put_iocb;
> > +
> > + spin_lock_irq(&ioctx->ctx_lock);
> > + if (unlikely(ioctx->dead)) {
> > + spin_unlock_irq(&ioctx->ctx_lock);
> > + ret = -EINVAL;
> > + goto out_put_iocb;
> > + }
> > + aio_run_iocb(iocb);
> > + spin_unlock_irq(&ioctx->ctx_lock);
> > +
> > + aio_put_req(iocb);
> > +
> > + blk_finish_plug(&plug);
> > + kiocb_batch_free(ioctx, &batch);
> > + put_ioctx(ioctx);
> > +
> > + return ret;
> > +out_put_iocb:
> > + aio_put_req(iocb); /* Drop extra ref to req */
> > + aio_put_req(iocb); /* Drop I/O ref to req */
> > +out:
> > + put_ioctx(ioctx);
> > + return ret;
> > +}
> > +
> > +static void vhost_blk_flush(struct vhost_blk *blk)
> > +{
> > + vhost_poll_flush(&blk->vq.poll);
> > +}
> > +
> > +static struct file *vhost_blk_stop_vq(struct vhost_blk *blk,
> > + struct vhost_virtqueue *vq)
> > +{
> > + struct file *file;
> > +
> > + mutex_lock(&vq->mutex);
> > + file = rcu_dereference_protected(vq->private_data,
> > + lockdep_is_held(&vq->mutex));
> > + rcu_assign_pointer(vq->private_data, NULL);
> > + mutex_unlock(&vq->mutex);
> > +
> > + return file;
> > +
> > +}
> > +
> > +static inline void vhost_blk_stop(struct vhost_blk *blk, struct file **file)
> > +{
> > +
> > + *file = vhost_blk_stop_vq(blk, &blk->vq);
> > +}
> > +
> > +/* Handle guest request */
> > +static int vhost_blk_do_req(struct vhost_virtqueue *vq,
> > + struct virtio_blk_outhdr *hdr,
> > + u16 head, u16 out, u16 in,
> > + struct file *file)
> > +{
> > + struct vhost_blk *blk = container_of(vq->dev, struct vhost_blk, dev);
> > + struct iovec *iov = &vq->iov[BLK_HDR + 1];
> > + loff_t offset = hdr->sector << 9;
> > + struct vhost_blk_req *req;
> > + u64 nr_vecs;
> > + int ret = 0;
> > + u8 status;
> > +
> > + if (hdr->type == VIRTIO_BLK_T_IN || hdr->type == VIRTIO_BLK_T_GET_ID)
> > + nr_vecs = in - 1;
> > + else
> > + nr_vecs = out - 1;
> > +
> > + req = &blk->reqs[head];
> > + req->head = head;
> > + req->status = blk->vq.iov[nr_vecs + 1].iov_base;
> > +
> > + switch (hdr->type) {
> > + case VIRTIO_BLK_T_OUT:
> > + ret = vhost_blk_io_submit(blk, file, req, iov, nr_vecs, offset,
> > + IOCB_CMD_PWRITEV);
> > + break;
> > + case VIRTIO_BLK_T_IN:
> > + ret = vhost_blk_io_submit(blk, file, req, iov, nr_vecs, offset,
> > + IOCB_CMD_PREADV);
> > + break;
> > + case VIRTIO_BLK_T_FLUSH:
> > + ret = vfs_fsync(file, 1);
> > + status = ret < 0 ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
> > + ret = vhost_blk_set_status(blk, req->status, status);
> > + if (!ret)
> > + vhost_add_used_and_signal(&blk->dev, vq, head, ret);
> > + break;
> > + case VIRTIO_BLK_T_GET_ID:
> > + /* TODO: need a real ID string */
> > + ret = snprintf(vq->iov[BLK_HDR + 1].iov_base,
> > + VIRTIO_BLK_ID_BYTES, "VHOST-BLK-DISK");
> > + status = ret < 0 ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
> > + ret = vhost_blk_set_status(blk, req->status, status);
> > + if (!ret)
> > + vhost_add_used_and_signal(&blk->dev, vq, head,
> > + VIRTIO_BLK_ID_BYTES);
> > + break;
> > + default:
> > + pr_warn("Unsupported request type %d\n", hdr->type);
> > + vhost_discard_vq_desc(vq, 1);
> > + ret = -EFAULT;
> > + break;
> > + }
>
> There doesn't appear to be any error handling in the event that
> vhost_blk_io_submit fails. It would appear that you leak the ring queue
> entry since you never push anything onto the used queue.
>
> I think you need to handle io_submit() failing too with EAGAIN. Relying
> on min nr_events == queue_size seems like a dangerous assumption to me
> particularly since queue_size tends to be very large and max_nr_events
> is a fixed size global pool.
>
> To properly handle EAGAIN, you effectively need to implement flow
> control and back off reading the virtqueue until you can submit requests again.
>
> Of course, the million dollar question is why would using AIO in the
> kernel be faster than using AIO in userspace?
>
> When using eventfd, there is no heavy weight exit on the notify path.
> It should just be the difference between scheduling a kernel thread vs
> scheduling a userspace thread.
Actually AIO from userspace involves a kernel thread too, doesn't it?
> There's simply no way that that's a 60%
> difference in performance.
>
> Regards,
>
> Anthony Liguori
^ permalink raw reply
* Re: [PATCH RESEND 5/5] vhost-blk: Add vhost-blk support
From: Anthony Liguori @ 2012-07-19 13:05 UTC (permalink / raw)
To: Asias He, linux-kernel; +Cc: virtualization, kvm, Michael S. Tsirkin
In-Reply-To: <1342169711-12386-6-git-send-email-asias@redhat.com>
Asias He <asias@redhat.com> writes:
> vhost-blk is a in kernel virito-blk device accelerator.
>
> This patch is based on Liu Yuan's implementation with various
> improvements and bug fixes. Notably, this patch makes guest notify and
> host completion processing in parallel which gives about 60% performance
> improvement compared to Liu Yuan's implementation.
>
> Performance evaluation:
> -----------------------------
> The comparison is between kvm tool with usersapce implementation and kvm
> tool with vhost-blk.
>
> 1) Fio with libaio ioengine on Fusion IO device
> With bio-based IO path, sequential read/write, random read/write
> IOPS boost : 8.4%, 15.3%, 10.4%, 14.6%
> Latency improvement: 8.5%, 15.4%, 10.4%, 15.1%
>
> 2) Fio with vsync ioengine on Fusion IO device
> With bio-based IO path, sequential read/write, random read/write
> IOPS boost : 10.5%, 4.8%, 5.2%, 5.6%
> Latency improvement: 11.4%, 5.0%, 5.2%, 5.8%
>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: kvm@vger.kernel.org
> Cc: virtualization@lists.linux-foundation.org
> Signed-off-by: Asias He <asias@redhat.com>
> ---
> drivers/vhost/Kconfig | 10 +
> drivers/vhost/Makefile | 2 +
> drivers/vhost/blk.c | 600 ++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/vhost/vhost.h | 5 +
> include/linux/vhost.h | 3 +
> 5 files changed, 620 insertions(+)
> create mode 100644 drivers/vhost/blk.c
>
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> index c387067..fa071a8 100644
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -16,4 +16,14 @@ config VHOST_NET
>
> To compile this driver as a module, choose M here: the module will
> be called vhost_net.
> +config VHOST_BLK
> + tristate "Host kernel accelerator for virtio blk (EXPERIMENTAL)"
> + depends on VHOST && BLOCK && AIO && EVENTFD && EXPERIMENTAL
> + ---help---
> + This kernel module can be loaded in host kernel to accelerate
> + guest block with virtio_blk. Not to be confused with virtio_blk
> + module itself which needs to be loaded in guest kernel.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called vhost_blk.
>
> diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile
> index cd36885..aa461d5 100644
> --- a/drivers/vhost/Makefile
> +++ b/drivers/vhost/Makefile
> @@ -1,4 +1,6 @@
> obj-$(CONFIG_VHOST) += vhost.o
> obj-$(CONFIG_VHOST_NET) += vhost_net.o
> +obj-$(CONFIG_VHOST_BLK) += vhost_blk.o
>
> vhost_net-y := net.o
> +vhost_blk-y := blk.o
> diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
> new file mode 100644
> index 0000000..6a94894
> --- /dev/null
> +++ b/drivers/vhost/blk.c
> @@ -0,0 +1,600 @@
> +/*
> + * Copyright (C) 2011 Taobao, Inc.
> + * Author: Liu Yuan <tailai.ly@taobao.com>
> + *
> + * Copyright (C) 2012 Red Hat, Inc.
> + * Author: Asias He <asias@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.
> + *
> + * virtio-blk server in host kernel.
> + */
> +
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/vhost.h>
> +#include <linux/virtio_blk.h>
> +#include <linux/eventfd.h>
> +#include <linux/mutex.h>
> +#include <linux/file.h>
> +#include <linux/mmu_context.h>
> +#include <linux/anon_inodes.h>
> +#include <linux/kthread.h>
> +#include <linux/blkdev.h>
> +
> +#include "vhost.h"
> +
> +#define BLK_HDR 0
> +
> +enum {
> + VHOST_BLK_VQ_REQ = 0,
> + VHOST_BLK_VQ_MAX = 1,
> +};
> +
> +struct vhost_blk_req {
> + u16 head;
> + u8 *status;
> +};
> +
> +struct vhost_blk {
> + struct task_struct *worker_host_kick;
> + struct task_struct *worker;
> + struct vhost_blk_req *reqs;
> + struct vhost_virtqueue vq;
> + struct eventfd_ctx *ectx;
> + struct io_event *ioevent;
> + struct kioctx *ioctx;
> + struct vhost_dev dev;
> + struct file *efile;
> + u64 ioevent_nr;
> + bool stop;
> +};
> +
> +static inline int vhost_blk_read_events(struct vhost_blk *blk, long nr)
> +{
> + mm_segment_t old_fs = get_fs();
> + int ret;
> +
> + set_fs(KERNEL_DS);
> + ret = read_events(blk->ioctx, nr, nr, blk->ioevent, NULL);
> + set_fs(old_fs);
> +
> + return ret;
> +}
> +
> +static int vhost_blk_setup(struct vhost_blk *blk)
> +{
> + struct kioctx *ctx;
> +
> + if (blk->ioctx)
> + return 0;
> +
> + blk->ioevent_nr = blk->vq.num;
> + ctx = ioctx_alloc(blk->ioevent_nr);
> + if (IS_ERR(ctx)) {
> + pr_err("Failed to ioctx_alloc");
> + return PTR_ERR(ctx);
> + }
Not that it's very likely that ioctx_alloc will fail in practice.
There's a fixed number of events that can be allocated that's currently
0x10000. If you have a ring queue size of 1024 (which is normal) then
that limits you to 64 vhost-blk devices.
Realistically, I don't think you can only do aio with vhost-blk because
of this (and many other) limitations. It's necessary to be able to fall
back to a thread pool because AIO cannot be relied upon.
> + put_ioctx(ctx);
> + blk->ioctx = ctx;
> +
> + blk->ioevent = kmalloc(sizeof(struct io_event) * blk->ioevent_nr,
> + GFP_KERNEL);
> + if (!blk->ioevent) {
> + pr_err("Failed to allocate memory for io_events");
> + return -ENOMEM;
> + }
> +
> + blk->reqs = kmalloc(sizeof(struct vhost_blk_req) * blk->ioevent_nr,
> + GFP_KERNEL);
> + if (!blk->reqs) {
> + pr_err("Failed to allocate memory for vhost_blk_req");
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +static inline int vhost_blk_set_status(struct vhost_blk *blk, u8 *statusp,
> + u8 status)
> +{
> + if (copy_to_user(statusp, &status, sizeof(status))) {
> + vq_err(&blk->vq, "Failed to write status\n");
> + vhost_discard_vq_desc(&blk->vq, 1);
> + return -EFAULT;
> + }
> +
> + return 0;
> +}
> +
> +static void vhost_blk_enable_vq(struct vhost_blk *blk,
> + struct vhost_virtqueue *vq)
> +{
> + wake_up_process(blk->worker_host_kick);
> +}
> +
> +static int vhost_blk_io_submit(struct vhost_blk *blk, struct file *file,
> + struct vhost_blk_req *req,
> + struct iovec *iov, u64 nr_vecs, loff_t offset,
> + int opcode)
> +{
> + struct kioctx *ioctx = blk->ioctx;
> + mm_segment_t oldfs = get_fs();
> + struct kiocb_batch batch;
> + struct blk_plug plug;
> + struct kiocb *iocb;
> + int ret;
> +
> + if (!try_get_ioctx(ioctx)) {
> + pr_info("Failed to get ioctx");
> + return -EAGAIN;
> + }
> +
> + atomic_long_inc_not_zero(&file->f_count);
> + eventfd_ctx_get(blk->ectx);
> +
> + /* TODO: batch to 1 is not good! */
> + kiocb_batch_init(&batch, 1);
> + blk_start_plug(&plug);
> +
> + iocb = aio_get_req(ioctx, &batch);
> + if (unlikely(!iocb)) {
> + ret = -EAGAIN;
> + goto out;
> + }
> +
> + iocb->ki_filp = file;
> + iocb->ki_pos = offset;
> + iocb->ki_buf = (void *)iov;
> + iocb->ki_left = nr_vecs;
> + iocb->ki_nbytes = nr_vecs;
> + iocb->ki_opcode = opcode;
> + iocb->ki_obj.user = req;
> + iocb->ki_eventfd = blk->ectx;
> +
> + set_fs(KERNEL_DS);
> + ret = aio_setup_iocb(iocb, false);
> + set_fs(oldfs);
> + if (unlikely(ret))
> + goto out_put_iocb;
> +
> + spin_lock_irq(&ioctx->ctx_lock);
> + if (unlikely(ioctx->dead)) {
> + spin_unlock_irq(&ioctx->ctx_lock);
> + ret = -EINVAL;
> + goto out_put_iocb;
> + }
> + aio_run_iocb(iocb);
> + spin_unlock_irq(&ioctx->ctx_lock);
> +
> + aio_put_req(iocb);
> +
> + blk_finish_plug(&plug);
> + kiocb_batch_free(ioctx, &batch);
> + put_ioctx(ioctx);
> +
> + return ret;
> +out_put_iocb:
> + aio_put_req(iocb); /* Drop extra ref to req */
> + aio_put_req(iocb); /* Drop I/O ref to req */
> +out:
> + put_ioctx(ioctx);
> + return ret;
> +}
> +
> +static void vhost_blk_flush(struct vhost_blk *blk)
> +{
> + vhost_poll_flush(&blk->vq.poll);
> +}
> +
> +static struct file *vhost_blk_stop_vq(struct vhost_blk *blk,
> + struct vhost_virtqueue *vq)
> +{
> + struct file *file;
> +
> + mutex_lock(&vq->mutex);
> + file = rcu_dereference_protected(vq->private_data,
> + lockdep_is_held(&vq->mutex));
> + rcu_assign_pointer(vq->private_data, NULL);
> + mutex_unlock(&vq->mutex);
> +
> + return file;
> +
> +}
> +
> +static inline void vhost_blk_stop(struct vhost_blk *blk, struct file **file)
> +{
> +
> + *file = vhost_blk_stop_vq(blk, &blk->vq);
> +}
> +
> +/* Handle guest request */
> +static int vhost_blk_do_req(struct vhost_virtqueue *vq,
> + struct virtio_blk_outhdr *hdr,
> + u16 head, u16 out, u16 in,
> + struct file *file)
> +{
> + struct vhost_blk *blk = container_of(vq->dev, struct vhost_blk, dev);
> + struct iovec *iov = &vq->iov[BLK_HDR + 1];
> + loff_t offset = hdr->sector << 9;
> + struct vhost_blk_req *req;
> + u64 nr_vecs;
> + int ret = 0;
> + u8 status;
> +
> + if (hdr->type == VIRTIO_BLK_T_IN || hdr->type == VIRTIO_BLK_T_GET_ID)
> + nr_vecs = in - 1;
> + else
> + nr_vecs = out - 1;
> +
> + req = &blk->reqs[head];
> + req->head = head;
> + req->status = blk->vq.iov[nr_vecs + 1].iov_base;
> +
> + switch (hdr->type) {
> + case VIRTIO_BLK_T_OUT:
> + ret = vhost_blk_io_submit(blk, file, req, iov, nr_vecs, offset,
> + IOCB_CMD_PWRITEV);
> + break;
> + case VIRTIO_BLK_T_IN:
> + ret = vhost_blk_io_submit(blk, file, req, iov, nr_vecs, offset,
> + IOCB_CMD_PREADV);
> + break;
> + case VIRTIO_BLK_T_FLUSH:
> + ret = vfs_fsync(file, 1);
> + status = ret < 0 ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
> + ret = vhost_blk_set_status(blk, req->status, status);
> + if (!ret)
> + vhost_add_used_and_signal(&blk->dev, vq, head, ret);
> + break;
> + case VIRTIO_BLK_T_GET_ID:
> + /* TODO: need a real ID string */
> + ret = snprintf(vq->iov[BLK_HDR + 1].iov_base,
> + VIRTIO_BLK_ID_BYTES, "VHOST-BLK-DISK");
> + status = ret < 0 ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
> + ret = vhost_blk_set_status(blk, req->status, status);
> + if (!ret)
> + vhost_add_used_and_signal(&blk->dev, vq, head,
> + VIRTIO_BLK_ID_BYTES);
> + break;
> + default:
> + pr_warn("Unsupported request type %d\n", hdr->type);
> + vhost_discard_vq_desc(vq, 1);
> + ret = -EFAULT;
> + break;
> + }
There doesn't appear to be any error handling in the event that
vhost_blk_io_submit fails. It would appear that you leak the ring queue
entry since you never push anything onto the used queue.
I think you need to handle io_submit() failing too with EAGAIN. Relying
on min nr_events == queue_size seems like a dangerous assumption to me
particularly since queue_size tends to be very large and max_nr_events
is a fixed size global pool.
To properly handle EAGAIN, you effectively need to implement flow
control and back off reading the virtqueue until you can submit requests again.
Of course, the million dollar question is why would using AIO in the
kernel be faster than using AIO in userspace?
When using eventfd, there is no heavy weight exit on the notify path.
It should just be the difference between scheduling a kernel thread vs
scheduling a userspace thread. There's simply no way that that's a 60%
difference in performance.
Regards,
Anthony Liguori
^ permalink raw reply
* Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages
From: Rafael Aquini @ 2012-07-19 12:32 UTC (permalink / raw)
To: Andrew Morton
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Michael S. Tsirkin,
linux-kernel, virtualization, linux-mm, Andi Kleen, Minchan Kim,
Rafael Aquini
In-Reply-To: <20120718182944.24f59012.akpm@linux-foundation.org>
On Wed, Jul 18, 2012 at 06:29:44PM -0700, Andrew Morton wrote:
> On Wed, 18 Jul 2012 22:00:48 -0300 Rafael Aquini <aquini@redhat.com> wrote:
>
> > > So the function needs a better name - one which communicates that it is
> > > a balloon page *for the purposes of processing by the compaction code*.
> > > Making the function private to compaction.c would help with that, if
> > > feasible.
> > >
> >
> > How about this (adjusted) approach:
>
> it fails checkpatch ;)
>
Ugh! it fails due to a lacking whitespace... will fix that right away.
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -1629,8 +1629,7 @@ static inline unsigned int debug_guardpage_minorder(void)
> > { return 0; }
> > static inline bool page_is_guard(struct page *page) { return false; }
> > #endif /* CONFIG_DEBUG_PAGEALLOC */
> >
> > -#if (defined(CONFIG_VIRTIO_BALLOON) || \
> > - defined(CONFIG_VIRTIO_BALLOON_MODULE)) && defined(CONFIG_COMPACTION)
> > +#if (defined(CONFIG_VIRTIO_BALLOON) ||defined(CONFIG_VIRTIO_BALLOON_MODULE))
> > extern bool putback_balloon_page(struct page *);
> > extern struct address_space *balloon_mapping;
> >
> > @@ -1638,11 +1637,13 @@ static inline bool is_balloon_page(struct page *page)
> > {
> > return (page->mapping && page->mapping == balloon_mapping);
> > }
> > +#if defined(CONFIG_COMPACTION)
> > +static inline bool balloon_compaction_enabled(void) { return true; }
> > #else
> > -static inline bool is_balloon_page(struct page *page) { return false; }
> > -static inline bool isolate_balloon_page(struct page *page) { return false; }
> > -static inline bool putback_balloon_page(struct page *page) { return false; }
> > -#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && COMPACTION */
> > +static inline bool putback_balloon_page(struct page *page) { return false; }
> > +static inline bool balloon_compaction_enabled(void) { return false; }
> > +#endif /* CONFIG_COMPACTION */
> > +#endif /* (CONFIG_VIRTIO_BALLOON || CONFIG_VIRTIO_BALLOON_MODULE) */
> >
> > #endif /* __KERNEL__ */
> > #endif /* _LINUX_MM_H */
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 59c7bc5..f5f6a7d 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -78,7 +78,8 @@ void putback_lru_pages(struct list_head *l)
> > list_del(&page->lru);
> > dec_zone_page_state(page, NR_ISOLATED_ANON +
> > page_is_file_cache(page));
> > - if (unlikely(is_balloon_page(page)))
> > + if (unlikely(is_balloon_page(page)) &&
> > + balloon_compaction_enabled())
>
> well, that helps readability. But what does is_balloon_page() return
> when invoked on a balloon page when CONFIG_COMPACTION=n? False,
> methinks.
It will (now) return the right thing accordingly to the page->mapping tests.
>
> I think the code as you previously had it was OK, but the
> is_balloon_page() name is misleading. It really wants to be called
> is_potentially_compactible_balloon_page() :( Maybe rename it to
> compactible_balloon_page()?
With all due respect, sir, I don't believe renaming it is the right thing to do.
My major supporting reason is since Lumpy Reclaim is already evicted it looks
natural CONFIG_COMPACTION=y becoming a permanent feature, thus making that
preprocessor test useless and the renamed function signature nonsense, IMHO.
That's why I keep respectfully figthing against your argument.
Here goes another suggestion, to keep is_balloon_page() name as is. This way I
believe all concerns are potentially addressed, as there's no implicit and
misleading relationship between is_balloon_page and CONFIG_COMPACTION=y anymore,
as well as there are no potential build breakages due to (unexpected) config options.
diff --git a/include/linux/mm.h b/include/linux/mm.h
index b36d08c..e29ad44 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1629,5 +1629,30 @@ static inline unsigned int debug_guardpage_minorder(void)
{ return 0; }
static inline bool page_is_guard(struct page *page) { return false; }
#endif /* CONFIG_DEBUG_PAGEALLOC */
+#if (defined(CONFIG_VIRTIO_BALLOON) || defined(CONFIG_VIRTIO_BALLOON_MODULE))
+extern bool putback_balloon_page(struct page *);
+extern struct address_space *balloon_mapping;
+
+static inline bool is_balloon_page(struct page *page)
+{
+ return (page->mapping && page->mapping == balloon_mapping);
+}
+
+static inline bool balloon_compaction_enabled(void)
+{
+#if defined(CONFIG_COMPACTION)
+ return true;
+#else
+ return false;
+#endif /* CONFIG_COMPACTION */
+}
+
+#else
+static inline bool isolate_balloon_page(struct page *page) { return false; }
+static inline bool putback_balloon_page(struct page *page) { return false; }
+static inline bool is_balloon_page(struct page *page) { return false; }
+static inline bool balloon_compaction_enabled(void) { return false; }
+#endif /* (CONFIG_VIRTIO_BALLOON || CONFIG_VIRTIO_BALLOON_MODULE) */
+
#endif /* __KERNEL__ */
#endif /* _LINUX_MM_H */
^ permalink raw reply related
* Re: [RFC-v2 0/4] tcm_vhost+cmwq fabric driver code for-3.6
From: Paolo Bonzini @ 2012-07-19 7:30 UTC (permalink / raw)
To: James Bottomley
Cc: Jens Axboe, Anthony Liguori, kvm-devel, linux-scsi,
Michael S. Tsirkin, lf-virt, Christoph Hellwig, Anthony Liguori,
target-devel, Zhi Yong Wu, Christoph Hellwig, Stefan Hajnoczi
In-Reply-To: <1342682881.3059.3.camel@dabdike.int.hansenpartnership.com>
Il 19/07/2012 09:28, James Bottomley ha scritto:
>> > INQUIRY responses (at least vendor/product/type) should not change.
> INQUIRY responses often change for arrays because a firmware upgrade
> enables new features and new features have to declare themselves,
> usually in the INQUIRY data. What you mean, I think, is that previously
> exposed features in INQUIRY data, as well as strings
> (vendor/product/type, as you say), shouldn't change, but unexposed data
> (read 0 in the fields) may.
What I meant is that it's unlikely that Windows fingerprinting is using
anything but vendor/product/type, because everything else can change.
Paolo
^ permalink raw reply
* Re: [RFC-v2 0/4] tcm_vhost+cmwq fabric driver code for-3.6
From: Paolo Bonzini @ 2012-07-19 6:00 UTC (permalink / raw)
To: Anthony Liguori
Cc: Jens Axboe, Christoph Hellwig, kvm-devel, linux-scsi,
Michael S. Tsirkin, lf-virt, James Bottomley, Anthony Liguori,
target-devel, Zhi Yong Wu, Christoph Hellwig, Stefan Hajnoczi
In-Reply-To: <50070A8D.7020203@us.ibm.com>
Il 18/07/2012 21:12, Anthony Liguori ha scritto:
> Is that true for all OSes? Linux may handle things gracefully if UNMAP
> starts throwing errors but that doesn't mean that Windows will.
There is so much USB crap (not just removable, think of ATA-to-USB
enclosures) that they have to deal with pretty much everything.
> Windows does this with a points system and I do believe that INQUIRY
> responses from any local disks are included in this tally.
INQUIRY responses (at least vendor/product/type) should not change.
Paolo
^ permalink raw reply
* Re: [PATCH 1/1] Drivers: hv: Change the hex constant to a decimal constant
From: Jeff Garzik @ 2012-07-19 4:04 UTC (permalink / raw)
To: K. Y. Srinivasan, gregkh; +Cc: linux-kernel, devel, virtualization, olaf, apw
In-Reply-To: <1342652245-931-1-git-send-email-kys@microsoft.com>
On 07/18/2012 06:57 PM, K. Y. Srinivasan wrote:
> The hex constant chosen for HV_LINUX_GUEST_ID_HI was offensive, update
> to use
> the decimal equivalent instead.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
Acked-by: Jeff Garzik <jgarzik@redhat.com>
^ permalink raw reply
* RE: 0xB16B00B5? Really? (was Re: Move hyperv out of the drivers/staging/ directory)
From: KY Srinivasan @ 2012-07-19 2:11 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Greg KH (gregkh@linuxfoundation.org),
virtualization@lists.osdl.org, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org
In-Reply-To: <4FFFF711.8040003@redhat.com>
> -----Original Message-----
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
> Bonzini
> Sent: Friday, July 13, 2012 6:23 AM
> To: KY Srinivasan
> Cc: Greg KH; devel@linuxdriverproject.org; linux-kernel@vger.kernel.org;
> virtualization@lists.osdl.org
> Subject: 0xB16B00B5? Really? (was Re: Move hyperv out of the drivers/staging/
> directory)
>
> Il 04/10/2011 21:34, Greg KH ha scritto:
> > diff --git a/drivers/staging/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> > similarity index 99%
> > rename from drivers/staging/hv/hyperv_vmbus.h
> > rename to drivers/hv/hyperv_vmbus.h
> > index 3d2d836..8261cb6 100644
> > --- a/drivers/staging/hv/hyperv_vmbus.h
> > +++ b/drivers/hv/hyperv_vmbus.h
> > @@ -28,8 +28,7 @@
> > #include <linux/list.h>
> > #include <asm/sync_bitops.h>
> > #include <linux/atomic.h>
> > -
> > -#include "hyperv.h"
> > +#include <linux/hyperv.h>
> >
> > /*
> > * The below CPUID leaves are present if
> VersionAndFeatures.HypervisorPresent
>
> git's rename detection snips away this gem:
>
> +#define HV_LINUX_GUEST_ID_LO 0x00000000
> +#define HV_LINUX_GUEST_ID_HI 0xB16B00B5
> +#define HV_LINUX_GUEST_ID (((u64)HV_LINUX_GUEST_ID_HI
> << 32) | \
> + HV_LINUX_GUEST_ID_LO)
>
> Somone was trying to be funny, I guess.
>
> KY, I suppose you have access to Hyper-V code or can ask someone who does.
> Is this signature actually used in the Hyper-V host code?
Paolo,
As I noted earlier, this is just a guest ID that needs to be registered with the
hypervisor. Thanks for reporting this issue and on behalf of Microsoft, I would
like to apologize for this offensive string. I have submitted a patch to fix this issue.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages
From: Andrew Morton @ 2012-07-19 1:29 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Michael S. Tsirkin,
linux-kernel, virtualization, linux-mm, Andi Kleen, Minchan Kim,
Rafael Aquini
In-Reply-To: <20120719010047.GD2313@t510.redhat.com>
On Wed, 18 Jul 2012 22:00:48 -0300 Rafael Aquini <aquini@redhat.com> wrote:
> > So the function needs a better name - one which communicates that it is
> > a balloon page *for the purposes of processing by the compaction code*.
> > Making the function private to compaction.c would help with that, if
> > feasible.
> >
>
> How about this (adjusted) approach:
it fails checkpatch ;)
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1629,8 +1629,7 @@ static inline unsigned int debug_guardpage_minorder(void)
> { return 0; }
> static inline bool page_is_guard(struct page *page) { return false; }
> #endif /* CONFIG_DEBUG_PAGEALLOC */
>
> -#if (defined(CONFIG_VIRTIO_BALLOON) || \
> - defined(CONFIG_VIRTIO_BALLOON_MODULE)) && defined(CONFIG_COMPACTION)
> +#if (defined(CONFIG_VIRTIO_BALLOON) ||defined(CONFIG_VIRTIO_BALLOON_MODULE))
> extern bool putback_balloon_page(struct page *);
> extern struct address_space *balloon_mapping;
>
> @@ -1638,11 +1637,13 @@ static inline bool is_balloon_page(struct page *page)
> {
> return (page->mapping && page->mapping == balloon_mapping);
> }
> +#if defined(CONFIG_COMPACTION)
> +static inline bool balloon_compaction_enabled(void) { return true; }
> #else
> -static inline bool is_balloon_page(struct page *page) { return false; }
> -static inline bool isolate_balloon_page(struct page *page) { return false; }
> -static inline bool putback_balloon_page(struct page *page) { return false; }
> -#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && COMPACTION */
> +static inline bool putback_balloon_page(struct page *page) { return false; }
> +static inline bool balloon_compaction_enabled(void) { return false; }
> +#endif /* CONFIG_COMPACTION */
> +#endif /* (CONFIG_VIRTIO_BALLOON || CONFIG_VIRTIO_BALLOON_MODULE) */
>
> #endif /* __KERNEL__ */
> #endif /* _LINUX_MM_H */
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 59c7bc5..f5f6a7d 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -78,7 +78,8 @@ void putback_lru_pages(struct list_head *l)
> list_del(&page->lru);
> dec_zone_page_state(page, NR_ISOLATED_ANON +
> page_is_file_cache(page));
> - if (unlikely(is_balloon_page(page)))
> + if (unlikely(is_balloon_page(page)) &&
> + balloon_compaction_enabled())
well, that helps readability. But what does is_balloon_page() return
when invoked on a balloon page when CONFIG_COMPACTION=n? False,
methinks.
I think the code as you previously had it was OK, but the
is_balloon_page() name is misleading. It really wants to be called
is_potentially_compactible_balloon_page() :( Maybe rename it to
compactible_balloon_page()?
^ permalink raw reply
* Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages
From: Rafael Aquini @ 2012-07-19 1:00 UTC (permalink / raw)
To: Andrew Morton
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Michael S. Tsirkin,
linux-kernel, virtualization, linux-mm, Andi Kleen, Minchan Kim,
Rafael Aquini
In-Reply-To: <20120718161239.9449e6b5.akpm@linux-foundation.org>
On Wed, Jul 18, 2012 at 04:12:39PM -0700, Andrew Morton wrote:
> On Wed, 18 Jul 2012 20:07:07 -0300
> Rafael Aquini <aquini@redhat.com> wrote:
>
> > >
> > > > +}
> > > > +#else
> > > > +static inline bool is_balloon_page(struct page *page) { return false; }
> > > > +static inline bool isolate_balloon_page(struct page *page) { return false; }
> > > > +static inline bool putback_balloon_page(struct page *page) { return false; }
> > > > +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && COMPACTION */
> > >
> > > This means that if CONFIG_VIRTIO_BALLOON=y and CONFIG_COMPACTION=n,
> > > is_balloon_page() will always return NULL. IOW, no pages are balloon
> > > pages! This is wrong.
> > >
> > I believe it's right, actually, as we can see CONFIG_COMPACTION=n associated with
> > CONFIG_MIGRATION=y (and CONFIG_VIRTIO_BALLOON=y).
> > For such config case we cannot perform the is_balloon_page() test branches
> > placed on mm/migration.c
>
> No, it isn't right. Look at the name: "is_balloon_page". If a caller
> runs is_balloon_page() against a balloon page with CONFIG_COMPACTION=n
> then they will get "false", which is incorrect.
>
You're right, I got your point.
> So the function needs a better name - one which communicates that it is
> a balloon page *for the purposes of processing by the compaction code*.
> Making the function private to compaction.c would help with that, if
> feasible.
>
How about this (adjusted) approach:
diff --git a/include/linux/mm.h b/include/linux/mm.h
index b94f17a..02a8f80 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1629,8 +1629,7 @@ static inline unsigned int debug_guardpage_minorder(void)
{ return 0; }
static inline bool page_is_guard(struct page *page) { return false; }
#endif /* CONFIG_DEBUG_PAGEALLOC */
-#if (defined(CONFIG_VIRTIO_BALLOON) || \
- defined(CONFIG_VIRTIO_BALLOON_MODULE)) && defined(CONFIG_COMPACTION)
+#if (defined(CONFIG_VIRTIO_BALLOON) ||defined(CONFIG_VIRTIO_BALLOON_MODULE))
extern bool putback_balloon_page(struct page *);
extern struct address_space *balloon_mapping;
@@ -1638,11 +1637,13 @@ static inline bool is_balloon_page(struct page *page)
{
return (page->mapping && page->mapping == balloon_mapping);
}
+#if defined(CONFIG_COMPACTION)
+static inline bool balloon_compaction_enabled(void) { return true; }
#else
-static inline bool is_balloon_page(struct page *page) { return false; }
-static inline bool isolate_balloon_page(struct page *page) { return false; }
-static inline bool putback_balloon_page(struct page *page) { return false; }
-#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && COMPACTION */
+static inline bool putback_balloon_page(struct page *page) { return false; }
+static inline bool balloon_compaction_enabled(void) { return false; }
+#endif /* CONFIG_COMPACTION */
+#endif /* (CONFIG_VIRTIO_BALLOON || CONFIG_VIRTIO_BALLOON_MODULE) */
#endif /* __KERNEL__ */
#endif /* _LINUX_MM_H */
diff --git a/mm/migrate.c b/mm/migrate.c
index 59c7bc5..f5f6a7d 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -78,7 +78,8 @@ void putback_lru_pages(struct list_head *l)
list_del(&page->lru);
dec_zone_page_state(page, NR_ISOLATED_ANON +
page_is_file_cache(page));
- if (unlikely(is_balloon_page(page)))
+ if (unlikely(is_balloon_page(page)) &&
+ balloon_compaction_enabled())
WARN_ON(!putback_balloon_page(page));
else
putback_lru_page(page);
@@ -786,7 +787,7 @@ static int __unmap_and_move(struct page *page, struct page
*newpage,
}
}
- if (is_balloon_page(page)) {
+ if (is_balloon_page(page) && balloon_compaction_enabled()) {
/*
* A ballooned page does not need any special attention from
* physical to virtual reverse mapping procedures.
@@ -867,7 +868,7 @@ static int unmap_and_move(new_page_t get_new_page, unsigned
long private,
rc = __unmap_and_move(page, newpage, force, offlining, mode);
- if (is_balloon_page(newpage)) {
+ if (is_balloon_page(newpage) && balloon_compaction_enabled()) {
/*
* A ballooned page has been migrated already. Now, it is the
* time to wrap-up counters, handle the old page back to Buddy
^ permalink raw reply related
* Re: [PATCH v4 2/3] virtio_balloon: introduce migration primitives to balloon pages
From: Rafael Aquini @ 2012-07-18 23:16 UTC (permalink / raw)
To: Andrew Morton
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Michael S. Tsirkin,
linux-kernel, virtualization, linux-mm, Andi Kleen, Minchan Kim,
Rafael Aquini
In-Reply-To: <20120718154908.14704344.akpm@linux-foundation.org>
Howdy Andrew,
Thanks for taking the time to go through this work and provide me with such good
feedback.
On Wed, Jul 18, 2012 at 03:49:08PM -0700, Andrew Morton wrote:
> On Tue, 17 Jul 2012 13:50:42 -0300
> Rafael Aquini <aquini@redhat.com> wrote:
>
> > Besides making balloon pages movable at allocation time and introducing
> > the necessary primitives to perform balloon page migration/compaction,
> > this patch also introduces the following locking scheme to provide the
> > proper synchronization and protection for struct virtio_balloon elements
> > against concurrent accesses due to parallel operations introduced by
> > memory compaction / page migration.
> > - balloon_lock (mutex) : synchronizes the access demand to elements of
> > struct virtio_balloon and its queue operations;
> > - pages_lock (spinlock): special protection to balloon pages list against
> > concurrent list handling operations;
> >
> > ...
> >
> > + balloon_mapping->a_ops = &virtio_balloon_aops;
> > + balloon_mapping->backing_dev_info = (void *)vb;
>
> hoo boy. We're making page->mapping->backing_dev_info point at a
> struct which does not have type `struct backing_dev_info'. And then we
> are exposing that page to core MM functions. So we're hoping that core
> MM will never walk down page->mapping->backing_dev_info and explode.
>
> That's nasty, hacky and fragile.
Shame on me, on this one.
Mea culpa: I took this approach, originally, because I stuck the spinlock within
the struct virtio_balloon and this was the easiest way to recover it just by
having the page pointer. I did this stupidity because on earlier stages of this
patch some functions that demanded access to that list spinlock were placed
outside the balloon driver's code -- this is a left-over
(I know, it's a total lame excuse, but it's the truth)
This is easily fixable, however, as the balloon page list spinlock is now only
being accessed within driver's code and it can be declared outside the struct.
^ permalink raw reply
* Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages
From: Andrew Morton @ 2012-07-18 23:12 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Michael S. Tsirkin,
linux-kernel, virtualization, linux-mm, Andi Kleen, Minchan Kim,
Rafael Aquini
In-Reply-To: <20120718230706.GB2313@t510.redhat.com>
On Wed, 18 Jul 2012 20:07:07 -0300
Rafael Aquini <aquini@redhat.com> wrote:
> >
> > > +}
> > > +#else
> > > +static inline bool is_balloon_page(struct page *page) { return false; }
> > > +static inline bool isolate_balloon_page(struct page *page) { return false; }
> > > +static inline bool putback_balloon_page(struct page *page) { return false; }
> > > +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && COMPACTION */
> >
> > This means that if CONFIG_VIRTIO_BALLOON=y and CONFIG_COMPACTION=n,
> > is_balloon_page() will always return NULL. IOW, no pages are balloon
> > pages! This is wrong.
> >
> I believe it's right, actually, as we can see CONFIG_COMPACTION=n associated with
> CONFIG_MIGRATION=y (and CONFIG_VIRTIO_BALLOON=y).
> For such config case we cannot perform the is_balloon_page() test branches
> placed on mm/migration.c
No, it isn't right. Look at the name: "is_balloon_page". If a caller
runs is_balloon_page() against a balloon page with CONFIG_COMPACTION=n
then they will get "false", which is incorrect.
So the function needs a better name - one which communicates that it is
a balloon page *for the purposes of processing by the compaction code*.
Making the function private to compaction.c would help with that, if
feasible.
^ permalink raw reply
* Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages
From: Rafael Aquini @ 2012-07-18 23:07 UTC (permalink / raw)
To: Andrew Morton
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Michael S. Tsirkin,
linux-kernel, virtualization, linux-mm, Andi Kleen, Minchan Kim,
Rafael Aquini
In-Reply-To: <20120718154605.cb0591bc.akpm@linux-foundation.org>
Howdy Andrew,
Thanks for taking the time to go through this work and provide me with such good
feedback.
On Wed, Jul 18, 2012 at 03:46:05PM -0700, Andrew Morton wrote:
> On Tue, 17 Jul 2012 13:50:41 -0300
> Rafael Aquini <aquini@redhat.com> wrote:
>
> > This patch introduces the helper functions as well as the necessary changes
> > to teach compaction and migration bits how to cope with pages which are
> > part of a guest memory balloon, in order to make them movable by memory
> > compaction procedures.
> >
> > ...
> >
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -1629,5 +1629,20 @@ static inline unsigned int debug_guardpage_minorder(void) { return 0; }
> > static inline bool page_is_guard(struct page *page) { return false; }
> > #endif /* CONFIG_DEBUG_PAGEALLOC */
> >
> > +#if (defined(CONFIG_VIRTIO_BALLOON) || \
> > + defined(CONFIG_VIRTIO_BALLOON_MODULE)) && defined(CONFIG_COMPACTION)
> > +extern bool putback_balloon_page(struct page *);
> > +extern struct address_space *balloon_mapping;
> > +
> > +static inline bool is_balloon_page(struct page *page)
> > +{
> > + return (page->mapping == balloon_mapping) ? true : false;
>
> You can simply do
>
> return page->mapping == balloon_mapping;
Yes, I will do
return (page->mapping && page->mapping == balloon_mapping);
actually. I just got a case of NULL pointer deref while running on bare-metal
with no balloon driver loaded.
>
> > +}
> > +#else
> > +static inline bool is_balloon_page(struct page *page) { return false; }
> > +static inline bool isolate_balloon_page(struct page *page) { return false; }
> > +static inline bool putback_balloon_page(struct page *page) { return false; }
> > +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && COMPACTION */
>
> This means that if CONFIG_VIRTIO_BALLOON=y and CONFIG_COMPACTION=n,
> is_balloon_page() will always return NULL. IOW, no pages are balloon
> pages! This is wrong.
>
I believe it's right, actually, as we can see CONFIG_COMPACTION=n associated with
CONFIG_MIGRATION=y (and CONFIG_VIRTIO_BALLOON=y).
For such config case we cannot perform the is_balloon_page() test branches
placed on mm/migration.c
> I'm not sure what to do about this, apart from renaming the function to
> is_compactible_balloon_page() or something similarly aawkward.
>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox