Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755147AbXEDNIm (ORCPT ); Fri, 4 May 2007 09:08:42 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1755146AbXEDNIm (ORCPT ); Fri, 4 May 2007 09:08:42 -0400 Received: from ug-out-1314.google.com ([66.249.92.169]:38390 "EHLO ug-out-1314.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755147AbXEDNIk (ORCPT ); Fri, 4 May 2007 09:08:40 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=beta; h=received:message-id:date:from:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references; b=nis2YogBhPQxFb9z7LQDQAurQ8XI/PNEjuSzR+fPnPdHBvR1h/NprdOosMQO0PJjeFI6WorY60hH9/6xOOf+0mhyrmxvCfydRXF+b8oDfS/YxgT+KEHTisstUcjpi8iGRFEaDCKQdykDxFUXbA7BcHBPHXBqHuBVD1fzwydZ0ag= Message-ID: Date: Fri, 4 May 2007 18:38:37 +0530 From: "Satyam Sharma" To: "Andrew Morton" Subject: Re: [PATCH] SCSI: Remove redundant GFP_KERNEL type flag in kmalloc(). Cc: "Robert P. J. Day" , "Linux Kernel Mailing List" , linux-scsi@vger.kernel.org, "James Bottomley" In-Reply-To: <20070504011535.ff7181fb.akpm@linux-foundation.org> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <20070504011535.ff7181fb.akpm@linux-foundation.org> Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2287 Lines: 56 On 5/4/07, Andrew Morton wrote: > [...] > > > > - aic_dev = kmalloc(sizeof(struct aic_dev_data), GFP_ATOMIC | GFP_KERNEL); > > + aic_dev = kmalloc(sizeof(struct aic_dev_data), GFP_ATOMIC); > > No, this converts the allocation from a robust one which can sleep into a > flakey one which cannot. > > If we want to just clean this code up, we should switch to > > GFP_KERNEL|__GFP_HIGH > > and add a comment explaining why we're turning on __GFP_HIGH (pointlessly, > I suspect). > > However I suspect what the code really meant to do was to use just > GFP_KERNEL. Yes. A recap from http://lkml.org/lkml/2007/4/28/160 ... > > So combining GFP_ATOMIC with GFP_KERNEL gives you > > "allow io, allow fs, allow waiting, and use emergency pools when it's > > getting tight" which to me looks like a valid, but probably unwanted > > combination. and: > As a matter of style, the author there could've written (GFP_KERNEL | > __GFP_HIGH) instead, but I'm not sure how much sense that makes > because once we specify GFP_KERNEL, we essentially bring out the heavy > weaponry to *make* some free space for ourselves if it isn't there > already (and we're prepared to sleep when all that happens) -- so > where's the need left to scavenge into emergency pools anymore? > __GFP_HIGH only makes sense for poor atomic contexts for whom sleeping > (when we try_to_free other pages to satisfy our allocation request) is > not an option, which is precisely how things are presently. > > [...] > 1. If you're in_atomic() context, that GFP_KERNEL is a BUG and *must* > be removed. > > 2. If not, that GFP_ATOMIC is redundant / nonsensical and *can* be removed. It _was_ established towards the end of that thread that neither of the two cases are atomic contexts. And it would still make sense to remove the redundant __GFP_HIGH, as we don't want to unnecessarily deplete the emergency reserve pool for normal GFP_KERNEL cases thus making ENOMEM on some future kmalloc(GFP_ATOMIC) more likely. I'll send out the patches. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/