lkml.org 
[lkml]   [2011]   [Dec]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: + mempool-fix-first-round-failure-behavior.patch added to -mm tree
Hello,

> For the initial allocation, mempool passes modified gfp mask to the
> backing allocator so that it doesn't try too hard when there are reserved
> elements waiting in the pool; however, when that allocation fails and pool
> is empty too, it either waits for the pool to be replenished before
> retrying or fails if !__GFP_WAIT.
>
> * If the caller was calling in with GFP_ATOMIC, it never gets to try
> emergency reserve. Allocations which would have succeeded without
> mempool may fail, which is just wrong.
>
> * Allocation which could have succeeded after a bit of reclaim now has
> to wait on the reserved items and it's not like mempool doesn't retry
> with the original gfp mask. It just does that *after* someone returns
> an element, pointlessly delaying things.
>
> Fix it by retrying immediately with the gfp mask requested by the caller
> if the first round of allocation attempts fails with modified mask.

I can't even explain why this (simple!) logic looks confusing to me,
with or without the patch. A couple of questions:

1. Why do we remove __GFP_WAIT unconditionally before the the
very 1st allocation?

2. Why do we always restore it after io_schedule(), even if
we have the reserved items?

IOW, what do you think about the patch below instead?

Oleg.

--- x/mm/mempool.c
+++ x/mm/mempool.c
@@ -212,10 +212,12 @@ void * mempool_alloc(mempool_t *pool, gf
gfp_mask |= __GFP_NORETRY; /* don't loop in __alloc_pages */
gfp_mask |= __GFP_NOWARN; /* failures are OK */

- gfp_temp = gfp_mask & ~(__GFP_WAIT|__GFP_IO);
-
repeat_alloc:

+ gfp_temp = gfp_mask;
+ if (pool->curr_nr)
+ gfp_temp &= ~(__GFP_WAIT|__GFP_IO);
+
element = pool->alloc(gfp_temp, pool->pool_data);
if (likely(element != NULL))
return element;
@@ -229,13 +231,15 @@ repeat_alloc:
}

/* We must not sleep in the GFP_ATOMIC case */
- if (!(gfp_mask & __GFP_WAIT)) {
+ if (!(gfp_temp & __GFP_WAIT)) {
spin_unlock_irqrestore(&pool->lock, flags);
+ /* raced with another mempool_alloc? */
+ if (gfp_mask & __GFP_WAIT)
+ goto repeat_alloc;
return NULL;
}

/* Let's wait for someone else to return an element to @pool */
- gfp_temp = gfp_mask;
init_wait(&wait);
prepare_to_wait(&pool->wait, &wait, TASK_UNINTERRUPTIBLE);



\
 
 \ /
  Last update: 2011-12-22 17:47    [W:0.118 / U:0.660 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site