Re: [PATCH 4/5] Add sysctl for default hstate nodes_allowed.

[Lee Schermerhorn <Lee.Schermerhorn@hp.com>]

On Wed, 2009-06-17 at 14:41 +0100, Mel Gorman wrote:
> On Tue, Jun 16, 2009 at 09:53:08AM -0400, Lee Schermerhorn wrote:
> > [PATCH 4/5] add sysctl for default hstate nodes_allowed.
> > 
> > Against:  17may09 mmotm
> > 
> > This patch adds a sysctl -- /proc/sys/vm/hugepages_nodes_allowed --
> > to set/query the default hstate's nodes_allowed.  I don't know
> > that this patch is required, given that we have the per hstate
> > controls in /sys/kernel/mm/hugepages/*. However, we've added sysctls
> > for other recent hugepages controls, like nr_overcommit_hugepages,
> > so I've followed that convention.
> > 
> 
> Yeah, it's somewhat expected that what is in /proc/sys/vm is information
> on the default hugepage size.
> 
> > Factor the formatting of the nodes_allowed mask out of nodes_allowed_show()
> > for use by both that function and the hugetlb_nodes_allowed_handler().
> > 
> > Signed-off-by: Lee Schermerhorn <lee.schermerhorn@hp>
> > 
> >  include/linux/hugetlb.h |    1 +
> >  kernel/sysctl.c         |    8 ++++++++
> >  mm/hugetlb.c            |   43 ++++++++++++++++++++++++++++++++++++++-----
> >  3 files changed, 47 insertions(+), 5 deletions(-)
> > 
> > Index: linux-2.6.30-rc8-mmotm-090603-1633/include/linux/hugetlb.h
> > ===================================================================
> > --- linux-2.6.30-rc8-mmotm-090603-1633.orig/include/linux/hugetlb.h	2009-06-04 12:59:32.000000000 -0400
> > +++ linux-2.6.30-rc8-mmotm-090603-1633/include/linux/hugetlb.h	2009-06-04 12:59:35.000000000 -0400
> > @@ -22,6 +22,7 @@ void reset_vma_resv_huge_pages(struct vm
> >  int hugetlb_sysctl_handler(struct ctl_table *, int, struct file *, void __user *, size_t *, loff_t *);
> >  int hugetlb_overcommit_handler(struct ctl_table *, int, struct file *, void __user *, size_t *, loff_t *);
> >  int hugetlb_treat_movable_handler(struct ctl_table *, int, struct file *, void __user *, size_t *, loff_t *);
> > +int hugetlb_nodes_allowed_handler(struct ctl_table *, int, struct file *, void __user *, size_t *, loff_t *);
> >  int copy_hugetlb_page_range(struct mm_struct *, struct mm_struct *, struct vm_area_struct *);
> >  int follow_hugetlb_page(struct mm_struct *, struct vm_area_struct *, struct page **, struct vm_area_struct **, unsigned long *, int *, int, int);
> >  void unmap_hugepage_range(struct vm_area_struct *,
> > Index: linux-2.6.30-rc8-mmotm-090603-1633/kernel/sysctl.c
> > ===================================================================
> > --- linux-2.6.30-rc8-mmotm-090603-1633.orig/kernel/sysctl.c	2009-06-04 12:59:26.000000000 -0400
> > +++ linux-2.6.30-rc8-mmotm-090603-1633/kernel/sysctl.c	2009-06-04 12:59:35.000000000 -0400
> > @@ -1108,6 +1108,14 @@ static struct ctl_table vm_table[] = {
> >  		.extra1		= (void *)&hugetlb_zero,
> >  		.extra2		= (void *)&hugetlb_infinity,
> >  	},
> > +	{
> > +		.ctl_name	= CTL_UNNUMBERED,
> > +		.procname	= "hugepages_nodes_allowed",
> > +		.data		= NULL,
> > +		.maxlen		= sizeof(unsigned long),
> > +		.mode		= 0644,
> > +		.proc_handler	= &hugetlb_nodes_allowed_handler,
> > +	},
> >  #endif
> >  	{
> >  		.ctl_name	= VM_LOWMEM_RESERVE_RATIO,
> > Index: linux-2.6.30-rc8-mmotm-090603-1633/mm/hugetlb.c
> > ===================================================================
> > --- linux-2.6.30-rc8-mmotm-090603-1633.orig/mm/hugetlb.c	2009-06-04 12:59:33.000000000 -0400
> > +++ linux-2.6.30-rc8-mmotm-090603-1633/mm/hugetlb.c	2009-06-04 12:59:35.000000000 -0400
> > @@ -1354,19 +1354,27 @@ static ssize_t nr_overcommit_hugepages_s
> >  }
> >  HSTATE_ATTR(nr_overcommit_hugepages);
> >  
> > -static ssize_t nodes_allowed_show(struct kobject *kobj,
> > -					struct kobj_attribute *attr, char *buf)
> > +static int format_hstate_nodes_allowed(struct hstate *h, char *buf,
> > +					size_t buflen)
> >  {
> > -	struct hstate *h = kobj_to_hstate(kobj);
> >  	int len = 3;
> >  
> >  	if (h->nodes_allowed == &node_online_map)
> >  		strcpy(buf, "all");
> >  	else
> > -		len = nodelist_scnprintf(buf, PAGE_SIZE,
> > +		len = nodelist_scnprintf(buf, buflen,
> >  					*h->nodes_allowed);
> > +	return len;
> > +
> > +}
> 
> This looks like unnecessary churn and could have been done in the earlier
> patch introducing nodes_allowed_show()

Yes.  I couldn't see creating the separate function if we weren't going
to have the sysctl also formatting the nodes_allowed.  If all agree that
we want the sysctl, I can fold this patch in with patch 2/5.

> 
> > +
> > +static ssize_t nodes_allowed_show(struct kobject *kobj,
> > +					struct kobj_attribute *attr, char *buf)
> > +{
> > +	struct hstate *h = kobj_to_hstate(kobj);
> > +	int len =  format_hstate_nodes_allowed(h, buf, PAGE_SIZE);
> >  
> > -	if (len)
> > +	if (len && (len +1) < PAGE_SIZE)
> >  		buf[len++] = '\n';
> >  
> >  	return len;
> > @@ -1684,6 +1692,31 @@ int hugetlb_overcommit_handler(struct ct
> >  	return 0;
> >  }
> >  
> > +#define NODES_ALLOWED_MAX 64
> > +int hugetlb_nodes_allowed_handler(struct ctl_table *table, int write,
> > +			struct file *file, void __user *buffer,
> > +			size_t *length, loff_t *ppos)
> > +{
> > +	struct hstate *h = &default_hstate;
> > +	int ret = 0;
> > +
> > +	if (write) {
> > +		(void)set_hstate_nodes_allowed(h, buffer, 1);
> > +	} else {
> > +		char buf[NODES_ALLOWED_MAX];
> > +		struct ctl_table tbl = {
> > +			.data = buf,
> > +			.maxlen = NODES_ALLOWED_MAX,
> > +		};
> > +		int len =  format_hstate_nodes_allowed(h, buf, sizeof(buf));
> > +
> > +		if (len)
> > +			ret = proc_dostring(&tbl, write, file, buffer,
> > +						 length, ppos);
> > +	}
> > +	return ret;
> > +}
> > +
> >  #endif /* CONFIG_SYSCTL */
> >  
> >  void hugetlb_report_meminfo(struct seq_file *m)
> > 
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>