[PATCH] Don't use unnecessary GFP_ATOMIC in sg.c

[PATCH] Don't use unnecessary GFP_ATOMIC in sg.c

[Andi Kleen]

Don't use unnecessary GFP_ATOMIC in sg.c

sg.c uses GFP_ATOMIC for a lot of allocations where it isn't necessary.
There are no locks hold and I don't see any other reasons why GFP_ATOMIC
should be needed here. So remove them for more reliable allocations.

Depends on the earlier GFP_DMA patchkit, but only because it happens
to patch the same places (no real functional dependency) 

Tested by burning a CD on a kernel with all lock/sleep/etc. debugging enabled.

Signed-off-by: Andi Kleen <ak@suse.de>

---
 drivers/scsi/sg.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Index: linux/drivers/scsi/sg.c
===================================================================
--- linux.orig/drivers/scsi/sg.c
+++ linux/drivers/scsi/sg.c
@@ -745,7 +745,7 @@ sg_common_write(Sg_fd * sfp, Sg_request 
 	if (scsi_execute_async(sdp->device, cmnd, hp->cmd_len, data_dir, srp->data.buffer,
 				hp->dxfer_len, srp->data.k_use_sg, timeout,
 				SG_DEFAULT_RETRIES, srp, sg_cmd_done,
-				GFP_ATOMIC)) {
+				0)) {
 		SCSI_LOG_TIMEOUT(1, printk("sg_common_write: scsi_execute_async failed\n"));
 		/*
 		 * most likely out of mem, but could also be a bad map
@@ -1654,7 +1654,7 @@ static int
 sg_build_sgat(Sg_scatter_hold * schp, const Sg_fd * sfp, int tablesize)
 {
 	int sg_bufflen = tablesize * sizeof(struct scatterlist);
-	gfp_t gfp_flags = GFP_ATOMIC | __GFP_NOWARN;
+	gfp_t gfp_flags = __GFP_NOWARN;
 
 	schp->buffer = kzalloc(sg_bufflen, gfp_flags);
 	if (!schp->buffer)
@@ -1694,7 +1694,7 @@ st_map_user_pages(struct scatterlist *sg
 	if (count == 0)
 		return 0;
 
-	if ((pages = kmalloc(max_pages * sizeof(*pages), GFP_ATOMIC)) == NULL)
+	if ((pages = kmalloc(max_pages * sizeof(*pages), 0)) == NULL)
 		return -ENOMEM;
 
         /* Try to fault in all of the necessary pages */
@@ -2323,7 +2323,7 @@ sg_add_sfp(Sg_device * sdp, int dev)
 	unsigned long iflags;
 	int bufflen;
 
-	sfp = kzalloc(sizeof(*sfp), GFP_ATOMIC | __GFP_NOWARN);
+	sfp = kzalloc(sizeof(*sfp), GFP_KERNEL|__GFP_NOWARN);
 	if (!sfp)
 		return NULL;
 
@@ -2452,7 +2452,7 @@ sg_page_malloc(int rqSz, int *retSzp)
 	if ((rqSz <= 0) || (NULL == retSzp))
 		return resp;
 
-	page_mask = GFP_ATOMIC | __GFP_COMP | __GFP_NOWARN;
+	page_mask = __GFP_COMP | __GFP_NOWARN;
 
 	for (order = 0, a_size = PAGE_SIZE; a_size < rqSz;
 	     order++, a_size <<= 1) ;

Re: [PATCH] Don't use unnecessary GFP_ATOMIC in sg.c

[James Bottomley]

On Mon, 2008-02-25 at 00:36 +0100, Andi Kleen wrote:
> Don't use unnecessary GFP_ATOMIC in sg.c
> 
> sg.c uses GFP_ATOMIC for a lot of allocations where it isn't necessary.
> There are no locks hold and I don't see any other reasons why GFP_ATOMIC
> should be needed here. So remove them for more reliable allocations.
> 
> Depends on the earlier GFP_DMA patchkit, but only because it happens
> to patch the same places (no real functional dependency) 
> 
> Tested by burning a CD on a kernel with all lock/sleep/etc. debugging enabled.

I'm afraid this is wrong.  Those paths are used in the block layer issue
path.  Most of the time this path does have user context.  However, it's
legal to call it from tasklet context, and most error retries do this.
In that case, your GFP_KERNEL will have problems, so these have to be
GFP_ATOMIC, I'm afraid.

James

Re: [PATCH] Don't use unnecessary GFP_ATOMIC in sg.c

[Andi Kleen]

On Mon, Feb 25, 2008 at 06:43:08AM -0800, James Bottomley wrote:
> On Mon, 2008-02-25 at 00:36 +0100, Andi Kleen wrote:
> > Don't use unnecessary GFP_ATOMIC in sg.c
> > 
> > sg.c uses GFP_ATOMIC for a lot of allocations where it isn't necessary.
> > There are no locks hold and I don't see any other reasons why GFP_ATOMIC
> > should be needed here. So remove them for more reliable allocations.
> > 
> > Depends on the earlier GFP_DMA patchkit, but only because it happens
> > to patch the same places (no real functional dependency) 
> > 
> > Tested by burning a CD on a kernel with all lock/sleep/etc. debugging enabled.
> 
> I'm afraid this is wrong.  Those paths are used in the block layer issue
> path.  Most of the time this path does have user context.  However, it's
> legal to call it from tasklet context, and most error retries do this.
> In that case, your GFP_KERNEL will have problems, so these have to be
> GFP_ATOMIC, I'm afraid.

In what path? I couldn't find one while reading the code. But I don't
claim to understand it completely so I might have missed something.

-Andi

Re: [PATCH] Don't use unnecessary GFP_ATOMIC in sg.c

[James Bottomley]

On Mon, 2008-02-25 at 15:56 +0100, Andi Kleen wrote:
> On Mon, Feb 25, 2008 at 06:43:08AM -0800, James Bottomley wrote:
> > On Mon, 2008-02-25 at 00:36 +0100, Andi Kleen wrote:
> > > Don't use unnecessary GFP_ATOMIC in sg.c
> > > 
> > > sg.c uses GFP_ATOMIC for a lot of allocations where it isn't necessary.
> > > There are no locks hold and I don't see any other reasons why GFP_ATOMIC
> > > should be needed here. So remove them for more reliable allocations.
> > > 
> > > Depends on the earlier GFP_DMA patchkit, but only because it happens
> > > to patch the same places (no real functional dependency) 
> > > 
> > > Tested by burning a CD on a kernel with all lock/sleep/etc. debugging enabled.
> > 
> > I'm afraid this is wrong.  Those paths are used in the block layer issue
> > path.  Most of the time this path does have user context.  However, it's
> > legal to call it from tasklet context, and most error retries do this.
> > In that case, your GFP_KERNEL will have problems, so these have to be
> > GFP_ATOMIC, I'm afraid.
> 
> In what path? I couldn't find one while reading the code. But I don't
> claim to understand it completely so I might have missed something.

It's a fundamental lack of guarantee of the API.  Block *may* give you
user context but doesn't guarantee it.  It's so we can call
blk_run_queue from the block tasklet.

This is usually done by queue restarts.  SCSI does it via
scsi_next_command, which is usually in tasklet context.  You'll really
only see this for a TCQ device (which no CDs are).

James

Re: [PATCH] Don't use unnecessary GFP_ATOMIC in sg.c

[Andi Kleen]

On Mon, Feb 25, 2008 at 07:00:34AM -0800, James Bottomley wrote:
> 
> On Mon, 2008-02-25 at 15:56 +0100, Andi Kleen wrote:
> > On Mon, Feb 25, 2008 at 06:43:08AM -0800, James Bottomley wrote:
> > > On Mon, 2008-02-25 at 00:36 +0100, Andi Kleen wrote:
> > > > Don't use unnecessary GFP_ATOMIC in sg.c
> > > > 
> > > > sg.c uses GFP_ATOMIC for a lot of allocations where it isn't necessary.
> > > > There are no locks hold and I don't see any other reasons why GFP_ATOMIC
> > > > should be needed here. So remove them for more reliable allocations.
> > > > 
> > > > Depends on the earlier GFP_DMA patchkit, but only because it happens
> > > > to patch the same places (no real functional dependency) 
> > > > 
> > > > Tested by burning a CD on a kernel with all lock/sleep/etc. debugging enabled.
> > > 
> > > I'm afraid this is wrong.  Those paths are used in the block layer issue
> > > path.  Most of the time this path does have user context.  However, it's
> > > legal to call it from tasklet context, and most error retries do this.
> > > In that case, your GFP_KERNEL will have problems, so these have to be
> > > GFP_ATOMIC, I'm afraid.
> > 
> > In what path? I couldn't find one while reading the code. But I don't
> > claim to understand it completely so I might have missed something.

Hmm, but as far as I can figure out these allocations are only used in code
that does copy_from_user or get_user_pages or wait_event_*  or
some other API that sleeps. That task let path you're talking about
must be well hidden. I would appreciate if someone in the know
could point me to the path I'm missing.

Anyways I hadn't expected it to be that controversal and since that
patch is only a side show I retract it for now.

-Andi