linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/1] mm: Export a function to read vm_committed_as
@ 2012-11-11  2:25 K. Y. Srinivasan
  2012-11-11  2:35 ` David Rientjes
  0 siblings, 1 reply; 12+ messages in thread
From: K. Y. Srinivasan @ 2012-11-11  2:25 UTC (permalink / raw)
  To: gregkh, linux-kernel, devel, olaf, apw, andi, akpm, linux-mm,
	kamezawa.hiroyuki, mhocko, hannes, yinghan
  Cc: K. Y. Srinivasan

It may be useful to be able to access vm_committed_as from device
drivers. On the Hyper-V platform, the host has a policy engine to balance
the available physical memory amongst all competing virtual machines
hosted on a given node. This policy engine is driven by a number of metrics
including the memory pressure reported by the guests. The balloon driver
for Linux on Hyper-V uses this function to report memory pressure back to
the host.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 include/linux/mman.h |    2 ++
 mm/mmap.c            |   11 +++++++++++
 mm/nommu.c           |   11 +++++++++++
 3 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/include/linux/mman.h b/include/linux/mman.h
index d09dde1..d53dc3d 100644
--- a/include/linux/mman.h
+++ b/include/linux/mman.h
@@ -11,6 +11,8 @@ extern int sysctl_overcommit_memory;
 extern int sysctl_overcommit_ratio;
 extern struct percpu_counter vm_committed_as;
 
+unsigned long read_vm_committed_as(void);
+
 static inline void vm_acct_memory(long pages)
 {
 	percpu_counter_add(&vm_committed_as, pages);
diff --git a/mm/mmap.c b/mm/mmap.c
index 2d94235..e527239 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -89,6 +89,17 @@ int sysctl_max_map_count __read_mostly = DEFAULT_MAX_MAP_COUNT;
 struct percpu_counter vm_committed_as ____cacheline_aligned_in_smp;
 
 /*
+ * A wrapper to read vm_committed_as that can be used by external modules.
+ */
+
+unsigned long read_vm_committed_as(void)
+{
+	return percpu_counter_read_positive(&vm_committed_as);
+}
+
+EXPORT_SYMBOL_GPL(read_vm_committed_as);
+
+/*
  * Check that a process has enough memory to allocate a new virtual
  * mapping. 0 means there is enough memory for the allocation to
  * succeed and -ENOMEM implies there is not.
diff --git a/mm/nommu.c b/mm/nommu.c
index 45131b4..dbbd0aa 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -66,6 +66,17 @@ int heap_stack_gap = 0;
 
 atomic_long_t mmap_pages_allocated;
 
+/*
+ * A wrapper to read vm_committed_as that can be used by external modules.
+ */
+
+unsigned long read_vm_committed_as(void)
+{
+	return percpu_counter_read_positive(&vm_committed_as);
+}
+
+EXPORT_SYMBOL_GPL(read_vm_committed_as);
+
 EXPORT_SYMBOL(mem_map);
 EXPORT_SYMBOL(num_physpages);
 
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/1] mm: Export a function to read vm_committed_as
  2012-11-11  2:25 [PATCH 1/1] mm: Export a function to read vm_committed_as K. Y. Srinivasan
@ 2012-11-11  2:35 ` David Rientjes
  2012-11-11  9:24   ` KY Srinivasan
  2012-11-12 21:36   ` KY Srinivasan
  0 siblings, 2 replies; 12+ messages in thread
From: David Rientjes @ 2012-11-11  2:35 UTC (permalink / raw)
  To: K. Y. Srinivasan
  Cc: gregkh, linux-kernel, devel, olaf, apw, andi, akpm, linux-mm,
	kamezawa.hiroyuki, mhocko, hannes, yinghan

On Sat, 10 Nov 2012, K. Y. Srinivasan wrote:

> diff --git a/mm/mmap.c b/mm/mmap.c
> index 2d94235..e527239 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -89,6 +89,17 @@ int sysctl_max_map_count __read_mostly = DEFAULT_MAX_MAP_COUNT;
>  struct percpu_counter vm_committed_as ____cacheline_aligned_in_smp;
>  
>  /*
> + * A wrapper to read vm_committed_as that can be used by external modules.
> + */
> +
> +unsigned long read_vm_committed_as(void)
> +{
> +	return percpu_counter_read_positive(&vm_committed_as);
> +}
> +
> +EXPORT_SYMBOL_GPL(read_vm_committed_as);
> +
> +/*
>   * Check that a process has enough memory to allocate a new virtual
>   * mapping. 0 means there is enough memory for the allocation to
>   * succeed and -ENOMEM implies there is not.

This is precisely what I didn't want to see; I was expecting that this 
function was going to have some name that would describe what a hypervisor 
would use it for, regardless of its implementation and current use of 
vm_committed_as.  read_vm_committed_as() misses the entire point of the 
suggestion and a few people have mentioned that they think this 
implementation will evolve over time.

Please think of what you're trying to determine in the code that will 
depend on this and then convert the existing user in 
drivers/xen/xen-selfballoon.c.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH 1/1] mm: Export a function to read vm_committed_as
  2012-11-11  2:35 ` David Rientjes
@ 2012-11-11  9:24   ` KY Srinivasan
  2012-11-12 21:53     ` David Rientjes
  2012-11-12 21:36   ` KY Srinivasan
  1 sibling, 1 reply; 12+ messages in thread
From: KY Srinivasan @ 2012-11-11  9:24 UTC (permalink / raw)
  To: David Rientjes
  Cc: gregkh, linux-kernel, devel, olaf, apw, andi, akpm, linux-mm,
	kamezawa.hiroyuki, mhocko, hannes, yinghan



> -----Original Message-----
> From: David Rientjes [mailto:rientjes@google.com]
> Sent: Saturday, November 10, 2012 9:35 PM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> andi@firstfloor.org; akpm@linux-foundation.org; linux-mm@kvack.org;
> kamezawa.hiroyuki@gmail.com; mhocko@suse.cz; hannes@cmpxchg.org;
> yinghan@google.com
> Subject: Re: [PATCH 1/1] mm: Export a function to read vm_committed_as
> 
> On Sat, 10 Nov 2012, K. Y. Srinivasan wrote:
> 
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 2d94235..e527239 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -89,6 +89,17 @@ int sysctl_max_map_count __read_mostly =
> DEFAULT_MAX_MAP_COUNT;
> >  struct percpu_counter vm_committed_as ____cacheline_aligned_in_smp;
> >
> >  /*
> > + * A wrapper to read vm_committed_as that can be used by external
> modules.
> > + */
> > +
> > +unsigned long read_vm_committed_as(void)
> > +{
> > +	return percpu_counter_read_positive(&vm_committed_as);
> > +}
> > +
> > +EXPORT_SYMBOL_GPL(read_vm_committed_as);
> > +
> > +/*
> >   * Check that a process has enough memory to allocate a new virtual
> >   * mapping. 0 means there is enough memory for the allocation to
> >   * succeed and -ENOMEM implies there is not.
> 
> This is precisely what I didn't want to see; I was expecting that this
> function was going to have some name that would describe what a hypervisor
> would use it for, regardless of its implementation and current use of
> vm_committed_as.  read_vm_committed_as() misses the entire point of the
> suggestion and a few people have mentioned that they think this
> implementation will evolve over time.
> 
> Please think of what you're trying to determine in the code that will
> depend on this and then convert the existing user in
> drivers/xen/xen-selfballoon.c.

David,

Thanks for the prompt response. For the Linux balloon driver for Hyper-V, I need access
to the metric that reflects the system wide memory commitment made by the guest kernel. 
In the Hyper-V case, this information is one of the many metrics used to drive the policy engine
on the host. Granted, the interface name I have chosen here could be more generic; how about
read_mem_commit_info(void). I am open to suggestions here.

With regards to making changes to the Xen self ballooning code, I would like to separate that patch
from the patch that implements the exported mechanism to access the memory commitment information.
Once we settle on this patch, I can submit the patch to fix the Xen self ballooning driver to use this new
interface along with the Hyper-V balloon driver that is currently blocked on resolving this issue.

Regards,

K. Y




^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH 1/1] mm: Export a function to read vm_committed_as
  2012-11-11  2:35 ` David Rientjes
  2012-11-11  9:24   ` KY Srinivasan
@ 2012-11-12 21:36   ` KY Srinivasan
  1 sibling, 0 replies; 12+ messages in thread
From: KY Srinivasan @ 2012-11-12 21:36 UTC (permalink / raw)
  To: David Rientjes
  Cc: gregkh, linux-kernel, devel, olaf, apw, andi, akpm, linux-mm,
	kamezawa.hiroyuki, mhocko, hannes, yinghan



> -----Original Message-----
> From: KY Srinivasan
> Sent: Sunday, November 11, 2012 4:24 AM
> To: 'David Rientjes'
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> andi@firstfloor.org; akpm@linux-foundation.org; linux-mm@kvack.org;
> kamezawa.hiroyuki@gmail.com; mhocko@suse.cz; hannes@cmpxchg.org;
> yinghan@google.com
> Subject: RE: [PATCH 1/1] mm: Export a function to read vm_committed_as
> 
> 
> 
> > -----Original Message-----
> > From: David Rientjes [mailto:rientjes@google.com]
> > Sent: Saturday, November 10, 2012 9:35 PM
> > To: KY Srinivasan
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> > andi@firstfloor.org; akpm@linux-foundation.org; linux-mm@kvack.org;
> > kamezawa.hiroyuki@gmail.com; mhocko@suse.cz; hannes@cmpxchg.org;
> > yinghan@google.com
> > Subject: Re: [PATCH 1/1] mm: Export a function to read vm_committed_as
> >
> > On Sat, 10 Nov 2012, K. Y. Srinivasan wrote:
> >
> > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > index 2d94235..e527239 100644
> > > --- a/mm/mmap.c
> > > +++ b/mm/mmap.c
> > > @@ -89,6 +89,17 @@ int sysctl_max_map_count __read_mostly =
> > DEFAULT_MAX_MAP_COUNT;
> > >  struct percpu_counter vm_committed_as ____cacheline_aligned_in_smp;
> > >
> > >  /*
> > > + * A wrapper to read vm_committed_as that can be used by external
> > modules.
> > > + */
> > > +
> > > +unsigned long read_vm_committed_as(void)
> > > +{
> > > +	return percpu_counter_read_positive(&vm_committed_as);
> > > +}
> > > +
> > > +EXPORT_SYMBOL_GPL(read_vm_committed_as);
> > > +
> > > +/*
> > >   * Check that a process has enough memory to allocate a new virtual
> > >   * mapping. 0 means there is enough memory for the allocation to
> > >   * succeed and -ENOMEM implies there is not.
> >
> > This is precisely what I didn't want to see; I was expecting that this
> > function was going to have some name that would describe what a hypervisor
> > would use it for, regardless of its implementation and current use of
> > vm_committed_as.  read_vm_committed_as() misses the entire point of the
> > suggestion and a few people have mentioned that they think this
> > implementation will evolve over time.
> >
> > Please think of what you're trying to determine in the code that will
> > depend on this and then convert the existing user in
> > drivers/xen/xen-selfballoon.c.
> 
> David,
> 
> Thanks for the prompt response. For the Linux balloon driver for Hyper-V, I need
> access
> to the metric that reflects the system wide memory commitment made by the
> guest kernel.
> In the Hyper-V case, this information is one of the many metrics used to drive the
> policy engine
> on the host. Granted, the interface name I have chosen here could be more
> generic; how about
> read_mem_commit_info(void). I am open to suggestions here.
> 
> With regards to making changes to the Xen self ballooning code, I would like to
> separate that patch
> from the patch that implements the exported mechanism to access the memory
> commitment information.
> Once we settle on this patch, I can submit the patch to fix the Xen self ballooning
> driver to use this new
> interface along with the Hyper-V balloon driver that is currently blocked on
> resolving this issue.

David,

I want to finalize this ASAP. Please give me your feedback so I can move this forward.

Regards,

K. Y



^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH 1/1] mm: Export a function to read vm_committed_as
  2012-11-11  9:24   ` KY Srinivasan
@ 2012-11-12 21:53     ` David Rientjes
  2012-11-12 22:58       ` KY Srinivasan
  0 siblings, 1 reply; 12+ messages in thread
From: David Rientjes @ 2012-11-12 21:53 UTC (permalink / raw)
  To: KY Srinivasan
  Cc: gregkh, linux-kernel, devel, olaf, apw, andi, akpm, linux-mm,
	kamezawa.hiroyuki, mhocko, hannes, yinghan

On Sun, 11 Nov 2012, KY Srinivasan wrote:

> Thanks for the prompt response. For the Linux balloon driver for Hyper-V, I need access
> to the metric that reflects the system wide memory commitment made by the guest kernel. 
> In the Hyper-V case, this information is one of the many metrics used to drive the policy engine
> on the host. Granted, the interface name I have chosen here could be more generic; how about
> read_mem_commit_info(void). I am open to suggestions here.
> 

I would suggest vm_memory_committed() and there shouldn't be a comment 
describing that this is just a wrapper for modules to read 
vm_committed_as, that's apparent from the implementation: it should be 
describing exactly what this value represents and why it is a useful 
metric (at least in the case that you're concerned about).

> With regards to making changes to the Xen self ballooning code, I would like to separate that patch
> from the patch that implements the exported mechanism to access the memory commitment information.

Why?  Is xen using it for a different inference?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH 1/1] mm: Export a function to read vm_committed_as
  2012-11-12 21:53     ` David Rientjes
@ 2012-11-12 22:58       ` KY Srinivasan
  2012-11-12 23:32         ` Dan Magenheimer
  0 siblings, 1 reply; 12+ messages in thread
From: KY Srinivasan @ 2012-11-12 22:58 UTC (permalink / raw)
  To: David Rientjes
  Cc: gregkh, linux-kernel, devel, olaf, apw, andi, akpm, linux-mm,
	kamezawa.hiroyuki, mhocko, hannes, yinghan, dan.magenheimer



> -----Original Message-----
> From: David Rientjes [mailto:rientjes@google.com]
> Sent: Monday, November 12, 2012 4:54 PM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> andi@firstfloor.org; akpm@linux-foundation.org; linux-mm@kvack.org;
> kamezawa.hiroyuki@gmail.com; mhocko@suse.cz; hannes@cmpxchg.org;
> yinghan@google.com
> Subject: RE: [PATCH 1/1] mm: Export a function to read vm_committed_as
> 
> On Sun, 11 Nov 2012, KY Srinivasan wrote:
> 
> > Thanks for the prompt response. For the Linux balloon driver for Hyper-V, I
> need access
> > to the metric that reflects the system wide memory commitment made by the
> guest kernel.
> > In the Hyper-V case, this information is one of the many metrics used to drive
> the policy engine
> > on the host. Granted, the interface name I have chosen here could be more
> generic; how about
> > read_mem_commit_info(void). I am open to suggestions here.
> >
> 
> I would suggest vm_memory_committed() and there shouldn't be a comment
> describing that this is just a wrapper for modules to read
> vm_committed_as, that's apparent from the implementation: it should be
> describing exactly what this value represents and why it is a useful
> metric (at least in the case that you're concerned about).

Will do; thanks.
> 
> > With regards to making changes to the Xen self ballooning code, I would like to
> separate that patch
> > from the patch that implements the exported mechanism to access the
> memory commitment information.
> 
> Why?  Is xen using it for a different inference?

I think it is good to separate these patches. Dan (copied here) wrote the code for the
Xen self balloon driver. If it is ok with him I can submit the patch for Xen as well. 


Regards,

K. Y



> 



^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH 1/1] mm: Export a function to read vm_committed_as
  2012-11-12 22:58       ` KY Srinivasan
@ 2012-11-12 23:32         ` Dan Magenheimer
  2012-11-12 23:49           ` David Rientjes
  2012-11-13  5:16           ` KY Srinivasan
  0 siblings, 2 replies; 12+ messages in thread
From: Dan Magenheimer @ 2012-11-12 23:32 UTC (permalink / raw)
  To: KY Srinivasan, David Rientjes, Konrad Wilk
  Cc: gregkh, linux-kernel, devel, olaf, apw, andi, akpm, linux-mm,
	kamezawa.hiroyuki, mhocko, hannes, yinghan

> From: KY Srinivasan [mailto:kys@microsoft.com]
> Sent: Monday, November 12, 2012 3:58 PM
> To: David Rientjes
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> olaf@aepfle.de; apw@canonical.com; andi@firstfloor.org; akpm@linux-foundation.org; linux-mm@kvack.org;
> kamezawa.hiroyuki@gmail.com; mhocko@suse.cz; hannes@cmpxchg.org; yinghan@google.com; Dan Magenheimer
> Subject: RE: [PATCH 1/1] mm: Export a function to read vm_committed_as
> 
> > -----Original Message-----
> > From: David Rientjes [mailto:rientjes@google.com]
> > Sent: Monday, November 12, 2012 4:54 PM
> > To: KY Srinivasan
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> > andi@firstfloor.org; akpm@linux-foundation.org; linux-mm@kvack.org;
> > kamezawa.hiroyuki@gmail.com; mhocko@suse.cz; hannes@cmpxchg.org;
> > yinghan@google.com
> > Subject: RE: [PATCH 1/1] mm: Export a function to read vm_committed_as
> >
> > On Sun, 11 Nov 2012, KY Srinivasan wrote:
> >
> > > Thanks for the prompt response. For the Linux balloon driver for Hyper-V, I
> > need access
> > > to the metric that reflects the system wide memory commitment made by the
> > guest kernel.
> > > In the Hyper-V case, this information is one of the many metrics used to drive
> > the policy engine
> > > on the host. Granted, the interface name I have chosen here could be more
> > generic; how about
> > > read_mem_commit_info(void). I am open to suggestions here.
> > >
> >
> > I would suggest vm_memory_committed() and there shouldn't be a comment
> > describing that this is just a wrapper for modules to read
> > vm_committed_as, that's apparent from the implementation: it should be
> > describing exactly what this value represents and why it is a useful
> > metric (at least in the case that you're concerned about).
> 
> Will do; thanks.
> >
> > > With regards to making changes to the Xen self ballooning code, I would like to
> > separate that patch
> > > from the patch that implements the exported mechanism to access the
> > memory commitment information.
> >
> > Why?  Is xen using it for a different inference?
> 
> I think it is good to separate these patches. Dan (copied here) wrote the code for the
> Xen self balloon driver. If it is ok with him I can submit the patch for Xen as well.

Hi KY --

If I understand correctly, this would be only a cosmetic (function renaming) change
to the Xen selfballooning code.  If so, then I will be happy to Ack when I
see the patch.  However, Konrad (konrad.wilk@oracle.com) is the maintainer
for all Xen code so you should ask him... and (from previous painful experience)
it can be difficult to sync even very simple interdependent changes going through
different maintainers without breaking linux-next.  So I can't offer any
help with that process, only commiseration. :-(

Dan

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH 1/1] mm: Export a function to read vm_committed_as
  2012-11-12 23:32         ` Dan Magenheimer
@ 2012-11-12 23:49           ` David Rientjes
  2012-11-13  5:24             ` KY Srinivasan
  2012-11-13  5:16           ` KY Srinivasan
  1 sibling, 1 reply; 12+ messages in thread
From: David Rientjes @ 2012-11-12 23:49 UTC (permalink / raw)
  To: Dan Magenheimer
  Cc: KY Srinivasan, Konrad Wilk, gregkh, linux-kernel, devel, olaf,
	apw, andi, akpm, linux-mm, kamezawa.hiroyuki, mhocko, hannes,
	yinghan

On Mon, 12 Nov 2012, Dan Magenheimer wrote:

> > > Why?  Is xen using it for a different inference?
> > 
> > I think it is good to separate these patches. Dan (copied here) wrote the code for the
> > Xen self balloon driver. If it is ok with him I can submit the patch for Xen as well.
> 
> Hi KY --
> 
> If I understand correctly, this would be only a cosmetic (function renaming) change
> to the Xen selfballooning code.  If so, then I will be happy to Ack when I
> see the patch.  However, Konrad (konrad.wilk@oracle.com) is the maintainer
> for all Xen code so you should ask him... and (from previous painful experience)
> it can be difficult to sync even very simple interdependent changes going through
> different maintainers without breaking linux-next.  So I can't offer any
> help with that process, only commiseration. :-(
> 

I think this should be done in the same patch as the function getting 
introduced with a cc to Konrad and routed through -mm; even better, 
perhaps he'll have some useful comments for how this is used for xen that 
can be included for context.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH 1/1] mm: Export a function to read vm_committed_as
  2012-11-12 23:32         ` Dan Magenheimer
  2012-11-12 23:49           ` David Rientjes
@ 2012-11-13  5:16           ` KY Srinivasan
  1 sibling, 0 replies; 12+ messages in thread
From: KY Srinivasan @ 2012-11-13  5:16 UTC (permalink / raw)
  To: Dan Magenheimer, David Rientjes, Konrad Wilk
  Cc: gregkh, linux-kernel, devel, olaf, apw, andi, akpm, linux-mm,
	kamezawa.hiroyuki, mhocko, hannes, yinghan



> -----Original Message-----
> From: Dan Magenheimer [mailto:dan.magenheimer@oracle.com]
> Sent: Monday, November 12, 2012 6:32 PM
> To: KY Srinivasan; David Rientjes; Konrad Wilk
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> andi@firstfloor.org; akpm@linux-foundation.org; linux-mm@kvack.org;
> kamezawa.hiroyuki@gmail.com; mhocko@suse.cz; hannes@cmpxchg.org;
> yinghan@google.com
> Subject: RE: [PATCH 1/1] mm: Export a function to read vm_committed_as
> 
> > From: KY Srinivasan [mailto:kys@microsoft.com]
> > Sent: Monday, November 12, 2012 3:58 PM
> > To: David Rientjes
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org;
> > olaf@aepfle.de; apw@canonical.com; andi@firstfloor.org; akpm@linux-
> foundation.org; linux-mm@kvack.org;
> > kamezawa.hiroyuki@gmail.com; mhocko@suse.cz; hannes@cmpxchg.org;
> yinghan@google.com; Dan Magenheimer
> > Subject: RE: [PATCH 1/1] mm: Export a function to read vm_committed_as
> >
> > > -----Original Message-----
> > > From: David Rientjes [mailto:rientjes@google.com]
> > > Sent: Monday, November 12, 2012 4:54 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> > > andi@firstfloor.org; akpm@linux-foundation.org; linux-mm@kvack.org;
> > > kamezawa.hiroyuki@gmail.com; mhocko@suse.cz; hannes@cmpxchg.org;
> > > yinghan@google.com
> > > Subject: RE: [PATCH 1/1] mm: Export a function to read vm_committed_as
> > >
> > > On Sun, 11 Nov 2012, KY Srinivasan wrote:
> > >
> > > > Thanks for the prompt response. For the Linux balloon driver for Hyper-V, I
> > > need access
> > > > to the metric that reflects the system wide memory commitment made by
> the
> > > guest kernel.
> > > > In the Hyper-V case, this information is one of the many metrics used to
> drive
> > > the policy engine
> > > > on the host. Granted, the interface name I have chosen here could be more
> > > generic; how about
> > > > read_mem_commit_info(void). I am open to suggestions here.
> > > >
> > >
> > > I would suggest vm_memory_committed() and there shouldn't be a
> comment
> > > describing that this is just a wrapper for modules to read
> > > vm_committed_as, that's apparent from the implementation: it should be
> > > describing exactly what this value represents and why it is a useful
> > > metric (at least in the case that you're concerned about).
> >
> > Will do; thanks.
> > >
> > > > With regards to making changes to the Xen self ballooning code, I would like
> to
> > > separate that patch
> > > > from the patch that implements the exported mechanism to access the
> > > memory commitment information.
> > >
> > > Why?  Is xen using it for a different inference?
> >
> > I think it is good to separate these patches. Dan (copied here) wrote the code
> for the
> > Xen self balloon driver. If it is ok with him I can submit the patch for Xen as well.
> 
> Hi KY --
> 
> If I understand correctly, this would be only a cosmetic (function renaming)
> change
> to the Xen selfballooning code.  If so, then I will be happy to Ack when I
> see the patch.  However, Konrad (konrad.wilk@oracle.com) is the maintainer
> for all Xen code so you should ask him... and (from previous painful experience)
> it can be difficult to sync even very simple interdependent changes going through
> different maintainers without breaking linux-next.  So I can't offer any
> help with that process, only commiseration. :-(
> 
> Dan
> 

Dan,

Thank you. I will send the patches out soon.

Regards,

K. Y


^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH 1/1] mm: Export a function to read vm_committed_as
  2012-11-12 23:49           ` David Rientjes
@ 2012-11-13  5:24             ` KY Srinivasan
  2012-11-13 15:41               ` Dan Magenheimer
  0 siblings, 1 reply; 12+ messages in thread
From: KY Srinivasan @ 2012-11-13  5:24 UTC (permalink / raw)
  To: David Rientjes, Dan Magenheimer
  Cc: Konrad Wilk, gregkh, linux-kernel, devel, olaf, apw, andi, akpm,
	linux-mm, kamezawa.hiroyuki, mhocko, hannes, yinghan



> -----Original Message-----
> From: David Rientjes [mailto:rientjes@google.com]
> Sent: Monday, November 12, 2012 6:49 PM
> To: Dan Magenheimer
> Cc: KY Srinivasan; Konrad Wilk; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org; olaf@aepfle.de;
> apw@canonical.com; andi@firstfloor.org; akpm@linux-foundation.org; linux-
> mm@kvack.org; kamezawa.hiroyuki@gmail.com; mhocko@suse.cz;
> hannes@cmpxchg.org; yinghan@google.com
> Subject: RE: [PATCH 1/1] mm: Export a function to read vm_committed_as
> 
> On Mon, 12 Nov 2012, Dan Magenheimer wrote:
> 
> > > > Why?  Is xen using it for a different inference?
> > >
> > > I think it is good to separate these patches. Dan (copied here) wrote the code
> for the
> > > Xen self balloon driver. If it is ok with him I can submit the patch for Xen as
> well.
> >
> > Hi KY --
> >
> > If I understand correctly, this would be only a cosmetic (function renaming)
> change
> > to the Xen selfballooning code.  If so, then I will be happy to Ack when I
> > see the patch.  However, Konrad (konrad.wilk@oracle.com) is the maintainer
> > for all Xen code so you should ask him... and (from previous painful experience)
> > it can be difficult to sync even very simple interdependent changes going
> through
> > different maintainers without breaking linux-next.  So I can't offer any
> > help with that process, only commiseration. :-(
> >
> 
> I think this should be done in the same patch as the function getting
> introduced with a cc to Konrad and routed through -mm; even better,
> perhaps he'll have some useful comments for how this is used for xen that
> can be included for context.
> 
Ok; I will send out a single patch. I am hoping this can be applied soon as Hyper-V balloon
driver is queued behind this.

Regards,

K. Y




^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH 1/1] mm: Export a function to read vm_committed_as
  2012-11-13  5:24             ` KY Srinivasan
@ 2012-11-13 15:41               ` Dan Magenheimer
  2012-11-13 21:04                 ` David Rientjes
  0 siblings, 1 reply; 12+ messages in thread
From: Dan Magenheimer @ 2012-11-13 15:41 UTC (permalink / raw)
  To: KY Srinivasan, David Rientjes
  Cc: Konrad Wilk, gregkh, linux-kernel, devel, olaf, apw, andi, akpm,
	linux-mm, kamezawa.hiroyuki, mhocko, hannes, yinghan

> From: KY Srinivasan [mailto:kys@microsoft.com]
> Subject: RE: [PATCH 1/1] mm: Export a function to read vm_committed_as
> 
> > From: David Rientjes [mailto:rientjes@google.com]
> > Sent: Monday, November 12, 2012 6:49 PM
> > To: Dan Magenheimer
> > Cc: KY Srinivasan; Konrad Wilk; gregkh@linuxfoundation.org; linux-
> > kernel@vger.kernel.org; devel@linuxdriverproject.org; olaf@aepfle.de;
> > apw@canonical.com; andi@firstfloor.org; akpm@linux-foundation.org; linux-
> > mm@kvack.org; kamezawa.hiroyuki@gmail.com; mhocko@suse.cz;
> > hannes@cmpxchg.org; yinghan@google.com
> > Subject: RE: [PATCH 1/1] mm: Export a function to read vm_committed_as
> >
> > On Mon, 12 Nov 2012, Dan Magenheimer wrote:
> >
> > > > > Why?  Is xen using it for a different inference?
> > > >
> > > > I think it is good to separate these patches. Dan (copied here) wrote the code
> > for the
> > > > Xen self balloon driver. If it is ok with him I can submit the patch for Xen as
> > well.
> > >
> > > Hi KY --
> > >
> > > If I understand correctly, this would be only a cosmetic (function renaming)
> > change
> > > to the Xen selfballooning code.  If so, then I will be happy to Ack when I
> > > see the patch.  However, Konrad (konrad.wilk@oracle.com) is the maintainer
> > > for all Xen code so you should ask him... and (from previous painful experience)
> > > it can be difficult to sync even very simple interdependent changes going
> > through
> > > different maintainers without breaking linux-next.  So I can't offer any
> > > help with that process, only commiseration. :-(
> > >
> >
> > I think this should be done in the same patch as the function getting
> > introduced with a cc to Konrad and routed through -mm; even better,
> > perhaps he'll have some useful comments for how this is used for xen that
> > can be included for context.
> >
> Ok; I will send out a single patch. I am hoping this can be applied soon as Hyper-V balloon
> driver is queued behind this.
> 
> Regards,
> K. Y

David --

Having caught up on the thread now, I'm a bit confused about your
requirement for KY to patch the Xen selfballooning code.

The data item we are talking about here, committed_as, is defined
by a kernel<->userland ABI, visible to userland via /proc/meminfo.
The Xen selfballoon driver accesses it within the kernel as
a built-in; this driver could potentially be loaded as a
module but currently cannot.

KY is simply asking that the data item be exported so that he can
use it from a new module.  No change to the Xen selfballoon driver
is necessary right now and requiring one only gets in the way of the
patch.  At some future time, the Xen selfballoon driver can, at its
leisure, switch to use the new exported function but need not
unless/until it is capable of being loaded as a module.

And, IIUC, you are asking that KY's proposed new function include a
comment about how it is used by Xen?  How many kernel globals/functions
document at their point of declaration the intent of all the in-kernel
users that use/call them?  That seems a bit unreasonable.  There is a
very long explanatory comment at the beginning of the Xen
selfballoon driver code already.

So I will ack KY's patch (I see it was just sent) but will leave
it up to Konrad and GregKH and Andrew to decide whether to
include the fragment patching the Xen selfballoon driver.

Dan

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH 1/1] mm: Export a function to read vm_committed_as
  2012-11-13 15:41               ` Dan Magenheimer
@ 2012-11-13 21:04                 ` David Rientjes
  0 siblings, 0 replies; 12+ messages in thread
From: David Rientjes @ 2012-11-13 21:04 UTC (permalink / raw)
  To: Dan Magenheimer
  Cc: KY Srinivasan, Konrad Wilk, gregkh, linux-kernel, devel, olaf,
	apw, andi, akpm, linux-mm, kamezawa.hiroyuki, mhocko, hannes,
	yinghan

On Tue, 13 Nov 2012, Dan Magenheimer wrote:

> KY is simply asking that the data item be exported so that he can
> use it from a new module.  No change to the Xen selfballoon driver
> is necessary right now and requiring one only gets in the way of the
> patch.  At some future time, the Xen selfballoon driver can, at its
> leisure, switch to use the new exported function but need not
> unless/until it is capable of being loaded as a module.
> 

That's obvious.

> And, IIUC, you are asking that KY's proposed new function include a
> comment about how it is used by Xen?  How many kernel globals/functions
> document at their point of declaration the intent of all the in-kernel
> users that use/call them?  That seems a bit unreasonable.  There is a
> very long explanatory comment at the beginning of the Xen
> selfballoon driver code already.
> 

Sorry, I don't think it's unreasonable at all: if you're going to be using 
a symbol which was always assumed to be internal to the VM for other 
purposes and then that usage becomes convoluted with additional usage like 
in KY's patch, then no VM hacker will ever know what a change to that 
symbol means outside of the VM.  There's been a lot of confusion about why 
this heuristic is needed outside the VM and whether the symbol is actually 
the correct choice, so verbosity as to the intent of what it is to 
represent is helpful for a maintainable kernel.

Presumably xen is hijacking that symbol for a similar purpose to KY's 
purpose, but perhaps I was too optimistic that others would help to 
solidify the semantics in which it is being used and describe it 
concisely.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2012-11-13 21:05 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-11  2:25 [PATCH 1/1] mm: Export a function to read vm_committed_as K. Y. Srinivasan
2012-11-11  2:35 ` David Rientjes
2012-11-11  9:24   ` KY Srinivasan
2012-11-12 21:53     ` David Rientjes
2012-11-12 22:58       ` KY Srinivasan
2012-11-12 23:32         ` Dan Magenheimer
2012-11-12 23:49           ` David Rientjes
2012-11-13  5:24             ` KY Srinivasan
2012-11-13 15:41               ` Dan Magenheimer
2012-11-13 21:04                 ` David Rientjes
2012-11-13  5:16           ` KY Srinivasan
2012-11-12 21:36   ` KY Srinivasan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).