Re: [PATCH 3/3] [PATCH] hugetlb: Enforce quotas during reservation for shared mappings

[Dave Hansen <haveblue@us.ibm.com>]

On Wed, 2007-10-24 at 06:24 -0700, Adam Litke wrote:
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index eaade8c..5fc075e 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -769,6 +769,7 @@ static int hugetlb_no_page(struct mm_struct *mm, struct vm_area_struct *vma,
>  	struct page *page;
>  	struct address_space *mapping;
>  	pte_t new_pte;
> +	int shared_page = vma->vm_flags & VM_SHARED;
> 
>  	mapping = vma->vm_file->f_mapping;
>  	idx = ((address - vma->vm_start) >> HPAGE_SHIFT)
> @@ -784,23 +785,24 @@ retry:
>  		size = i_size_read(mapping->host) >> HPAGE_SHIFT;
>  		if (idx >= size)
>  			goto out;
> -		if (hugetlb_get_quota(mapping, 1))
> +		/* Shared pages are quota-accounted at reservation/mmap time */
> +		if (!shared_page && hugetlb_get_quota(mapping, 1))
>  			goto out;
>  		page = alloc_huge_page(vma, address);

Since alloc_huge_page() gets the VMA it could, in theory, be doing the
accounting.  The other user, hugetlb_cow(), seems to have a similar code
path.  But, it doesn't have to worry about shared_page, right?  We can
only have COWs on MAP_PRIVATE.

I'm just trying to find ways to future-proof the quotas since they
already got screwed up once.  The fewer call sites we have for them, the
fewer places they can get screwed up. :)

>  		if (!page) {
> -			hugetlb_put_quota(mapping, 1);
> +			if (!shared_page)
> +				hugetlb_put_quota(mapping, 1);
>  			ret = VM_FAULT_OOM;
>  			goto out;
>  		}
>  		clear_huge_page(page, address);
> 
> -		if (vma->vm_flags & VM_SHARED) {
> +		if (shared_page) {
>  			int err;
> 
>  			err = add_to_page_cache(page, mapping, idx, GFP_KERNEL);
>  			if (err) {
>  				put_page(page);
> -				hugetlb_put_quota(mapping, 1);
>  				if (err == -EEXIST)
>  					goto retry;
>  				goto out;

To where was this quota put moved?  Is it because we're in a fault path
here, and shared pages don't modify quotas during faults, only at
mmap/truncate() time now?

>  backout:
>  	spin_unlock(&mm->page_table_lock);
> -	hugetlb_put_quota(mapping, 1);
> +	if (!shared_page)
> +		hugetlb_put_quota(mapping, 1);
>  	unlock_page(page);
>  	put_page(page);
>  	goto out;
> @@ -1144,6 +1147,8 @@ int hugetlb_reserve_pages(struct inode *inode, long from, long to)
>  	if (chg < 0)
>  		return chg;
> 
> +	if (hugetlb_get_quota(inode->i_mapping, chg))
> +		return -ENOSPC;
>  	ret = hugetlb_acct_memory(chg);
>  	if (ret < 0)
>  		return ret;
> @@ -1154,5 +1159,6 @@ int hugetlb_reserve_pages(struct inode *inode, long from, long to)
>  void hugetlb_unreserve_pages(struct inode *inode, long offset, long freed)
>  {
>  	long chg = region_truncate(&inode->i_mapping->private_list, offset);
> -	hugetlb_acct_memory(freed - chg);
> +	hugetlb_put_quota(inode->i_mapping, (chg - freed));
> +	hugetlb_acct_memory(-(chg - freed));
>  }

Would it be any easier to just do all of the quota operations in
_either_ truncate_hugepages() or in here?  Could you skip the quota
operation in truncate_hugepages()'s while() loop, and just put the quota
for the entire region in hugetlb_unreserve_pages()?

void hugetlb_unreserve_pages(struct inode *inode, long offset, long already_freed)
{
 	long total_truncated = region_truncate(&inode->i_mapping->private_list, offset);
	long newly_freed = total_truncated - already_freed;
	hugetlb_put_quota(inode->i_mapping, newly_freed);
	hugetlb_acct_memory(-newly_freed);
}

I do see several hugetlb_put_quota()/hugetlb_acct_memory() pairs next to
each other.  Do they deserve to be lumped together in one helper?

-- Dave

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