Add a basic balloon driver to take advantage of the dynamic memory
management functionality supported on Windows hosts. Windows requires
the guests to support both memory hot add as well as ballooning. In this patch
we are adding the basic balloon driver. Memory hot add will be added in a
subsequent patch and this requires some changes yet to be made on the host
side.
The policy engine on Windows hosts needs the guest to post memory status
periodically and it looks like committed_as is what it is expecting.
Since vm_committed_as is not currently an exported symbol, this patch set
also exports this symbol.
K. Y. Srinivasan (2):
mm: Export vm_committed_as
Drivers: hv: Add Hyper-V balloon driver
drivers/hv/Kconfig | 6 +
drivers/hv/Makefile | 1 +
drivers/hv/hv_balloon.c | 1043 +++++++++++++++++++++++++++++++++++++++++++++++
mm/mmap.c | 1 +
mm/nommu.c | 1 +
5 files changed, 1052 insertions(+), 0 deletions(-)
create mode 100644 drivers/hv/hv_balloon.c
--
1.7.4.1
The policy engine on the host expects the guest to report the
committed_as. Since this variable is not exported,
export this symbol.
Signed-off-by: K. Y. Srinivasan <[email protected]>
---
mm/mmap.c | 1 +
mm/nommu.c | 1 +
2 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/mm/mmap.c b/mm/mmap.c
index 2d94235..01439d4 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -87,6 +87,7 @@ int sysctl_max_map_count __read_mostly = DEFAULT_MAX_MAP_COUNT;
* other variables. It can be updated by several CPUs frequently.
*/
struct percpu_counter vm_committed_as ____cacheline_aligned_in_smp;
+EXPORT_SYMBOL(vm_committed_as);
/*
* Check that a process has enough memory to allocate a new virtual
diff --git a/mm/nommu.c b/mm/nommu.c
index 45131b4..3914b7e 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -68,6 +68,7 @@ atomic_long_t mmap_pages_allocated;
EXPORT_SYMBOL(mem_map);
EXPORT_SYMBOL(num_physpages);
+EXPORT_SYMBOL(vm_committed_as);
/* list of mapped, potentially shareable regions */
static struct kmem_cache *vm_region_jar;
--
1.7.4.1
Add the basic balloon driver. 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.
Signed-off-by: K. Y. Srinivasan <[email protected]>
Reviewed-by: Haiyang Zhang <[email protected]>
---
drivers/hv/Kconfig | 6 +
drivers/hv/Makefile | 1 +
drivers/hv/hv_balloon.c | 1043 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 1050 insertions(+), 0 deletions(-)
create mode 100644 drivers/hv/hv_balloon.c
diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
index 70f5dde..75af1d4 100644
--- a/drivers/hv/Kconfig
+++ b/drivers/hv/Kconfig
@@ -13,4 +13,10 @@ config HYPERV_UTILS
help
Select this option to enable the Hyper-V Utilities.
+config HYPERV_BALLOON
+ tristate "Microsoft Hyper-V Balloon driver"
+ depends on HYPERV
+ help
+ Select this option to enable the Hyper-V Utilities.
+
endmenu
diff --git a/drivers/hv/Makefile b/drivers/hv/Makefile
index a23938b..e6abfa0 100644
--- a/drivers/hv/Makefile
+++ b/drivers/hv/Makefile
@@ -1,5 +1,6 @@
obj-$(CONFIG_HYPERV) += hv_vmbus.o
obj-$(CONFIG_HYPERV_UTILS) += hv_utils.o
+obj-$(CONFIG_HYPERV_BALLOON) += hv_balloon.o
hv_vmbus-y := vmbus_drv.o \
hv.o connection.o channel.o \
diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
new file mode 100644
index 0000000..e543524
--- /dev/null
+++ b/drivers/hv/hv_balloon.c
@@ -0,0 +1,1043 @@
+/*
+ * Copyright (c) 2012, Microsoft Corporation.
+ *
+ * Author:
+ * K. Y. Srinivasan <[email protected]>
+ *
+ * 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.
+ *
+ *
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/mman.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/kthread.h>
+#include <linux/completion.h>
+#include <linux/memory_hotplug.h>
+#include <linux/memory.h>
+#include <linux/notifier.h>
+#include <linux/mman.h>
+#include <linux/percpu_counter.h>
+
+#include <linux/hyperv.h>
+
+/*
+ * We begin with definitions supporting the Dynamic Memory protocol
+ * with the host.
+ *
+ * Begin protocol definitions.
+ */
+
+
+
+/*
+ * Protocol versions. The low word is the minor version, the high word the major
+ * version.
+ *
+ * History:
+ * Initial version 1.0
+ * Changed to 0.1 on 2009/03/25
+ * Changes to 0.2 on 2009/05/14
+ * Changes to 0.3 on 2009/12/03
+ * Changed to 1.0 on 2011/04/05
+ */
+
+#define DYNMEM_MAKE_VERSION(Major, Minor) ((__u32)(((Major) << 16) | (Minor)))
+#define DYNMEM_MAJOR_VERSION(Version) ((__u32)(Version) >> 16)
+#define DYNMEM_MINOR_VERSION(Version) ((__u32)(Version) & 0xff)
+
+enum {
+ DYNMEM_PROTOCOL_VERSION_1 = DYNMEM_MAKE_VERSION(0, 3),
+ DYNMEM_PROTOCOL_VERSION_2 = DYNMEM_MAKE_VERSION(1, 0),
+
+ DYNMEM_PROTOCOL_VERSION_WIN7 = DYNMEM_PROTOCOL_VERSION_1,
+ DYNMEM_PROTOCOL_VERSION_WIN8 = DYNMEM_PROTOCOL_VERSION_2,
+
+ DYNMEM_PROTOCOL_VERSION_CURRENT = DYNMEM_PROTOCOL_VERSION_WIN8
+};
+
+
+
+/*
+ * Message Types
+ */
+
+enum dm_message_type {
+ /*
+ * Version 0.3
+ */
+ DM_ERROR = 0,
+ DM_VERSION_REQUEST = 1,
+ DM_VERSION_RESPONSE = 2,
+ DM_CAPABILITIES_REPORT = 3,
+ DM_CAPABILITIES_RESPONSE = 4,
+ DM_STATUS_REPORT = 5,
+ DM_BALLOON_REQUEST = 6,
+ DM_BALLOON_RESPONSE = 7,
+ DM_UNBALLOON_REQUEST = 8,
+ DM_UNBALLOON_RESPONSE = 9,
+ DM_MEM_HOT_ADD_REQUEST = 10,
+ DM_MEM_HOT_ADD_RESPONSE = 11,
+ DM_VERSION_03_MAX = 11,
+ /*
+ * Version 1.0.
+ */
+ DM_INFO_MESSAGE = 12,
+ DM_VERSION_1_MAX = 12
+};
+
+
+/*
+ * Structures defining the dynamic memory management
+ * protocol.
+ */
+
+union dm_version {
+ struct {
+ __u16 minor_version;
+ __u16 major_version;
+ };
+ __u32 version;
+} __packed;
+
+
+union dm_caps {
+ struct {
+ __u64 balloon:1;
+ __u64 hot_add:1;
+ __u64 reservedz:62;
+ } cap_bits;
+ __u64 caps;
+} __packed;
+
+union dm_mem_page_range {
+ struct {
+ /*
+ * The PFN number of the first page in the range.
+ * 40 bits is the architectural limit of a PFN
+ * number for AMD64.
+ */
+ __u64 start_page:40;
+ /*
+ * The number of pages in the range.
+ */
+ __u64 page_cnt:24;
+ } finfo;
+ __u64 page_range;
+} __packed;
+
+
+
+/*
+ * The header for all dynamic memory messages:
+ *
+ * type: Type of the message.
+ * size: Size of the message in bytes; including the header.
+ * trans_id: The guest is responsible for manufacturing this ID.
+ */
+
+struct dm_header {
+ __u16 type;
+ __u16 size;
+ __u32 trans_id;
+} __packed;
+
+/*
+ * A generic message format for dynamic memory.
+ * Specific message formats are defined later in the file.
+ */
+
+struct dm_message {
+ struct dm_header hdr;
+ __u8 data[]; /* enclosed message */
+} __packed;
+
+
+/*
+ * Specific message types supporting the dynamic memory protocol.
+ */
+
+/*
+ * Version negotiation message. Sent from the guest to the host.
+ * The guest is free to try different versions until the host
+ * accepts the version.
+ *
+ * dm_version: The protocol version requested.
+ * is_last_attempt: If TRUE, this is the last version guest will request.
+ * reservedz: Reserved field, set to zero.
+ */
+
+struct dm_version_request {
+ struct dm_header hdr;
+ union dm_version version;
+ __u32 is_last_attempt:1;
+ __u32 reservedz:31;
+} __packed;
+
+/*
+ * Version response message; Host to Guest and indicates
+ * if the host has accepted the version sent by the guest.
+ *
+ * is_accepted: If TRUE, host has accepted the version and the guest
+ * should proceed to the next stage of the protocol. FALSE indicates that
+ * guest should re-try with a different version.
+ *
+ * reservedz: Reserved field, set to zero.
+ */
+
+struct dm_version_response {
+ struct dm_header hdr;
+ __u64 is_accepted:1;
+ __u64 reservedz:63;
+} __packed;
+
+/*
+ * Message reporting capabilities. This is sent from the guest to the
+ * host.
+ */
+
+struct dm_capabilities {
+ struct dm_header hdr;
+ union dm_caps caps;
+ __u64 min_page_cnt;
+ __u64 max_page_number;
+} __packed;
+
+/*
+ * Response to the capabilities message. This is sent from the host to the
+ * guest. This message notifies if the host has accepted the guest's
+ * capabilities. If the host has not accepted, the guest must shutdown
+ * the service.
+ *
+ * is_accepted: Indicates if the host has accepted guest's capabilities.
+ * reservedz: Must be 0.
+ */
+
+struct dm_capabilities_resp_msg {
+ struct dm_header hdr;
+ __u64 is_accepted:1;
+ __u64 reservedz:63;
+} __packed;
+
+/*
+ * This message is used to report memory pressure from the guest.
+ * This message is not part of any transaction and there is no
+ * response to this message.
+ *
+ * num_avail: Available memory in pages.
+ * num_committed: Committed memory in pages.
+ * page_file_size: The accumulated size of all page files
+ * in the system in pages.
+ * zero_free: The nunber of zero and free pages.
+ * page_file_writes: The writes to the page file in pages.
+ * io_diff: An indicator of file cache efficiency or page file activity,
+ * calculated as File Cache Page Fault Count - Page Read Count.
+ * This value is in pages.
+ *
+ * Some of these metrics are Windows specific and fortunately
+ * the algorithm on the host side that computes the guest memory
+ * pressure only uses num_committed value.
+ */
+
+struct dm_status {
+ struct dm_header hdr;
+ __u64 num_avail;
+ __u64 num_committed;
+ __u64 page_file_size;
+ __u64 zero_free;
+ __u32 page_file_writes;
+ __u32 io_diff;
+} __packed;
+
+
+/*
+ * Message to ask the guest to allocate memory - balloon up message.
+ * This message is sent from the host to the guest. The guest may not be
+ * able to allocate as much memory as requested.
+ *
+ * num_pages: number of pages to allocate.
+ */
+
+struct dm_balloon {
+ struct dm_header hdr;
+ __u32 num_pages;
+ __u32 reservedz;
+} __packed;
+
+
+/*
+ * Balloon response message; this message is sent from the guest
+ * to the host in response to the balloon message.
+ *
+ * reservedz: Reserved; must be set to zero.
+ * more_pages: If FALSE, this is the last message of the transaction.
+ * if TRUE there will atleast one more message from the guest.
+ *
+ * range_count: The number of ranges in the range array.
+ *
+ * range_array: An array of page ranges returned to the host.
+ *
+ */
+
+struct dm_balloon_response {
+ struct dm_header hdr;
+ __u32 reservedz;
+ __u32 more_pages:1;
+ __u32 range_count:31;
+ union dm_mem_page_range range_array[];
+} __packed;
+
+/*
+ * Un-balloon message; this message is sent from the host
+ * to the guest to give guest more memory.
+ *
+ * more_pages: If FALSE, this is the last message of the transaction.
+ * if TRUE there will atleast one more message from the guest.
+ *
+ * reservedz: Reserved; must be set to zero.
+ *
+ * range_count: The number of ranges in the range array.
+ *
+ * range_array: An array of page ranges returned to the host.
+ *
+ */
+
+struct dm_unballoon_request {
+ struct dm_header hdr;
+ __u32 more_pages:1;
+ __u32 reservedz:31;
+ __u32 range_count;
+ union dm_mem_page_range range_array[];
+} __packed;
+
+/*
+ * Un-balloon response message; this message is sent from the guest
+ * to the host in response to an unballoon request.
+ *
+ */
+
+struct dm_unballoon_response {
+ struct dm_header hdr;
+} __packed;
+
+
+/*
+ * Hot add request message. Message sent from the host to the guest.
+ *
+ * mem_range: Memory range to hot add.
+ *
+ * On Linux we currently don't support this since we cannot hot add
+ * arbitrary granularity of memory.
+ */
+
+struct dm_hot_add {
+ struct dm_header hdr;
+ union dm_mem_page_range range;
+} __packed;
+
+/*
+ * Hot add response message.
+ * This message is sent by the guest to report the status of a hot add request.
+ * If page_count is less than the requested page count, then the host should
+ * assume all further hot add requests will fail, since this indicates that
+ * the guest has hit an upper physical memory barrier.
+ *
+ * Hot adds may also fail due to low resources; in this case, the guest must
+ * not complete this message until the hot add can succeed, and the host must
+ * not send a new hot add request until the response is sent.
+ * If VSC fails to hot add memory DYNMEM_NUMBER_OF_UNSUCCESSFUL_HOTADD_ATTEMPTS
+ * times it fails the request.
+ *
+ *
+ * page_count: number of pages that were successfully hot added.
+ *
+ * result: result of the operation 1: success, 0: failure.
+ *
+ */
+
+struct dm_hot_add_response {
+ struct dm_header hdr;
+ __u32 page_count;
+ __u32 result;
+} __packed;
+
+/*
+ * Types of information sent from host to the guest.
+ */
+
+enum dm_info_type {
+ INFO_TYPE_MAX_PAGE_CNT = 0,
+ MAX_INFO_TYPE
+};
+
+
+/*
+ * Header for the information message.
+ */
+
+struct dm_info_header {
+ enum dm_info_type type;
+ __u32 data_size;
+} __packed;
+
+/*
+ * This message is sent from the host to the guest to pass
+ * some relevant information (win8 addition).
+ *
+ * reserved: no used.
+ * info_size: size of the information blob.
+ * info: information blob.
+ */
+
+struct dm_info_msg {
+ struct dm_info_header header;
+ __u32 reserved;
+ __u32 info_size;
+ __u8 info[];
+};
+
+/*
+ * End protocol definitions.
+ */
+
+static int hot_add;
+
+module_param(hot_add, int, S_IRUGO);
+MODULE_PARM_DESC(hot_add, "If set attempt memory hot_add");
+
+static atomic_t trans_id = ATOMIC_INIT(0);
+
+static int dm_ring_size = (5 * PAGE_SIZE);
+
+/*
+ * Driver specific state.
+ */
+
+enum hv_dm_state {
+ DM_INITIALIZING = 0,
+ DM_INITIALIZED,
+ DM_BALLOON_UP,
+ DM_BALLOON_DOWN,
+ DM_HOT_ADD,
+ DM_INIT_ERROR
+};
+
+
+static __u8 recv_buffer[PAGE_SIZE];
+static __u8 *send_buffer;
+#define PAGES_IN_2M 512
+
+struct hv_dynmem_device {
+ struct hv_device *dev;
+ enum hv_dm_state state;
+ struct completion host_event;
+ struct completion config_event;
+
+ /*
+ * Number of pages we have currently ballooned out.
+ * We use this to determine if we can unload this
+ * driver.
+ */
+ unsigned int num_pages_ballooned;
+
+ /*
+ * This thread handles both balloon/hot-add
+ * requests from the host as well as notifying
+ * the host with regards to memory pressure in
+ * the guest.
+ */
+ struct task_struct *thread;
+
+ /*
+ * We start with the highest version we can support
+ * and downgrade based on the host; we save here the
+ * next version to try.
+ */
+ __u32 next_version;
+};
+
+static struct hv_dynmem_device dm_device;
+
+static void hot_add_req(struct hv_dynmem_device *dm, struct dm_hot_add *msg)
+{
+
+ struct dm_hot_add_response resp;
+
+ /*
+ * Currently we do not support hot add.
+ * Just fail the request.
+ */
+
+ memset(&resp, 0, sizeof(struct dm_hot_add_response));
+ resp.hdr.type = DM_MEM_HOT_ADD_RESPONSE;
+ resp.hdr.size = sizeof(struct dm_hot_add_response);
+ resp.hdr.trans_id = atomic_inc_return(&trans_id);
+
+ resp.page_count = 0;
+ resp.result = 0;
+
+ dm->state = DM_INITIALIZED;
+ vmbus_sendpacket(dm->dev->channel, &resp,
+ sizeof(struct dm_hot_add_response),
+ (unsigned long)NULL,
+ VM_PKT_DATA_INBAND, 0);
+
+}
+
+static void process_info(struct hv_dynmem_device *dm, struct dm_info_msg *msg)
+{
+ switch (msg->header.type) {
+ case INFO_TYPE_MAX_PAGE_CNT:
+ pr_info("Received INFO_TYPE_MAX_PAGE_CNT\n");
+ pr_info("Data Size is %d\n", msg->header.data_size);
+ break;
+ default:
+ pr_info("Received Unknown type: %d\n", msg->header.type);
+ }
+}
+
+/*
+ * Post our status as it relates memory pressure to the
+ * host. Host expects the guests to post this status
+ * periodically at 1 second intervals.
+ *
+ * The metrics specified in this protocol are very Windows
+ * specific and so we cook up numbers here to convey our memory
+ * pressure.
+ */
+
+static void post_status(struct hv_dynmem_device *dm)
+{
+ struct dm_status status;
+
+
+ memset(&status, 0, sizeof(struct dm_status));
+ status.hdr.type = DM_STATUS_REPORT;
+ status.hdr.size = sizeof(struct dm_status);
+ status.hdr.trans_id = atomic_inc_return(&trans_id);
+
+
+ status.num_committed = percpu_counter_read_positive(&vm_committed_as);
+
+ vmbus_sendpacket(dm->dev->channel, &status,
+ sizeof(struct dm_status),
+ (unsigned long)NULL,
+ VM_PKT_DATA_INBAND, 0);
+
+}
+
+
+
+void free_balloon_pages(struct hv_dynmem_device *dm,
+ union dm_mem_page_range *range_array)
+{
+ int num_pages = range_array->finfo.page_cnt;
+ __u64 start_frame = range_array->finfo.start_page;
+ struct page *pg;
+ int i;
+
+ for (i = 0; i < num_pages; i++) {
+ pg = pfn_to_page(i + start_frame);
+ __free_page(pg);
+ totalram_pages++;
+ dm->num_pages_ballooned--;
+ }
+}
+
+
+
+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));
+
+ if (!pg) {
+ *alloc_error = true;
+ return i * alloc_unit;
+ }
+
+ totalram_pages -= alloc_unit;
+
+ 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;
+}
+
+
+
+static void balloon_up(struct hv_dynmem_device *dm, struct dm_balloon *req)
+{
+ int num_pages = req->num_pages;
+ int num_ballooned = 0;
+ struct dm_balloon_response *bl_resp;
+ int alloc_unit;
+ int ret;
+ bool alloc_error = false;
+ bool done = false;
+ int i;
+
+
+ /*
+ * Currently, we only support 4k allocations.
+ */
+ alloc_unit = 1;
+
+ while (!done) {
+ bl_resp = (struct dm_balloon_response *)send_buffer;
+ memset(send_buffer, 0, PAGE_SIZE);
+ bl_resp->hdr.type = DM_BALLOON_RESPONSE;
+ bl_resp->hdr.trans_id = atomic_inc_return(&trans_id);
+ bl_resp->hdr.size = sizeof(struct dm_balloon_response);
+ bl_resp->more_pages = 1;
+
+
+ num_pages -= num_ballooned;
+ num_ballooned = alloc_balloon_pages(dm, num_pages,
+ bl_resp, alloc_unit,
+ &alloc_error);
+
+ if ((alloc_error) || (num_ballooned == num_pages)) {
+ bl_resp->more_pages = 0;
+ done = true;
+ dm->state = DM_INITIALIZED;
+ }
+
+ /*
+ * We are pushing a lot of data through the channel;
+ * deal with transient failures caused because of the
+ * lack of space in the ring buffer.
+ */
+
+ do {
+ ret = vmbus_sendpacket(dm_device.dev->channel,
+ bl_resp,
+ bl_resp->hdr.size,
+ (unsigned long)NULL,
+ VM_PKT_DATA_INBAND, 0);
+
+ if (ret == -EAGAIN)
+ msleep(20);
+
+ } while (ret == -EAGAIN);
+
+ if (ret) {
+ /*
+ * Free up the memory we allocatted.
+ */
+ pr_info("Balloon response failed\n");
+
+ for (i = 0; i < bl_resp->range_count; i++)
+ free_balloon_pages(dm,
+ &bl_resp->range_array[i]);
+
+ done = true;
+ }
+ }
+
+}
+
+static void balloon_down(struct hv_dynmem_device *dm,
+ struct dm_unballoon_request *req)
+{
+ union dm_mem_page_range *range_array = req->range_array;
+ int range_count = req->range_count;
+ struct dm_unballoon_response resp;
+ int i;
+
+ for (i = 0; i < range_count; i++)
+ free_balloon_pages(dm, &range_array[i]);
+
+ if (req->more_pages == 1)
+ return;
+
+ memset(&resp, 0, sizeof(struct dm_unballoon_response));
+ resp.hdr.type = DM_UNBALLOON_RESPONSE;
+ resp.hdr.trans_id = atomic_inc_return(&trans_id);
+ resp.hdr.size = sizeof(struct dm_unballoon_response);
+
+ vmbus_sendpacket(dm_device.dev->channel, &resp,
+ sizeof(struct dm_unballoon_response),
+ (unsigned long)NULL,
+ VM_PKT_DATA_INBAND, 0);
+
+ dm->state = DM_INITIALIZED;
+}
+
+static void balloon_onchannelcallback(void *context);
+
+static int dm_thread_func(void *dm_dev)
+{
+ struct hv_dynmem_device *dm = dm_dev;
+ int t;
+ unsigned long scan_start;
+
+ while (!kthread_should_stop()) {
+ t = wait_for_completion_timeout(&dm_device.config_event, 1*HZ);
+ /*
+ * The host expects us to post information on the memory
+ * pressure every second.
+ */
+
+ if (t == 0)
+ post_status(dm);
+
+ scan_start = jiffies;
+ switch (dm->state) {
+ case DM_BALLOON_UP:
+ balloon_up(dm, (struct dm_balloon *)recv_buffer);
+ break;
+
+ case DM_HOT_ADD:
+ hot_add_req(dm, (struct dm_hot_add *)recv_buffer);
+ break;
+ default:
+ break;
+ }
+
+ if (!time_in_range(jiffies, scan_start, scan_start + HZ))
+ post_status(dm);
+
+ }
+
+ return 0;
+}
+
+
+static void version_resp(struct hv_dynmem_device *dm,
+ struct dm_version_response *vresp)
+{
+ struct dm_version_request version_req;
+ int ret;
+
+ if (vresp->is_accepted) {
+ /*
+ * We are done; wakeup the
+ * context waiting for version
+ * negotiation.
+ */
+ complete(&dm->host_event);
+ return;
+ }
+ /*
+ * If there are more versions to try, continue
+ * with negotiations; if not
+ * shutdown the service since we are not able
+ * to negotiate a suitable version number
+ * with the host.
+ */
+ if (dm->next_version == 0)
+ goto version_error;
+
+ dm->next_version = 0;
+ memset(&version_req, 0, sizeof(struct dm_version_request));
+ version_req.hdr.type = DM_VERSION_REQUEST;
+ version_req.hdr.size = sizeof(struct dm_version_request);
+ version_req.hdr.trans_id = atomic_inc_return(&trans_id);
+ version_req.version.version = DYNMEM_PROTOCOL_VERSION_WIN7;
+ version_req.is_last_attempt = 1;
+
+ ret = vmbus_sendpacket(dm->dev->channel, &version_req,
+ sizeof(struct dm_version_request),
+ (unsigned long)NULL,
+ VM_PKT_DATA_INBAND, 0);
+
+ if (ret)
+ goto version_error;
+
+ return;
+
+version_error:
+ dm->state = DM_INIT_ERROR;
+ complete(&dm->host_event);
+}
+
+static void cap_resp(struct hv_dynmem_device *dm,
+ struct dm_capabilities_resp_msg *cap_resp)
+{
+ if (!cap_resp->is_accepted) {
+ pr_info("Capabilities not accepted by host\n");
+ dm->state = DM_INIT_ERROR;
+ }
+ complete(&dm->host_event);
+}
+
+static void balloon_onchannelcallback(void *context)
+{
+ struct hv_device *dev = context;
+ u32 recvlen;
+ u64 requestid;
+ struct dm_message *dm_msg;
+ struct dm_header *dm_hdr;
+ struct hv_dynmem_device *dm = hv_get_drvdata(dev);
+
+ memset(recv_buffer, 0, sizeof(recv_buffer));
+ vmbus_recvpacket(dev->channel, recv_buffer,
+ PAGE_SIZE, &recvlen, &requestid);
+
+ if (recvlen > 0) {
+ dm_msg = (struct dm_message *)recv_buffer;
+ dm_hdr = &dm_msg->hdr;
+
+ switch (dm_hdr->type) {
+ case DM_VERSION_RESPONSE:
+ version_resp(dm,
+ (struct dm_version_response *)dm_msg);
+ break;
+
+ case DM_CAPABILITIES_RESPONSE:
+ cap_resp(dm,
+ (struct dm_capabilities_resp_msg *)dm_msg);
+ break;
+
+ case DM_BALLOON_REQUEST:
+ dm->state = DM_BALLOON_UP;
+ complete(&dm->config_event);
+ break;
+
+ case DM_UNBALLOON_REQUEST:
+ dm->state = DM_BALLOON_DOWN;
+ balloon_down(dm,
+ (struct dm_unballoon_request *)recv_buffer);
+ break;
+
+ case DM_MEM_HOT_ADD_REQUEST:
+ dm->state = DM_HOT_ADD;
+ complete(&dm->config_event);
+ break;
+
+ case DM_INFO_MESSAGE:
+ process_info(dm, (struct dm_info_msg *)dm_msg);
+ break;
+
+ default:
+ pr_err("Unhandled message: type: %d\n", dm_hdr->type);
+
+ }
+ }
+
+}
+
+static int balloon_probe(struct hv_device *dev,
+ const struct hv_vmbus_device_id *dev_id)
+{
+ int ret, t;
+ struct dm_version_request version_req;
+ struct dm_capabilities cap_msg;
+
+ /*
+ * First allocate a send buffer.
+ */
+
+ send_buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
+ if (!send_buffer)
+ return -ENOMEM;
+
+ ret = vmbus_open(dev->channel, dm_ring_size, dm_ring_size, NULL, 0,
+ balloon_onchannelcallback, dev);
+
+ if (ret)
+ return ret;
+
+ dm_device.dev = dev;
+ dm_device.state = DM_INITIALIZING;
+ dm_device.next_version = DYNMEM_PROTOCOL_VERSION_WIN7;
+ init_completion(&dm_device.host_event);
+ init_completion(&dm_device.config_event);
+
+ dm_device.thread =
+ kthread_run(dm_thread_func, &dm_device, "hv_balloon");
+ if (IS_ERR(dm_device.thread)) {
+ ret = PTR_ERR(dm_device.thread);
+ goto probe_error0;
+ }
+
+ hv_set_drvdata(dev, &dm_device);
+ /*
+ * Initiate the hand shake with the host and negotiate
+ * a version that the host can support. We start with the
+ * highest version number and go down if the host cannot
+ * support it.
+ */
+ memset(&version_req, 0, sizeof(struct dm_version_request));
+ version_req.hdr.type = DM_VERSION_REQUEST;
+ version_req.hdr.size = sizeof(struct dm_version_request);
+ version_req.hdr.trans_id = atomic_inc_return(&trans_id);
+ version_req.version.version = DYNMEM_PROTOCOL_VERSION_WIN8;
+ version_req.is_last_attempt = 0;
+
+ ret = vmbus_sendpacket(dev->channel, &version_req,
+ sizeof(struct dm_version_request),
+ (unsigned long)NULL,
+ VM_PKT_DATA_INBAND,
+ VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+ if (ret)
+ goto probe_error1;
+
+ t = wait_for_completion_timeout(&dm_device.host_event, 5*HZ);
+ if (t == 0) {
+ ret = -ETIMEDOUT;
+ goto probe_error1;
+ }
+
+ /*
+ * If we could not negotiate a compatible version with the host
+ * fail the probe function.
+ */
+ if (dm_device.state == DM_INIT_ERROR) {
+ ret = -ETIMEDOUT;
+ goto probe_error1;
+ }
+ /*
+ * Now submit our capabilities to the host.
+ */
+ memset(&cap_msg, 0, sizeof(struct dm_capabilities));
+ cap_msg.hdr.type = DM_CAPABILITIES_REPORT;
+ cap_msg.hdr.size = sizeof(struct dm_capabilities);
+ cap_msg.hdr.trans_id = atomic_inc_return(&trans_id);
+
+ cap_msg.caps.cap_bits.balloon = 1;
+ /*
+ * While we currently don't support hot-add,
+ * we still advertise this capability since the
+ * host requires that guests partcipating in the
+ * dynamic memory protocol support hot add.
+ */
+ cap_msg.caps.cap_bits.hot_add = 1;
+
+ /*
+ * Currently the host does not use these
+ * values and we set them to what is done in the
+ * Windows driver.
+ */
+ cap_msg.min_page_cnt = 0;
+ cap_msg.max_page_number = -1;
+
+ ret = vmbus_sendpacket(dev->channel, &cap_msg,
+ sizeof(struct dm_capabilities),
+ (unsigned long)NULL,
+ VM_PKT_DATA_INBAND,
+ VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+ if (ret)
+ goto probe_error1;
+
+ t = wait_for_completion_timeout(&dm_device.host_event, 5*HZ);
+ if (t == 0) {
+ ret = -ETIMEDOUT;
+ goto probe_error1;
+ }
+
+ /*
+ * If the host does not like our capabilities,
+ * fail the probe function.
+ */
+ if (dm_device.state == DM_INIT_ERROR) {
+ ret = -ETIMEDOUT;
+ goto probe_error1;
+ }
+
+ dm_device.state = DM_INITIALIZED;
+
+ return 0;
+
+probe_error1:
+ kthread_stop(dm_device.thread);
+
+probe_error0:
+ vmbus_close(dev->channel);
+ return ret;
+}
+
+static int balloon_remove(struct hv_device *dev)
+{
+ struct hv_dynmem_device *dm = hv_get_drvdata(dev);
+
+ /*
+ * If we have ballooned pages out, simply fail
+ * the operation.
+ */
+
+ if (dm->num_pages_ballooned != 0)
+ return -EBUSY;
+
+ vmbus_close(dev->channel);
+ kthread_stop(dm->thread);
+
+ return 0;
+}
+
+static const struct hv_vmbus_device_id id_table[] = {
+ /* Dynamic Memory Class ID */
+ /* 525074DC-8985-46e2-8057-A307DC18A502 */
+ { VMBUS_DEVICE(0xdc, 0x74, 0x50, 0X52, 0x85, 0x89, 0xe2, 0x46,
+ 0x80, 0x57, 0xa3, 0x07, 0xdc, 0x18, 0xa5, 0x02)
+ },
+ { },
+};
+
+MODULE_DEVICE_TABLE(vmbus, id_table);
+
+static struct hv_driver balloon_drv = {
+ .name = "hv_balloon",
+ .id_table = id_table,
+ .probe = balloon_probe,
+ .remove = balloon_remove,
+};
+
+static int __init init_balloon_drv(void)
+{
+
+ return vmbus_driver_register(&balloon_drv);
+}
+
+static void exit_balloon_drv(void)
+{
+
+ vmbus_driver_unregister(&balloon_drv);
+}
+
+module_init(init_balloon_drv);
+module_exit(exit_balloon_drv);
+
+MODULE_DESCRIPTION("Hyper-V Balloon");
+MODULE_VERSION(HV_DRV_VERSION);
+MODULE_LICENSE("GPL");
--
1.7.4.1
On Sun, Oct 07, 2012 at 04:59:45PM -0700, K. Y. Srinivasan wrote:
> The policy engine on the host expects the guest to report the
> committed_as. Since this variable is not exported,
> export this symbol.
Why are these symbols not needed by either Xen or KVM or vmware, which
I think all support the same thing, right?
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -87,6 +87,7 @@ int sysctl_max_map_count __read_mostly = DEFAULT_MAX_MAP_COUNT;
> * other variables. It can be updated by several CPUs frequently.
> */
> struct percpu_counter vm_committed_as ____cacheline_aligned_in_smp;
> +EXPORT_SYMBOL(vm_committed_as);
>
> /*
> * Check that a process has enough memory to allocate a new virtual
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 45131b4..3914b7e 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -68,6 +68,7 @@ atomic_long_t mmap_pages_allocated;
>
> EXPORT_SYMBOL(mem_map);
> EXPORT_SYMBOL(num_physpages);
> +EXPORT_SYMBOL(vm_committed_as);
EXPORT_SYMBOL_GPL() for these new ones?
greg k-h
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 <[email protected]>
> + *
> + * 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
> -----Original Message-----
> From: Greg KH [mailto:[email protected]]
> Sent: Sunday, October 07, 2012 8:44 PM
> To: KY Srinivasan
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH 1/2] mm: Export vm_committed_as
>
> On Sun, Oct 07, 2012 at 04:59:45PM -0700, K. Y. Srinivasan wrote:
> > The policy engine on the host expects the guest to report the
> > committed_as. Since this variable is not exported,
> > export this symbol.
>
> Why are these symbols not needed by either Xen or KVM or vmware, which
> I think all support the same thing, right?
The basic balloon driver does not need this symbol since the basic balloon driver
is not automatically driven by the host. On the Windows host we have a policy engine that
drives the balloon driver based on both guest level memory pressure that the guest
reports as well as other system level metrics the host maintains. We need this symbol to
drive the policy engine on the host.
Regards,
K. Y
> -----Original Message-----
> From: Greg KH [mailto:[email protected]]
> Sent: Sunday, October 07, 2012 8:45 PM
> To: KY Srinivasan
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> 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 <[email protected]>
> > + *
> > + * 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
"K. Y. Srinivasan" <[email protected]> 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.
On Mon, Oct 08, 2012 at 03:35:50AM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Greg KH [mailto:[email protected]]
> > Sent: Sunday, October 07, 2012 8:44 PM
> > To: KY Srinivasan
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected]
> > Subject: Re: [PATCH 1/2] mm: Export vm_committed_as
> >
> > On Sun, Oct 07, 2012 at 04:59:45PM -0700, K. Y. Srinivasan wrote:
> > > The policy engine on the host expects the guest to report the
> > > committed_as. Since this variable is not exported,
> > > export this symbol.
> >
> > Why are these symbols not needed by either Xen or KVM or vmware, which
> > I think all support the same thing, right?
>
> The basic balloon driver does not need this symbol since the basic balloon driver
> is not automatically driven by the host. On the Windows host we have a policy engine that
> drives the balloon driver based on both guest level memory pressure that the guest
> reports as well as other system level metrics the host maintains. We need this symbol to
> drive the policy engine on the host.
Ok, but you're going to have to get the -mm developers to agree that
this is ok before I can accept it.
thanks,
greg k-h
> -----Original Message-----
> From: Greg KH [mailto:[email protected]]
> Sent: Monday, October 08, 2012 9:36 AM
> To: KY Srinivasan
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH 1/2] mm: Export vm_committed_as
>
> On Mon, Oct 08, 2012 at 03:35:50AM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:[email protected]]
> > > Sent: Sunday, October 07, 2012 8:44 PM
> > > To: KY Srinivasan
> > > Cc: [email protected]; [email protected];
> [email protected];
> > > [email protected]; [email protected]; [email protected]
> > > Subject: Re: [PATCH 1/2] mm: Export vm_committed_as
> > >
> > > On Sun, Oct 07, 2012 at 04:59:45PM -0700, K. Y. Srinivasan wrote:
> > > > The policy engine on the host expects the guest to report the
> > > > committed_as. Since this variable is not exported,
> > > > export this symbol.
> > >
> > > Why are these symbols not needed by either Xen or KVM or vmware, which
> > > I think all support the same thing, right?
> >
> > The basic balloon driver does not need this symbol since the basic balloon driver
> > is not automatically driven by the host. On the Windows host we have a policy
> engine that
> > drives the balloon driver based on both guest level memory pressure that the
> guest
> > reports as well as other system level metrics the host maintains. We need this
> symbol to
> > drive the policy engine on the host.
>
> Ok, but you're going to have to get the -mm developers to agree that
> this is ok before I can accept it.
Andi Kleen suggested that I send the patch to Andrew Morton for acceptance.
That is why I have included both Andrew Morton and Andi Kleen in the email list
for these patches. Should I have sent that patch to a different email alias for
acceptance by the mm developers.
Regards,
K. Y
> -----Original Message-----
> From: Rusty Russell [mailto:[email protected]]
> Sent: Monday, October 08, 2012 1:46 AM
> To: KY Srinivasan; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: KY Srinivasan
> Subject: Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
>
> "K. Y. Srinivasan" <[email protected]> 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
On Sun, 7 Oct 2012 16:59:46 -0700
"K. Y. Srinivasan" <[email protected]> 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;
> +}
>
> ...
>
On Mon, 8 Oct 2012 06:35:39 -0700
Greg KH <[email protected]> wrote:
> On Mon, Oct 08, 2012 at 03:35:50AM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:[email protected]]
> > > Sent: Sunday, October 07, 2012 8:44 PM
> > > To: KY Srinivasan
> > > Cc: [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected]; [email protected]
> > > Subject: Re: [PATCH 1/2] mm: Export vm_committed_as
> > >
> > > On Sun, Oct 07, 2012 at 04:59:45PM -0700, K. Y. Srinivasan wrote:
> > > > The policy engine on the host expects the guest to report the
> > > > committed_as. Since this variable is not exported,
> > > > export this symbol.
> > >
> > > Why are these symbols not needed by either Xen or KVM or vmware, which
> > > I think all support the same thing, right?
> >
> > The basic balloon driver does not need this symbol since the basic balloon driver
> > is not automatically driven by the host. On the Windows host we have a policy engine that
> > drives the balloon driver based on both guest level memory pressure that the guest
> > reports as well as other system level metrics the host maintains. We need this symbol to
> > drive the policy engine on the host.
>
> Ok, but you're going to have to get the -mm developers to agree that
> this is ok before I can accept it.
Well I guess it won't kill us.
I do wonder what relevance vm_committed_as has to processes which are
running within a memcg container?
> -----Original Message-----
> From: Andrew Morton [mailto:[email protected]]
> Sent: Tuesday, October 09, 2012 3:48 PM
> To: Greg KH
> Cc: KY Srinivasan; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH 1/2] mm: Export vm_committed_as
>
> On Mon, 8 Oct 2012 06:35:39 -0700
> Greg KH <[email protected]> wrote:
>
> > On Mon, Oct 08, 2012 at 03:35:50AM +0000, KY Srinivasan wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Greg KH [mailto:[email protected]]
> > > > Sent: Sunday, October 07, 2012 8:44 PM
> > > > To: KY Srinivasan
> > > > Cc: [email protected]; [email protected];
> [email protected];
> > > > [email protected]; [email protected]; [email protected]
> > > > Subject: Re: [PATCH 1/2] mm: Export vm_committed_as
> > > >
> > > > On Sun, Oct 07, 2012 at 04:59:45PM -0700, K. Y. Srinivasan wrote:
> > > > > The policy engine on the host expects the guest to report the
> > > > > committed_as. Since this variable is not exported,
> > > > > export this symbol.
> > > >
> > > > Why are these symbols not needed by either Xen or KVM or vmware, which
> > > > I think all support the same thing, right?
> > >
> > > The basic balloon driver does not need this symbol since the basic balloon
> driver
> > > is not automatically driven by the host. On the Windows host we have a policy
> engine that
> > > drives the balloon driver based on both guest level memory pressure that the
> guest
> > > reports as well as other system level metrics the host maintains. We need this
> symbol to
> > > drive the policy engine on the host.
> >
> > Ok, but you're going to have to get the -mm developers to agree that
> > this is ok before I can accept it.
>
> Well I guess it won't kill us.
Thanks.
K. Y
> -----Original Message-----
> From: Andrew Morton [mailto:[email protected]]
> Sent: Tuesday, October 09, 2012 3:45 PM
> To: KY Srinivasan
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
>
> On Sun, 7 Oct 2012 16:59:46 -0700
> "K. Y. Srinivasan" <[email protected]> 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
On Wed, 10 Oct 2012 00:09:12 +0000 KY Srinivasan <[email protected]> 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.
On Wed, 10 Oct 2012 00:11:28 +0000 KY Srinivasan <[email protected]> wrote:
>
>
> > -----Original Message-----
> > From: Andrew Morton [mailto:[email protected]]
> > Sent: Tuesday, October 09, 2012 3:48 PM
> > To: Greg KH
> > Cc: KY Srinivasan; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected]
> > Subject: Re: [PATCH 1/2] mm: Export vm_committed_as
> >
> > On Mon, 8 Oct 2012 06:35:39 -0700
> > Greg KH <[email protected]> wrote:
> >
> > > On Mon, Oct 08, 2012 at 03:35:50AM +0000, KY Srinivasan wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Greg KH [mailto:[email protected]]
> > > > > Sent: Sunday, October 07, 2012 8:44 PM
> > > > > To: KY Srinivasan
> > > > > Cc: [email protected]; [email protected];
> > [email protected];
> > > > > [email protected]; [email protected]; [email protected]
> > > > > Subject: Re: [PATCH 1/2] mm: Export vm_committed_as
> > > > >
> > > > > On Sun, Oct 07, 2012 at 04:59:45PM -0700, K. Y. Srinivasan wrote:
> > > > > > The policy engine on the host expects the guest to report the
> > > > > > committed_as. Since this variable is not exported,
> > > > > > export this symbol.
> > > > >
> > > > > Why are these symbols not needed by either Xen or KVM or vmware, which
> > > > > I think all support the same thing, right?
> > > >
> > > > The basic balloon driver does not need this symbol since the basic balloon
> > driver
> > > > is not automatically driven by the host. On the Windows host we have a policy
> > engine that
> > > > drives the balloon driver based on both guest level memory pressure that the
> > guest
> > > > reports as well as other system level metrics the host maintains. We need this
> > symbol to
> > > > drive the policy engine on the host.
> > >
> > > Ok, but you're going to have to get the -mm developers to agree that
> > > this is ok before I can accept it.
> >
> > Well I guess it won't kill us.
>
>
> Thanks.
>
The other part of my email seems to have vanished. Which makes me sad,
because I was rather interested in the answer.
> -----Original Message-----
> From: Andrew Morton [mailto:[email protected]]
> Sent: Tuesday, October 09, 2012 9:15 PM
> To: KY Srinivasan
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH 2/2] Drivers: hv: Add Hyper-V balloon driver
>
> On Wed, 10 Oct 2012 00:09:12 +0000 KY Srinivasan <[email protected]> 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
> -----Original Message-----
> From: Andrew Morton [mailto:[email protected]]
> Sent: Tuesday, October 09, 2012 9:17 PM
> To: KY Srinivasan
> Cc: Greg KH; [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH 1/2] mm: Export vm_committed_as
>
> On Wed, 10 Oct 2012 00:11:28 +0000 KY Srinivasan <[email protected]> wrote:
>
> >
> >
> > > -----Original Message-----
> > > From: Andrew Morton [mailto:[email protected]]
> > > Sent: Tuesday, October 09, 2012 3:48 PM
> > > To: Greg KH
> > > Cc: KY Srinivasan; [email protected]; [email protected];
> > > [email protected]; [email protected]; [email protected]
> > > Subject: Re: [PATCH 1/2] mm: Export vm_committed_as
> > >
> > > On Mon, 8 Oct 2012 06:35:39 -0700
> > > Greg KH <[email protected]> wrote:
> > >
> > > > On Mon, Oct 08, 2012 at 03:35:50AM +0000, KY Srinivasan wrote:
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Greg KH [mailto:[email protected]]
> > > > > > Sent: Sunday, October 07, 2012 8:44 PM
> > > > > > To: KY Srinivasan
> > > > > > Cc: [email protected]; [email protected];
> > > [email protected];
> > > > > > [email protected]; [email protected]; [email protected]
> > > > > > Subject: Re: [PATCH 1/2] mm: Export vm_committed_as
> > > > > >
> > > > > > On Sun, Oct 07, 2012 at 04:59:45PM -0700, K. Y. Srinivasan wrote:
> > > > > > > The policy engine on the host expects the guest to report the
> > > > > > > committed_as. Since this variable is not exported,
> > > > > > > export this symbol.
> > > > > >
> > > > > > Why are these symbols not needed by either Xen or KVM or vmware,
> which
> > > > > > I think all support the same thing, right?
> > > > >
> > > > > The basic balloon driver does not need this symbol since the basic balloon
> > > driver
> > > > > is not automatically driven by the host. On the Windows host we have a
> policy
> > > engine that
> > > > > drives the balloon driver based on both guest level memory pressure that
> the
> > > guest
> > > > > reports as well as other system level metrics the host maintains. We need
> this
> > > symbol to
> > > > > drive the policy engine on the host.
> > > >
> > > > Ok, but you're going to have to get the -mm developers to agree that
> > > > this is ok before I can accept it.
> > >
> > > Well I guess it won't kill us.
> >
> >
> > Thanks.
> >
>
> The other part of my email seems to have vanished. Which makes me sad,
> because I was rather interested in the answer.
Andrew,
I am truly sorry for not answering your question. To be honest, I was not quite sure
what the issue was, given from what I can tell, the vm_committed_as is maintained
globally and would represent the overall memory commitment of the system. Is this not the
case.
Regards,
K. Y
On 10/09/2012 09:44 PM, Andrew Morton wrote:
> On Sun, 7 Oct 2012 16:59:46 -0700
> "K. Y. Srinivasan" <[email protected]> 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
On 10/09/2012 06:14 PM, Andrew Morton wrote:
> On Wed, 10 Oct 2012 00:09:12 +0000 KY Srinivasan <[email protected]> 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
On Wed, 10 Oct 2012 16:34:37 -0700
Jeremy Fitzhardinge <[email protected]> wrote:
> On 10/09/2012 06:14 PM, Andrew Morton wrote:
> > On Wed, 10 Oct 2012 00:09:12 +0000 KY Srinivasan <[email protected]> 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.
Andrew Morton <[email protected]> writes:
> On Wed, 10 Oct 2012 16:34:37 -0700
> Jeremy Fitzhardinge <[email protected]> wrote:
>
>> On 10/09/2012 06:14 PM, Andrew Morton wrote:
>> > On Wed, 10 Oct 2012 00:09:12 +0000 KY Srinivasan <[email protected]> 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.
> -----Original Message-----
> From: Andrew Morton [mailto:[email protected]]
> Sent: Tuesday, October 09, 2012 3:48 PM
> To: Greg KH
> Cc: KY Srinivasan; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH 1/2] mm: Export vm_committed_as
>
> On Mon, 8 Oct 2012 06:35:39 -0700
> Greg KH <[email protected]> wrote:
>
> > On Mon, Oct 08, 2012 at 03:35:50AM +0000, KY Srinivasan wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Greg KH [mailto:[email protected]]
> > > > Sent: Sunday, October 07, 2012 8:44 PM
> > > > To: KY Srinivasan
> > > > Cc: [email protected]; [email protected];
> [email protected];
> > > > [email protected]; [email protected]; [email protected]
> > > > Subject: Re: [PATCH 1/2] mm: Export vm_committed_as
> > > >
> > > > On Sun, Oct 07, 2012 at 04:59:45PM -0700, K. Y. Srinivasan wrote:
> > > > > The policy engine on the host expects the guest to report the
> > > > > committed_as. Since this variable is not exported,
> > > > > export this symbol.
> > > >
> > > > Why are these symbols not needed by either Xen or KVM or vmware, which
> > > > I think all support the same thing, right?
> > >
> > > The basic balloon driver does not need this symbol since the basic balloon
> driver
> > > is not automatically driven by the host. On the Windows host we have a policy
> engine that
> > > drives the balloon driver based on both guest level memory pressure that the
> guest
> > > reports as well as other system level metrics the host maintains. We need this
> symbol to
> > > drive the policy engine on the host.
> >
> > Ok, but you're going to have to get the -mm developers to agree that
> > this is ok before I can accept it.
>
> Well I guess it won't kill us.
Andrew,
I presumed this was an Ack from you with regards to exporting the
symbol. Looks like Greg is waiting to hear from you before he can check
these patches in. Could you provide an explicit Ack.
Regards,
K. Y
>
> I do wonder what relevance vm_committed_as has to processes which are
> running within a memcg container?
>
>
>
On Sat, 3 Nov 2012 14:09:38 +0000
KY Srinivasan <[email protected]> wrote:
>
>
> > >
> > > Ok, but you're going to have to get the -mm developers to agree that
> > > this is ok before I can accept it.
> >
> > Well I guess it won't kill us.
>
> Andrew,
>
> I presumed this was an Ack from you with regards to exporting the
> symbol. Looks like Greg is waiting to hear from you before he can check
> these patches in. Could you provide an explicit Ack.
>
Well, I do have some qualms about exporting vm_committed_as to modules.
vm_committed_as is a global thing and only really makes sense in a
non-containerised system. If the application is running within a
memory cgroup then vm_enough_memory() and the global overcommit policy
are at best irrelevant and misleading.
If use of vm_committed_as is indeed a bad thing, then exporting it to
modules might increase the amount of badness in the kernel.
I don't think these qualms are serious enough to stand in the way of
this patch, but I'd be interested in hearing the memcg developers'
thoughts on the matter?
Perhaps you could provide a detailed description of why your module
actually needs this? Precisely what information is it looking for
and why? If we know that then perhaps a more comfortable alternative
can be found.
> -----Original Message-----
> From: Andrew Morton [mailto:[email protected]]
> Sent: Monday, November 05, 2012 4:45 PM
> To: KY Srinivasan
> Cc: Greg KH; [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Hiroyuki Kamezawa; Michal Hocko; Johannes Weiner; Ying Han
> Subject: Re: [PATCH 1/2] mm: Export vm_committed_as
>
> On Sat, 3 Nov 2012 14:09:38 +0000
> KY Srinivasan <[email protected]> wrote:
>
> >
> >
> > > >
> > > > Ok, but you're going to have to get the -mm developers to agree that
> > > > this is ok before I can accept it.
> > >
> > > Well I guess it won't kill us.
> >
> > Andrew,
> >
> > I presumed this was an Ack from you with regards to exporting the
> > symbol. Looks like Greg is waiting to hear from you before he can check
> > these patches in. Could you provide an explicit Ack.
> >
>
> Well, I do have some qualms about exporting vm_committed_as to modules.
>
> vm_committed_as is a global thing and only really makes sense in a
> non-containerised system. If the application is running within a
> memory cgroup then vm_enough_memory() and the global overcommit policy
> are at best irrelevant and misleading.
>
> If use of vm_committed_as is indeed a bad thing, then exporting it to
> modules might increase the amount of badness in the kernel.
>
>
> I don't think these qualms are serious enough to stand in the way of
> this patch, but I'd be interested in hearing the memcg developers'
> thoughts on the matter?
>
>
> Perhaps you could provide a detailed description of why your module
> actually needs this? Precisely what information is it looking for
> and why? If we know that then perhaps a more comfortable alternative
> can be found.
The Hyper-V host has a policy engine for managing available physical memory across
competing virtual machines. This policy decision is based on a number of parameters
including the memory pressure reported by the guest. Currently, the pressure calculation is
based on the memory commitment made by the guest. From what I can tell, the ratio of
currently allocated physical memory to the current memory commitment made by the guest
(vm_committed_as) is used as one of the parameters in making the memory balancing decision on
the host. This is what Windows guests report to the host. So, I need some measure of memory
commitments made by the Linux guest. This is the reason I want export vm_committed_as.
Regards,
K. Y
On Mon, 5 Nov 2012, KY Srinivasan wrote:
> The Hyper-V host has a policy engine for managing available physical memory across
> competing virtual machines. This policy decision is based on a number of parameters
> including the memory pressure reported by the guest. Currently, the pressure calculation is
> based on the memory commitment made by the guest. From what I can tell, the ratio of
> currently allocated physical memory to the current memory commitment made by the guest
> (vm_committed_as) is used as one of the parameters in making the memory balancing decision on
> the host. This is what Windows guests report to the host. So, I need some measure of memory
> commitments made by the Linux guest. This is the reason I want export vm_committed_as.
>
I don't think you should export the symbol itself to modules but rather a
helper function that returns s64 that just wraps
percpu_counter_read_positive() which your driver could use instead.
(And why percpu_counter_read_positive() returns a signed type is a
mystery.)
On Mon 05-11-12 22:12:25, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Andrew Morton [mailto:[email protected]]
> > Sent: Monday, November 05, 2012 4:45 PM
> > To: KY Srinivasan
> > Cc: Greg KH; [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > Hiroyuki Kamezawa; Michal Hocko; Johannes Weiner; Ying Han
> > Subject: Re: [PATCH 1/2] mm: Export vm_committed_as
> >
> > On Sat, 3 Nov 2012 14:09:38 +0000
> > KY Srinivasan <[email protected]> wrote:
> >
> > >
> > >
> > > > >
> > > > > Ok, but you're going to have to get the -mm developers to agree that
> > > > > this is ok before I can accept it.
> > > >
> > > > Well I guess it won't kill us.
> > >
> > > Andrew,
> > >
> > > I presumed this was an Ack from you with regards to exporting the
> > > symbol. Looks like Greg is waiting to hear from you before he can check
> > > these patches in. Could you provide an explicit Ack.
> > >
> >
> > Well, I do have some qualms about exporting vm_committed_as to modules.
> >
> > vm_committed_as is a global thing and only really makes sense in a
> > non-containerised system. If the application is running within a
> > memory cgroup then vm_enough_memory() and the global overcommit policy
> > are at best irrelevant and misleading.
> >
> > If use of vm_committed_as is indeed a bad thing, then exporting it to
> > modules might increase the amount of badness in the kernel.
> >
> >
> > I don't think these qualms are serious enough to stand in the way of
> > this patch, but I'd be interested in hearing the memcg developers'
> > thoughts on the matter?
> >
> >
> > Perhaps you could provide a detailed description of why your module
> > actually needs this? Precisely what information is it looking for
> > and why? If we know that then perhaps a more comfortable alternative
> > can be found.
>
> The Hyper-V host has a policy engine for managing available physical
> memory across competing virtual machines. This policy decision
> is based on a number of parameters including the memory pressure
> reported by the guest. Currently, the pressure calculation is based
> on the memory commitment made by the guest. From what I can tell, the
> ratio of currently allocated physical memory to the current memory
> commitment made by the guest (vm_committed_as) is used as one of the
> parameters in making the memory balancing decision on the host. This
> is what Windows guests report to the host. So, I need some measure of
> memory commitments made by the Linux guest. This is the reason I want
> export vm_committed_as.
So IIUC it will be guest who reports the value and the guest runs in the
ring-0 so it is not in any user process context, right?
If this is correct then memcg doesn't play any role here.
--
Michal Hocko
SUSE Labs
> -----Original Message-----
> From: Michal Hocko [mailto:[email protected]] On Behalf Of Michal Hocko
> Sent: Tuesday, November 06, 2012 4:06 AM
> To: KY Srinivasan
> Cc: Andrew Morton; Greg KH; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; Hiroyuki Kamezawa; Johannes Weiner; Ying Han
> Subject: Re: [PATCH 1/2] mm: Export vm_committed_as
>
> On Mon 05-11-12 22:12:25, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Andrew Morton [mailto:[email protected]]
> > > Sent: Monday, November 05, 2012 4:45 PM
> > > To: KY Srinivasan
> > > Cc: Greg KH; [email protected]; [email protected];
> [email protected];
> > > [email protected]; [email protected]; [email protected];
> > > Hiroyuki Kamezawa; Michal Hocko; Johannes Weiner; Ying Han
> > > Subject: Re: [PATCH 1/2] mm: Export vm_committed_as
> > >
> > > On Sat, 3 Nov 2012 14:09:38 +0000
> > > KY Srinivasan <[email protected]> wrote:
> > >
> > > >
> > > >
> > > > > >
> > > > > > Ok, but you're going to have to get the -mm developers to agree that
> > > > > > this is ok before I can accept it.
> > > > >
> > > > > Well I guess it won't kill us.
> > > >
> > > > Andrew,
> > > >
> > > > I presumed this was an Ack from you with regards to exporting the
> > > > symbol. Looks like Greg is waiting to hear from you before he can check
> > > > these patches in. Could you provide an explicit Ack.
> > > >
> > >
> > > Well, I do have some qualms about exporting vm_committed_as to modules.
> > >
> > > vm_committed_as is a global thing and only really makes sense in a
> > > non-containerised system. If the application is running within a
> > > memory cgroup then vm_enough_memory() and the global overcommit
> policy
> > > are at best irrelevant and misleading.
> > >
> > > If use of vm_committed_as is indeed a bad thing, then exporting it to
> > > modules might increase the amount of badness in the kernel.
> > >
> > >
> > > I don't think these qualms are serious enough to stand in the way of
> > > this patch, but I'd be interested in hearing the memcg developers'
> > > thoughts on the matter?
> > >
> > >
> > > Perhaps you could provide a detailed description of why your module
> > > actually needs this? Precisely what information is it looking for
> > > and why? If we know that then perhaps a more comfortable alternative
> > > can be found.
> >
> > The Hyper-V host has a policy engine for managing available physical
> > memory across competing virtual machines. This policy decision
> > is based on a number of parameters including the memory pressure
> > reported by the guest. Currently, the pressure calculation is based
> > on the memory commitment made by the guest. From what I can tell, the
> > ratio of currently allocated physical memory to the current memory
> > commitment made by the guest (vm_committed_as) is used as one of the
> > parameters in making the memory balancing decision on the host. This
> > is what Windows guests report to the host. So, I need some measure of
> > memory commitments made by the Linux guest. This is the reason I want
> > export vm_committed_as.
>
> So IIUC it will be guest who reports the value and the guest runs in the
> ring-0 so it is not in any user process context, right?
> If this is correct then memcg doesn't play any role here.
Thanks Michal. Yes, the kernel driver reports this metric to the host.
Andrew, let me know how I should proceed here.
Thanks,
K. Y
On Mon 05-11-12 14:33:12, David Rientjes wrote:
> On Mon, 5 Nov 2012, KY Srinivasan wrote:
>
> > The Hyper-V host has a policy engine for managing available physical memory across
> > competing virtual machines. This policy decision is based on a number of parameters
> > including the memory pressure reported by the guest. Currently, the pressure calculation is
> > based on the memory commitment made by the guest. From what I can tell, the ratio of
> > currently allocated physical memory to the current memory commitment made by the guest
> > (vm_committed_as) is used as one of the parameters in making the memory balancing decision on
> > the host. This is what Windows guests report to the host. So, I need some measure of memory
> > commitments made by the Linux guest. This is the reason I want export vm_committed_as.
> >
>
> I don't think you should export the symbol itself to modules but rather a
> helper function that returns s64 that just wraps
> percpu_counter_read_positive() which your driver could use instead.
Agreed, we should rather make sure that nobody can manipulate the value
from modules.
> (And why percpu_counter_read_positive() returns a signed type is a
> mystery.)
Strange indeed. The last commit changed it from long to s64 to suport
values bigger than 2^31 but even the original long doesn't make much
sense to me.
--
Michal Hocko
SUSE Labs
> -----Original Message-----
> From: KY Srinivasan
> Sent: Tuesday, November 06, 2012 7:53 AM
> To: 'Michal Hocko'
> Cc: Andrew Morton; Greg KH; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; Hiroyuki Kamezawa; Johannes Weiner; Ying Han
> Subject: RE: [PATCH 1/2] mm: Export vm_committed_as
>
>
>
> > -----Original Message-----
> > From: Michal Hocko [mailto:[email protected]] On Behalf Of Michal Hocko
> > Sent: Tuesday, November 06, 2012 4:06 AM
> > To: KY Srinivasan
> > Cc: Andrew Morton; Greg KH; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected]; linux-
> > [email protected]; Hiroyuki Kamezawa; Johannes Weiner; Ying Han
> > Subject: Re: [PATCH 1/2] mm: Export vm_committed_as
> >
> > On Mon 05-11-12 22:12:25, KY Srinivasan wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Andrew Morton [mailto:[email protected]]
> > > > Sent: Monday, November 05, 2012 4:45 PM
> > > > To: KY Srinivasan
> > > > Cc: Greg KH; [email protected]; [email protected];
> > [email protected];
> > > > [email protected]; [email protected]; [email protected];
> > > > Hiroyuki Kamezawa; Michal Hocko; Johannes Weiner; Ying Han
> > > > Subject: Re: [PATCH 1/2] mm: Export vm_committed_as
> > > >
> > > > On Sat, 3 Nov 2012 14:09:38 +0000
> > > > KY Srinivasan <[email protected]> wrote:
> > > >
> > > > >
> > > > >
> > > > > > >
> > > > > > > Ok, but you're going to have to get the -mm developers to agree that
> > > > > > > this is ok before I can accept it.
> > > > > >
> > > > > > Well I guess it won't kill us.
> > > > >
> > > > > Andrew,
> > > > >
> > > > > I presumed this was an Ack from you with regards to exporting the
> > > > > symbol. Looks like Greg is waiting to hear from you before he can check
> > > > > these patches in. Could you provide an explicit Ack.
> > > > >
> > > >
> > > > Well, I do have some qualms about exporting vm_committed_as to
> modules.
> > > >
> > > > vm_committed_as is a global thing and only really makes sense in a
> > > > non-containerised system. If the application is running within a
> > > > memory cgroup then vm_enough_memory() and the global overcommit
> > policy
> > > > are at best irrelevant and misleading.
> > > >
> > > > If use of vm_committed_as is indeed a bad thing, then exporting it to
> > > > modules might increase the amount of badness in the kernel.
> > > >
> > > >
> > > > I don't think these qualms are serious enough to stand in the way of
> > > > this patch, but I'd be interested in hearing the memcg developers'
> > > > thoughts on the matter?
> > > >
> > > >
> > > > Perhaps you could provide a detailed description of why your module
> > > > actually needs this? Precisely what information is it looking for
> > > > and why? If we know that then perhaps a more comfortable alternative
> > > > can be found.
> > >
> > > The Hyper-V host has a policy engine for managing available physical
> > > memory across competing virtual machines. This policy decision
> > > is based on a number of parameters including the memory pressure
> > > reported by the guest. Currently, the pressure calculation is based
> > > on the memory commitment made by the guest. From what I can tell, the
> > > ratio of currently allocated physical memory to the current memory
> > > commitment made by the guest (vm_committed_as) is used as one of the
> > > parameters in making the memory balancing decision on the host. This
> > > is what Windows guests report to the host. So, I need some measure of
> > > memory commitments made by the Linux guest. This is the reason I want
> > > export vm_committed_as.
> >
> > So IIUC it will be guest who reports the value and the guest runs in the
> > ring-0 so it is not in any user process context, right?
> > If this is correct then memcg doesn't play any role here.
>
> Thanks Michal. Yes, the kernel driver reports this metric to the host.
> Andrew, let me know how I should proceed here.
Ping.
Regards,
K. Y
>
> Thanks,
>
> K. Y
On Thu, 8 Nov 2012, KY Srinivasan wrote:
> > Thanks Michal. Yes, the kernel driver reports this metric to the host.
> > Andrew, let me know how I should proceed here.
>
> Ping.
>
Could you respond to my email in this thread?
> -----Original Message-----
> From: David Rientjes [mailto:[email protected]]
> Sent: Monday, November 05, 2012 5:33 PM
> To: KY Srinivasan
> Cc: Andrew Morton; Greg KH; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; Hiroyuki Kamezawa; Michal Hocko; Johannes Weiner; Ying Han
> Subject: RE: [PATCH 1/2] mm: Export vm_committed_as
>
> On Mon, 5 Nov 2012, KY Srinivasan wrote:
>
> > The Hyper-V host has a policy engine for managing available physical memory
> across
> > competing virtual machines. This policy decision is based on a number of
> parameters
> > including the memory pressure reported by the guest. Currently, the pressure
> calculation is
> > based on the memory commitment made by the guest. From what I can tell,
> the ratio of
> > currently allocated physical memory to the current memory commitment made
> by the guest
> > (vm_committed_as) is used as one of the parameters in making the memory
> balancing decision on
> > the host. This is what Windows guests report to the host. So, I need some
> measure of memory
> > commitments made by the Linux guest. This is the reason I want export
> vm_committed_as.
> >
>
> I don't think you should export the symbol itself to modules but rather a
> helper function that returns s64 that just wraps
> percpu_counter_read_positive() which your driver could use instead.
>
> (And why percpu_counter_read_positive() returns a signed type is a
> mystery.)
Yes, this makes sense. I just want to access (read) this metric. Andrew, if you are willing to
take this patch, I could send one.
Regards,
K. Y
On Thu, 8 Nov 2012 22:01:33 +0000
KY Srinivasan <[email protected]> wrote:
>
>
> > -----Original Message-----
> > From: David Rientjes [mailto:[email protected]]
> > Sent: Monday, November 05, 2012 5:33 PM
> > To: KY Srinivasan
> > Cc: Andrew Morton; Greg KH; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected]; linux-
> > [email protected]; Hiroyuki Kamezawa; Michal Hocko; Johannes Weiner; Ying Han
> > Subject: RE: [PATCH 1/2] mm: Export vm_committed_as
> >
> > On Mon, 5 Nov 2012, KY Srinivasan wrote:
> >
> > > The Hyper-V host has a policy engine for managing available physical memory
> > across
> > > competing virtual machines. This policy decision is based on a number of
> > parameters
> > > including the memory pressure reported by the guest. Currently, the pressure
> > calculation is
> > > based on the memory commitment made by the guest. From what I can tell,
> > the ratio of
> > > currently allocated physical memory to the current memory commitment made
> > by the guest
> > > (vm_committed_as) is used as one of the parameters in making the memory
> > balancing decision on
> > > the host. This is what Windows guests report to the host. So, I need some
> > measure of memory
> > > commitments made by the Linux guest. This is the reason I want export
> > vm_committed_as.
> > >
> >
> > I don't think you should export the symbol itself to modules but rather a
> > helper function that returns s64 that just wraps
> > percpu_counter_read_positive() which your driver could use instead.
> >
> > (And why percpu_counter_read_positive() returns a signed type is a
> > mystery.)
>
> Yes, this makes sense. I just want to access (read) this metric. Andrew, if you are willing to
> take this patch, I could send one.
Sure. I suppose that's better, although any module which modifies
committed_as would never pass review (rofl).
> -----Original Message-----
> From: Andrew Morton [mailto:[email protected]]
> Sent: Thursday, November 08, 2012 5:05 PM
> To: KY Srinivasan
> Cc: David Rientjes; Greg KH; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; Hiroyuki Kamezawa; Michal Hocko; Johannes Weiner; Ying Han
> Subject: Re: [PATCH 1/2] mm: Export vm_committed_as
>
> On Thu, 8 Nov 2012 22:01:33 +0000
> KY Srinivasan <[email protected]> wrote:
>
> >
> >
> > > -----Original Message-----
> > > From: David Rientjes [mailto:[email protected]]
> > > Sent: Monday, November 05, 2012 5:33 PM
> > > To: KY Srinivasan
> > > Cc: Andrew Morton; Greg KH; [email protected]; [email protected];
> > > [email protected]; [email protected]; [email protected];
> linux-
> > > [email protected]; Hiroyuki Kamezawa; Michal Hocko; Johannes Weiner; Ying
> Han
> > > Subject: RE: [PATCH 1/2] mm: Export vm_committed_as
> > >
> > > On Mon, 5 Nov 2012, KY Srinivasan wrote:
> > >
> > > > The Hyper-V host has a policy engine for managing available physical
> memory
> > > across
> > > > competing virtual machines. This policy decision is based on a number of
> > > parameters
> > > > including the memory pressure reported by the guest. Currently, the
> pressure
> > > calculation is
> > > > based on the memory commitment made by the guest. From what I can
> tell,
> > > the ratio of
> > > > currently allocated physical memory to the current memory commitment
> made
> > > by the guest
> > > > (vm_committed_as) is used as one of the parameters in making the
> memory
> > > balancing decision on
> > > > the host. This is what Windows guests report to the host. So, I need some
> > > measure of memory
> > > > commitments made by the Linux guest. This is the reason I want export
> > > vm_committed_as.
> > > >
> > >
> > > I don't think you should export the symbol itself to modules but rather a
> > > helper function that returns s64 that just wraps
> > > percpu_counter_read_positive() which your driver could use instead.
> > >
> > > (And why percpu_counter_read_positive() returns a signed type is a
> > > mystery.)
> >
> > Yes, this makes sense. I just want to access (read) this metric. Andrew, if you
> are willing to
> > take this patch, I could send one.
>
> Sure. I suppose that's better, although any module which modifies
> committed_as would never pass review (rofl).
Thanks Andrew; I will send the patch out along with the appropriately modified balloon driver patch.
Regards,
K. Y
>
>
On Thu, 8 Nov 2012, Andrew Morton wrote:
> > > I don't think you should export the symbol itself to modules but rather a
> > > helper function that returns s64 that just wraps
> > > percpu_counter_read_positive() which your driver could use instead.
> > >
> > > (And why percpu_counter_read_positive() returns a signed type is a
> > > mystery.)
> >
> > Yes, this makes sense. I just want to access (read) this metric. Andrew, if you are willing to
> > take this patch, I could send one.
>
> Sure. I suppose that's better, although any module which modifies
> committed_as would never pass review (rofl).
>
I was thinking of a function that all hypervisors can use (since xen also
uses it) that can be well documented and maintain the semantics that they
expect, whether that relines on vm_commited_as in the future or not.
On Thu, 8 Nov 2012 14:14:35 -0800 (PST)
David Rientjes <[email protected]> wrote:
> On Thu, 8 Nov 2012, Andrew Morton wrote:
>
> > > > I don't think you should export the symbol itself to modules but rather a
> > > > helper function that returns s64 that just wraps
> > > > percpu_counter_read_positive() which your driver could use instead.
> > > >
> > > > (And why percpu_counter_read_positive() returns a signed type is a
> > > > mystery.)
> > >
> > > Yes, this makes sense. I just want to access (read) this metric. Andrew, if you are willing to
> > > take this patch, I could send one.
> >
> > Sure. I suppose that's better, although any module which modifies
> > committed_as would never pass review (rofl).
> >
>
> I was thinking of a function that all hypervisors can use (since xen also
> uses it) that can be well documented and maintain the semantics that they
> expect, whether that relines on vm_commited_as in the future or not.
Yes, it would be nice to have some central site where people can go to
understand what's happening here.
It's still unclear to me that committed_as is telling the
hypervisors precisely what they want to know.