linux-kernel - Re: + mempool-fix-first-round-failure-behavior.patch added to -mm tree

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20111222164958.GG17084@google.com>
Date:	Thu, 22 Dec 2011 08:49:58 -0800
From:	Tejun Heo <tj@...nel.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: + mempool-fix-first-round-failure-behavior.patch added to -mm
 tree

Hello, Oleg.

On Thu, Dec 22, 2011 at 05:39:00PM +0100, Oleg Nesterov wrote:
> I can't even explain why this (simple!) logic looks confusing to me,

Yeah, gfp_mask and temp confused me pretty good too.

> with or without the patch. A couple of questions:
> 
> 	1. Why do we remove __GFP_WAIT unconditionally before the the
> 	   very 1st allocation?

To avoid blocking when there's pool sitting around.

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

No idea.

> @@ -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);

Yeah, this one definitely looks better & makes more sense.  Andrew,
please feel free to drop mine and take this one.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/