[openib-general] [PATCH] huge pages support

[Eli Cohen]

On Thu, 2006-08-17 at 09:06 -0700, Roland Dreier wrote:
> My first reaction on reading this is that it looks like we need some
> more help from mm/ to make this cleaner.
> 
>  > +			for (j = 0; j < umem->page_size / PAGE_SIZE; ++j)
>  > +				put_page(chunk->page_list[i].page);
> 
> This just looks really bad... you're assuming that calling put_page()
> on a different page than the one get_user_pages() gave you is OK.
> Does that actually work?  (I don't remember the details of how hugetlb
> pages work)
A huge page is a compound page that is comprised of a number of pages
and the first one represents all the others so it is safe to do this.

> 
>  > +	if (hpage == -1)
>  > +		hpage = is_vm_hugetlb_page(vma);
>  > +	else
>  > +		if (hpage != is_vm_hugetlb_page(vma))
>  > +			return -ENOMEM;
> 
> Users aren't allowed to register regions that span both hugetlb and
> non-hugetlb pages??  Why not?

It is not that easy anyway to obtain huge pages and even more unlikely
that a user will want to register a region with different page sizes so
I just don't allow that. But I think these cases can be allowed by using
PAGE_SIZE registration.

> 
>  > +	if (!(PAGE_SIZE / (sizeof (struct page *) << shift_diff))) {
> 
> shift_diff is used here...
> 
>  > +		ret = -ENOMEM;
>  > +		goto exit_up;
>  > +	}
>  > +
>  > +	page_size = 1 << page_shift;
>  > +	page_mask = ~(page_size - 1);
>  > +	shift_diff = page_shift - PAGE_SHIFT;
> 
> and set down here... don't you get a warning about "is used uninitialized"?


Oops. I didn't see any warning. This if should moved a few lines down.
> 
> Also, if someone tries to register a hugetlb region and a regular page
> isn't big enough to hold all the pointers, then they just lose??  This
> would be a real problem if/when GB pages are implemented on amd64 --
> there would be no way to register such a region.
> 
> 
> Basically this patch seems to be trying to undo what follow_hugetlb_pages()
> does to create normal pages for the pieces of a hugetlb page.  It
> seems like it would be better to have a new function like get_user_pages()
> that returned its result in a struct scatterlist so that it could give
> back huge pages directly.
> 

I agree about that.