Re: [patch 13/18] hugetlb: support boot allocate different sizes

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

On 25.04.2008 [20:50:39 +0200], Andi Kleen wrote:
> Nishanth Aravamudan wrote:
> 
> > When would this be the case (the list is already init'd)?
> 
> It can happen inside the series before all the final checks are in
> with multiple arguments. In theory it could be removed at the end,
> but then it doesn't hurt.

Ok, I guess that indicates to me an ordering issue, but perhaps it is
unavoidable.

> >>  	for (i = 0; i < h->max_huge_pages; ++i) {
> >>  		if (h->order >= MAX_ORDER) {
> >> @@ -594,7 +597,7 @@ static void __init hugetlb_init_hstate(s
> >>  		} else if (!alloc_fresh_huge_page(h))
> >>  			break;
> >>  	}
> >> -	h->max_huge_pages = h->free_huge_pages = h->nr_huge_pages = i;
> >> +	h->max_huge_pages = i;
> > 
> > Why don't we need to set these other values anymore?
> 
> Because the low level functions handle them already (as a simple grep
> would have told you)

[12:36:40]nacc@arkanoid:~/linux/views/linux-2.6-work$ rgrep free_huge_pages *
arch/x86/ia32/ia32entry.S:	.quad quiet_ni_syscall 	/* free_huge_pages */
include/linux/hugetlb.h:	unsigned long free_huge_pages;
include/linux/hugetlb.h:	unsigned int free_huge_pages_node[MAX_NUMNODES];
mm/hugetlb.c: * Protects updates to hugepage_freelists, nr_huge_pages, and free_huge_pages
mm/hugetlb.c:	h->free_huge_pages++;
mm/hugetlb.c:	h->free_huge_pages_node[nid]++;
mm/hugetlb.c:			h->free_huge_pages--;
mm/hugetlb.c:			h->free_huge_pages_node[nid]--;
mm/hugetlb.c:			h->free_huge_pages--;
mm/hugetlb.c:			h->free_huge_pages_node[nid]--;
mm/hugetlb.c:	needed = (h->resv_huge_pages + delta) - h->free_huge_pages;
mm/hugetlb.c:	 * because either resv_huge_pages or free_huge_pages may have changed.
mm/hugetlb.c:			(h->free_huge_pages + allocated);
mm/hugetlb.c:			h->free_huge_pages--;
mm/hugetlb.c:			h->free_huge_pages_node[nid]--;
mm/hugetlb.c:	if (h->free_huge_pages > h->resv_huge_pages)
mm/hugetlb.c:			h->free_huge_pages);
mm/hugetlb.c:			h->free_huge_pages--;
mm/hugetlb.c:			h->free_huge_pages_node[page_to_nid(page)]--;
mm/hugetlb.c:	min_count = h->resv_huge_pages + h->nr_huge_pages - h->free_huge_pages;
mm/hugetlb.c:	n += dump_field(buf + n, offsetof(struct hstate, free_huge_pages));
mm/hugetlb.c:						free_huge_pages_node[nid]));
mm/hugetlb.c:		if (delta > cpuset_mems_nr(h->free_huge_pages_node)) {

Hrm, I don't see a single assignment to free_huge_pages there.

grep'ing through the patches from Nick's series, I don't see any there
either (which would indicate I misapplied a patch).

Andi, I'm doing a review of the patches because it is needed (I haven't
seen a comprehensive set of responses yet) and because my work depends
on these patches doing the right thing. I would appreciate it if you
could give me slightly more useful responses -- for instance, the aside
comment in your reply was entirely unnecessary, as grep didn't shed any
insight *and* I am looking at the code in question as I work.  Instead
please try to help me understand the patches.

If I didn't hope differently, I'd believe you don't want me to review
the patches at all.

> > I think it's use should be restricted to the sysctl as much as
> > possible (and the sysctl's should be updated to only do work if
> > write is set).  Does that seem sane to you?
> 
> Fundamental rule of programming: Information should be only kept at a
> single place if possible.

Ok ... I'm tired of reading one-sentence responses that don't answer my
questions and come across as insulting. The current patches duplicate
max_huge_pages *already*. My point was reduction. So if your response
was meant to be

	"Yes, that does seem sane."

then that is all you needed to write. If it was

	"No, that does not seem sane."

that would have been equally fine. But what you wrote has neither a
"yes" nor a "no" in it.

Thanks,
Nish

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center

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