Hello Jens,
As you know there are multiple drivers that both allocate a scatter/gather
list and populate that list with pages. This patch series moves the code for
allocating and freeing such scatterlists from these drivers into
lib/scatterlist.c. Please consider this patch series for kernel v4.15.
Notes:
- Only the ib_srpt driver changes have been tested but none of the other
drivers have been retested.
- The next step is to introduce a caching mechanism for these scatterlists
and make the nvmet/rdma and SCSI target drivers use that caching mechanism
since for these drivers sgl allocation occurs in the hot path.
Changes compared to v1:
- Moved the sgl_alloc*() and sgl_free() functions from a new source file into
lib/scatterlist.c.
- Changed the argument order for the sgl_alloc*() functions such that the
(pointer to) the output argument comes last.
Thanks,
Bart.
Bart Van Assche (8):
lib/scatterlist: Introduce sgl_alloc() and sgl_free()
crypto: scompress - use sgl_alloc() and sgl_free()
nvmet/fc: Use sgl_alloc() and sgl_free()
nvmet/rdma: Use sgl_alloc() and sgl_free()
target: Use sgl_alloc_order() and sgl_free()
scsi/ipr: Use sgl_alloc_order() and sgl_free_order()
scsi/pmcraid: Remove an unused structure member
scsi/pmcraid: Use sgl_alloc_order() and sgl_free_order()
crypto/Kconfig | 1 +
crypto/scompress.c | 51 +---------------
drivers/nvme/target/Kconfig | 2 +
drivers/nvme/target/fc.c | 36 +----------
drivers/nvme/target/rdma.c | 63 ++------------------
drivers/scsi/Kconfig | 2 +
drivers/scsi/ipr.c | 49 +++------------
drivers/scsi/ipr.h | 2 +-
drivers/scsi/pmcraid.c | 43 ++------------
drivers/scsi/pmcraid.h | 3 +-
drivers/target/Kconfig | 1 +
drivers/target/target_core_transport.c | 46 ++-------------
include/linux/scatterlist.h | 10 ++++
lib/Kconfig | 4 ++
lib/scatterlist.c | 105 +++++++++++++++++++++++++++++++++
15 files changed, 151 insertions(+), 267 deletions(-)
--
2.14.2
Use the sgl_alloc_order() and sgl_free() functions instead of open
coding these functions.
Signed-off-by: Bart Van Assche <[email protected]>
Cc: Nicholas A. Bellinger <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Sagi Grimberg <[email protected]>
---
drivers/target/Kconfig | 1 +
drivers/target/target_core_transport.c | 46 +++-------------------------------
2 files changed, 5 insertions(+), 42 deletions(-)
diff --git a/drivers/target/Kconfig b/drivers/target/Kconfig
index e2bc99980f75..4c44d7bed01a 100644
--- a/drivers/target/Kconfig
+++ b/drivers/target/Kconfig
@@ -5,6 +5,7 @@ menuconfig TARGET_CORE
select CONFIGFS_FS
select CRC_T10DIF
select BLK_SCSI_REQUEST # only for scsi_command_size_tbl..
+ select SGL_ALLOC
default n
help
Say Y or M here to enable the TCM Storage Engine and ConfigFS enabled
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 836d552b0385..9bbd08be9d60 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2293,13 +2293,7 @@ static void target_complete_ok_work(struct work_struct *work)
void target_free_sgl(struct scatterlist *sgl, int nents)
{
- struct scatterlist *sg;
- int count;
-
- for_each_sg(sgl, sg, nents, count)
- __free_page(sg_page(sg));
-
- kfree(sgl);
+ sgl_free(sgl);
}
EXPORT_SYMBOL(target_free_sgl);
@@ -2423,42 +2417,10 @@ int
target_alloc_sgl(struct scatterlist **sgl, unsigned int *nents, u32 length,
bool zero_page, bool chainable)
{
- struct scatterlist *sg;
- struct page *page;
- gfp_t zero_flag = (zero_page) ? __GFP_ZERO : 0;
- unsigned int nalloc, nent;
- int i = 0;
-
- nalloc = nent = DIV_ROUND_UP(length, PAGE_SIZE);
- if (chainable)
- nalloc++;
- sg = kmalloc_array(nalloc, sizeof(struct scatterlist), GFP_KERNEL);
- if (!sg)
- return -ENOMEM;
+ gfp_t gfp = GFP_KERNEL | (zero_page ? __GFP_ZERO : 0);
- sg_init_table(sg, nalloc);
-
- while (length) {
- u32 page_len = min_t(u32, length, PAGE_SIZE);
- page = alloc_page(GFP_KERNEL | zero_flag);
- if (!page)
- goto out;
-
- sg_set_page(&sg[i], page, page_len, 0);
- length -= page_len;
- i++;
- }
- *sgl = sg;
- *nents = nent;
- return 0;
-
-out:
- while (i > 0) {
- i--;
- __free_page(sg_page(&sg[i]));
- }
- kfree(sg);
- return -ENOMEM;
+ *sgl = sgl_alloc_order(length, 0, chainable, gfp, nents);
+ return *sgl ? 0 : -ENOMEM;
}
EXPORT_SYMBOL(target_alloc_sgl);
--
2.14.2
Use the sgl_alloc() and sgl_free() functions instead of open coding
these functions.
Signed-off-by: Bart Van Assche <[email protected]>
Reviewed-by: Johannes Thumshirn <[email protected]>
Cc: Keith Busch <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Sagi Grimberg <[email protected]>
---
drivers/nvme/target/Kconfig | 1 +
drivers/nvme/target/rdma.c | 63 +++------------------------------------------
2 files changed, 5 insertions(+), 59 deletions(-)
diff --git a/drivers/nvme/target/Kconfig b/drivers/nvme/target/Kconfig
index 4d9715630e21..5f4f8b16685f 100644
--- a/drivers/nvme/target/Kconfig
+++ b/drivers/nvme/target/Kconfig
@@ -29,6 +29,7 @@ config NVME_TARGET_RDMA
tristate "NVMe over Fabrics RDMA target support"
depends on INFINIBAND
depends on NVME_TARGET
+ select SGL_ALLOC
help
This enables the NVMe RDMA target support, which allows exporting NVMe
devices over RDMA.
diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 76d2bb793afe..363d44c10b68 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -185,59 +185,6 @@ nvmet_rdma_put_rsp(struct nvmet_rdma_rsp *rsp)
spin_unlock_irqrestore(&rsp->queue->rsps_lock, flags);
}
-static void nvmet_rdma_free_sgl(struct scatterlist *sgl, unsigned int nents)
-{
- struct scatterlist *sg;
- int count;
-
- if (!sgl || !nents)
- return;
-
- for_each_sg(sgl, sg, nents, count)
- __free_page(sg_page(sg));
- kfree(sgl);
-}
-
-static int nvmet_rdma_alloc_sgl(struct scatterlist **sgl, unsigned int *nents,
- u32 length)
-{
- struct scatterlist *sg;
- struct page *page;
- unsigned int nent;
- int i = 0;
-
- nent = DIV_ROUND_UP(length, PAGE_SIZE);
- sg = kmalloc_array(nent, sizeof(struct scatterlist), GFP_KERNEL);
- if (!sg)
- goto out;
-
- sg_init_table(sg, nent);
-
- while (length) {
- u32 page_len = min_t(u32, length, PAGE_SIZE);
-
- page = alloc_page(GFP_KERNEL);
- if (!page)
- goto out_free_pages;
-
- sg_set_page(&sg[i], page, page_len, 0);
- length -= page_len;
- i++;
- }
- *sgl = sg;
- *nents = nent;
- return 0;
-
-out_free_pages:
- while (i > 0) {
- i--;
- __free_page(sg_page(&sg[i]));
- }
- kfree(sg);
-out:
- return NVME_SC_INTERNAL;
-}
-
static int nvmet_rdma_alloc_cmd(struct nvmet_rdma_device *ndev,
struct nvmet_rdma_cmd *c, bool admin)
{
@@ -484,7 +431,7 @@ static void nvmet_rdma_release_rsp(struct nvmet_rdma_rsp *rsp)
}
if (rsp->req.sg != &rsp->cmd->inline_sg)
- nvmet_rdma_free_sgl(rsp->req.sg, rsp->req.sg_cnt);
+ sgl_free(rsp->req.sg);
if (unlikely(!list_empty_careful(&queue->rsp_wr_wait_list)))
nvmet_rdma_process_wr_wait_list(queue);
@@ -620,16 +567,14 @@ static u16 nvmet_rdma_map_sgl_keyed(struct nvmet_rdma_rsp *rsp,
u32 len = get_unaligned_le24(sgl->length);
u32 key = get_unaligned_le32(sgl->key);
int ret;
- u16 status;
/* no data command? */
if (!len)
return 0;
- status = nvmet_rdma_alloc_sgl(&rsp->req.sg, &rsp->req.sg_cnt,
- len);
- if (status)
- return status;
+ rsp->req.sg = sgl_alloc(len, GFP_KERNEL, &rsp->req.sg_cnt);
+ if (!rsp->req.sg)
+ return NVME_SC_INTERNAL;
ret = rdma_rw_ctx_init(&rsp->rw, cm_id->qp, cm_id->port_num,
rsp->req.sg, rsp->req.sg_cnt, 0, addr, key,
--
2.14.2
Use the sgl_alloc_order() and sgl_free_order() functions instead
of open coding these functions.
Signed-off-by: Bart Van Assche <[email protected]>
Reviewed-by: Johannes Thumshirn <[email protected]>
Cc: [email protected]
Cc: Martin K. Petersen <[email protected]>
Cc: Brian King <[email protected]>
---
drivers/scsi/Kconfig | 1 +
drivers/scsi/ipr.c | 49 ++++++++-----------------------------------------
drivers/scsi/ipr.h | 2 +-
3 files changed, 10 insertions(+), 42 deletions(-)
diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 41366339b950..d11e75e76195 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -1058,6 +1058,7 @@ config SCSI_IPR
depends on PCI && SCSI && ATA
select FW_LOADER
select IRQ_POLL
+ select SGL_ALLOC
---help---
This driver supports the IBM Power Linux family RAID adapters.
This includes IBM pSeries 5712, 5703, 5709, and 570A, as well
diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index f838bd73befa..6fed5db6255e 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -3815,10 +3815,8 @@ static struct device_attribute ipr_iopoll_weight_attr = {
**/
static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
{
- int sg_size, order, bsize_elem, num_elem, i, j;
+ int sg_size, order;
struct ipr_sglist *sglist;
- struct scatterlist *scatterlist;
- struct page *page;
/* Get the minimum size per scatter/gather element */
sg_size = buf_len / (IPR_MAX_SGLIST - 1);
@@ -3826,45 +3824,18 @@ static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
/* Get the actual size per element */
order = get_order(sg_size);
- /* Determine the actual number of bytes per element */
- bsize_elem = PAGE_SIZE * (1 << order);
-
- /* Determine the actual number of sg entries needed */
- if (buf_len % bsize_elem)
- num_elem = (buf_len / bsize_elem) + 1;
- else
- num_elem = buf_len / bsize_elem;
-
/* Allocate a scatter/gather list for the DMA */
- sglist = kzalloc(sizeof(struct ipr_sglist) +
- (sizeof(struct scatterlist) * (num_elem - 1)),
- GFP_KERNEL);
-
+ sglist = kzalloc(sizeof(struct ipr_sglist), GFP_KERNEL);
if (sglist == NULL) {
ipr_trace;
return NULL;
}
-
- scatterlist = sglist->scatterlist;
- sg_init_table(scatterlist, num_elem);
-
sglist->order = order;
- sglist->num_sg = num_elem;
-
- /* Allocate a bunch of sg elements */
- for (i = 0; i < num_elem; i++) {
- page = alloc_pages(GFP_KERNEL, order);
- if (!page) {
- ipr_trace;
-
- /* Free up what we already allocated */
- for (j = i - 1; j >= 0; j--)
- __free_pages(sg_page(&scatterlist[j]), order);
- kfree(sglist);
- return NULL;
- }
-
- sg_set_page(&scatterlist[i], page, 0, 0);
+ sglist->scatterlist = sgl_alloc_order(buf_len, order, false, GFP_KERNEL,
+ &sglist->num_sg);
+ if (!sglist->scatterlist) {
+ kfree(sglist);
+ return NULL;
}
return sglist;
@@ -3882,11 +3853,7 @@ static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
**/
static void ipr_free_ucode_buffer(struct ipr_sglist *sglist)
{
- int i;
-
- for (i = 0; i < sglist->num_sg; i++)
- __free_pages(sg_page(&sglist->scatterlist[i]), sglist->order);
-
+ sgl_free_order(sglist->scatterlist, sglist->order);
kfree(sglist);
}
diff --git a/drivers/scsi/ipr.h b/drivers/scsi/ipr.h
index c7f0e9e3cd7d..93570734cbfb 100644
--- a/drivers/scsi/ipr.h
+++ b/drivers/scsi/ipr.h
@@ -1454,7 +1454,7 @@ struct ipr_sglist {
u32 num_sg;
u32 num_dma_sg;
u32 buffer_len;
- struct scatterlist scatterlist[1];
+ struct scatterlist *scatterlist;
};
enum ipr_sdt_state {
--
2.14.2
Use the sgl_alloc_order() and sgl_free_order() functions instead
of open coding these functions.
Signed-off-by: Bart Van Assche <[email protected]>
Reviewed-by: Johannes Thumshirn <[email protected]>
Cc: [email protected]
Cc: Martin K. Petersen <[email protected]>
Cc: Anil Ravindranath <[email protected]>
---
drivers/scsi/Kconfig | 1 +
drivers/scsi/pmcraid.c | 43 ++++---------------------------------------
drivers/scsi/pmcraid.h | 2 +-
3 files changed, 6 insertions(+), 40 deletions(-)
diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index d11e75e76195..632200ec36a6 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -1576,6 +1576,7 @@ config ZFCP
config SCSI_PMCRAID
tristate "PMC SIERRA Linux MaxRAID adapter support"
depends on PCI && SCSI && NET
+ select SGL_ALLOC
---help---
This driver supports the PMC SIERRA MaxRAID adapters.
diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
index b4d6cd8cd1ad..95fc0352f9bb 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -3232,12 +3232,7 @@ static int pmcraid_build_ioadl(
*/
static void pmcraid_free_sglist(struct pmcraid_sglist *sglist)
{
- int i;
-
- for (i = 0; i < sglist->num_sg; i++)
- __free_pages(sg_page(&(sglist->scatterlist[i])),
- sglist->order);
-
+ sgl_free_order(sglist->scatterlist, sglist->order);
kfree(sglist);
}
@@ -3254,50 +3249,20 @@ static void pmcraid_free_sglist(struct pmcraid_sglist *sglist)
static struct pmcraid_sglist *pmcraid_alloc_sglist(int buflen)
{
struct pmcraid_sglist *sglist;
- struct scatterlist *scatterlist;
- struct page *page;
- int num_elem, i, j;
int sg_size;
int order;
- int bsize_elem;
sg_size = buflen / (PMCRAID_MAX_IOADLS - 1);
order = (sg_size > 0) ? get_order(sg_size) : 0;
- bsize_elem = PAGE_SIZE * (1 << order);
-
- /* Determine the actual number of sg entries needed */
- if (buflen % bsize_elem)
- num_elem = (buflen / bsize_elem) + 1;
- else
- num_elem = buflen / bsize_elem;
/* Allocate a scatter/gather list for the DMA */
- sglist = kzalloc(sizeof(struct pmcraid_sglist) +
- (sizeof(struct scatterlist) * (num_elem - 1)),
- GFP_KERNEL);
-
+ sglist = kzalloc(sizeof(struct pmcraid_sglist), GFP_KERNEL);
if (sglist == NULL)
return NULL;
- scatterlist = sglist->scatterlist;
- sg_init_table(scatterlist, num_elem);
sglist->order = order;
- sglist->num_sg = num_elem;
- sg_size = buflen;
-
- for (i = 0; i < num_elem; i++) {
- page = alloc_pages(GFP_KERNEL|GFP_DMA|__GFP_ZERO, order);
- if (!page) {
- for (j = i - 1; j >= 0; j--)
- __free_pages(sg_page(&scatterlist[j]), order);
- kfree(sglist);
- return NULL;
- }
-
- sg_set_page(&scatterlist[i], page,
- sg_size < bsize_elem ? sg_size : bsize_elem, 0);
- sg_size -= bsize_elem;
- }
+ sgl_alloc_order(buflen, order, false,
+ GFP_KERNEL | GFP_DMA | __GFP_ZERO, &sglist->num_sg);
return sglist;
}
diff --git a/drivers/scsi/pmcraid.h b/drivers/scsi/pmcraid.h
index 44da91712115..754ef30927e2 100644
--- a/drivers/scsi/pmcraid.h
+++ b/drivers/scsi/pmcraid.h
@@ -542,7 +542,7 @@ struct pmcraid_sglist {
u32 order;
u32 num_sg;
u32 num_dma_sg;
- struct scatterlist scatterlist[1];
+ struct scatterlist *scatterlist;
};
/* page D0 inquiry data of focal point resource */
--
2.14.2
Many kernel drivers contain code that allocates and frees both a
scatterlist and the pages that populate that scatterlist.
Introduce functions in lib/scatterlist.c that perform these tasks
instead of duplicating this functionality in multiple drivers.
Only include these functions in the build if CONFIG_SGL_ALLOC=y
to avoid that the kernel size increases if this functionality is
not used.
Signed-off-by: Bart Van Assche <[email protected]>
---
include/linux/scatterlist.h | 10 +++++
lib/Kconfig | 4 ++
lib/scatterlist.c | 105 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 119 insertions(+)
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 4b3286ac60c8..3642511918d5 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -266,6 +266,16 @@ int sg_alloc_table_from_pages(struct sg_table *sgt,
unsigned long offset, unsigned long size,
gfp_t gfp_mask);
+#ifdef CONFIG_SGL_ALLOC
+struct scatterlist *sgl_alloc_order(unsigned long long length,
+ unsigned int order, bool chainable,
+ gfp_t gfp, unsigned int *nent_p);
+struct scatterlist *sgl_alloc(unsigned long long length, gfp_t gfp,
+ unsigned int *nent_p);
+void sgl_free_order(struct scatterlist *sgl, int order);
+void sgl_free(struct scatterlist *sgl);
+#endif /* CONFIG_SGL_ALLOC */
+
size_t sg_copy_buffer(struct scatterlist *sgl, unsigned int nents, void *buf,
size_t buflen, off_t skip, bool to_buffer);
diff --git a/lib/Kconfig b/lib/Kconfig
index b1445b22a6de..8396c4cfa1ab 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -413,6 +413,10 @@ config HAS_DMA
depends on !NO_DMA
default y
+config SGL_ALLOC
+ bool
+ default n
+
config DMA_NOOP_OPS
bool
depends on HAS_DMA && (!64BIT || ARCH_DMA_ADDR_T_64BIT)
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index be7b4dd6b68d..e2b5a78ceb44 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -433,6 +433,111 @@ int sg_alloc_table_from_pages(struct sg_table *sgt,
}
EXPORT_SYMBOL(sg_alloc_table_from_pages);
+#ifdef CONFIG_SGL_ALLOC
+
+/**
+ * sgl_alloc_order - allocate a scatterlist and its pages
+ * @length: Length in bytes of the scatterlist. Must be at least one
+ * @order: Second argument for alloc_pages()
+ * @chainable: Whether or not to allocate an extra element in the scatterlist
+ * for scatterlist chaining purposes
+ * @gfp: Memory allocation flags
+ * @nent_p: [out] Number of entries in the scatterlist that have pages
+ *
+ * Returns: A pointer to an initialized scatterlist or %NULL upon failure.
+ */
+struct scatterlist *sgl_alloc_order(unsigned long long length,
+ unsigned int order, bool chainable,
+ gfp_t gfp, unsigned int *nent_p)
+{
+ struct scatterlist *sgl, *sg;
+ struct page *page;
+ unsigned int nent, nalloc;
+ u32 elem_len;
+
+ nent = round_up(length, PAGE_SIZE << order) >> (PAGE_SHIFT + order);
+ /* Check for integer overflow */
+ if (length > (nent << (PAGE_SHIFT + order)))
+ return NULL;
+ nalloc = nent;
+ if (chainable) {
+ /* Check for integer overflow */
+ if (nalloc + 1 < nalloc)
+ return NULL;
+ nalloc++;
+ }
+ sgl = kmalloc_array(nalloc, sizeof(struct scatterlist),
+ (gfp & ~GFP_DMA) | __GFP_ZERO);
+ if (!sgl)
+ return NULL;
+
+ sg_init_table(sgl, nent);
+ sg = sgl;
+ while (length) {
+ elem_len = min_t(u64, length, PAGE_SIZE << order);
+ page = alloc_pages(gfp, order);
+ if (!page) {
+ sgl_free(sgl);
+ return NULL;
+ }
+
+ sg_set_page(sg, page, elem_len, 0);
+ length -= elem_len;
+ sg = sg_next(sg);
+ }
+ WARN_ON_ONCE(sg);
+ if (nent_p)
+ *nent_p = nent;
+ return sgl;
+}
+EXPORT_SYMBOL(sgl_alloc_order);
+
+/**
+ * sgl_alloc - allocate a scatterlist and its pages
+ * @length: Length in bytes of the scatterlist
+ * @gfp: Memory allocation flags
+ * @nent_p: [out] Number of entries in the scatterlist
+ *
+ * Returns: A pointer to an initialized scatterlist or %NULL upon failure.
+ */
+struct scatterlist *sgl_alloc(unsigned long long length, gfp_t gfp,
+ unsigned int *nent_p)
+{
+ return sgl_alloc_order(length, 0, false, gfp, nent_p);
+}
+EXPORT_SYMBOL(sgl_alloc);
+
+/**
+ * sgl_free_order - free a scatterlist and its pages
+ * @sgl: Scatterlist with one or more elements
+ * @order: Second argument for __free_pages()
+ */
+void sgl_free_order(struct scatterlist *sgl, int order)
+{
+ struct scatterlist *sg;
+ struct page *page;
+
+ for (sg = sgl; sg; sg = sg_next(sg)) {
+ page = sg_page(sg);
+ if (page)
+ __free_pages(page, order);
+ }
+ kfree(sgl);
+}
+EXPORT_SYMBOL(sgl_free_order);
+
+/**
+ * sgl_free - free a scatterlist and its pages
+ * @sgl: Scatterlist with one or more elements
+ */
+void sgl_free(struct scatterlist *sgl)
+{
+ sgl_free_order(sgl, 0);
+}
+EXPORT_SYMBOL(sgl_free);
+
+#endif /* CONFIG_SGL_ALLOC */
+
void __sg_page_iter_start(struct sg_page_iter *piter,
struct scatterlist *sglist, unsigned int nents,
unsigned long pgoffset)
--
2.14.2
Use the sgl_alloc() and sgl_free() functions instead of open coding
these functions.
Signed-off-by: Bart Van Assche <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Herbert Xu <[email protected]>
---
crypto/Kconfig | 1 +
crypto/scompress.c | 51 ++-------------------------------------------------
2 files changed, 3 insertions(+), 49 deletions(-)
diff --git a/crypto/Kconfig b/crypto/Kconfig
index 0a121f9ddf8e..a0667dd284ff 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -105,6 +105,7 @@ config CRYPTO_KPP
config CRYPTO_ACOMP2
tristate
select CRYPTO_ALGAPI2
+ select SGL_ALLOC
config CRYPTO_ACOMP
tristate
diff --git a/crypto/scompress.c b/crypto/scompress.c
index 2075e2c4e7df..968bbcf65c94 100644
--- a/crypto/scompress.c
+++ b/crypto/scompress.c
@@ -140,53 +140,6 @@ static int crypto_scomp_init_tfm(struct crypto_tfm *tfm)
return ret;
}
-static void crypto_scomp_sg_free(struct scatterlist *sgl)
-{
- int i, n;
- struct page *page;
-
- if (!sgl)
- return;
-
- n = sg_nents(sgl);
- for_each_sg(sgl, sgl, n, i) {
- page = sg_page(sgl);
- if (page)
- __free_page(page);
- }
-
- kfree(sgl);
-}
-
-static struct scatterlist *crypto_scomp_sg_alloc(size_t size, gfp_t gfp)
-{
- struct scatterlist *sgl;
- struct page *page;
- int i, n;
-
- n = ((size - 1) >> PAGE_SHIFT) + 1;
-
- sgl = kmalloc_array(n, sizeof(struct scatterlist), gfp);
- if (!sgl)
- return NULL;
-
- sg_init_table(sgl, n);
-
- for (i = 0; i < n; i++) {
- page = alloc_page(gfp);
- if (!page)
- goto err;
- sg_set_page(sgl + i, page, PAGE_SIZE, 0);
- }
-
- return sgl;
-
-err:
- sg_mark_end(sgl + i);
- crypto_scomp_sg_free(sgl);
- return NULL;
-}
-
static int scomp_acomp_comp_decomp(struct acomp_req *req, int dir)
{
struct crypto_acomp *tfm = crypto_acomp_reqtfm(req);
@@ -220,7 +173,7 @@ static int scomp_acomp_comp_decomp(struct acomp_req *req, int dir)
scratch_dst, &req->dlen, *ctx);
if (!ret) {
if (!req->dst) {
- req->dst = crypto_scomp_sg_alloc(req->dlen, GFP_ATOMIC);
+ req->dst = sgl_alloc(req->dlen, GFP_ATOMIC, NULL);
if (!req->dst)
goto out;
}
@@ -274,7 +227,7 @@ int crypto_init_scomp_ops_async(struct crypto_tfm *tfm)
crt->compress = scomp_acomp_compress;
crt->decompress = scomp_acomp_decompress;
- crt->dst_free = crypto_scomp_sg_free;
+ crt->dst_free = sgl_free;
crt->reqsize = sizeof(void *);
return 0;
--
2.14.2
Signed-off-by: Bart Van Assche <[email protected]>
Reviewed-by: Johannes Thumshirn <[email protected]>
Cc: [email protected]
Cc: Martin K. Petersen <[email protected]>
Cc: Anil Ravindranath <[email protected]>
---
drivers/scsi/pmcraid.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/scsi/pmcraid.h b/drivers/scsi/pmcraid.h
index 8bfac72a242b..44da91712115 100644
--- a/drivers/scsi/pmcraid.h
+++ b/drivers/scsi/pmcraid.h
@@ -542,7 +542,6 @@ struct pmcraid_sglist {
u32 order;
u32 num_sg;
u32 num_dma_sg;
- u32 buffer_len;
struct scatterlist scatterlist[1];
};
--
2.14.2
Use the sgl_alloc() and sgl_free() functions instead of open coding
these functions.
Signed-off-by: Bart Van Assche <[email protected]>
Reviewed-by: Johannes Thumshirn <[email protected]>
Cc: Keith Busch <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: James Smart <[email protected]>
Cc: Sagi Grimberg <[email protected]>
---
drivers/nvme/target/Kconfig | 1 +
drivers/nvme/target/fc.c | 36 ++----------------------------------
2 files changed, 3 insertions(+), 34 deletions(-)
diff --git a/drivers/nvme/target/Kconfig b/drivers/nvme/target/Kconfig
index 03e4ab65fe77..4d9715630e21 100644
--- a/drivers/nvme/target/Kconfig
+++ b/drivers/nvme/target/Kconfig
@@ -39,6 +39,7 @@ config NVME_TARGET_FC
tristate "NVMe over Fabrics FC target driver"
depends on NVME_TARGET
depends on HAS_DMA
+ select SGL_ALLOC
help
This enables the NVMe FC target support, which allows exporting NVMe
devices over FC.
diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
index 58e010bdda3e..20f96038c272 100644
--- a/drivers/nvme/target/fc.c
+++ b/drivers/nvme/target/fc.c
@@ -1683,31 +1683,12 @@ static int
nvmet_fc_alloc_tgt_pgs(struct nvmet_fc_fcp_iod *fod)
{
struct scatterlist *sg;
- struct page *page;
unsigned int nent;
- u32 page_len, length;
- int i = 0;
- length = fod->total_length;
- nent = DIV_ROUND_UP(length, PAGE_SIZE);
- sg = kmalloc_array(nent, sizeof(struct scatterlist), GFP_KERNEL);
+ sg = sgl_alloc(fod->total_length, GFP_KERNEL, &nent);
if (!sg)
goto out;
- sg_init_table(sg, nent);
-
- while (length) {
- page_len = min_t(u32, length, PAGE_SIZE);
-
- page = alloc_page(GFP_KERNEL);
- if (!page)
- goto out_free_pages;
-
- sg_set_page(&sg[i], page, page_len, 0);
- length -= page_len;
- i++;
- }
-
fod->data_sg = sg;
fod->data_sg_cnt = nent;
fod->data_sg_cnt = fc_dma_map_sg(fod->tgtport->dev, sg, nent,
@@ -1717,14 +1698,6 @@ nvmet_fc_alloc_tgt_pgs(struct nvmet_fc_fcp_iod *fod)
return 0;
-out_free_pages:
- while (i > 0) {
- i--;
- __free_page(sg_page(&sg[i]));
- }
- kfree(sg);
- fod->data_sg = NULL;
- fod->data_sg_cnt = 0;
out:
return NVME_SC_INTERNAL;
}
@@ -1732,18 +1705,13 @@ nvmet_fc_alloc_tgt_pgs(struct nvmet_fc_fcp_iod *fod)
static void
nvmet_fc_free_tgt_pgs(struct nvmet_fc_fcp_iod *fod)
{
- struct scatterlist *sg;
- int count;
-
if (!fod->data_sg || !fod->data_sg_cnt)
return;
fc_dma_unmap_sg(fod->tgtport->dev, fod->data_sg, fod->data_sg_cnt,
((fod->io_dir == NVMET_FCP_WRITE) ?
DMA_FROM_DEVICE : DMA_TO_DEVICE));
- for_each_sg(fod->data_sg, sg, fod->data_sg_cnt, count)
- __free_page(sg_page(sg));
- kfree(fod->data_sg);
+ sgl_free(fod->data_sg);
fod->data_sg = NULL;
fod->data_sg_cnt = 0;
}
--
2.14.2
On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Many kernel drivers contain code that allocates and frees both a
> scatterlist and the pages that populate that scatterlist.
> Introduce functions in lib/scatterlist.c that perform these tasks
> instead of duplicating this functionality in multiple drivers.
> Only include these functions in the build if CONFIG_SGL_ALLOC=y
> to avoid that the kernel size increases if this functionality is
> not used.
>
> Signed-off-by: Bart Van Assche <[email protected]>
> ---
> include/linux/scatterlist.h | 10 +++++
> lib/Kconfig | 4 ++
> lib/scatterlist.c | 105 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 119 insertions(+)
>
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Use the sgl_alloc() and sgl_free() functions instead of open coding
> these functions.
>
> Signed-off-by: Bart Van Assche <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: Herbert Xu <[email protected]>
> ---
> crypto/Kconfig | 1 +
> crypto/scompress.c | 51 ++-------------------------------------------------
> 2 files changed, 3 insertions(+), 49 deletions(-)
> Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Use the sgl_alloc() and sgl_free() functions instead of open coding
> these functions.
>
> Signed-off-by: Bart Van Assche <[email protected]>
> Reviewed-by: Johannes Thumshirn <[email protected]>
> Cc: Keith Busch <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: James Smart <[email protected]>
> Cc: Sagi Grimberg <[email protected]>
> ---
> drivers/nvme/target/Kconfig | 1 +
> drivers/nvme/target/fc.c | 36 ++----------------------------------
> 2 files changed, 3 insertions(+), 34 deletions(-)
>
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Use the sgl_alloc() and sgl_free() functions instead of open coding
> these functions.
>
> Signed-off-by: Bart Van Assche <[email protected]>
> Reviewed-by: Johannes Thumshirn <[email protected]>
> Cc: Keith Busch <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Sagi Grimberg <[email protected]>
> ---
> drivers/nvme/target/Kconfig | 1 +
> drivers/nvme/target/rdma.c | 63 +++------------------------------------------
> 2 files changed, 5 insertions(+), 59 deletions(-)
>
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Use the sgl_alloc_order() and sgl_free() functions instead of open
> coding these functions.
>
> Signed-off-by: Bart Van Assche <[email protected]>
> Cc: Nicholas A. Bellinger <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Hannes Reinecke <[email protected]>
> Cc: Sagi Grimberg <[email protected]>
> ---
> drivers/target/Kconfig | 1 +
> drivers/target/target_core_transport.c | 46 +++-------------------------------
> 2 files changed, 5 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/target/Kconfig b/drivers/target/Kconfig
> index e2bc99980f75..4c44d7bed01a 100644
> --- a/drivers/target/Kconfig
> +++ b/drivers/target/Kconfig
> @@ -5,6 +5,7 @@ menuconfig TARGET_CORE
> select CONFIGFS_FS
> select CRC_T10DIF
> select BLK_SCSI_REQUEST # only for scsi_command_size_tbl..
> + select SGL_ALLOC
> default n
> help
> Say Y or M here to enable the TCM Storage Engine and ConfigFS enabled
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 836d552b0385..9bbd08be9d60 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2293,13 +2293,7 @@ static void target_complete_ok_work(struct work_struct *work)
>
> void target_free_sgl(struct scatterlist *sgl, int nents)
> {
> - struct scatterlist *sg;
> - int count;
> -
> - for_each_sg(sgl, sg, nents, count)
> - __free_page(sg_page(sg));
> -
> - kfree(sgl);
> + sgl_free(sgl);
> }
> EXPORT_SYMBOL(target_free_sgl);
>
> @@ -2423,42 +2417,10 @@ int
> target_alloc_sgl(struct scatterlist **sgl, unsigned int *nents, u32 length,
> bool zero_page, bool chainable)
> {
> - struct scatterlist *sg;
> - struct page *page;
> - gfp_t zero_flag = (zero_page) ? __GFP_ZERO : 0;
> - unsigned int nalloc, nent;
> - int i = 0;
> -
> - nalloc = nent = DIV_ROUND_UP(length, PAGE_SIZE);
> - if (chainable)
> - nalloc++;
> - sg = kmalloc_array(nalloc, sizeof(struct scatterlist), GFP_KERNEL);
> - if (!sg)
> - return -ENOMEM;
> + gfp_t gfp = GFP_KERNEL | (zero_page ? __GFP_ZERO : 0);
>
> - sg_init_table(sg, nalloc);
> -
> - while (length) {
> - u32 page_len = min_t(u32, length, PAGE_SIZE);
> - page = alloc_page(GFP_KERNEL | zero_flag);
> - if (!page)
> - goto out;
> -
> - sg_set_page(&sg[i], page, page_len, 0);
> - length -= page_len;
> - i++;
> - }
> - *sgl = sg;
> - *nents = nent;
> - return 0;
> -
> -out:
> - while (i > 0) {
> - i--;
> - __free_page(sg_page(&sg[i]));
> - }
> - kfree(sg);
> - return -ENOMEM;
> + *sgl = sgl_alloc_order(length, 0, chainable, gfp, nents);
> + return *sgl ? 0 : -ENOMEM;
> }
> EXPORT_SYMBOL(target_alloc_sgl);
>
> The calling convention from target_alloc_sgl() is decidedly dodgy.
Can't we convert it into returning the sgl, and remove the first parameter?
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Use the sgl_alloc_order() and sgl_free_order() functions instead
> of open coding these functions.
>
> Signed-off-by: Bart Van Assche <[email protected]>
> Reviewed-by: Johannes Thumshirn <[email protected]>
> Cc: [email protected]
> Cc: Martin K. Petersen <[email protected]>
> Cc: Brian King <[email protected]>
> ---
> drivers/scsi/Kconfig | 1 +
> drivers/scsi/ipr.c | 49 ++++++++-----------------------------------------
> drivers/scsi/ipr.h | 2 +-
> 3 files changed, 10 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index 41366339b950..d11e75e76195 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -1058,6 +1058,7 @@ config SCSI_IPR
> depends on PCI && SCSI && ATA
> select FW_LOADER
> select IRQ_POLL
> + select SGL_ALLOC
> ---help---
> This driver supports the IBM Power Linux family RAID adapters.
> This includes IBM pSeries 5712, 5703, 5709, and 570A, as well
> diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
> index f838bd73befa..6fed5db6255e 100644
> --- a/drivers/scsi/ipr.c
> +++ b/drivers/scsi/ipr.c
> @@ -3815,10 +3815,8 @@ static struct device_attribute ipr_iopoll_weight_attr = {
> **/
> static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
> {
> - int sg_size, order, bsize_elem, num_elem, i, j;
> + int sg_size, order;
> struct ipr_sglist *sglist;
> - struct scatterlist *scatterlist;
> - struct page *page;
>
> /* Get the minimum size per scatter/gather element */
> sg_size = buf_len / (IPR_MAX_SGLIST - 1);
> @@ -3826,45 +3824,18 @@ static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
> /* Get the actual size per element */
> order = get_order(sg_size);
>
> - /* Determine the actual number of bytes per element */
> - bsize_elem = PAGE_SIZE * (1 << order);
> -
> - /* Determine the actual number of sg entries needed */
> - if (buf_len % bsize_elem)
> - num_elem = (buf_len / bsize_elem) + 1;
> - else
> - num_elem = buf_len / bsize_elem;
> -
> /* Allocate a scatter/gather list for the DMA */
> - sglist = kzalloc(sizeof(struct ipr_sglist) +
> - (sizeof(struct scatterlist) * (num_elem - 1)),
> - GFP_KERNEL);
> -
> + sglist = kzalloc(sizeof(struct ipr_sglist), GFP_KERNEL);
> if (sglist == NULL) {
> ipr_trace;
> return NULL;
> }
> -
> - scatterlist = sglist->scatterlist;
> - sg_init_table(scatterlist, num_elem);
> -
> sglist->order = order;
> - sglist->num_sg = num_elem;
> -
> - /* Allocate a bunch of sg elements */
> - for (i = 0; i < num_elem; i++) {
> - page = alloc_pages(GFP_KERNEL, order);
> - if (!page) {
> - ipr_trace;
> -
> - /* Free up what we already allocated */
> - for (j = i - 1; j >= 0; j--)
> - __free_pages(sg_page(&scatterlist[j]), order);
> - kfree(sglist);
> - return NULL;
> - }
> -
> - sg_set_page(&scatterlist[i], page, 0, 0);
> + sglist->scatterlist = sgl_alloc_order(buf_len, order, false, GFP_KERNEL,
> + &sglist->num_sg);
> + if (!sglist->scatterlist) {
> + kfree(sglist);
> + return NULL;
> }
>
> return sglist;
> @@ -3882,11 +3853,7 @@ static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
> **/
> static void ipr_free_ucode_buffer(struct ipr_sglist *sglist)
> {
> - int i;
> -
> - for (i = 0; i < sglist->num_sg; i++)
> - __free_pages(sg_page(&sglist->scatterlist[i]), sglist->order);
> -
> + sgl_free_order(sglist->scatterlist, sglist->order);
> kfree(sglist);
> }
>
> diff --git a/drivers/scsi/ipr.h b/drivers/scsi/ipr.h
> index c7f0e9e3cd7d..93570734cbfb 100644
> --- a/drivers/scsi/ipr.h
> +++ b/drivers/scsi/ipr.h
> @@ -1454,7 +1454,7 @@ struct ipr_sglist {
> u32 num_sg;
> u32 num_dma_sg;
> u32 buffer_len;
> - struct scatterlist scatterlist[1];
> + struct scatterlist *scatterlist;
> };
>
> enum ipr_sdt_state {
>
Not sure if this is a valid conversion.
Originally the driver would allocate a single buffer; with this buffer
we have two distinct buffers.
Given that this is used to download the microcode I'm not sure if this
isn't a hardware-dependent structure which requires a single buffer
including the sglist.
Brian, can you shed some light here?
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Signed-off-by: Bart Van Assche <[email protected]>
> Reviewed-by: Johannes Thumshirn <[email protected]>
> Cc: [email protected]
> Cc: Martin K. Petersen <[email protected]>
> Cc: Anil Ravindranath <[email protected]>
> ---
> drivers/scsi/pmcraid.h | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/scsi/pmcraid.h b/drivers/scsi/pmcraid.h
> index 8bfac72a242b..44da91712115 100644
> --- a/drivers/scsi/pmcraid.h
> +++ b/drivers/scsi/pmcraid.h
> @@ -542,7 +542,6 @@ struct pmcraid_sglist {
> u32 order;
> u32 num_sg;
> u32 num_dma_sg;
> - u32 buffer_len;
> struct scatterlist scatterlist[1];
> };
>
>
This actually is the same story that we've had with ipr (and, looking at
the code, those two drivers look awfully similar ...).
pmcraid_sglist looks as if it's a hardware-dependent structure, so just
removing one entry from the middle of a structure might not be a good idea.
But this is something for the pmcraid folks to clarify.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Use the sgl_alloc_order() and sgl_free_order() functions instead
> of open coding these functions.
>
> Signed-off-by: Bart Van Assche <[email protected]>
> Reviewed-by: Johannes Thumshirn <[email protected]>
> Cc: [email protected]
> Cc: Martin K. Petersen <[email protected]>
> Cc: Anil Ravindranath <[email protected]>
> ---
> drivers/scsi/Kconfig | 1 +
> drivers/scsi/pmcraid.c | 43 ++++---------------------------------------
> drivers/scsi/pmcraid.h | 2 +-
> 3 files changed, 6 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index d11e75e76195..632200ec36a6 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -1576,6 +1576,7 @@ config ZFCP
> config SCSI_PMCRAID
> tristate "PMC SIERRA Linux MaxRAID adapter support"
> depends on PCI && SCSI && NET
> + select SGL_ALLOC
> ---help---
> This driver supports the PMC SIERRA MaxRAID adapters.
>
> diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
> index b4d6cd8cd1ad..95fc0352f9bb 100644
> --- a/drivers/scsi/pmcraid.c
> +++ b/drivers/scsi/pmcraid.c
> @@ -3232,12 +3232,7 @@ static int pmcraid_build_ioadl(
> */
> static void pmcraid_free_sglist(struct pmcraid_sglist *sglist)
> {
> - int i;
> -
> - for (i = 0; i < sglist->num_sg; i++)
> - __free_pages(sg_page(&(sglist->scatterlist[i])),
> - sglist->order);
> -
> + sgl_free_order(sglist->scatterlist, sglist->order);
> kfree(sglist);
> }
>
> @@ -3254,50 +3249,20 @@ static void pmcraid_free_sglist(struct pmcraid_sglist *sglist)
> static struct pmcraid_sglist *pmcraid_alloc_sglist(int buflen)
> {
> struct pmcraid_sglist *sglist;
> - struct scatterlist *scatterlist;
> - struct page *page;
> - int num_elem, i, j;
> int sg_size;
> int order;
> - int bsize_elem;
>
> sg_size = buflen / (PMCRAID_MAX_IOADLS - 1);
> order = (sg_size > 0) ? get_order(sg_size) : 0;
> - bsize_elem = PAGE_SIZE * (1 << order);
> -
> - /* Determine the actual number of sg entries needed */
> - if (buflen % bsize_elem)
> - num_elem = (buflen / bsize_elem) + 1;
> - else
> - num_elem = buflen / bsize_elem;
>
> /* Allocate a scatter/gather list for the DMA */
> - sglist = kzalloc(sizeof(struct pmcraid_sglist) +
> - (sizeof(struct scatterlist) * (num_elem - 1)),
> - GFP_KERNEL);
> -
> + sglist = kzalloc(sizeof(struct pmcraid_sglist), GFP_KERNEL);
> if (sglist == NULL)
> return NULL;
>
> - scatterlist = sglist->scatterlist;
> - sg_init_table(scatterlist, num_elem);
> sglist->order = order;
> - sglist->num_sg = num_elem;
> - sg_size = buflen;
> -
> - for (i = 0; i < num_elem; i++) {
> - page = alloc_pages(GFP_KERNEL|GFP_DMA|__GFP_ZERO, order);
> - if (!page) {
> - for (j = i - 1; j >= 0; j--)
> - __free_pages(sg_page(&scatterlist[j]), order);
> - kfree(sglist);
> - return NULL;
> - }
> -
> - sg_set_page(&scatterlist[i], page,
> - sg_size < bsize_elem ? sg_size : bsize_elem, 0);
> - sg_size -= bsize_elem;
> - }
> + sgl_alloc_order(buflen, order, false,
> + GFP_KERNEL | GFP_DMA | __GFP_ZERO, &sglist->num_sg);
>
> return sglist;
> }
> diff --git a/drivers/scsi/pmcraid.h b/drivers/scsi/pmcraid.h
> index 44da91712115..754ef30927e2 100644
> --- a/drivers/scsi/pmcraid.h
> +++ b/drivers/scsi/pmcraid.h
> @@ -542,7 +542,7 @@ struct pmcraid_sglist {
> u32 order;
> u32 num_sg;
> u32 num_dma_sg;
> - struct scatterlist scatterlist[1];
> + struct scatterlist *scatterlist;
> };
>
> /* page D0 inquiry data of focal point resource */
>
The comment to ipr applied here, too.
We need input from the pmcraid folks if this is a valid conversion.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
Thanks Bart,
Reviewed-by: Johannes Thumshirn <[email protected]>
--
Johannes Thumshirn Storage
[email protected] +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 Tue, 2017-10-17 at 08:21 +0200, Hannes Reinecke wrote:
> On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> > Signed-off-by: Bart Van Assche <[email protected]>
> > Reviewed-by: Johannes Thumshirn <[email protected]>
> > Cc: [email protected]
> > Cc: Martin K. Petersen <[email protected]>
> > Cc: Anil Ravindranath <[email protected]>
> > ---
> > drivers/scsi/pmcraid.h | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/scsi/pmcraid.h b/drivers/scsi/pmcraid.h
> > index 8bfac72a242b..44da91712115 100644
> > --- a/drivers/scsi/pmcraid.h
> > +++ b/drivers/scsi/pmcraid.h
> > @@ -542,7 +542,6 @@ struct pmcraid_sglist {
> > u32 order;
> > u32 num_sg;
> > u32 num_dma_sg;
> > - u32 buffer_len;
> > struct scatterlist scatterlist[1];
> > };
> >
> >
>
> This actually is the same story that we've had with ipr (and, looking at
> the code, those two drivers look awfully similar ...).
> pmcraid_sglist looks as if it's a hardware-dependent structure, so just
> removing one entry from the middle of a structure might not be a good idea.
> But this is something for the pmcraid folks to clarify.
Hello Hannes,
Sorry but I don't see how a structure that contains a struct scatterlist
could be hardware-dependent?
Thanks,
Bart.
On Tue, 2017-10-17 at 08:14 +0200, Hannes Reinecke wrote:
> On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> > [ ... ]
> > void target_free_sgl(struct scatterlist *sgl, int nents)
> > {
> > - struct scatterlist *sg;
> > - int count;
> > -
> > - for_each_sg(sgl, sg, nents, count)
> > - __free_page(sg_page(sg));
> > -
> > - kfree(sgl);
> > + sgl_free(sgl);
> > }
> > EXPORT_SYMBOL(target_free_sgl);
> >
> > @@ -2423,42 +2417,10 @@ int
> > target_alloc_sgl(struct scatterlist **sgl, unsigned int *nents, u32 length,
> > bool zero_page, bool chainable)
> > {
> > - [ ... ]
> > + *sgl = sgl_alloc_order(length, 0, chainable, gfp, nents);
> > + return *sgl ? 0 : -ENOMEM;
> > }
> > EXPORT_SYMBOL(target_alloc_sgl);
> >
> > The calling convention from target_alloc_sgl() is decidedly dodgy.
>
> Can't we convert it into returning the sgl, and remove the first parameter?
Hello Hannes,
Another option is to remove the target_alloc_sgl() and target_free_sgl() functions
and to make LIO target drivers call sgl_alloc_order() and sgl_free_order() directly.
Do you perhaps have a preference for one of these two approaches?
Thanks,
Bart.
On 10/17/2017 01:19 AM, Hannes Reinecke wrote:
> On 10/17/2017 12:49 AM, Bart Van Assche wrote:
>> Use the sgl_alloc_order() and sgl_free_order() functions instead
>> of open coding these functions.
>>
>> Signed-off-by: Bart Van Assche <[email protected]>
>> Reviewed-by: Johannes Thumshirn <[email protected]>
>> Cc: [email protected]
>> Cc: Martin K. Petersen <[email protected]>
>> Cc: Brian King <[email protected]>
>> ---
>> drivers/scsi/Kconfig | 1 +
>> drivers/scsi/ipr.c | 49 ++++++++-----------------------------------------
>> drivers/scsi/ipr.h | 2 +-
>> 3 files changed, 10 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
>> index 41366339b950..d11e75e76195 100644
>> --- a/drivers/scsi/Kconfig
>> +++ b/drivers/scsi/Kconfig
>> @@ -1058,6 +1058,7 @@ config SCSI_IPR
>> depends on PCI && SCSI && ATA
>> select FW_LOADER
>> select IRQ_POLL
>> + select SGL_ALLOC
>> ---help---
>> This driver supports the IBM Power Linux family RAID adapters.
>> This includes IBM pSeries 5712, 5703, 5709, and 570A, as well
>> diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
>> index f838bd73befa..6fed5db6255e 100644
>> --- a/drivers/scsi/ipr.c
>> +++ b/drivers/scsi/ipr.c
>> @@ -3815,10 +3815,8 @@ static struct device_attribute ipr_iopoll_weight_attr = {
>> **/
>> static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
>> {
>> - int sg_size, order, bsize_elem, num_elem, i, j;
>> + int sg_size, order;
>> struct ipr_sglist *sglist;
>> - struct scatterlist *scatterlist;
>> - struct page *page;
>>
>> /* Get the minimum size per scatter/gather element */
>> sg_size = buf_len / (IPR_MAX_SGLIST - 1);
>> @@ -3826,45 +3824,18 @@ static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
>> /* Get the actual size per element */
>> order = get_order(sg_size);
>>
>> - /* Determine the actual number of bytes per element */
>> - bsize_elem = PAGE_SIZE * (1 << order);
>> -
>> - /* Determine the actual number of sg entries needed */
>> - if (buf_len % bsize_elem)
>> - num_elem = (buf_len / bsize_elem) + 1;
>> - else
>> - num_elem = buf_len / bsize_elem;
>> -
>> /* Allocate a scatter/gather list for the DMA */
>> - sglist = kzalloc(sizeof(struct ipr_sglist) +
>> - (sizeof(struct scatterlist) * (num_elem - 1)),
>> - GFP_KERNEL);
>> -
>> + sglist = kzalloc(sizeof(struct ipr_sglist), GFP_KERNEL);
>> if (sglist == NULL) {
>> ipr_trace;
>> return NULL;
>> }
>> -
>> - scatterlist = sglist->scatterlist;
>> - sg_init_table(scatterlist, num_elem);
>> -
>> sglist->order = order;
>> - sglist->num_sg = num_elem;
>> -
>> - /* Allocate a bunch of sg elements */
>> - for (i = 0; i < num_elem; i++) {
>> - page = alloc_pages(GFP_KERNEL, order);
>> - if (!page) {
>> - ipr_trace;
>> -
>> - /* Free up what we already allocated */
>> - for (j = i - 1; j >= 0; j--)
>> - __free_pages(sg_page(&scatterlist[j]), order);
>> - kfree(sglist);
>> - return NULL;
>> - }
>> -
>> - sg_set_page(&scatterlist[i], page, 0, 0);
>> + sglist->scatterlist = sgl_alloc_order(buf_len, order, false, GFP_KERNEL,
>> + &sglist->num_sg);
>> + if (!sglist->scatterlist) {
>> + kfree(sglist);
>> + return NULL;
>> }
>>
>> return sglist;
>> @@ -3882,11 +3853,7 @@ static struct ipr_sglist *ipr_alloc_ucode_buffer(int buf_len)
>> **/
>> static void ipr_free_ucode_buffer(struct ipr_sglist *sglist)
>> {
>> - int i;
>> -
>> - for (i = 0; i < sglist->num_sg; i++)
>> - __free_pages(sg_page(&sglist->scatterlist[i]), sglist->order);
>> -
>> + sgl_free_order(sglist->scatterlist, sglist->order);
>> kfree(sglist);
>> }
>>
>> diff --git a/drivers/scsi/ipr.h b/drivers/scsi/ipr.h
>> index c7f0e9e3cd7d..93570734cbfb 100644
>> --- a/drivers/scsi/ipr.h
>> +++ b/drivers/scsi/ipr.h
>> @@ -1454,7 +1454,7 @@ struct ipr_sglist {
>> u32 num_sg;
>> u32 num_dma_sg;
>> u32 buffer_len;
>> - struct scatterlist scatterlist[1];
>> + struct scatterlist *scatterlist;
>> };
>>
>> enum ipr_sdt_state {
>>
> Not sure if this is a valid conversion.
> Originally the driver would allocate a single buffer; with this buffer
> we have two distinct buffers.
> Given that this is used to download the microcode I'm not sure if this
> isn't a hardware-dependent structure which requires a single buffer
> including the sglist.
> Brian, can you shed some light here?
The struct ipr_sglist is not a hardware defined data structure, so on initial
glance, this should be OK. I'll load it up and give it a try to make sure
it doesn't break code download.
Thanks,
Brian
--
Brian King
Power Linux I/O
IBM Linux Technology Center
On Wed, 2017-10-18 at 15:57 -0500, Brian King wrote:
> On 10/17/2017 01:19 AM, Hannes Reinecke wrote:
> > On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> > > [ ... ]
> >
> > Not sure if this is a valid conversion.
> > Originally the driver would allocate a single buffer; with this buffer
> > we have two distinct buffers.
> > Given that this is used to download the microcode I'm not sure if this
> > isn't a hardware-dependent structure which requires a single buffer
> > including the sglist.
> > Brian, can you shed some light here?
>
> The struct ipr_sglist is not a hardware defined data structure, so on initial
> glance, this should be OK. I'll load it up and give it a try to make sure
> it doesn't break code download.
Hello Brian,
Have you already obtained any test results?
Thanks,
Bart.
On 10/30/2017 03:37 PM, Bart Van Assche wrote:
> On Wed, 2017-10-18 at 15:57 -0500, Brian King wrote:
>> On 10/17/2017 01:19 AM, Hannes Reinecke wrote:
>>> On 10/17/2017 12:49 AM, Bart Van Assche wrote:
>>>> [ ... ]
>>>
>>> Not sure if this is a valid conversion.
>>> Originally the driver would allocate a single buffer; with this buffer
>>> we have two distinct buffers.
>>> Given that this is used to download the microcode I'm not sure if this
>>> isn't a hardware-dependent structure which requires a single buffer
>>> including the sglist.
>>> Brian, can you shed some light here?
>>
>> The struct ipr_sglist is not a hardware defined data structure, so on initial
>> glance, this should be OK. I'll load it up and give it a try to make sure
>> it doesn't break code download.
>
> Hello Brian,
>
> Have you already obtained any test results?
Bart,
Yes. I tried this out on an ipr adapter and it looks fine.
Acked-by: Brian King <[email protected]>
Thanks,
Brian
--
Brian King
Power Linux I/O
IBM Linux Technology Center
On 10/30/2017 10:01 PM, Brian King wrote:
> On 10/30/2017 03:37 PM, Bart Van Assche wrote:
>> On Wed, 2017-10-18 at 15:57 -0500, Brian King wrote:
>>> On 10/17/2017 01:19 AM, Hannes Reinecke wrote:
>>>> On 10/17/2017 12:49 AM, Bart Van Assche wrote:
>>>>> [ ... ]
>>>>
>>>> Not sure if this is a valid conversion.
>>>> Originally the driver would allocate a single buffer; with this buffer
>>>> we have two distinct buffers.
>>>> Given that this is used to download the microcode I'm not sure if this
>>>> isn't a hardware-dependent structure which requires a single buffer
>>>> including the sglist.
>>>> Brian, can you shed some light here?
>>>
>>> The struct ipr_sglist is not a hardware defined data structure, so on initial
>>> glance, this should be OK. I'll load it up and give it a try to make sure
>>> it doesn't break code download.
>>
>> Hello Brian,
>>
>> Have you already obtained any test results?
>
> Bart,
>
> Yes. I tried this out on an ipr adapter and it looks fine.
>
> Acked-by: Brian King <[email protected]>
>
Thanks for the confirmation.
Bart, you can add my
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
On Mon, 2017-10-16 at 15:49 -0700, Bart Van Assche wrote:
> Use the sgl_alloc() and sgl_free() functions instead of open coding
> these functions.
>
> Signed-off-by: Bart Van Assche <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: Herbert Xu <[email protected]>
Ard and/or Herbert, can you please have a look at this patch and let us know
whether or not it looks fine to you?
Thanks,
Bart.
On 1 November 2017 at 14:50, Bart Van Assche <[email protected]> wrote:
> On Mon, 2017-10-16 at 15:49 -0700, Bart Van Assche wrote:
>> Use the sgl_alloc() and sgl_free() functions instead of open coding
>> these functions.
>>
>> Signed-off-by: Bart Van Assche <[email protected]>
>> Cc: Ard Biesheuvel <[email protected]>
>> Cc: Herbert Xu <[email protected]>
>
> Ard and/or Herbert, can you please have a look at this patch and let us know
> whether or not it looks fine to you?
>
The patch itself does not look unreasonable, but I can't find
sgl_alloc() anywhere in the source tree. Given that you have cc'ed me
on this patch only, I can only assume that you are adding this as part
of the series, but without any context, I can't really review this,
sorry.
On Wed, 2017-11-01 at 15:17 +0000, Ard Biesheuvel wrote:
> On 1 November 2017 at 14:50, Bart Van Assche <[email protected]> wrote:
> > On Mon, 2017-10-16 at 15:49 -0700, Bart Van Assche wrote:
> > > Use the sgl_alloc() and sgl_free() functions instead of open coding
> > > these functions.
> > >
> > > Signed-off-by: Bart Van Assche <[email protected]>
> > > Cc: Ard Biesheuvel <[email protected]>
> > > Cc: Herbert Xu <[email protected]>
> >
> > Ard and/or Herbert, can you please have a look at this patch and let us know
> > whether or not it looks fine to you?
>
> The patch itself does not look unreasonable, but I can't find
> sgl_alloc() anywhere in the source tree. Given that you have cc'ed me
> on this patch only, I can only assume that you are adding this as part
> of the series, but without any context, I can't really review this,
> sorry.
Hello Ard,
Do you expect to be Cc-ed personally or is Cc-ing the linux-crypto mailing
list sufficient? The linux-crypto mailing list was Cc-ed for the entire patch
series as one can see here:
https://www.mail-archive.com/[email protected]/msg28485.html.
Thanks,
Bart.
On 1 November 2017 at 15:45, Bart Van Assche <[email protected]> wrote:
> On Wed, 2017-11-01 at 15:17 +0000, Ard Biesheuvel wrote:
>> On 1 November 2017 at 14:50, Bart Van Assche <[email protected]> wrote:
>> > On Mon, 2017-10-16 at 15:49 -0700, Bart Van Assche wrote:
>> > > Use the sgl_alloc() and sgl_free() functions instead of open coding
>> > > these functions.
>> > >
>> > > Signed-off-by: Bart Van Assche <[email protected]>
>> > > Cc: Ard Biesheuvel <[email protected]>
>> > > Cc: Herbert Xu <[email protected]>
>> >
>> > Ard and/or Herbert, can you please have a look at this patch and let us know
>> > whether or not it looks fine to you?
>>
>> The patch itself does not look unreasonable, but I can't find
>> sgl_alloc() anywhere in the source tree. Given that you have cc'ed me
>> on this patch only, I can only assume that you are adding this as part
>> of the series, but without any context, I can't really review this,
>> sorry.
>
> Hello Ard,
>
> Do you expect to be Cc-ed personally or is Cc-ing the linux-crypto mailing
> list sufficient? The linux-crypto mailing list was Cc-ed for the entire patch
> series as one can see here:
> https://www.mail-archive.com/[email protected]/msg28485.html.
>
I guess people's opinions may differ regarding what they want to be
cc'ed on, but in general, you should at least cc everyone on the cover
letter if you cc them on individual patches, and in my case, I'd
rather have the whole series even if only a single patch is relevant
to me.