Messages in this thread Patch in this message | | | Date | Wed, 11 Feb 2009 11:15:22 +0000 | From | Mel Gorman <> | Subject | Re: [PATCH] Do not account for the address space used by hugetlbfs using VM_ACCOUNT V2 (Was Linus 2.6.29-rc4) |
| |
On Tue, Feb 10, 2009 at 03:45:49PM -0800, Andrew Morton wrote: > On Tue, 10 Feb 2009 14:02:27 +0000 > Mel Gorman <mel@csn.ul.ie> wrote: > > > On Sun, Feb 08, 2009 at 01:01:04PM -0800, Linus Torvalds wrote: > > > > > > Another week (and a half), another -rc. > > > > > > Arch updates (sparc, blackfin), driver updates (dvb, mmc, ide), ACPI > > > updates, FS updates (ubifs, btrfs), you name it. It's all there. > > > > > > But more importantly, people really have been working on regressions, and > > > hopefully this closes a nice set of the top one, and hopefully without > > > introducing too many new ones. > > > > > > > Hugepages are currently busted due to commit > > fc8744adc870a8d4366908221508bb113d8b72ee and the problem is in > > 2.6.29-rc4. There was a bit of a discussion but it didn't get very far and > > then I went offline for the weekend. Here is a revised patch that tries to > > bring hugetlbfs more in line with what the core VM is doing by dealing with > > VM_ACCOUNT and VM_NORESERVE. > > > > ============= > > Do not account for the address space used by hugetlbfs using VM_ACCOUNT > > > > When overcommit is disabled, the core VM accounts for pages used by anonymous > > shared, private mappings and special mappings. It keeps track of VMAs that > > should be accounted for with VM_ACCOUNT and VMAs that never had a reserve > > with VM_NORESERVE. > > > > Overcommit for hugetlbfs is much riskier than overcommit for base pages > > due to contiguity requirements. It avoids overcommiting on both shared and > > private mappings using reservation counters that are checked and updated > > during mmap(). This ensures (within limits) that hugepages exist in the > > future when faults occurs or it is too easy to applications to be SIGKILLed. > > > > As hugetlbfs makes its own reservations of a different unit to the base page > > size, VM_ACCOUNT should never be set. Even if the units were correct, we would > > double account for the usage in the core VM and hugetlbfs. VM_NORESERVE may > > be set because an application can request no reserves be made for hugetlbfs > > at the risk of getting killed later. > > > > With commit fc8744adc870a8d4366908221508bb113d8b72ee, VM_NORESERVE and > > VM_ACCOUNT are getting unconditionally set for hugetlbfs-backed mappings. This > > breaks the accounting for both the core VM and hugetlbfs, can trigger an > > OOM storm when hugepage pools are too small lockups and corrupted counters > > otherwise are used. This patch brings hugetlbfs more in line with how the > > core VM treats VM_NORESERVE but prevents VM_ACCOUNT being set. > > > > ... > > > > --- a/fs/hugetlbfs/inode.c > > +++ b/fs/hugetlbfs/inode.c > > @@ -108,7 +108,8 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma) > > > > if (hugetlb_reserve_pages(inode, > > vma->vm_pgoff >> huge_page_order(h), > > - len >> huge_page_shift(h), vma)) > > + len >> huge_page_shift(h), vma, > > + vma->vm_flags)) > > goto out; > > > > ret = 0; > > @@ -947,7 +948,7 @@ static int can_do_hugetlb_shm(void) > > can_do_mlock()); > > } > > > > -struct file *hugetlb_file_setup(const char *name, size_t size) > > +struct file *hugetlb_file_setup(const char *name, size_t size, int acctflag) > > This superundocumented acctflag looks like a poorly-named boolean. But > it is fact a composition of VM_foo bits. >
It's matching naming at the call site. The call to shmem_file_setup() looks like
file = shmem_file_setup(name, size, acctflag);
so, the call to hugetlb_file_setup looks like
hugetlb_file_setup(name, size, acctflag);
and the name in the signature matches. shmem doesn't match like this, it just called its parameter "flags" and uses a different unit for size. The different in unit is bad in itself and I need to check that out.
> Would it not be clearer to name it `vm_flags'?
It's not all the flags either so vm_flags is confusing for other reasons. At least, at the time of writing the flags were meant to be VM_ACCOUNT and VM_NORESERVE but in reality, only VM_NORESERVE appears to be sent in but maybe that will change.
That said..... newseg() passes VM_NORESERVE as a flag to shmem_file_setup(). It in turn checks for VM_ACCOUNT in shmem_acct_size(), not VM_NORESERVE. Right now we may have an issue there too. Later it uses shmem_acct_block() but at that point VM_ACCOUNT has been set properly. shmem also has a VM_ACCT macro that has very little to do with VM_ACCOUNT. Naming rocks.
I'll look at a patch that makes the signatures of *_file_setup() match. I'll then check if it should be vm_acct_flags (both VM_ACCOUNT and VM_NORESERVE possible) or vm_resv_flag (VM_NORESERVE only).
> > > { > > int error = -ENOMEM; > > struct file *file; > > @@ -981,7 +982,8 @@ struct file *hugetlb_file_setup(const char *name, size_t size) > > > > error = -ENOMEM; > > if (hugetlb_reserve_pages(inode, 0, > > - size >> huge_page_shift(hstate_inode(inode)), NULL)) > > + size >> huge_page_shift(hstate_inode(inode)), NULL, > > + acctflag)) > > goto out_inode; > > > > d_instantiate(dentry, inode); > > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h > > index f1d2fba..af09660 100644 > > --- a/include/linux/hugetlb.h > > +++ b/include/linux/hugetlb.h > > @@ -33,7 +33,8 @@ unsigned long hugetlb_total_pages(void); > > int hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma, > > unsigned long address, int write_access); > > int hugetlb_reserve_pages(struct inode *inode, long from, long to, > > - struct vm_area_struct *vma); > > + struct vm_area_struct *vma, > > + int acctflags); > > here it went plural. >
Because I was thinking of VM_ACCOUNT and VM_NORESERVE, hence plural.
> > void hugetlb_unreserve_pages(struct inode *inode, long offset, long freed); > > > > extern unsigned long hugepages_treat_as_movable; > > @@ -138,7 +139,7 @@ static inline struct hugetlbfs_sb_info *HUGETLBFS_SB(struct super_block *sb) > > > > extern const struct file_operations hugetlbfs_file_operations; > > extern struct vm_operations_struct hugetlb_vm_ops; > > -struct file *hugetlb_file_setup(const char *name, size_t); > > +struct file *hugetlb_file_setup(const char *name, size_t, int); > > Here it is omitted altogether. We now have one named parameter and two > secret ones. >
Dirt, that was matching what was happening for size_t but it's fugly. I should have fixed size_t while I was there rather than making it look worse.
> Also, the patch forgot to update this: > > #define hugetlb_file_setup(name,size) ERR_PTR(-ENOSYS) > > Which I assume breaks CONFIG_HUGETLBFS=n. >
Yeah, it broke and has been fixed since.
> > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index e8ddc98..3235615 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -1129,8 +1129,7 @@ extern unsigned long do_mmap_pgoff(struct file *file, unsigned long addr, > > unsigned long flag, unsigned long pgoff); > > extern unsigned long mmap_region(struct file *file, unsigned long addr, > > unsigned long len, unsigned long flags, > > - unsigned int vm_flags, unsigned long pgoff, > > - int accountable); > > + unsigned int vm_flags, unsigned long pgoff); > > And there is a vm_flags which has type `unsigned int'. > > Your newly-added should-have-been-called-vm_flags has type `int'. > > The vm_flagses in `struct vm_region' and `struct vm_area_struct' have type > `unsigned long'. >
Yes, they all should have been unsigned long.
> It'd be nice to get these consistent. We only have two bits left in > the vm_flags namespace so arguably we could add a vm_flags_t while > fixing this up, with a view to makeing it u64 later. Maybe. >
I'll look at a vm_flags_t that works similar in principal to gfp_t and making the signatures match up.
-- Mel Gorman Part-time Phd Student Linux Technology Center University of Limerick IBM Dublin Software Lab
| |