> -----Original Message----- > From: James Bottomley [mailto:James.Bottomley@xxxxxxxxxxxxxxxxxxxxx] > Sent: Thursday, March 13, 2014 4:00 PM > To: Saxena, Sumit > Cc: Wei Yongjun; DL-MegaRAID Linux; simon.puels@xxxxxxxxx; > jkosina@xxxxxxx; yongjun_wei@xxxxxxxxxxxxxxxxx; linux- > scsi@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] [SCSI] megaraid: use GFP_ATOMIC under spin lock > > On Wed, 2014-01-08 at 12:16 +0000, Saxena, Sumit wrote: > > > > >-----Original Message----- > > >From: Wei Yongjun [mailto:weiyj.lk@xxxxxxxxx] > > >Sent: Friday, December 20, 2013 8:38 AM > > >To: DL-MegaRAID Linux; JBottomley@xxxxxxxxxxxxx; > > >simon.puels@xxxxxxxxx; jkosina@xxxxxxx > > >Cc: yongjun_wei@xxxxxxxxxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx > > >Subject: [PATCH] [SCSI] megaraid: use GFP_ATOMIC under spin lock > > > > > >From: Wei Yongjun <yongjun_wei@xxxxxxxxxxxxxxxxx> > > > > > >A spin lock is taken here so we should use GFP_ATOMIC. > > > > > >Signed-off-by: Wei Yongjun <yongjun_wei@xxxxxxxxxxxxxxxxx> > > >--- > > > drivers/scsi/megaraid/megaraid_mm.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > >diff --git a/drivers/scsi/megaraid/megaraid_mm.c > > >b/drivers/scsi/megaraid/megaraid_mm.c > > >index a706927..99fa5d3 100644 > > >--- a/drivers/scsi/megaraid/megaraid_mm.c > > >+++ b/drivers/scsi/megaraid/megaraid_mm.c > > >@@ -570,7 +570,7 @@ mraid_mm_attach_buf(mraid_mmadp_t *adp, > uioc_t > > >*kioc, int xferlen) > > > > > > kioc->pool_index = right_pool; > > > kioc->free_buf = 1; > > >- kioc->buf_vaddr = pci_pool_alloc(pool->handle, GFP_KERNEL, > > >+ kioc->buf_vaddr = pci_pool_alloc(pool->handle, > > >GFP_ATOMIC, > > > &kioc->buf_paddr); > > > spin_unlock_irqrestore(&pool->lock, flags); > > > > > > > > Acked-by: Sumit Saxena <sumit.saxena@xxxxxxx> > > This doesn't look like the right fix to me. What is the function of > pool->lock around this code region? The only data you use from the pool > is pool->handle which is set up at init time ... there's no need to use a lock to > read a piece of data which is effectively constant, so just drop the lock. Just trying to understand why pool->lock was used here. Looks like this code may require synchronization using pool->lock while modifying anything in kioc. Below code is part of "mraid_mm_dealloc_kioc".. and here also pool->lock is used before accessing Kioc->free_buf variable. /* This routine may be called in non-isr context also */ spin_lock_irqsave(&pool->lock, flags); /* * While attaching the dma buffer, if we didn't get the * required buffer from the pool, we would have allocated * it at the run time and set the free_buf flag. We must * free that buffer. Otherwise, just mark that the buffer is * not in use */ if (kioc->free_buf == 1) pci_pool_free(pool->handle, kioc->buf_vaddr, kioc->buf_paddr); else pool->in_use = 0; spin_unlock_irqrestore(&pool->lock, flags); This code is not actively change now by LSI, so doing too much of experiment may break something. Considering pool->lock usage as a valid (As you suggested it may be possible candidate for optimization.), do you consider including change proposed in this patch ? ` Kashyap > > James > > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
