scatterlist: add sg_alloc_table_from_pages function

PATCH Superseded Tomasz Stanislawski 4 participants 7 messages May 8, 2012 – Jun 6, 2012

Subsystems (3)

Memory management Media (V4L/DVB) Library code

lore

Share & tools

lore

Tomasz Stanislawski

author

2012-05-08 09:50 UTC #0 lore raw

PATCH v3 1/1 scatterlist: add sg_alloc_table_from_pages function

This patch adds a new constructor for an sg table. The table is constructed from an array of struct pages. All contiguous chunks of the pages are merged into a single sg nodes. A user may provide an offset and a size of a buffer if the buffer is not page-aligned.

The function is dedicated for DMABUF exporters which often perform conversion from an page array to a scatterlist. Moreover the scatterlist should be squashed in order to save memory and to speed-up the process of DMA mapping using dma_map_sg.

The code is based on the patch 'v4l: vb2-dma-contig: add support for scatterlist in userptr mode' and hints from Laurent Pinchart.

Signed-off-by Tomasz Stanislawski <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 include/linux/scatterlist.h |    4 +++
 lib/scatterlist.c           |   64 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 0 deletions(-)

include/linux/scatterlist.h +4 -0

index ac9586d..7b600da 100644
@@ -214,6 +214,10 @@ void sg_free_table(struct sg_table *);
 int __sg_alloc_table(struct sg_table *, unsigned int, unsigned int, gfp_t,
 		     sg_alloc_fn *);
 int sg_alloc_table(struct sg_table *, unsigned int, gfp_t);
+int sg_alloc_table_from_pages(struct sg_table *sgt,
+	struct page **pages, unsigned int n_pages,
+	unsigned long offset, unsigned long size,
+	gfp_t gfp_mask);
 
 size_t sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
 			   void *buf, size_t buflen);

lib/scatterlist.c +64 -0

70 lines not on this page · raw message

Signature · 1 line

1.7.5.4

Daniel Vetter

Acked-by on v3 1/1

2012-05-08 10:01 UTC #1 lore raw

Quoted 10 lines · Tomasz Stanislawski

This patch adds a new constructor for an sg table. The table is constructed from an array of struct pages. All contiguous chunks of the pages are merged into a single sg nodes. A user may provide an offset and a size of a buffer if the buffer is not page-aligned.
The function is dedicated for DMABUF exporters which often perform conversion from an page array to a scatterlist. Moreover the scatterlist should be squashed in order to save memory and to speed-up the process of DMA mapping using dma_map_sg.

Quoted 5 lines

The code is based on the patch 'v4l: vb2-dma-contig: add support for scatterlist in userptr mode' and hints from Laurent Pinchart.
Signed-off-by Tomasz Stanislawski <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>

Looks neat and useful and we could use this to kill the copy in drm_prime.c. So

Acked-by Daniel Vetter <[email protected]>

Quoted 102 lines

---
 include/linux/scatterlist.h |    4 +++
 lib/scatterlist.c           |   64 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 0 deletions(-)

include/linux/scatterlist.h +4 -0

index ac9586d..7b600da 100644
@@ -214,6 +214,10 @@ void sg_free_table(struct sg_table *);
 int __sg_alloc_table(struct sg_table *, unsigned int, unsigned int, gfp_t,
 		     sg_alloc_fn *);
 int sg_alloc_table(struct sg_table *, unsigned int, gfp_t);
+int sg_alloc_table_from_pages(struct sg_table *sgt,
+	struct page **pages, unsigned int n_pages,
+	unsigned long offset, unsigned long size,
+	gfp_t gfp_mask);
 
 size_t sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
 			   void *buf, size_t buflen);

lib/scatterlist.c +64 -0

index 6096e89..85868a1 100644
@@ -319,6 +319,70 @@ int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask)
 EXPORT_SYMBOL(sg_alloc_table);
 
 /**
+ * sg_alloc_table_from_pages - Allocate and initialize an sg table from
+ *			       an array of pages
+ * @sgt:	The sg table header to use
+ * @pages:	Pointer to an array of page pointers
+ * @n_pages:	Number of pages in the pages array
+ * @offset:     Offset from start of the first page to the start of a buffer
+ * @size:       Number of valid bytes in the buffer (after offset)
+ * @gfp_mask:	GFP allocation mask
+ *
+ *  Description:
+ *    Allocate and initialize an sg table from a list of pages. Continuous
+ *    ranges of the pages are squashed into a single scatterlist node. A user
+ *    may provide an offset at a start and a size of valid data in a buffer
+ *    specified by the page array. The returned sg table is released by
+ *    sg_free_table.
+ *
+ * Returns:
+ *   0 on success, negative error on failure
+ **/
+int sg_alloc_table_from_pages(struct sg_table *sgt,
+	struct page **pages, unsigned int n_pages,
+	unsigned long offset, unsigned long size,
+	gfp_t gfp_mask)
+{
+	unsigned int chunks;
+	unsigned int i;
+	unsigned int cur_page;
+	int ret;
+	struct scatterlist *s;
+
+	/* compute number of contiguous chunks */
+	chunks = 1;
+	for (i = 1; i < n_pages; ++i)
+		if (page_to_pfn(pages[i]) != page_to_pfn(pages[i - 1]) + 1)
+			++chunks;
+
+	ret = sg_alloc_table(sgt, chunks, gfp_mask);
+	if (unlikely(ret))
+		return ret;
+
+	/* merging chunks and putting them into the scatterlist */
+	cur_page = 0;
+	for_each_sg(sgt->sgl, s, sgt->orig_nents, i) {
+		unsigned long chunk_size;
+		unsigned int j;
+
+		/* looking for the end of the current chunk */
+		for (j = cur_page + 1; j < n_pages; ++j)
+			if (page_to_pfn(pages[j]) !=
+			    page_to_pfn(pages[j - 1]) + 1)
+				break;
+
+		chunk_size = ((j - cur_page) << PAGE_SHIFT) - offset;
+		sg_set_page(s, pages[cur_page], min(size, chunk_size), offset);
+		size -= chunk_size;
+		offset = 0;
+		cur_page = j;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(sg_alloc_table_from_pages);
+
+/**
  * sg_miter_start - start mapping iteration over a sg list
  * @miter: sg mapping iter to be started
  * @sgl: sg list to iterate over

Signature · 6 lines

1.7.5.4

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Signature · 3 lines

Daniel Vetter
Mail: [email protected]
Mobile: +41 (0)79 365 57 48

Laurent Pinchart

Acked-by on v3 1/1

2012-05-08 11:14 UTC #2 lore raw

In reply to Tomasz Stanislawski

Hi Tomasz,

Thank you for the patch.

Quoted 10 lines · On Tuesday 08 May 2012 11:50:33 Tomasz Stanislawski wrote:

This patch adds a new constructor for an sg table. The table is constructed from an array of struct pages. All contiguous chunks of the pages are merged into a single sg nodes. A user may provide an offset and a size of a buffer if the buffer is not page-aligned.
The function is dedicated for DMABUF exporters which often perform conversion from an page array to a scatterlist. Moreover the scatterlist should be squashed in order to save memory and to speed-up the process of DMA mapping using dma_map_sg.

Quoted 5 lines

The code is based on the patch 'v4l: vb2-dma-contig: add support for scatterlist in userptr mode' and hints from Laurent Pinchart.
Signed-off-by Tomasz Stanislawski <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Quoted 96 lines

---
 include/linux/scatterlist.h |    4 +++
 lib/scatterlist.c           |   64

+++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 0 deletions(-)

include/linux/scatterlist.h +4 -0

index ac9586d..7b600da 100644
@@ -214,6 +214,10 @@ void sg_free_table(struct sg_table *);
 int __sg_alloc_table(struct sg_table *, unsigned int, unsigned int, gfp_t,
 		     sg_alloc_fn *);
 int sg_alloc_table(struct sg_table *, unsigned int, gfp_t);
+int sg_alloc_table_from_pages(struct sg_table *sgt,
+	struct page **pages, unsigned int n_pages,
+	unsigned long offset, unsigned long size,
+	gfp_t gfp_mask);
 
 size_t sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
 			   void *buf, size_t buflen);

lib/scatterlist.c +11 -0

index 6096e89..85868a1 100644
@@ -319,6 +319,70 @@ int sg_alloc_table(struct sg_table *table, unsigned int
 nents, gfp_t gfp_mask) EXPORT_SYMBOL(sg_alloc_table);
 
 /**
+ * sg_alloc_table_from_pages - Allocate and initialize an sg table from
+ *			       an array of pages
+ * @sgt:	The sg table header to use
+ * @pages:	Pointer to an array of page pointers
+ * @n_pages:	Number of pages in the pages array
+ * @offset:     Offset from start of the first page to the start of a
 buffer + * @size:       Number of valid bytes in the buffer (after offset)
+ * @gfp_mask:	GFP allocation mask
+ *
+ *  Description:
+ *    Allocate and initialize an sg table from a list of pages. Continuous
+ *    ranges of the pages are squashed into a single scatterlist node. A

user + * may provide an offset at a start and a size of valid data in a
buffer + * specified by the page array. The returned sg table is
released by + * sg_free_table.
+ *
+ * Returns:
+ * 0 on success, negative error on failure
+ **/

lib/scatterlist.c +45 -0

+int sg_alloc_table_from_pages(struct sg_table *sgt,
+	struct page **pages, unsigned int n_pages,
+	unsigned long offset, unsigned long size,
+	gfp_t gfp_mask)
+{
+	unsigned int chunks;
+	unsigned int i;
+	unsigned int cur_page;
+	int ret;
+	struct scatterlist *s;
+
+	/* compute number of contiguous chunks */
+	chunks = 1;
+	for (i = 1; i < n_pages; ++i)
+		if (page_to_pfn(pages[i]) != page_to_pfn(pages[i - 1]) + 1)
+			++chunks;
+
+	ret = sg_alloc_table(sgt, chunks, gfp_mask);
+	if (unlikely(ret))
+		return ret;
+
+	/* merging chunks and putting them into the scatterlist */
+	cur_page = 0;
+	for_each_sg(sgt->sgl, s, sgt->orig_nents, i) {
+		unsigned long chunk_size;
+		unsigned int j;
+
+		/* looking for the end of the current chunk */
+		for (j = cur_page + 1; j < n_pages; ++j)
+			if (page_to_pfn(pages[j]) !=
+			    page_to_pfn(pages[j - 1]) + 1)
+				break;
+
+		chunk_size = ((j - cur_page) << PAGE_SHIFT) - offset;
+		sg_set_page(s, pages[cur_page], min(size, chunk_size), offset);
+		size -= chunk_size;
+		offset = 0;
+		cur_page = j;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(sg_alloc_table_from_pages);
+
+/**
  * sg_miter_start - start mapping iteration over a sg list
  * @miter: sg mapping iter to be started
  * @sgl: sg list to iterate over

Signature · 3 lines

Regards,

Laurent Pinchart

Andrew Morton

maintainer

2012-05-17 23:56 UTC #3 lore raw

In reply to Tomasz Stanislawski

Quoted 15 lines · On Tue, 08 May 2012 11:50:33 +0200 Tomasz Stanislawski…

This patch adds a new constructor for an sg table. The table is constructed from an array of struct pages. All contiguous chunks of the pages are merged into a single sg nodes. A user may provide an offset and a size of a buffer if the buffer is not page-aligned.
The function is dedicated for DMABUF exporters which often perform conversion from an page array to a scatterlist. Moreover the scatterlist should be squashed in order to save memory and to speed-up the process of DMA mapping using dma_map_sg.
The code is based on the patch 'v4l: vb2-dma-contig: add support for scatterlist in userptr mode' and hints from Laurent Pinchart.
...

Quoted 12 lines

/**
+ * sg_alloc_table_from_pages - Allocate and initialize an sg table from
+ * an array of pages
+ * @sgt: The sg table header to use
+ * @pages: Pointer to an array of page pointers
+ * @n_pages: Number of pages in the pages array
+ * @offset: Offset from start of the first page to the start of a buffer
+ * @size: Number of valid bytes in the buffer (after offset)
+ * @gfp_mask: GFP allocation mask
+ *
+ * Description:
+ * Allocate and initialize an sg table from a list of pages. Continuous

s/Continuous/Contiguous/

Quoted 8 lines

+ *    ranges of the pages are squashed into a single scatterlist node. A user
+ *    may provide an offset at a start and a size of valid data in a buffer
+ *    specified by the page array. The returned sg table is released by
+ *    sg_free_table.

+ *
+ * Returns:
+ * 0 on success, negative error on failure
+ **/

nit: Use */, not **/ here.

lib/scatterlist.c int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask) · line 344

Quoted 4 lines

lib/scatterlist.c +4 -0

+int sg_alloc_table_from_pages(struct sg_table *sgt,
+	struct page **pages, unsigned int n_pages,
+	unsigned long offset, unsigned long size,
+	gfp_t gfp_mask)

I guess a 32-bit n_pages is OK. A 16TB IO seems enough ;)

lib/scatterlist.c int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask) · line 347

Quoted 3 lines

lib/scatterlist.c +3 -0

+{
+	unsigned int chunks;
+	unsigned int i;

erk, please choose a different name for this. When a C programmer sees "i", he very much assumes it has type "int". Making it unsigned causes surprise.

And don't rename it to "u"! Let's give it a nice meaningful name. pageno?

lib/scatterlist.c int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask) · line 355

Quoted 8 lines

lib/scatterlist.c +8 -0

+	unsigned int cur_page;
+	int ret;
+	struct scatterlist *s;
+
+	/* compute number of contiguous chunks */
+	chunks = 1;
+	for (i = 1; i < n_pages; ++i)
+		if (page_to_pfn(pages[i]) != page_to_pfn(pages[i - 1]) + 1)

This assumes that if two pages have contiguous pfn's then they are physically contiguous. Is that true for all architectures and memory models, including sparsemem? See sparse_encode_mem_map().

lib/scatterlist.c int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask) · line 366

Quoted 11 lines

lib/scatterlist.c +11 -0

+			++chunks;
+
+	ret = sg_alloc_table(sgt, chunks, gfp_mask);
+	if (unlikely(ret))
+		return ret;
+
+	/* merging chunks and putting them into the scatterlist */
+	cur_page = 0;
+	for_each_sg(sgt->sgl, s, sgt->orig_nents, i) {
+		unsigned long chunk_size;
+		unsigned int j;

"j" is an "int", too.

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+		/* looking for the end of the current chunk */

s/looking/look/

Quoted 5 lines

lib/scatterlist.c +5 -0

+		for (j = cur_page + 1; j < n_pages; ++j)
+			if (page_to_pfn(pages[j]) !=
+			    page_to_pfn(pages[j - 1]) + 1)
+				break;
+

lib/scatterlist.c int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask) · line 383

Quoted 10 lines

lib/scatterlist.c +10 -0

+		chunk_size = ((j - cur_page) << PAGE_SHIFT) - offset;
+		sg_set_page(s, pages[cur_page], min(size, chunk_size), offset);
+		size -= chunk_size;
+		offset = 0;
+		cur_page = j;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(sg_alloc_table_from_pages);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Tomasz Stanislawski

author

2012-05-21 14:01 UTC #4 lore raw

Hi Andrew,
Thank you for your review,
Please refer to the comments below.

Quoted 31 lines · Andrew Morton

Quoted 15 lines · On Tue, 08 May 2012 11:50:33 +0200 Tomasz Stanislawski…
This patch adds a new constructor for an sg table. The table is constructed from an array of struct pages. All contiguous chunks of the pages are merged into a single sg nodes. A user may provide an offset and a size of a buffer if the buffer is not page-aligned.
The function is dedicated for DMABUF exporters which often perform conversion from an page array to a scatterlist. Moreover the scatterlist should be squashed in order to save memory and to speed-up the process of DMA mapping using dma_map_sg.
The code is based on the patch 'v4l: vb2-dma-contig: add support for scatterlist in userptr mode' and hints from Laurent Pinchart.
...
Quoted 12 lines
/**
+ * sg_alloc_table_from_pages - Allocate and initialize an sg table from
+ * an array of pages
+ * @sgt: The sg table header to use
+ * @pages: Pointer to an array of page pointers
+ * @n_pages: Number of pages in the pages array
+ * @offset: Offset from start of the first page to the start of a buffer
+ * @size: Number of valid bytes in the buffer (after offset)
+ * @gfp_mask: GFP allocation mask
+ *
+ * Description:
+ * Allocate and initialize an sg table from a list of pages. Continuous

Quoted 2 lines

s/Continuous/Contiguous/

Ok. Thanks for noticing it.

Quoted 11 lines

Quoted 8 lines

+ *    ranges of the pages are squashed into a single scatterlist node. A user
+ *    may provide an offset at a start and a size of valid data in a buffer
+ *    specified by the page array. The returned sg table is released by
+ *    sg_free_table.

+ *
+ * Returns:
+ * 0 on success, negative error on failure
+ **/

nit: Use */, not **/ here.

Quoted 7 lines

lib/scatterlist.c int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask) · line 344
Quoted 4 lines
lib/scatterlist.c +4 -0
+int sg_alloc_table_from_pages(struct sg_table *sgt,
+	struct page **pages, unsigned int n_pages,
+	unsigned long offset, unsigned long size,
+	gfp_t gfp_mask)
I guess a 32-bit n_pages is OK. A 16TB IO seems enough ;)

Do you think that 'unsigned long' for offset is too big?

Ad n_pages. Assuming that Moore's law holds it will take circa 25 years before the limit of 16 TB is reached :) for high-end scatterlist operations.
Or I can change the type of n_pages to 'unsigned long' now at no cost :).

Quoted 10 lines

lib/scatterlist.c int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask) · line 347
Quoted 3 lines
lib/scatterlist.c +3 -0
+{
+	unsigned int chunks;
+	unsigned int i;
erk, please choose a different name for this. When a C programmer sees "i", he very much assumes it has type "int". Making it unsigned causes surprise.
And don't rename it to "u"! Let's give it a nice meaningful name. pageno?

The problem is that 'i' is a natural name for a loop counter. This exactly how 'i' is used in this function.
The type 'int' was used in the initial version of the code. It was changed to avoid 'unsigned vs signed' comparisons in the loop condition.

AFAIK, in the kernel code developers try to avoid Hungarian notation. A name of a variable should reflect its purpose, not its type. I can change the name of 'i' to 'pageno' and 'j' to 'pageno2' (?) but I think it will make the code less reliable.

Quoted 4 lines

lib/scatterlist.c +4 -0

+	unsigned int cur_page;
+	int ret;
+	struct scatterlist *s;
+

Quoted 9 lines

lib/scatterlist.c int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask) · line 355
Quoted 4 lines
lib/scatterlist.c +4 -0
+	/* compute number of contiguous chunks */
+	chunks = 1;
+	for (i = 1; i < n_pages; ++i)
+		if (page_to_pfn(pages[i]) != page_to_pfn(pages[i - 1]) + 1)
This assumes that if two pages have contiguous pfn's then they are physically contiguous. Is that true for all architectures and memory models, including sparsemem? See sparse_encode_mem_map().

This is a very good questions. I did some research and I had looked for all pfn_to_phys implementations in the kernel code. I found that all conversions are performed by bit shifting. Therefore I expect that assumption that contiguous PFNs imply contiguous physical addresses is true for all architectures supported by Linux kernel.

Quoted 5 lines

lib/scatterlist.c +5 -0

+			++chunks;
+
+	ret = sg_alloc_table(sgt, chunks, gfp_mask);
+	if (unlikely(ret))
+		return ret;

Quoted 8 lines

lib/scatterlist.c int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask) · line 366
Quoted 6 lines
lib/scatterlist.c +6 -0
+
+	/* merging chunks and putting them into the scatterlist */
+	cur_page = 0;
+	for_each_sg(sgt->sgl, s, sgt->orig_nents, i) {
+		unsigned long chunk_size;
+		unsigned int j;
"j" is an "int", too.

Please refer to 'i'-arguments above.

Quoted 6 lines

Review
Quoted 3 lines
(quoted diff) +2 -0
+
+		/* looking for the end of the current chunk */
s/looking/look/

Quoted 7 lines

lib/scatterlist.c +7 -0

+		for (j = cur_page + 1; j < n_pages; ++j)
+			if (page_to_pfn(pages[j]) !=
+			    page_to_pfn(pages[j - 1]) + 1)
+				break;
+
+		chunk_size = ((j - cur_page) << PAGE_SHIFT) - offset;
+		sg_set_page(s, pages[cur_page], min(size, chunk_size), offset);

Quoted 10 lines

lib/scatterlist.c int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask) · line 383
Quoted 8 lines
lib/scatterlist.c +8 -0
+		size -= chunk_size;
+		offset = 0;
+		cur_page = j;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(sg_alloc_table_from_pages);

Regards,
Tomasz Stanislawski

Andrew Morton

maintainer

2012-05-22 20:10 UTC #5 lore raw

Quoted 8 lines · On Mon, 21 May 2012 16:01:50 +0200 Tomasz Stanislawski…

Quoted 7 lines
lib/scatterlist.c int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask) · line 344
Quoted 4 lines
lib/scatterlist.c +4 -0
+int sg_alloc_table_from_pages(struct sg_table *sgt,
+	struct page **pages, unsigned int n_pages,
+	unsigned long offset, unsigned long size,
+	gfp_t gfp_mask)
I guess a 32-bit n_pages is OK. A 16TB IO seems enough ;)

Quoted 7 lines

Do you think that 'unsigned long' for offset is too big?
Ad n_pages. Assuming that Moore's law holds it will take circa 25 years before the limit of 16 TB is reached :) for high-end scatterlist operations.
Or I can change the type of n_pages to 'unsigned long' now at no cost :).

By then it will be Someone Else's Problem ;)

Quoted 8 lines

Quoted 8 lines
lib/scatterlist.c int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask) · line 347
Quoted 3 lines
lib/scatterlist.c +3 -0
+{
+	unsigned int chunks;
+	unsigned int i;
erk, please choose a different name for this. When a C programmer sees "i", he very much assumes it has type "int". Making it unsigned causes surprise.

Quoted 4 lines

Quoted 2 lines
And don't rename it to "u"! Let's give it a nice meaningful name. pageno?
The problem is that 'i' is a natural name for a loop counter.

It's also the natural name for an integer. If a C programmer sees "i", he thinks "int". It's a Fortran thing ;)

Quoted 4 lines

AFAIK, in the kernel code developers try to avoid Hungarian notation. A name of a variable should reflect its purpose, not its type. I can change the name of 'i' to 'pageno' and 'j' to 'pageno2' (?) but I think it will make the code less reliable.

Well, one could do something radical such as using "p".

Tomasz Stanislawski

author

2012-06-06 12:14 UTC #6 lore raw

Quoted 13 lines · Andrew Morton

Quoted 10 lines · On Mon, 21 May 2012 16:01:50 +0200 Tomasz Stanislawski…
Quoted 7 lines
lib/scatterlist.c int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask) · line 344
Quoted 4 lines
lib/scatterlist.c +4 -0
+int sg_alloc_table_from_pages(struct sg_table *sgt,
+	struct page **pages, unsigned int n_pages,
+	unsigned long offset, unsigned long size,
+	gfp_t gfp_mask)
I guess a 32-bit n_pages is OK. A 16TB IO seems enough ;)
Do you think that 'unsigned long' for offset is too big?

Quoted 8 lines

Quoted 5 lines
Ad n_pages. Assuming that Moore's law holds it will take circa 25 years before the limit of 16 TB is reached :) for high-end scatterlist operations.
Or I can change the type of n_pages to 'unsigned long' now at no cost :).
By then it will be Someone Else's Problem ;)

Ok. So let's keep to 'unsigned int n_pages'.

Quoted 16 lines

Quoted 8 lines
Quoted 8 lines
lib/scatterlist.c int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask) · line 347
Quoted 3 lines
lib/scatterlist.c +3 -0
+{
+	unsigned int chunks;
+	unsigned int i;
erk, please choose a different name for this. When a C programmer sees "i", he very much assumes it has type "int". Making it unsigned causes surprise.
Quoted 4 lines
Quoted 2 lines
And don't rename it to "u"! Let's give it a nice meaningful name. pageno?
The problem is that 'i' is a natural name for a loop counter.
It's also the natural name for an integer. If a C programmer sees "i", he thinks "int". It's a Fortran thing ;)

Quoted 8 lines

Quoted 4 lines
AFAIK, in the kernel code developers try to avoid Hungarian notation. A name of a variable should reflect its purpose, not its type. I can change the name of 'i' to 'pageno' and 'j' to 'pageno2' (?) but I think it will make the code less reliable.
Well, one could do something radical such as using "p".

I can not change the type to 'int' due to 'signed vs unsigned' comparisons in the loop condition.
What do you think about changing the names 'i' -> 'p' and 'j' -> 'q'?

Regards,
Tomasz Stanislawski

Quoted 7 lines

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>