> -----Original Message----- > From: James Bottomley [mailto:James.Bottomley@xxxxxxxxxxxxxxxxxxxxx] > Sent: Wednesday, March 19, 2014 8:51 PM > To: Desai, Kashyap > Cc: Saxena, Sumit; 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-03-19 at 07:21 +0000, Desai, Kashyap wrote: > > > > > -----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->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. > > It's possible, but kioc seems to be allocated and freed per thread in the driver > (mraid_mm_ioctl) so looks to me to require no locking once you have it > allocated because there's a single thread of control per ioctl. > > > 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); > > In this code, pool->lock protects variables in the pool (in this case > ->in_use) which looks to be correct. It's the chunk further down that > appears to have no use for the locking. > > > 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 ? > > Well, if you make this change, kmalloc GFP_ATOMIC will fail far more than > kmalloc GFP_KERNEL. That failure will be passed straight back to the caller of > the ioctl which, presumably, is your raid tool. What will that do? Fail or > Retry? If it fails, removing the locking would be best, if it retries, we can > probably do the GFP_ATOMIC hack. James, I agree that with GFP_ATOMIC may fail frequently. Wei, I hope your found this issue and fixed as part of the issue.. Is this possible to try what James has suggested. ? Remove the lock pool->lock while accessing pool->handle without lock and keep GFG_KERNEL flag as it is. May be you need to do IOCTL stress to check there is no race condition. > > 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
