[PATCH] mm: fix confusing __GFP_REPEAT related comments

[PATCH] mm: fix confusing __GFP_REPEAT related comments

[Nishanth Aravamudan]

The definition and use of __GFP_REPEAT, __GFP_NOFAIL and __GFP_NORETRY
in the core VM have somewhat differing comments as to their actual
semantics. Annoyingly, the flags definition has inline and header
comments, which might be interpreted as not being equivalent. Just add
references to the header comments in the inline ones so they don't go
out of sync in the future. In their use in __alloc_pages() clarify that
the current implementation treats low-order allocations and __GFP_REPEAT
allocations as distinct cases, albeit currently with the same result.

To clarify, the flags' semantics are:

__GFP_NORETRY means try no harder than one run through __alloc_pages

__GFP_REPEAT means __GFP_NOFAIL

__GFP_NOFAIL means repeat forever

order <= PAGE_ALLOC_COSTLY_ORDER means __GFP_NOFAIL

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>

---

Andrew, I sent this patch in about a week ago, hoping for some responses
to the following questions, but didn't get any :/ Do you think you can
pull this into the next -mm? I am ok with waiting and reposting after
2.6.24, so that you can focus on stabilization, if you'd prefer.

>From the original e-mail:

I'm not sure if the clarified semantics are really desired, and am
hoping this patch clarifies that for me:

In looking at the callers using __GFP_REPEAT, not all handle failure --
should they be using __NOFAIL?

Should __GFP_REPEAT eventually be able to fail? That is, maybe it makes
sense to somehow monitor the success of reclaim? If we are able to free
an equivalent order of pages as requested but still can't allocate the
pages requested, then fail? This would at least allow *some* behavior
between NORETRY and NOFAIL... This would get closer to what I was trying
to achieve in my hugetlb-specific patch
(http://marc.info/?l=linux-mm&m=119515798913982&w=2) without such
specific changes.

Should __GFP_NORETRY mean no retrying anywhere? The default path through
the allocator, before we even check if we should or should not retry
appears to retry in some sense of the word. This is of less concern to
me than the other two, though.

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 7e93a9a..e4ed545 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -40,9 +40,9 @@ struct vm_area_struct;
 #define __GFP_FS	((__force gfp_t)0x80u)	/* Can call down to low-level FS? */
 #define __GFP_COLD	((__force gfp_t)0x100u)	/* Cache-cold page required */
 #define __GFP_NOWARN	((__force gfp_t)0x200u)	/* Suppress page allocation failure warning */
-#define __GFP_REPEAT	((__force gfp_t)0x400u)	/* Retry the allocation.  Might fail */
-#define __GFP_NOFAIL	((__force gfp_t)0x800u)	/* Retry for ever.  Cannot fail */
-#define __GFP_NORETRY	((__force gfp_t)0x1000u)/* Do not retry.  Might fail */
+#define __GFP_REPEAT	((__force gfp_t)0x400u)	/* See above */
+#define __GFP_NOFAIL	((__force gfp_t)0x800u)	/* See above */
+#define __GFP_NORETRY	((__force gfp_t)0x1000u)/* See above */
 #define __GFP_COMP	((__force gfp_t)0x4000u)/* Add compound page metadata */
 #define __GFP_ZERO	((__force gfp_t)0x8000u)/* Return zeroed page on success */
 #define __GFP_NOMEMALLOC ((__force gfp_t)0x10000u) /* Don't use emergency reserves */
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 4ffed1c..93c7a9d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1599,8 +1599,9 @@ nofail_alloc:
 	 * Don't let big-order allocations loop unless the caller explicitly
 	 * requests that.  Wait for some write requests to complete then retry.
 	 *
-	 * In this implementation, __GFP_REPEAT means __GFP_NOFAIL for order
-	 * <= 3, but that may not be true in other implementations.
+	 * In this implementation, either order <= PAGE_ALLOC_COSTLY_ORDER or
+	 * __GFP_REPEAT mean __GFP_NOFAIL, but that may not be true in other
+	 * implementations.
 	 */
 	do_retry = 0;
 	if (!(gfp_mask & __GFP_NORETRY)) {

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

Re: [PATCH] mm: fix confusing __GFP_REPEAT related comments

[Dave Hansen]

On Thu, 2007-11-29 at 13:48 -0800, Nishanth Aravamudan wrote:
> __GFP_NOFAIL means repeat forever
> 
> order <= PAGE_ALLOC_COSTLY_ORDER means __GFP_NOFAIL 

If this is true, why do we still pass in __GFP_REPEAT to the pgd_alloc()
functions (at least in x86's pgalloc_64.h and pgtable_32.c).  We don''t
ever have pagetables exceeding PAGE_ALLOC_COSTLY_ORDER, do we?

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

Re: [PATCH] mm: fix confusing __GFP_REPEAT related comments

[Nishanth Aravamudan]

On 29.11.2007 [15:14:40 -0800], Dave Hansen wrote:
> On Thu, 2007-11-29 at 13:48 -0800, Nishanth Aravamudan wrote:
> > __GFP_NOFAIL means repeat forever
> > 
> > order <= PAGE_ALLOC_COSTLY_ORDER means __GFP_NOFAIL 
> 
> If this is true, why do we still pass in __GFP_REPEAT to the
> pgd_alloc() functions (at least in x86's pgalloc_64.h and
> pgtable_32.c).  We don''t ever have pagetables exceeding
> PAGE_ALLOC_COSTLY_ORDER, do we?

That's a very good question. And is related to one of mine that you
snipped:

"In looking at the callers using __GFP_REPEAT, not all handle failure --
should they be using __NOFAIL?"

I *think* that all the current __GFP_REPEAT users are order <=
PAGE_ALLOC_CSOTLY_ORDER. Perhaps they all mean to use __GPF_NOFAIL? Some
don't handle failure immediately, but maybe their callers do, I haven't
had time to investigate fully.

And the whole gist, per the comments in mm/page_alloc.c, is that this is
all dependent upon this implementation of the VM. I think that means you
can't rely on those semantics being valid forever. So it's best for
callers to be as explicit as possible ... but in this case, I'm not sure
that the desired semantics actually exist.

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>

Re: [PATCH] mm: fix confusing __GFP_REPEAT related comments

[Dave Hansen]

On Thu, 2007-11-29 at 20:19 -0800, Nishanth Aravamudan wrote: 
> "In looking at the callers using __GFP_REPEAT, not all handle failure --
> should they be using __NOFAIL?"
> 
> I *think* that all the current __GFP_REPEAT users are order <=
> PAGE_ALLOC_CSOTLY_ORDER. Perhaps they all mean to use __GPF_NOFAIL? Some
> don't handle failure immediately, but maybe their callers do, I haven't
> had time to investigate fully.

I think we treat pagetable allocations just like normal ones with error
handling.  If I saw a pte_alloc() in a patch that was used without
checking for NULL, I'd certainly bitch about it.

In any case, if we want to nitpick, the *callers* haven't asked for
__GFP_NOFAIL, so they shouldn't be depending on a lack of failures.

> And the whole gist, per the comments in mm/page_alloc.c, is that this is
> all dependent upon this implementation of the VM. I think that means you
> can't rely on those semantics being valid forever. So it's best for
> callers to be as explicit as possible ... but in this case, I'm not sure
> that the desired semantics actually exist.

I don't really buy this "in this implementation of the VM" crap.  When
people go to figure out which functions and flags to use, they don't
just go look at headers.  They look at and depend on the
implementations.  If we change the implementations, we go change all the
callers, too.

Your patch highlights an existing problem: we're not being very good
with __GFP_REPEAT.  All of the pagetable users (on x86 at least) are
using __GFP_REPEAT, but effectively getting __GFP_NOFAIL.  There are
some other users around that might have larger buffers, but I think
pagetable pages are pretty guaranteed to stay <= 1 page in size. :)

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

Re: [PATCH] mm: fix confusing __GFP_REPEAT related comments

[Nishanth Aravamudan]

On 30.11.2007 [10:27:40 -0800], Dave Hansen wrote:
> On Thu, 2007-11-29 at 20:19 -0800, Nishanth Aravamudan wrote: 
> > "In looking at the callers using __GFP_REPEAT, not all handle failure --
> > should they be using __NOFAIL?"
> > 
> > I *think* that all the current __GFP_REPEAT users are order <=
> > PAGE_ALLOC_CSOTLY_ORDER. Perhaps they all mean to use __GPF_NOFAIL? Some
> > don't handle failure immediately, but maybe their callers do, I haven't
> > had time to investigate fully.
> 
> I think we treat pagetable allocations just like normal ones with
> error handling.  If I saw a pte_alloc() in a patch that was used
> without checking for NULL, I'd certainly bitch about it.

Hrm, you may be right. And it appears the only non-pagetable callers of
__GFP_REPEAT allocations are:

drivers/mmc/host/wbsd.c::wbsd_request_dma()
drivers/net/ppp_deflate.c::z_decomp_alloc()
drivers/s390/char/vmcp.c::vmcp_write()
net/core/sock.c::sock_alloc_send_pskb()

But those are of course only the explicit callers -- there are
presumably many others that are getting the same effect by passing a low
order.

> In any case, if we want to nitpick, the *callers* haven't asked for
> __GFP_NOFAIL, so they shouldn't be depending on a lack of failures.

I agree.

> > And the whole gist, per the comments in mm/page_alloc.c, is that this is
> > all dependent upon this implementation of the VM. I think that means you
> > can't rely on those semantics being valid forever. So it's best for
> > callers to be as explicit as possible ... but in this case, I'm not sure
> > that the desired semantics actually exist.
> 
> I don't really buy this "in this implementation of the VM" crap.  When
> people go to figure out which functions and flags to use, they don't
> just go look at headers.  They look at and depend on the
> implementations.  If we change the implementations, we go change all
> the callers, too.

I agree here, as well. I think that's why I'm asking ... if the
implementation is changed to perhaps different semantics: first, do we
have a set of semantics that are more desirably? second, do I interpret
the current callers flags as is and risk breaking some mild assumption
somewhere (that, for instance, while __GFP_REPEAT might fail, it doesn't
currently, so callers, while handling errors, really don't expect to
ever hit that code path?)

> Your patch highlights an existing problem: we're not being very good
> with __GFP_REPEAT.  All of the pagetable users (on x86 at least) are
> using __GFP_REPEAT, but effectively getting __GFP_NOFAIL.  There are
> some other users around that might have larger buffers, but I think
> pagetable pages are pretty guaranteed to stay <= 1 page in size. :)

Indeed.

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>

Re: [PATCH] mm: fix confusing __GFP_REPEAT related comments

[Dave Hansen]

On Fri, 2007-11-30 at 09:43 -0800, Nishanth Aravamudan wrote:
> 
> But those are of course only the explicit callers -- there are
> presumably many others that are getting the same effect by passing a low
> order. 

Yeah, and the socket function is just a helper and will have a number of
users.

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

Re: [PATCH] mm: fix confusing __GFP_REPEAT related comments

[William Lee Irwin III]

On Thu, Nov 29, 2007 at 01:48:28PM -0800, Nishanth Aravamudan wrote:
> The definition and use of __GFP_REPEAT, __GFP_NOFAIL and __GFP_NORETRY
> in the core VM have somewhat differing comments as to their actual
> semantics. Annoyingly, the flags definition has inline and header
> comments, which might be interpreted as not being equivalent. Just add
> references to the header comments in the inline ones so they don't go
> out of sync in the future. In their use in __alloc_pages() clarify that
> the current implementation treats low-order allocations and __GFP_REPEAT
> allocations as distinct cases, albeit currently with the same result.

This is a bit beyond the scope of the patch, but doesn't the obvious
livelock behavior here disturb anyone else?


-- wli

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

Re: [PATCH] mm: fix confusing __GFP_REPEAT related comments

[Nishanth Aravamudan]

On 02.12.2007 [03:58:57 -0800], William Lee Irwin III wrote:
> On Thu, Nov 29, 2007 at 01:48:28PM -0800, Nishanth Aravamudan wrote:
> > The definition and use of __GFP_REPEAT, __GFP_NOFAIL and __GFP_NORETRY
> > in the core VM have somewhat differing comments as to their actual
> > semantics. Annoyingly, the flags definition has inline and header
> > comments, which might be interpreted as not being equivalent. Just add
> > references to the header comments in the inline ones so they don't go
> > out of sync in the future. In their use in __alloc_pages() clarify that
> > the current implementation treats low-order allocations and __GFP_REPEAT
> > allocations as distinct cases, albeit currently with the same result.
> 
> This is a bit beyond the scope of the patch, but doesn't the obvious
> livelock behavior here disturb anyone else?

This was a concer to me as well, certainly. And perhaps an argument to
divorce low-order allocations from __GFP_REPEAT. I guess we hope reclaim
is good enough to eventually make enough progress ... however, if it
doesn't, I think we'll trigger this condition:


	if (likely(did_some_progress)) {
		...
	} else if ((gfp_mask & __GFP_FS) && !(gfp_mask & __GFP_NORETRY)) {
		try with high watermarks
		if still failing the alloc
			if PAGE_ALLOC_COSTLY_ORDER
				fail
		  	else
				OOM
	}

So, I think, the livelock condition is avoided in general as (for
low-order allocations), we can OOM to free memory, so the potentially
infinite loop should eventually finish?

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>

[PATCH] mm: fix confusing __GFP_REPEAT related comments

[Nishanth Aravamudan]

The definition and use of __GFP_REPEAT, __GFP_NOFAIL and __GFP_NORETRY
in the core VM have somewhat differing comments as to their actual
semantics. Annoyingly, the flags definition has inline and header
comments, which might be interpreted as not being equivalent. Just add
references to the header comments in the inline ones so they don't go
out of sync in the future. In their use in __alloc_pages() clarify that
the current implementation treats low-order allocations and __GFP_REPEAT
allocations as distinct cases.

To clarify, the flags' semantics are:

    __GFP_NORETRY means try no harder than one run through __alloc_pages

    __GFP_REPEAT means __GFP_NOFAIL

    __GFP_NOFAIL means repeat forever

    order <= PAGE_ALLOC_COSTLY_ORDER means __GFP_NOFAIL

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>

---

I'm not sure if the clarified semantics are really desired, and am
hoping this patch clarifies that for me:

In looking at the callers using __GFP_REPEAT, not all handle failure --
should they be using __NOFAIL?

Should __GFP_REPEAT eventually be able to fail? That is, maybe it makes
sense to somehow monitor the success of reclaim? If we are able to free
an equivalent order of pages as requested but still can't allocate the
pages requested, then fail? This would at least allow *some* behavior
between NORETRY and NOFAIL... This would get closer to what I was trying
to achieve in my hugetlb-specific patch
(http://marc.info/?l=linux-mm&m=119515798913982&w=2) without such
specific changes.

Should __GFP_NORETRY mean no retrying anywhere? The default path through
the allocator, before we even check if we should or should not retry
appears to retry in some sense of the word. This is of less concern to
me than the other two, though.

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 7e93a9a..e4ed545 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -40,9 +40,9 @@ struct vm_area_struct;
 #define __GFP_FS	((__force gfp_t)0x80u)	/* Can call down to low-level FS? */
 #define __GFP_COLD	((__force gfp_t)0x100u)	/* Cache-cold page required */
 #define __GFP_NOWARN	((__force gfp_t)0x200u)	/* Suppress page allocation failure warning */
-#define __GFP_REPEAT	((__force gfp_t)0x400u)	/* Retry the allocation.  Might fail */
-#define __GFP_NOFAIL	((__force gfp_t)0x800u)	/* Retry for ever.  Cannot fail */
-#define __GFP_NORETRY	((__force gfp_t)0x1000u)/* Do not retry.  Might fail */
+#define __GFP_REPEAT	((__force gfp_t)0x400u)	/* See above */
+#define __GFP_NOFAIL	((__force gfp_t)0x800u)	/* See above */
+#define __GFP_NORETRY	((__force gfp_t)0x1000u)/* See above */
 #define __GFP_COMP	((__force gfp_t)0x4000u)/* Add compound page metadata */
 #define __GFP_ZERO	((__force gfp_t)0x8000u)/* Return zeroed page on success */
 #define __GFP_NOMEMALLOC ((__force gfp_t)0x10000u) /* Don't use emergency reserves */
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index da69d83..71e2039 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1620,8 +1620,9 @@ nofail_alloc:
 	 * Don't let big-order allocations loop unless the caller explicitly
 	 * requests that.  Wait for some write requests to complete then retry.
 	 *
-	 * In this implementation, __GFP_REPEAT means __GFP_NOFAIL for order
-	 * <= 3, but that may not be true in other implementations.
+	 * In this implementation, either order <= PAGE_ALLOC_COSTLY_ORDER or
+	 * __GFP_REPEAT mean __GFP_NOFAIL, but that may not be true in other
+	 * implementations.
 	 */
 	do_retry = 0;
 	if (!(gfp_mask & __GFP_NORETRY)) {

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