Skip to contentSkip to conversation

Don't use unnecessary GFP_ATOMIC in sg.c

PATCH In review Andi Kleen 2 participants 5 messages Feb 24, 2008 – Feb 25, 2008

Subsystems (2)
SCSI SCSI SG driver
lore
Share & tools
lore

Andi Kleen

author
#0 lore raw

PATCH 1/1 Don't use unnecessary GFP_ATOMIC in sg.c

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 <[email protected]>
---
 drivers/scsi/sg.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/scsi/sg.c +5 -5
Index: 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) ;
maintainer
#1 lore raw
Quoted 10 lines · 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.

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

author
#2 lore raw
Quoted 10 lines · James Bottomley
Quoted 9 lines · 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)

Quoted 7 lines
Quoted 1 line

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

maintainer
#3 lore raw
Quoted 13 lines · Andi Kleen
Quoted 12 lines · James Bottomley
Quoted 10 lines · 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.

Quoted 8 lines
Quoted 5 lines

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

author
#4 lore raw
Quoted 15 lines · James Bottomley
Quoted 13 lines · Andi Kleen
Quoted 12 lines · James Bottomley
Quoted 10 lines · 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.

Quoted 8 lines
Quoted 8 lines
Quoted 5 lines

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