| Message ID | c2f16ce7d75be5869b5888fa5198c40a@mail.gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On 5/26/16 6:16 AM, Chaitra Basappa wrote: > Johannes, > Could you please let us know which application is being used for firmware > upgrade ?? Whether it is your own customized application or LSI provided > applications? It's the sas2flash app from the LSI web site. Specifically, from Installer_P20_for_Linux.zip. > Also our application use single contiguous memory buffer for ioctls and > hence mpt3sas driver is using single contiguous memory for the DMA > operation. > > If we use GFP_KERNEL flag then it may be possible that ioctl thread may > hang/wait for long, if it doesn't get required memory from the system. I don't think that's a good justification for using GFP_ATOMIC. GFP_ATOMIC is for "must not sleep" not "we'd like this to be relatively low latency." The mpt3sas code uses GFP_KERNEL in places that, at a glance, are more sensitive to latency. If there are uses of the ioctls that are extremely sensitive to latency and can handle the failures that may occur with use of GFP_ATOMIC, they should be documented and handled as separate cases. Transferring a firmware image isn't one of them. > We may need to test below patch thoroughly , as I don’t see allocation of > several non-contiguous chunks of memory in below patch..., The patch doesn't implement that part of it. Johannes was asking if having the memory be a contiguous chunk was a hardware requirement before writing the code to do scatter gather with multiple discontiguous ranges. -Jeff > Thanks, > Chaitra > > -----Original Message----- > From: Johannes Thumshirn [mailto:jthumshirn@suse.de] > Sent: Wednesday, May 25, 2016 2:49 PM > To: Chaitra P B; Suganath Prabu Subramani > Cc: Linux SCSI Mailinglist; Jeff Mahoney > Subject: mpt3sas: memory allocation for firmware upgrade DMA memory > question > > Hi Chaitra and Suganath, > > I've got a question regarding mpt3sas' memory allocation used when doing a > firmware upgrade. Currently you're doing a pci_alloc_consitent() which > tries to allocate memory via GFP_ATOMIC. This memory then is passed as a > single element on a sg_list. > > Jeff reported it returned -ENOMEM on his Server due to highly fragmented > memory. > > Is it required to have the memory for the DMA operation contiguous, or can > I just allocate several non-contiguous junks of memory and map it to a > sg_list? > > If not, is GFP_ATMOIC really needed? I've converted the driver to use > GFP_KERNEL but I'm a bit reluctant to test below patch on real hardware to > not brick the HBA. > > Thanks, > Johannes > > RFC patch for GFP_KERNEL allocation, though splitting into multiple sg > mapped elements is the preferred fix here: > > From 06e63654d887df7f740dc5abcb40d441a8da7fa5 Mon Sep 17 00:00:00 2001 > From: Johannes Thumshirn <jthumshirn@suse.de> > Date: Tue, 24 May 2016 17:25:59 +0200 > Subject: [RFC PATCH] mpt3sas: Don't do atomic memory allocations for > firmware update DMA > > Currently mpt3sas uses pci_alloc_consistent() to allocate memory for the > DMA used to do firmware updates. pci_alloc_consistent() in turn uses > GFP_ATOMIC allocations. On a host with high memory fragmention this can > lead to page allocation failures, as the DMA buffer holds the complete > firmware update and thus can need page allocations of higher orders. > > As the firmware update code path may sleep, convert allocation to a normal > kzalloc() call with GFP_KERNEL and map it to the DMA buffers. > > Reported-by: Jeff Mahoney <jeffm@suse.com> > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> > --- > drivers/scsi/mpt3sas/mpt3sas_ctl.c | 39 > ++++++++++++++++++++++++++++---------- > 1 file changed, 29 insertions(+), 10 deletions(-) > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c > b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > index 7d00f09..14be3cf 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > @@ -751,8 +751,7 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, > struct mpt3_ioctl_command karg, > > /* obtain dma-able memory for data transfer */ > if (data_out_sz) /* WRITE */ { > - data_out = pci_alloc_consistent(ioc->pdev, data_out_sz, > - &data_out_dma); > + data_out = kzalloc(data_out_sz, GFP_KERNEL); > if (!data_out) { > pr_err("failure at %s:%d/%s()!\n", __FILE__, > __LINE__, __func__); > @@ -760,6 +759,14 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, > struct mpt3_ioctl_command karg, > mpt3sas_base_free_smid(ioc, smid); > goto out; > } > + data_out_dma = pci_map_single(ioc->pdev, data_out, > + data_out_sz, > PCI_DMA_TODEVICE); > + if (dma_mapping_error(&ioc->pdev->dev, data_out_dma)) { > + ret = -EINVAL; > + mpt3sas_base_free_smid(ioc, smid); > + goto out_free_data_out; > + } > + > if (copy_from_user(data_out, karg.data_out_buf_ptr, > data_out_sz)) { > pr_err("failure at %s:%d/%s()!\n", __FILE__, @@ > -771,8 +778,7 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, struct > mpt3_ioctl_command karg, > } > > if (data_in_sz) /* READ */ { > - data_in = pci_alloc_consistent(ioc->pdev, data_in_sz, > - &data_in_dma); > + data_in = kzalloc(data_in_sz, GFP_KERNEL); > if (!data_in) { > pr_err("failure at %s:%d/%s()!\n", __FILE__, > __LINE__, __func__); > @@ -780,6 +786,13 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, > struct mpt3_ioctl_command karg, > mpt3sas_base_free_smid(ioc, smid); > goto out; > } > + data_in_dma = pci_map_single(ioc->pdev, data_in, > + data_in_sz, > PCI_DMA_FROMDEVICE); > + if (dma_mapping_error(&ioc->pdev->dev, data_in_dma)) { > + ret = -EINVAL; > + mpt3sas_base_free_smid(ioc, smid); > + goto out_free_data_in; > + } > } > > psge = (void *)request + (karg.data_sge_offset*4); @@ -1013,13 > +1026,19 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, struct > mpt3_ioctl_command karg, > out: > > /* free memory associated with sg buffers */ > - if (data_in) > - pci_free_consistent(ioc->pdev, data_in_sz, data_in, > - data_in_dma); > + if (data_in) { > + pci_unmap_single(ioc->pdev, data_in_dma, > + data_in_sz, PCI_DMA_TODEVICE); > +out_free_data_in: > + kfree(data_in); > + } > > - if (data_out) > - pci_free_consistent(ioc->pdev, data_out_sz, data_out, > - data_out_dma); > + if (data_out) { > + pci_unmap_single(ioc->pdev, data_out_dma, > + data_out_sz, PCI_DMA_FROMDEVICE); > +out_free_data_out: > + kfree(data_out); > + } > > kfree(mpi_request); > ioc->ctl_cmds.status = MPT3_CMD_NOT_USED; > -- > 1.8.5.6 > > > > -- > Johannes Thumshirn Storage > jthumshirn@suse.de +49 911 74053 689 > SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg > GF: Felix Imendörffer, Jane Smithard, Graham Norton > HRB 21284 (AG Nürnberg) > Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 >
On Thu, May 26, 2016 at 03:46:58PM +0530, Chaitra Basappa wrote: > Johannes, [...] > > If we use GFP_KERNEL flag then it may be possible that ioctl thread may > hang/wait for long, if it doesn't get required memory from the system. > > We may need to test below patch thoroughly , as I don’t see allocation of > several non-contiguous chunks of memory in below patch..., The question here is if it would be possible to replace the kmalloc() call with a vmalloc() (which is non-contiguous) and then pass the allocated memory to a sg list? Thanks, Johannes
On Thu, May 26, 2016 at 03:46:58PM +0530, Chaitra Basappa wrote: > Johannes, > Could you please let us know which application is being used for firmware > upgrade ?? Whether it is your own customized application or LSI provided > applications? > > Also our application use single contiguous memory buffer for ioctls and > hence mpt3sas driver is using single contiguous memory for the DMA > operation. > > If we use GFP_KERNEL flag then it may be possible that ioctl thread may > hang/wait for long, if it doesn't get required memory from the system. > > We may need to test below patch thoroughly , as I don’t see allocation of > several non-contiguous chunks of memory in below patch..., > Any results? What about my quest whether we can allocate the buffers by vmalloc() and then map it in a SG list? Thanks, Johannes
diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c index 7d00f09..14be3cf 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c @@ -751,8 +751,7 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command karg, /* obtain dma-able memory for data transfer */ if (data_out_sz) /* WRITE */ { - data_out = pci_alloc_consistent(ioc->pdev, data_out_sz, - &data_out_dma); + data_out = kzalloc(data_out_sz, GFP_KERNEL); if (!data_out) { pr_err("failure at %s:%d/%s()!\n", __FILE__,