Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
Andrew Morton writes:
> On Wed, 10 Oct 2012 16:34:37 -0700
> Jeremy Fitzhardinge wrote:
>
>> On 10/09/2012 06:14 PM, Andrew Morton wrote:
>> > On Wed, 10 Oct 2012 00:09:12 + KY Srinivasan
>> > wrote:
>> >
>> + if (!pg) {
>> + *alloc_error = true;
>> + return i * alloc_unit;
>> + }
>> +
>> + totalram_pages -= alloc_unit;
>> >>> Well, I'd consider totalram_pages to be an mm-private thing which drivers
>> >>> shouldn't muck with. Why is this done?
>> >> By modifying the totalram_pages, the information presented in
>> >> /proc/meminfo
>> >> correctly reflects what is currently assigned to the guest (MemTotal).
>> > eh? /proc/meminfo:MemTotal tells you the total memory in the machine.
>> > The only thing which should change it after boot is memory hotplug.
>> [...]
>> > Why on earth do balloon drivers do this? If the amount of memory which
>> > is consumed by balloons is interesting then it should be exported via a
>> > standalone metric, not by mucking with totalram_pages.
>>
>> Balloon drivers are trying to fake a form of page-by-page memory
>> hotplug. When they allocate memory from the kernel, they're actually
>> giving the pages back to the hypervisor to redistribute to other
>> guests. They reduce totalram_pages to try and reflect that the memory
>> is no longer the kernel (in Xen, at least, the pfns will no longer have
>> any physical page underlying them).
>>
>> I agree this is pretty ugly; it would be nice to have some better
>> interface to indicate what's going on. At one point I tried to use the
>> memory hotplug interfaces for larger-scale dynamic transfers of memory
>> between a domain and the host, but when I last looked at it, it was too
>> coarse grained and heavyweight to replace the balloon mechanism.
>>
>
> urgh.
>
> I suppose the least we can do here would be to stop directly dinking
> with totalram_pages and create some sort of interface for this
> operation. That interface would run the memory hotplug notifier so
> that code which cares about changes in the amount of physical memory
> can take appropriate steps. The implications would be that the balloon
> drivers would need to call this interface at low frequency (ie: batch
> the pages) and in some reasonably lock-free context.
>
> I guess that's solving a non-problem at this stage.
Yep. drivers/virtio/virtio_balloon manipulates it too. This, it's best
practice!
Cheers,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
Andrew Morton a...@linux-foundation.org writes:
On Wed, 10 Oct 2012 16:34:37 -0700
Jeremy Fitzhardinge jer...@goop.org wrote:
On 10/09/2012 06:14 PM, Andrew Morton wrote:
On Wed, 10 Oct 2012 00:09:12 + KY Srinivasan k...@microsoft.com
wrote:
+ if (!pg) {
+ *alloc_error = true;
+ return i * alloc_unit;
+ }
+
+ totalram_pages -= alloc_unit;
Well, I'd consider totalram_pages to be an mm-private thing which drivers
shouldn't muck with. Why is this done?
By modifying the totalram_pages, the information presented in
/proc/meminfo
correctly reflects what is currently assigned to the guest (MemTotal).
eh? /proc/meminfo:MemTotal tells you the total memory in the machine.
The only thing which should change it after boot is memory hotplug.
[...]
Why on earth do balloon drivers do this? If the amount of memory which
is consumed by balloons is interesting then it should be exported via a
standalone metric, not by mucking with totalram_pages.
Balloon drivers are trying to fake a form of page-by-page memory
hotplug. When they allocate memory from the kernel, they're actually
giving the pages back to the hypervisor to redistribute to other
guests. They reduce totalram_pages to try and reflect that the memory
is no longer the kernel (in Xen, at least, the pfns will no longer have
any physical page underlying them).
I agree this is pretty ugly; it would be nice to have some better
interface to indicate what's going on. At one point I tried to use the
memory hotplug interfaces for larger-scale dynamic transfers of memory
between a domain and the host, but when I last looked at it, it was too
coarse grained and heavyweight to replace the balloon mechanism.
urgh.
I suppose the least we can do here would be to stop directly dinking
with totalram_pages and create some sort of interface for this
operation. That interface would run the memory hotplug notifier so
that code which cares about changes in the amount of physical memory
can take appropriate steps. The implications would be that the balloon
drivers would need to call this interface at low frequency (ie: batch
the pages) and in some reasonably lock-free context.
I guess that's solving a non-problem at this stage.
Yep. drivers/virtio/virtio_balloon manipulates it too. This, it's best
practice!
Cheers,
Rusty.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
On Wed, 10 Oct 2012 16:34:37 -0700
Jeremy Fitzhardinge wrote:
> On 10/09/2012 06:14 PM, Andrew Morton wrote:
> > On Wed, 10 Oct 2012 00:09:12 + KY Srinivasan wrote:
> >
> +if (!pg) {
> +*alloc_error = true;
> +return i * alloc_unit;
> +}
> +
> +totalram_pages -= alloc_unit;
> >>> Well, I'd consider totalram_pages to be an mm-private thing which drivers
> >>> shouldn't muck with. Why is this done?
> >> By modifying the totalram_pages, the information presented in /proc/meminfo
> >> correctly reflects what is currently assigned to the guest (MemTotal).
> > eh? /proc/meminfo:MemTotal tells you the total memory in the machine.
> > The only thing which should change it after boot is memory hotplug.
> [...]
> > Why on earth do balloon drivers do this? If the amount of memory which
> > is consumed by balloons is interesting then it should be exported via a
> > standalone metric, not by mucking with totalram_pages.
>
> Balloon drivers are trying to fake a form of page-by-page memory
> hotplug. When they allocate memory from the kernel, they're actually
> giving the pages back to the hypervisor to redistribute to other
> guests. They reduce totalram_pages to try and reflect that the memory
> is no longer the kernel (in Xen, at least, the pfns will no longer have
> any physical page underlying them).
>
> I agree this is pretty ugly; it would be nice to have some better
> interface to indicate what's going on. At one point I tried to use the
> memory hotplug interfaces for larger-scale dynamic transfers of memory
> between a domain and the host, but when I last looked at it, it was too
> coarse grained and heavyweight to replace the balloon mechanism.
>
urgh.
I suppose the least we can do here would be to stop directly dinking
with totalram_pages and create some sort of interface for this
operation. That interface would run the memory hotplug notifier so
that code which cares about changes in the amount of physical memory
can take appropriate steps. The implications would be that the balloon
drivers would need to call this interface at low frequency (ie: batch
the pages) and in some reasonably lock-free context.
I guess that's solving a non-problem at this stage.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
On 10/09/2012 06:14 PM, Andrew Morton wrote:
> On Wed, 10 Oct 2012 00:09:12 + KY Srinivasan wrote:
>
+ if (!pg) {
+ *alloc_error = true;
+ return i * alloc_unit;
+ }
+
+ totalram_pages -= alloc_unit;
>>> Well, I'd consider totalram_pages to be an mm-private thing which drivers
>>> shouldn't muck with. Why is this done?
>> By modifying the totalram_pages, the information presented in /proc/meminfo
>> correctly reflects what is currently assigned to the guest (MemTotal).
> eh? /proc/meminfo:MemTotal tells you the total memory in the machine.
> The only thing which should change it after boot is memory hotplug.
[...]
> Why on earth do balloon drivers do this? If the amount of memory which
> is consumed by balloons is interesting then it should be exported via a
> standalone metric, not by mucking with totalram_pages.
Balloon drivers are trying to fake a form of page-by-page memory
hotplug. When they allocate memory from the kernel, they're actually
giving the pages back to the hypervisor to redistribute to other
guests. They reduce totalram_pages to try and reflect that the memory
is no longer the kernel (in Xen, at least, the pfns will no longer have
any physical page underlying them).
I agree this is pretty ugly; it would be nice to have some better
interface to indicate what's going on. At one point I tried to use the
memory hotplug interfaces for larger-scale dynamic transfers of memory
between a domain and the host, but when I last looked at it, it was too
coarse grained and heavyweight to replace the balloon mechanism.
J
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
On 10/09/2012 09:44 PM, Andrew Morton wrote: > On Sun, 7 Oct 2012 16:59:46 -0700 > "K. Y. Srinivasan" wrote: > >> Add the basic balloon driver. > > hm, how many balloon drivers does one kernel need? > > Although I see that the great majority of this code is hypervisor-specific. Much like each network driver is NIC specific. You could think up a framework into which hypervisor specific balloon drivers plug in, but you'll find that in each driver 85% of the code is devoted to talking to the hypervisor, 15% is outdated comments, and the rest is a call to alloc_pages() and a call to free_pages(). -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
On 10/09/2012 09:44 PM, Andrew Morton wrote: On Sun, 7 Oct 2012 16:59:46 -0700 K. Y. Srinivasan k...@microsoft.com wrote: Add the basic balloon driver. hm, how many balloon drivers does one kernel need? Although I see that the great majority of this code is hypervisor-specific. Much like each network driver is NIC specific. You could think up a framework into which hypervisor specific balloon drivers plug in, but you'll find that in each driver 85% of the code is devoted to talking to the hypervisor, 15% is outdated comments, and the rest is a call to alloc_pages() and a call to free_pages(). -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
On 10/09/2012 06:14 PM, Andrew Morton wrote:
On Wed, 10 Oct 2012 00:09:12 + KY Srinivasan k...@microsoft.com wrote:
+ if (!pg) {
+ *alloc_error = true;
+ return i * alloc_unit;
+ }
+
+ totalram_pages -= alloc_unit;
Well, I'd consider totalram_pages to be an mm-private thing which drivers
shouldn't muck with. Why is this done?
By modifying the totalram_pages, the information presented in /proc/meminfo
correctly reflects what is currently assigned to the guest (MemTotal).
eh? /proc/meminfo:MemTotal tells you the total memory in the machine.
The only thing which should change it after boot is memory hotplug.
[...]
Why on earth do balloon drivers do this? If the amount of memory which
is consumed by balloons is interesting then it should be exported via a
standalone metric, not by mucking with totalram_pages.
Balloon drivers are trying to fake a form of page-by-page memory
hotplug. When they allocate memory from the kernel, they're actually
giving the pages back to the hypervisor to redistribute to other
guests. They reduce totalram_pages to try and reflect that the memory
is no longer the kernel (in Xen, at least, the pfns will no longer have
any physical page underlying them).
I agree this is pretty ugly; it would be nice to have some better
interface to indicate what's going on. At one point I tried to use the
memory hotplug interfaces for larger-scale dynamic transfers of memory
between a domain and the host, but when I last looked at it, it was too
coarse grained and heavyweight to replace the balloon mechanism.
J
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
On Wed, 10 Oct 2012 16:34:37 -0700
Jeremy Fitzhardinge jer...@goop.org wrote:
On 10/09/2012 06:14 PM, Andrew Morton wrote:
On Wed, 10 Oct 2012 00:09:12 + KY Srinivasan k...@microsoft.com wrote:
+if (!pg) {
+*alloc_error = true;
+return i * alloc_unit;
+}
+
+totalram_pages -= alloc_unit;
Well, I'd consider totalram_pages to be an mm-private thing which drivers
shouldn't muck with. Why is this done?
By modifying the totalram_pages, the information presented in /proc/meminfo
correctly reflects what is currently assigned to the guest (MemTotal).
eh? /proc/meminfo:MemTotal tells you the total memory in the machine.
The only thing which should change it after boot is memory hotplug.
[...]
Why on earth do balloon drivers do this? If the amount of memory which
is consumed by balloons is interesting then it should be exported via a
standalone metric, not by mucking with totalram_pages.
Balloon drivers are trying to fake a form of page-by-page memory
hotplug. When they allocate memory from the kernel, they're actually
giving the pages back to the hypervisor to redistribute to other
guests. They reduce totalram_pages to try and reflect that the memory
is no longer the kernel (in Xen, at least, the pfns will no longer have
any physical page underlying them).
I agree this is pretty ugly; it would be nice to have some better
interface to indicate what's going on. At one point I tried to use the
memory hotplug interfaces for larger-scale dynamic transfers of memory
between a domain and the host, but when I last looked at it, it was too
coarse grained and heavyweight to replace the balloon mechanism.
urgh.
I suppose the least we can do here would be to stop directly dinking
with totalram_pages and create some sort of interface for this
operation. That interface would run the memory hotplug notifier so
that code which cares about changes in the amount of physical memory
can take appropriate steps. The implications would be that the balloon
drivers would need to call this interface at low frequency (ie: batch
the pages) and in some reasonably lock-free context.
I guess that's solving a non-problem at this stage.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
> -Original Message-
> From: Andrew Morton [mailto:a...@linux-foundation.org]
> Sent: Tuesday, October 09, 2012 9:15 PM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-kernel@vger.kernel.org;
> de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com;
> a...@firstfloor.org
> Subject: Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
>
> On Wed, 10 Oct 2012 00:09:12 + KY Srinivasan wrote:
>
> > > > + if (!pg) {
> > > > + *alloc_error = true;
> > > > + return i * alloc_unit;
> > > > + }
> > > > +
> > > > + totalram_pages -= alloc_unit;
> > >
> > > Well, I'd consider totalram_pages to be an mm-private thing which drivers
> > > shouldn't muck with. Why is this done?
> >
> > By modifying the totalram_pages, the information presented in /proc/meminfo
> > correctly reflects what is currently assigned to the guest (MemTotal).
>
> eh? /proc/meminfo:MemTotal tells you the total memory in the machine.
> The only thing which should change it after boot is memory hotplug.
>
> Modifying it in this manner puts the statistic into a state know as
> "wrong". And temporarily modifying it in this fashion will cause the
> tremendous amount of initialisation code which relies upon
> totalram_pages for sizing to also enter the "wrong" state.
>
> Why on earth do balloon drivers do this? If the amount of memory which
> is consumed by balloons is interesting then it should be exported via a
> standalone metric, not by mucking with totalram_pages.
I see your point. I will get rid of the code that manipulates the
totalram_pages.
Regards,
K. Y
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
On Wed, 10 Oct 2012 00:09:12 + KY Srinivasan wrote:
> > > + if (!pg) {
> > > + *alloc_error = true;
> > > + return i * alloc_unit;
> > > + }
> > > +
> > > + totalram_pages -= alloc_unit;
> >
> > Well, I'd consider totalram_pages to be an mm-private thing which drivers
> > shouldn't muck with. Why is this done?
>
> By modifying the totalram_pages, the information presented in /proc/meminfo
> correctly reflects what is currently assigned to the guest (MemTotal).
eh? /proc/meminfo:MemTotal tells you the total memory in the machine.
The only thing which should change it after boot is memory hotplug.
Modifying it in this manner puts the statistic into a state know as
"wrong". And temporarily modifying it in this fashion will cause the
tremendous amount of initialisation code which relies upon
totalram_pages for sizing to also enter the "wrong" state.
Why on earth do balloon drivers do this? If the amount of memory which
is consumed by balloons is interesting then it should be exported via a
standalone metric, not by mucking with totalram_pages.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
> -Original Message-
> From: Andrew Morton [mailto:a...@linux-foundation.org]
> Sent: Tuesday, October 09, 2012 3:45 PM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-kernel@vger.kernel.org;
> de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com;
> a...@firstfloor.org
> Subject: Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
>
> On Sun, 7 Oct 2012 16:59:46 -0700
> "K. Y. Srinivasan" wrote:
>
> > Add the basic balloon driver.
>
> hm, how many balloon drivers does one kernel need?
>
> Although I see that the great majority of this code is hypervisor-specific.
>
> > Windows hosts dynamically manage the guest
> > memory allocation via a combination memory hot add and ballooning. Memory
> > hot add is used to grow the guest memory upto the maximum memory that can
> be
> > allocatted to the guest. Ballooning is used to both shrink as well as expand
> > up to the max memory. Supporting hot add needs additional support from the
> > host. We will support hot add when this support is available. For now,
> > by setting the VM startup memory to the VM max memory, we can use
> > ballooning alone to dynamically manage memory allocation amongst
> > competing guests on a given host.
> >
> >
> > ...
> >
> > +static int alloc_balloon_pages(struct hv_dynmem_device *dm, int
> num_pages,
> > +struct dm_balloon_response *bl_resp, int alloc_unit,
> > +bool *alloc_error)
> > +{
> > + int i = 0;
> > + struct page *pg;
> > +
> > + if (num_pages < alloc_unit)
> > + return 0;
> > +
> > + for (i = 0; (i * alloc_unit) < num_pages; i++) {
> > + if (bl_resp->hdr.size + sizeof(union dm_mem_page_range) >
> > + PAGE_SIZE)
> > + return i * alloc_unit;
> > +
> > + pg = alloc_pages(GFP_HIGHUSER | __GFP_NORETRY |
> GFP_ATOMIC |
> > + __GFP_NOMEMALLOC | __GFP_NOWARN,
> > + get_order(alloc_unit << PAGE_SHIFT));
>
> This choice of GFP flags is basically impossible to understand, so I
> suggest that a comment be added explaining it all.
>
> I'm a bit surprised at the inclusion of GFP_ATOMIC as it will a) dip
> into page reserves, whcih might be undesirable and b) won't even
> reclaim clean pages, which seems desirable. I suggest this also be
> covered in the forthcoming code comment.
I will rework these flags and add appropriate comments.
>
> drivers/misc/vmw_balloon.c seems to me to have used better choices here.
>
> > + if (!pg) {
> > + *alloc_error = true;
> > + return i * alloc_unit;
> > + }
> > +
> > + totalram_pages -= alloc_unit;
>
> Well, I'd consider totalram_pages to be an mm-private thing which drivers
> shouldn't muck with. Why is this done?
By modifying the totalram_pages, the information presented in /proc/meminfo
correctly reflects what is currently assigned to the guest (MemTotal).
>
> drivers/xen/balloon.c and drivers/virtio/virtio_balloon.c also alter
> totalram_pages, also without explaining why.
> drivers/misc/vmw_balloon.c does not.
>
> > + dm->num_pages_ballooned += alloc_unit;
> > +
> > + bl_resp->range_count++;
> > + bl_resp->range_array[i].finfo.start_page =
> > + page_to_pfn(pg);
> > + bl_resp->range_array[i].finfo.page_cnt = alloc_unit;
> > + bl_resp->hdr.size += sizeof(union dm_mem_page_range);
> > +
> > + }
> > +
> > + return num_pages;
> > +}
> >
> > ...
> >
>
>
>
Thanks for the prompt review. I will address your comments and repost the
patches soon.
If it is ok with you, I am going to keep the code that manipulates
totalram_pages
(for reasons I listed above).
Regards,
K. Y
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
On Sun, 7 Oct 2012 16:59:46 -0700
"K. Y. Srinivasan" wrote:
> Add the basic balloon driver.
hm, how many balloon drivers does one kernel need?
Although I see that the great majority of this code is hypervisor-specific.
> Windows hosts dynamically manage the guest
> memory allocation via a combination memory hot add and ballooning. Memory
> hot add is used to grow the guest memory upto the maximum memory that can be
> allocatted to the guest. Ballooning is used to both shrink as well as expand
> up to the max memory. Supporting hot add needs additional support from the
> host. We will support hot add when this support is available. For now,
> by setting the VM startup memory to the VM max memory, we can use
> ballooning alone to dynamically manage memory allocation amongst
> competing guests on a given host.
>
>
> ...
>
> +static int alloc_balloon_pages(struct hv_dynmem_device *dm, int num_pages,
> + struct dm_balloon_response *bl_resp, int alloc_unit,
> + bool *alloc_error)
> +{
> + int i = 0;
> + struct page *pg;
> +
> + if (num_pages < alloc_unit)
> + return 0;
> +
> + for (i = 0; (i * alloc_unit) < num_pages; i++) {
> + if (bl_resp->hdr.size + sizeof(union dm_mem_page_range) >
> + PAGE_SIZE)
> + return i * alloc_unit;
> +
> + pg = alloc_pages(GFP_HIGHUSER | __GFP_NORETRY | GFP_ATOMIC |
> + __GFP_NOMEMALLOC | __GFP_NOWARN,
> + get_order(alloc_unit << PAGE_SHIFT));
This choice of GFP flags is basically impossible to understand, so I
suggest that a comment be added explaining it all.
I'm a bit surprised at the inclusion of GFP_ATOMIC as it will a) dip
into page reserves, whcih might be undesirable and b) won't even
reclaim clean pages, which seems desirable. I suggest this also be
covered in the forthcoming code comment.
drivers/misc/vmw_balloon.c seems to me to have used better choices here.
> + if (!pg) {
> + *alloc_error = true;
> + return i * alloc_unit;
> + }
> +
> + totalram_pages -= alloc_unit;
Well, I'd consider totalram_pages to be an mm-private thing which drivers
shouldn't muck with. Why is this done?
drivers/xen/balloon.c and drivers/virtio/virtio_balloon.c also alter
totalram_pages, also without explaining why.
drivers/misc/vmw_balloon.c does not.
> + dm->num_pages_ballooned += alloc_unit;
> +
> + bl_resp->range_count++;
> + bl_resp->range_array[i].finfo.start_page =
> + page_to_pfn(pg);
> + bl_resp->range_array[i].finfo.page_cnt = alloc_unit;
> + bl_resp->hdr.size += sizeof(union dm_mem_page_range);
> +
> + }
> +
> + return num_pages;
> +}
>
> ...
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
On Sun, 7 Oct 2012 16:59:46 -0700
K. Y. Srinivasan k...@microsoft.com wrote:
Add the basic balloon driver.
hm, how many balloon drivers does one kernel need?
Although I see that the great majority of this code is hypervisor-specific.
Windows hosts dynamically manage the guest
memory allocation via a combination memory hot add and ballooning. Memory
hot add is used to grow the guest memory upto the maximum memory that can be
allocatted to the guest. Ballooning is used to both shrink as well as expand
up to the max memory. Supporting hot add needs additional support from the
host. We will support hot add when this support is available. For now,
by setting the VM startup memory to the VM max memory, we can use
ballooning alone to dynamically manage memory allocation amongst
competing guests on a given host.
...
+static int alloc_balloon_pages(struct hv_dynmem_device *dm, int num_pages,
+ struct dm_balloon_response *bl_resp, int alloc_unit,
+ bool *alloc_error)
+{
+ int i = 0;
+ struct page *pg;
+
+ if (num_pages alloc_unit)
+ return 0;
+
+ for (i = 0; (i * alloc_unit) num_pages; i++) {
+ if (bl_resp-hdr.size + sizeof(union dm_mem_page_range)
+ PAGE_SIZE)
+ return i * alloc_unit;
+
+ pg = alloc_pages(GFP_HIGHUSER | __GFP_NORETRY | GFP_ATOMIC |
+ __GFP_NOMEMALLOC | __GFP_NOWARN,
+ get_order(alloc_unit PAGE_SHIFT));
This choice of GFP flags is basically impossible to understand, so I
suggest that a comment be added explaining it all.
I'm a bit surprised at the inclusion of GFP_ATOMIC as it will a) dip
into page reserves, whcih might be undesirable and b) won't even
reclaim clean pages, which seems desirable. I suggest this also be
covered in the forthcoming code comment.
drivers/misc/vmw_balloon.c seems to me to have used better choices here.
+ if (!pg) {
+ *alloc_error = true;
+ return i * alloc_unit;
+ }
+
+ totalram_pages -= alloc_unit;
Well, I'd consider totalram_pages to be an mm-private thing which drivers
shouldn't muck with. Why is this done?
drivers/xen/balloon.c and drivers/virtio/virtio_balloon.c also alter
totalram_pages, also without explaining why.
drivers/misc/vmw_balloon.c does not.
+ dm-num_pages_ballooned += alloc_unit;
+
+ bl_resp-range_count++;
+ bl_resp-range_array[i].finfo.start_page =
+ page_to_pfn(pg);
+ bl_resp-range_array[i].finfo.page_cnt = alloc_unit;
+ bl_resp-hdr.size += sizeof(union dm_mem_page_range);
+
+ }
+
+ return num_pages;
+}
...
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
-Original Message-
From: Andrew Morton [mailto:a...@linux-foundation.org]
Sent: Tuesday, October 09, 2012 3:45 PM
To: KY Srinivasan
Cc: gre...@linuxfoundation.org; linux-kernel@vger.kernel.org;
de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com;
a...@firstfloor.org
Subject: Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
On Sun, 7 Oct 2012 16:59:46 -0700
K. Y. Srinivasan k...@microsoft.com wrote:
Add the basic balloon driver.
hm, how many balloon drivers does one kernel need?
Although I see that the great majority of this code is hypervisor-specific.
Windows hosts dynamically manage the guest
memory allocation via a combination memory hot add and ballooning. Memory
hot add is used to grow the guest memory upto the maximum memory that can
be
allocatted to the guest. Ballooning is used to both shrink as well as expand
up to the max memory. Supporting hot add needs additional support from the
host. We will support hot add when this support is available. For now,
by setting the VM startup memory to the VM max memory, we can use
ballooning alone to dynamically manage memory allocation amongst
competing guests on a given host.
...
+static int alloc_balloon_pages(struct hv_dynmem_device *dm, int
num_pages,
+struct dm_balloon_response *bl_resp, int alloc_unit,
+bool *alloc_error)
+{
+ int i = 0;
+ struct page *pg;
+
+ if (num_pages alloc_unit)
+ return 0;
+
+ for (i = 0; (i * alloc_unit) num_pages; i++) {
+ if (bl_resp-hdr.size + sizeof(union dm_mem_page_range)
+ PAGE_SIZE)
+ return i * alloc_unit;
+
+ pg = alloc_pages(GFP_HIGHUSER | __GFP_NORETRY |
GFP_ATOMIC |
+ __GFP_NOMEMALLOC | __GFP_NOWARN,
+ get_order(alloc_unit PAGE_SHIFT));
This choice of GFP flags is basically impossible to understand, so I
suggest that a comment be added explaining it all.
I'm a bit surprised at the inclusion of GFP_ATOMIC as it will a) dip
into page reserves, whcih might be undesirable and b) won't even
reclaim clean pages, which seems desirable. I suggest this also be
covered in the forthcoming code comment.
I will rework these flags and add appropriate comments.
drivers/misc/vmw_balloon.c seems to me to have used better choices here.
+ if (!pg) {
+ *alloc_error = true;
+ return i * alloc_unit;
+ }
+
+ totalram_pages -= alloc_unit;
Well, I'd consider totalram_pages to be an mm-private thing which drivers
shouldn't muck with. Why is this done?
By modifying the totalram_pages, the information presented in /proc/meminfo
correctly reflects what is currently assigned to the guest (MemTotal).
drivers/xen/balloon.c and drivers/virtio/virtio_balloon.c also alter
totalram_pages, also without explaining why.
drivers/misc/vmw_balloon.c does not.
+ dm-num_pages_ballooned += alloc_unit;
+
+ bl_resp-range_count++;
+ bl_resp-range_array[i].finfo.start_page =
+ page_to_pfn(pg);
+ bl_resp-range_array[i].finfo.page_cnt = alloc_unit;
+ bl_resp-hdr.size += sizeof(union dm_mem_page_range);
+
+ }
+
+ return num_pages;
+}
...
Thanks for the prompt review. I will address your comments and repost the
patches soon.
If it is ok with you, I am going to keep the code that manipulates
totalram_pages
(for reasons I listed above).
Regards,
K. Y
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
On Wed, 10 Oct 2012 00:09:12 + KY Srinivasan k...@microsoft.com wrote:
+ if (!pg) {
+ *alloc_error = true;
+ return i * alloc_unit;
+ }
+
+ totalram_pages -= alloc_unit;
Well, I'd consider totalram_pages to be an mm-private thing which drivers
shouldn't muck with. Why is this done?
By modifying the totalram_pages, the information presented in /proc/meminfo
correctly reflects what is currently assigned to the guest (MemTotal).
eh? /proc/meminfo:MemTotal tells you the total memory in the machine.
The only thing which should change it after boot is memory hotplug.
Modifying it in this manner puts the statistic into a state know as
wrong. And temporarily modifying it in this fashion will cause the
tremendous amount of initialisation code which relies upon
totalram_pages for sizing to also enter the wrong state.
Why on earth do balloon drivers do this? If the amount of memory which
is consumed by balloons is interesting then it should be exported via a
standalone metric, not by mucking with totalram_pages.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
-Original Message-
From: Andrew Morton [mailto:a...@linux-foundation.org]
Sent: Tuesday, October 09, 2012 9:15 PM
To: KY Srinivasan
Cc: gre...@linuxfoundation.org; linux-kernel@vger.kernel.org;
de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com;
a...@firstfloor.org
Subject: Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
On Wed, 10 Oct 2012 00:09:12 + KY Srinivasan k...@microsoft.com wrote:
+ if (!pg) {
+ *alloc_error = true;
+ return i * alloc_unit;
+ }
+
+ totalram_pages -= alloc_unit;
Well, I'd consider totalram_pages to be an mm-private thing which drivers
shouldn't muck with. Why is this done?
By modifying the totalram_pages, the information presented in /proc/meminfo
correctly reflects what is currently assigned to the guest (MemTotal).
eh? /proc/meminfo:MemTotal tells you the total memory in the machine.
The only thing which should change it after boot is memory hotplug.
Modifying it in this manner puts the statistic into a state know as
wrong. And temporarily modifying it in this fashion will cause the
tremendous amount of initialisation code which relies upon
totalram_pages for sizing to also enter the wrong state.
Why on earth do balloon drivers do this? If the amount of memory which
is consumed by balloons is interesting then it should be exported via a
standalone metric, not by mucking with totalram_pages.
I see your point. I will get rid of the code that manipulates the
totalram_pages.
Regards,
K. Y
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
> -Original Message- > From: Rusty Russell [mailto:ru...@ozlabs.org] > Sent: Monday, October 08, 2012 1:46 AM > To: KY Srinivasan; gre...@linuxfoundation.org; linux-kernel@vger.kernel.org; > de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com; > a...@linux-foundation.org; a...@firstfloor.org > Cc: KY Srinivasan > Subject: Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver > > "K. Y. Srinivasan" writes: > > +static int hot_add; > > + > > +module_param(hot_add, int, S_IRUGO); > > +MODULE_PARM_DESC(hot_add, "If set attempt memory hot_add"); > > I think this should be a 'bool', but I can't tell, since it's not used > in this patch. You are right; currently, I unconditionally fail the hot add request. Hopefully, I will soon be able to support hot add once I get the needed host side changes. Regards, K. Y -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
-Original Message- From: Rusty Russell [mailto:ru...@ozlabs.org] Sent: Monday, October 08, 2012 1:46 AM To: KY Srinivasan; gre...@linuxfoundation.org; linux-kernel@vger.kernel.org; de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com; a...@linux-foundation.org; a...@firstfloor.org Cc: KY Srinivasan Subject: Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver K. Y. Srinivasan k...@microsoft.com writes: +static int hot_add; + +module_param(hot_add, int, S_IRUGO); +MODULE_PARM_DESC(hot_add, If set attempt memory hot_add); I think this should be a 'bool', but I can't tell, since it's not used in this patch. You are right; currently, I unconditionally fail the hot add request. Hopefully, I will soon be able to support hot add once I get the needed host side changes. Regards, K. Y -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
"K. Y. Srinivasan" writes: > +static int hot_add; > + > +module_param(hot_add, int, S_IRUGO); > +MODULE_PARM_DESC(hot_add, "If set attempt memory hot_add"); I think this should be a 'bool', but I can't tell, since it's not used in this patch. Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
> -Original Message- > From: Greg KH [mailto:gre...@linuxfoundation.org] > Sent: Sunday, October 07, 2012 8:45 PM > To: KY Srinivasan > Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org; > o...@aepfle.de; > a...@canonical.com; a...@linux-foundation.org; a...@firstfloor.org > Subject: Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver > > On Sun, Oct 07, 2012 at 04:59:46PM -0700, K. Y. Srinivasan wrote: > > +config HYPERV_BALLOON > > + tristate "Microsoft Hyper-V Balloon driver" > > + depends on HYPERV > > + help > > + Select this option to enable the Hyper-V Utilities. > > Your help text is wrong :( Sorry about that; I will fix it. > > > --- /dev/null > > +++ b/drivers/hv/hv_balloon.c > > @@ -0,0 +1,1043 @@ > > +/* > > + * Copyright (c) 2012, Microsoft Corporation. > > + * > > + * Author: > > + * K. Y. Srinivasan > > + * > > + * This program is free software; you can redistribute it and/or modify it > > + * under the terms of the GNU General Public License version 2 as published > > + * by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, but > > + * WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE > or > > + * NON INFRINGEMENT. See the GNU General Public License for more > > + * details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program; if not, write to the Free Software > > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 > > USA. > > Unless you promise to keep track of the FSF's office buildings for the > next 40+ years, just drop this paragraph entirely. Will do. Thanks, K. Y -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
On Sun, Oct 07, 2012 at 04:59:46PM -0700, K. Y. Srinivasan wrote: > +config HYPERV_BALLOON > + tristate "Microsoft Hyper-V Balloon driver" > + depends on HYPERV > + help > + Select this option to enable the Hyper-V Utilities. Your help text is wrong :( > --- /dev/null > +++ b/drivers/hv/hv_balloon.c > @@ -0,0 +1,1043 @@ > +/* > + * Copyright (c) 2012, Microsoft Corporation. > + * > + * Author: > + * K. Y. Srinivasan > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License version 2 as published > + * by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, but > + * WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or > + * NON INFRINGEMENT. See the GNU General Public License for more > + * details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. Unless you promise to keep track of the FSF's office buildings for the next 40+ years, just drop this paragraph entirely. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
On Sun, Oct 07, 2012 at 04:59:46PM -0700, K. Y. Srinivasan wrote: +config HYPERV_BALLOON + tristate Microsoft Hyper-V Balloon driver + depends on HYPERV + help + Select this option to enable the Hyper-V Utilities. Your help text is wrong :( --- /dev/null +++ b/drivers/hv/hv_balloon.c @@ -0,0 +1,1043 @@ +/* + * Copyright (c) 2012, Microsoft Corporation. + * + * Author: + * K. Y. Srinivasan k...@microsoft.com + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published + * by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or + * NON INFRINGEMENT. See the GNU General Public License for more + * details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. Unless you promise to keep track of the FSF's office buildings for the next 40+ years, just drop this paragraph entirely. greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
-Original Message- From: Greg KH [mailto:gre...@linuxfoundation.org] Sent: Sunday, October 07, 2012 8:45 PM To: KY Srinivasan Cc: linux-kernel@vger.kernel.org; de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com; a...@linux-foundation.org; a...@firstfloor.org Subject: Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver On Sun, Oct 07, 2012 at 04:59:46PM -0700, K. Y. Srinivasan wrote: +config HYPERV_BALLOON + tristate Microsoft Hyper-V Balloon driver + depends on HYPERV + help + Select this option to enable the Hyper-V Utilities. Your help text is wrong :( Sorry about that; I will fix it. --- /dev/null +++ b/drivers/hv/hv_balloon.c @@ -0,0 +1,1043 @@ +/* + * Copyright (c) 2012, Microsoft Corporation. + * + * Author: + * K. Y. Srinivasan k...@microsoft.com + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published + * by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or + * NON INFRINGEMENT. See the GNU General Public License for more + * details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. Unless you promise to keep track of the FSF's office buildings for the next 40+ years, just drop this paragraph entirely. Will do. Thanks, K. Y -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
K. Y. Srinivasan k...@microsoft.com writes: +static int hot_add; + +module_param(hot_add, int, S_IRUGO); +MODULE_PARM_DESC(hot_add, If set attempt memory hot_add); I think this should be a 'bool', but I can't tell, since it's not used in this patch. Cheers, Rusty. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
