Re: [patch 07/18] hugetlbfs: per mount hstates

[Nishanth Aravamudan <nacc@us.ibm.com>]

On 28.04.2008 [13:20:49 -0500], Adam Litke wrote:
> On Fri, 2008-04-25 at 15:39 -0700, Nishanth Aravamudan wrote: 
> > On 25.04.2008 [13:36:39 -0700], Nishanth Aravamudan wrote:
> > > On 25.04.2008 [11:09:33 -0700], Nishanth Aravamudan wrote:
> > > > On 23.04.2008 [11:53:09 +1000], npiggin@suse.de wrote:
> > > > > Add support to have individual hstates for each hugetlbfs mount
> > > > > 
> > > > > - Add a new pagesize= option to the hugetlbfs mount that allows setting
> > > > > the page size
> > > > > - Set up pointers to a suitable hstate for the set page size option
> > > > > to the super block and the inode and the vma.
> > > > > - Change the hstate accessors to use this information
> > > > > - Add code to the hstate init function to set parsed_hstate for command
> > > > > line processing
> > > > > - Handle duplicated hstate registrations to the make command line user proof
> > > > > 
> > > > > [np: take hstate out of hugetlbfs inode and vma->vm_private_data]
> > > > > 
> > > > > Signed-off-by: Andi Kleen <ak@suse.de>
> > > > > Signed-off-by: Nick Piggin <npiggin@suse.de>
> > > > > ---
> > > > >  fs/hugetlbfs/inode.c    |   48 ++++++++++++++++++++++++++++++++++++++----------
> > > > >  include/linux/hugetlb.h |   14 +++++++++-----
> > > > >  mm/hugetlb.c            |   16 +++-------------
> > > > >  mm/memory.c             |   18 ++++++++++++++++--
> > > > >  4 files changed, 66 insertions(+), 30 deletions(-)
> > > > > 
> > > > > Index: linux-2.6/include/linux/hugetlb.h
> > > > > ===================================================================
> > > > 
> > > > <snip>
> > > > 
> > > > > @@ -226,19 +228,21 @@ extern struct hstate hstates[HUGE_MAX_HS
> > > > > 
> > > > >  #define global_hstate (hstates[0])
> > > > > 
> > > > > -static inline struct hstate *hstate_vma(struct vm_area_struct *vma)
> > > > > +static inline struct hstate *hstate_inode(struct inode *i)
> > > > >  {
> > > > > -	return &global_hstate;
> > > > > +	struct hugetlbfs_sb_info *hsb;
> > > > > +	hsb = HUGETLBFS_SB(i->i_sb);
> > > > > +	return hsb->hstate;
> > > > >  }
> > > > > 
> > > > >  static inline struct hstate *hstate_file(struct file *f)
> > > > >  {
> > > > > -	return &global_hstate;
> > > > > +	return hstate_inode(f->f_dentry->d_inode);
> > > > >  }
> > > > > 
> > > > > -static inline struct hstate *hstate_inode(struct inode *i)
> > > > > +static inline struct hstate *hstate_vma(struct vm_area_struct *vma)
> > > > >  {
> > > > > -	return &global_hstate;
> > > > > +	return hstate_file(vma->vm_file);
> > > > 
> > > > Odd, diff seems to think you've moved these two functions around
> > > > (hstate_{vma,inode})...
> > > 
> > > Err, duh, which of course you have to because of the definitions :)
> > > 
> > > However, doesn't this now make a core hugetlb functionality (which
> > > really should only depend on CONFIG_HUGETLB_PAGE) depend on HUGETLBFS
> > > being set to have access to HUGETLBFS_SB()? That seems to go in the
> > > opposite direction from where we want to... Perhaps some of these
> > > functions should be in the CONFIG_HUGETLBFS section of hugetlb.h?
> > 
> > Even if you don't move anything as I had originally suggested, I think
> > you need to express the CONFIG_ dependencies more clearly (that now
> > HUGETLB_PAGE depends on HUGETLBFS, afaict).
> > 
> > Urgh, there's actually other similar issue(s) in this file already...
> > 
> > if CONFIG_HUGETLBFS, is_file_hugepages() is defined and calls
> > is_file_shm_hugepages(), but that is defined in shm.h, which is only
> > included if CONFIG_HUGETLB_PAGE... Adam, that seems buggy? Is this just
> > further evidence that our current separation of the two options is
> > bull-honky?
> 
> Yeah.  I'd say there is little reason to separate them anymore.  I am
> not an expert on the history here, but I suspect the original reason for
> separating CONFIG_HUGETLBFS and CONFIG_HUGETLB_PAGE was a lack of
> psychic abilities.  Hugetlbfs is ubiquitous now and there is no other
> valid way to use huge pages.  Even SHM_HUGETLB shared memory segments
> use hugetlbfs.

Yeah, I was thinking it might make sense to merge them now and then
separate them back out later, if we do add any other interfaces to
hugepages.

> One thing you should check is which config options are required for
> the hugetlb kernel mappings.  Otherwise, I think we are in the clear
> to merge them.

Yep, thanks,
Nish

--
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>