LinuxLists.cc - [PATCH] scatterlist: Update size type to support greater then 4GB size.

2018-12-12 06:25:02

Subject: [PATCH] scatterlist: Update size type to support greater then 4GB size.

From: Krishna Reddy <[email protected]>

In the cases where greater than 4GB allocations are required, current
definition of scatterlist doesn't support it. For example, Tegra devices
have more than 4GB of system memory available. So they are expected to
support larger allocation requests.
This patch updates the type of scatterlist members to support creating
scatterlist for allocations of size greater than 4GB size.
Updated the files that are necessary to fix compilation issues with updated
type of variables.

With definition of scatterlist changed in this patch, size of sg has
increased from 28 bytes to 40 bytes because of which allocations in nvme
driver are crossing order-0( as sizeof(struct scatterlist) is used in nvme
driver allocations ) i.e. they are not able to fit into single page.

Recently a patch to limit the nvme allocations to order-0 is mergerd.
'commit 943e942e6266f22babee5efeb00f8f672fbff5bd ("nvme-pci: limit
max IO size and segments to avoid high order allocations")'
Because of that patch, WARN log is getting printed in our case as
definition of scatterlist has changed. Alloc size of nvme is calculated as
NVME_MAX_SEGS * sizeof(struct scatterlist). As sizeof(struct scatterlist)
has changed from 28 bytes to 40 bytes, so updating NVME_MAX_SEGS from 127
to 88 to correspond to original nvme alloc size value.

Signed-off-by: Krishna Reddy <[email protected]>
Signed-off-by: Ashish Mhetre <[email protected]>
---
crypto/shash.c | 2 +-
drivers/ata/libata-sff.c | 2 +-
drivers/mmc/host/sdhci.c | 2 +-
drivers/mmc/host/toshsd.c | 2 +-
drivers/mmc/host/usdhi6rol0.c | 14 +++++++-------
drivers/nvme/host/pci.c | 8 ++++----
include/linux/nvme.h | 2 +-
include/linux/scatterlist.h | 8 ++++----
8 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/crypto/shash.c b/crypto/shash.c
index d21f04d..434970e 100644
--- a/crypto/shash.c
+++ b/crypto/shash.c
@@ -298,7 +298,7 @@ int shash_ahash_digest(struct ahash_request *req, struct shash_desc *desc)

if (nbytes &&
(sg = req->src, offset = sg->offset,
- nbytes < min(sg->length, ((unsigned int)(PAGE_SIZE)) - offset))) {
+ nbytes < min(sg->length, ((size_t)(PAGE_SIZE)) - offset))) {
void *data;

data = kmap_atomic(sg_page(sg));
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index c5ea0fc..675def6 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -810,7 +810,7 @@ static int __atapi_pio_bytes(struct ata_queued_cmd *qc, unsigned int bytes)
offset %= PAGE_SIZE;

/* don't overrun current sg */
- count = min(sg->length - qc->cursg_ofs, bytes);
+ count = min(sg->length - qc->cursg_ofs, (size_t)bytes);

/* don't cross page boundaries */
count = min(count, (unsigned int)PAGE_SIZE - offset);
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 99bdae5..bd84c0c 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1025,7 +1025,7 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
if (unlikely(length_mask | offset_mask)) {
for_each_sg(data->sg, sg, data->sg_len, i) {
if (sg->length & length_mask) {
- DBG("Reverting to PIO because of transfer size (%d)\n",
+ DBG("Reverting to PIO because of transfer size (%zd)\n",
sg->length);
host->flags &= ~SDHCI_REQ_USE_DMA;
break;
diff --git a/drivers/mmc/host/toshsd.c b/drivers/mmc/host/toshsd.c
index dd961c5..af00936 100644
--- a/drivers/mmc/host/toshsd.c
+++ b/drivers/mmc/host/toshsd.c
@@ -479,7 +479,7 @@ static void toshsd_start_data(struct toshsd_host *host, struct mmc_data *data)
{
unsigned int flags = SG_MITER_ATOMIC;

- dev_dbg(&host->pdev->dev, "setup data transfer: blocksize %08x nr_blocks %d, offset: %08x\n",
+ dev_dbg(&host->pdev->dev, "setup data transfer: blocksize %08x nr_blocks %d, offset: %08lx\n",
data->blksz, data->blocks, data->sg->offset);

host->data = data;
diff --git a/drivers/mmc/host/usdhi6rol0.c b/drivers/mmc/host/usdhi6rol0.c
index cd8b1b9..5fce5ff 100644
--- a/drivers/mmc/host/usdhi6rol0.c
+++ b/drivers/mmc/host/usdhi6rol0.c
@@ -316,7 +316,7 @@ static void usdhi6_blk_bounce(struct usdhi6_host *host,
struct mmc_data *data = host->mrq->data;
size_t blk_head = host->head_len;

- dev_dbg(mmc_dev(host->mmc), "%s(): CMD%u of %u SG: %ux%u @ 0x%x\n",
+ dev_dbg(mmc_dev(host->mmc), "%s(): CMD%u of %u SG: %ux%u @ 0x%lx\n",
__func__, host->mrq->cmd->opcode, data->sg_len,
data->blksz, data->blocks, sg->offset);

@@ -360,7 +360,7 @@ static void *usdhi6_sg_map(struct usdhi6_host *host)

WARN(host->pg.page, "%p not properly unmapped!\n", host->pg.page);
if (WARN(sg_dma_len(sg) % data->blksz,
- "SG size %u isn't a multiple of block size %u\n",
+ "SG size %zu isn't a multiple of block size %u\n",
sg_dma_len(sg), data->blksz))
return NULL;

@@ -383,7 +383,7 @@ static void *usdhi6_sg_map(struct usdhi6_host *host)
else
host->blk_page = host->pg.mapped;

- dev_dbg(mmc_dev(host->mmc), "Mapped %p (%lx) at %p + %u for CMD%u @ 0x%p\n",
+ dev_dbg(mmc_dev(host->mmc), "Mapped %p (%lx) at %p + %lu for CMD%u @ 0x%p\n",
host->pg.page, page_to_pfn(host->pg.page), host->pg.mapped,
sg->offset, host->mrq->cmd->opcode, host->mrq);

@@ -492,7 +492,7 @@ static void usdhi6_sg_advance(struct usdhi6_host *host)
host->sg = next;

if (WARN(next && sg_dma_len(next) % data->blksz,
- "SG size %u isn't a multiple of block size %u\n",
+ "SG size %zu isn't a multiple of block size %u\n",
sg_dma_len(next), data->blksz))
data->error = -EINVAL;

@@ -1044,7 +1044,7 @@ static int usdhi6_rq_start(struct usdhi6_host *host)
(data->blksz % 4 ||
data->sg->offset % 4))
dev_dbg(mmc_dev(host->mmc),
- "Bad SG of %u: %ux%u @ %u\n", data->sg_len,
+ "Bad SG of %u: %ux%u @ %lu\n", data->sg_len,
data->blksz, data->blocks, data->sg->offset);

/* Enable DMA for USDHI6_MIN_DMA bytes or more */
@@ -1056,7 +1056,7 @@ static int usdhi6_rq_start(struct usdhi6_host *host)
usdhi6_write(host, USDHI6_CC_EXT_MODE, USDHI6_CC_EXT_MODE_SDRW);

dev_dbg(mmc_dev(host->mmc),
- "%s(): request opcode %u, %u blocks of %u bytes in %u segments, %s %s @+0x%x%s\n",
+ "%s(): request opcode %u, %u blocks of %u bytes in %u segments, %s %s @+0x%lx%s\n",
__func__, cmd->opcode, data->blocks, data->blksz,
data->sg_len, use_dma ? "DMA" : "PIO",
data->flags & MMC_DATA_READ ? "read" : "write",
@@ -1704,7 +1704,7 @@ static void usdhi6_timeout_work(struct work_struct *work)
case USDHI6_WAIT_FOR_WRITE:
sg = host->sg ?: data->sg;
dev_dbg(mmc_dev(host->mmc),
- "%c: page #%u @ +0x%zx %ux%u in SG%u. Current SG %u bytes @ %u\n",
+ "%c: page #%u @ +0x%zx %ux%u in SG%u. Current SG %zu bytes @ %lu\n",
data->flags & MMC_DATA_READ ? 'R' : 'W', host->page_idx,
host->offset, data->blocks, data->blksz, data->sg_len,
sg_dma_len(sg), sg->offset);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index d668682..57ef89d 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -43,7 +43,7 @@
* require an sg allocation that needs more than a page of data.
*/
#define NVME_MAX_KB_SZ 4096
-#define NVME_MAX_SEGS 127
+#define NVME_MAX_SEGS 88

static int use_threaded_interrupts;
module_param(use_threaded_interrupts, int, 0);
@@ -552,8 +552,8 @@ static void nvme_print_sgl(struct scatterlist *sgl, int nents)

for_each_sg(sgl, sg, nents, i) {
dma_addr_t phys = sg_phys(sg);
- pr_warn("sg[%d] phys_addr:%pad offset:%d length:%d "
- "dma_address:%pad dma_length:%d\n",
+ pr_warn("sg[%d] phys_addr:%pad offset:%lu length:%zu "
+ "dma_address:%pad dma_length:%zu\n",
i, &phys, sg->offset, sg->length, &sg_dma_address(sg),
sg_dma_len(sg));
}
@@ -566,7 +566,7 @@ static blk_status_t nvme_pci_setup_prps(struct nvme_dev *dev,
struct dma_pool *pool;
int length = blk_rq_payload_bytes(req);
struct scatterlist *sg = iod->sg;
- int dma_len = sg_dma_len(sg);
+ u64 dma_len = sg_dma_len(sg);
u64 dma_addr = sg_dma_address(sg);
u32 page_size = dev->ctrl.page_size;
int offset = dma_addr & (page_size - 1);
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 68e91ef..0a07a29 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -587,7 +587,7 @@ enum {

struct nvme_sgl_desc {
__le64 addr;
- __le32 length;
+ __le64 length;
__u8 rsvd[3];
__u8 type;
};
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 093aa57..f6d3482 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -10,11 +10,11 @@

struct scatterlist {
unsigned long page_link;
- unsigned int offset;
- unsigned int length;
+ unsigned long offset;
+ size_t length;
dma_addr_t dma_address;
#ifdef CONFIG_NEED_SG_DMA_LENGTH
- unsigned int dma_length;
+ size_t dma_length;
#endif
};

@@ -114,7 +114,7 @@ static inline void sg_assign_page(struct scatterlist *sg, struct page *page)
*
**/
static inline void sg_set_page(struct scatterlist *sg, struct page *page,
- unsigned int len, unsigned int offset)
+ size_t len, unsigned long offset)
{
sg_assign_page(sg, page);
sg->offset = offset;
--
2.7.4

2018-12-12 06:29:54

by David Miller

[permalink] [raw]

Subject: Re: [PATCH] scatterlist: Update size type to support greater then 4GB size.

From: Ashish Mhetre <[email protected]>
Date: Wed, 12 Dec 2018 11:54:13 +0530

> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index 68e91ef..0a07a29 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -587,7 +587,7 @@ enum {
>
> struct nvme_sgl_desc {
> __le64 addr;
> - __le32 length;
> + __le64 length;
> __u8 rsvd[3];
> __u8 type;
> };

Isn't this a device or protocol defined datastructure? You can't just
change it like this.

2018-12-12 06:49:20

by Sagi Grimberg

[permalink] [raw]

Subject: Re: [PATCH] scatterlist: Update size type to support greater then 4GB size.

>> struct nvme_sgl_desc {
>> __le64 addr;
>> - __le32 length;
>> + __le64 length;
>> __u8 rsvd[3];
>> __u8 type;
>> };
>
> Isn't this a device or protocol defined datastructure? You can't just
> change it like this.

You're correct, we can't...
[Replied before seeing this issue was already highlighted]

The positive side is that it can safely be removed without affecting the
rest of the patch...

2018-12-14 14:14:12

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH] scatterlist: Update size type to support greater then 4GB size.

Attachments:

(No filename) (4.93 kB)
.config.gz (29.91 kB)
Download all attachments

2018-12-12 07:45:22

by Ashish Mhetre

[permalink] [raw]

Subject: Re: [PATCH] scatterlist: Update size type to support greater then 4GB size.

On 12/12/18 12:19 PM, Sagi Grimberg wrote:
>
>>> struct nvme_sgl_desc {
>>>       __le64    addr;
>>> -    __le32    length;
>>> +    __le64    length;
>>>       __u8    rsvd[3];
>>>       __u8    type;
>>> };
>>
>> Isn't this a device or protocol defined datastructure? You can't just
>> change it like this.
>
> You're correct, we can't...
> [Replied before seeing this issue was already highlighted]
>
> The positive side is that it can safely be removed without affecting the
> rest of the patch...

Ohh, I am not aware of this protocol defined data-structures. But it
seems that this need not be changed as Sagi is saying sg length for NVME
will never cross 32 bit size.
I'll send a new version removing this change. Thanks.

2018-12-12 06:46:56

by Sagi Grimberg

[permalink] [raw]

Subject: Re: [PATCH] scatterlist: Update size type to support greater then 4GB size.

> struct nvme_sgl_desc {
> __le64 addr;
> - __le32 length;
> + __le64 length;
> __u8 rsvd[3];
> __u8 type;
> };

in what world changing a wire protocol for this make sense?

please get rid of this hunk, NVMe will never cross the 32 bit sg element
size.

2018-12-14 14:14:10

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH] scatterlist: Update size type to support greater then 4GB size.

Hi Krishna,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.20-rc6 next-20181214]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Ashish-Mhetre/scatterlist-Update-size-type-to-support-greater-then-4GB-size/20181214-214801
config: x86_64-randconfig-x012-201849 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64

All error/warnings (new ones prefixed by >>):

In file included from include/linux/kernel.h:10:0,
from include/linux/list.h:9,
from include/linux/async.h:16,
from drivers/nvme/host/pci.c:16:
In function '_nvme_check_size',
inlined from 'nvme_exit' at drivers/nvme/host/pci.c:2748:2:
>> include/linux/compiler.h:372:38: error: call to '__compiletime_assert_206' declared with attribute error: BUILD_BUG_ON failed: sizeof(struct nvme_rw_command) != 64
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^
include/linux/compiler.h:353:4: note: in definition of macro '__compiletime_assert'
prefix ## suffix(); \
^~~~~~
include/linux/compiler.h:372:2: note: in expansion of macro '_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:45:37: note: in expansion of macro 'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:69:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
^~~~~~~~~~~~~~~~
>> drivers/nvme/host/pci.c:206:2: note: in expansion of macro 'BUILD_BUG_ON'
BUILD_BUG_ON(sizeof(struct nvme_rw_command) != 64);
^~~~~~~~~~~~
>> include/linux/compiler.h:372:38: error: call to '__compiletime_assert_210' declared with attribute error: BUILD_BUG_ON failed: sizeof(struct nvme_features) != 64
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^
include/linux/compiler.h:353:4: note: in definition of macro '__compiletime_assert'
prefix ## suffix(); \
^~~~~~
include/linux/compiler.h:372:2: note: in expansion of macro '_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:45:37: note: in expansion of macro 'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:69:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
^~~~~~~~~~~~~~~~
drivers/nvme/host/pci.c:210:2: note: in expansion of macro 'BUILD_BUG_ON'
BUILD_BUG_ON(sizeof(struct nvme_features) != 64);
^~~~~~~~~~~~
>> include/linux/compiler.h:372:38: error: call to '__compiletime_assert_213' declared with attribute error: BUILD_BUG_ON failed: sizeof(struct nvme_command) != 64
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^
include/linux/compiler.h:353:4: note: in definition of macro '__compiletime_assert'
prefix ## suffix(); \
^~~~~~
include/linux/compiler.h:372:2: note: in expansion of macro '_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:45:37: note: in expansion of macro 'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:69:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
^~~~~~~~~~~~~~~~
drivers/nvme/host/pci.c:213:2: note: in expansion of macro 'BUILD_BUG_ON'
BUILD_BUG_ON(sizeof(struct nvme_command) != 64);
^~~~~~~~~~~~
--
In file included from include/linux/kernel.h:10:0,
from include/linux/list.h:9,
from include/linux/async.h:16,
from drivers/nvme//host/pci.c:16:
In function '_nvme_check_size',
inlined from 'nvme_exit' at drivers/nvme//host/pci.c:2748:2:
>> include/linux/compiler.h:372:38: error: call to '__compiletime_assert_206' declared with attribute error: BUILD_BUG_ON failed: sizeof(struct nvme_rw_command) != 64
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^
include/linux/compiler.h:353:4: note: in definition of macro '__compiletime_assert'
prefix ## suffix(); \
^~~~~~
include/linux/compiler.h:372:2: note: in expansion of macro '_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:45:37: note: in expansion of macro 'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:69:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
^~~~~~~~~~~~~~~~
drivers/nvme//host/pci.c:206:2: note: in expansion of macro 'BUILD_BUG_ON'
BUILD_BUG_ON(sizeof(struct nvme_rw_command) != 64);
^~~~~~~~~~~~
>> include/linux/compiler.h:372:38: error: call to '__compiletime_assert_210' declared with attribute error: BUILD_BUG_ON failed: sizeof(struct nvme_features) != 64
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^
include/linux/compiler.h:353:4: note: in definition of macro '__compiletime_assert'
prefix ## suffix(); \
^~~~~~
include/linux/compiler.h:372:2: note: in expansion of macro '_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:45:37: note: in expansion of macro 'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:69:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
^~~~~~~~~~~~~~~~
drivers/nvme//host/pci.c:210:2: note: in expansion of macro 'BUILD_BUG_ON'
BUILD_BUG_ON(sizeof(struct nvme_features) != 64);
^~~~~~~~~~~~
>> include/linux/compiler.h:372:38: error: call to '__compiletime_assert_213' declared with attribute error: BUILD_BUG_ON failed: sizeof(struct nvme_command) != 64
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^
include/linux/compiler.h:353:4: note: in definition of macro '__compiletime_assert'
prefix ## suffix(); \
^~~~~~
include/linux/compiler.h:372:2: note: in expansion of macro '_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:45:37: note: in expansion of macro 'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:69:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
^~~~~~~~~~~~~~~~
drivers/nvme//host/pci.c:213:2: note: in expansion of macro 'BUILD_BUG_ON'
BUILD_BUG_ON(sizeof(struct nvme_command) != 64);
^~~~~~~~~~~~

vim +/BUILD_BUG_ON +206 drivers/nvme/host/pci.c

b60503ba drivers/block/nvme.c Matthew Wilcox 2011-01-20 200
b60503ba drivers/block/nvme.c Matthew Wilcox 2011-01-20 201 /*
b60503ba drivers/block/nvme.c Matthew Wilcox 2011-01-20 202 * Check we didin't inadvertently grow the command struct
b60503ba drivers/block/nvme.c Matthew Wilcox 2011-01-20 203 */
b60503ba drivers/block/nvme.c Matthew Wilcox 2011-01-20 204 static inline void _nvme_check_size(void)
b60503ba drivers/block/nvme.c Matthew Wilcox 2011-01-20 205 {
b60503ba drivers/block/nvme.c Matthew Wilcox 2011-01-20 @206 BUILD_BUG_ON(sizeof(struct nvme_rw_command) != 64);
b60503ba drivers/block/nvme.c Matthew Wilcox 2011-01-20 207 BUILD_BUG_ON(sizeof(struct nvme_create_cq) != 64);
b60503ba drivers/block/nvme.c Matthew Wilcox 2011-01-20 208 BUILD_BUG_ON(sizeof(struct nvme_create_sq) != 64);
b60503ba drivers/block/nvme.c Matthew Wilcox 2011-01-20 209 BUILD_BUG_ON(sizeof(struct nvme_delete_queue) != 64);
b60503ba drivers/block/nvme.c Matthew Wilcox 2011-01-20 210 BUILD_BUG_ON(sizeof(struct nvme_features) != 64);
f8ebf840 drivers/block/nvme-core.c Vishal Verma 2013-03-27 211 BUILD_BUG_ON(sizeof(struct nvme_format_cmd) != 64);
c30341dc drivers/block/nvme-core.c Keith Busch 2013-12-10 212 BUILD_BUG_ON(sizeof(struct nvme_abort_cmd) != 64);
b60503ba drivers/block/nvme.c Matthew Wilcox 2011-01-20 213 BUILD_BUG_ON(sizeof(struct nvme_command) != 64);
0add5e8e drivers/nvme/host/pci.c Johannes Thumshirn 2017-06-07 214 BUILD_BUG_ON(sizeof(struct nvme_id_ctrl) != NVME_IDENTIFY_DATA_SIZE);
0add5e8e drivers/nvme/host/pci.c Johannes Thumshirn 2017-06-07 215 BUILD_BUG_ON(sizeof(struct nvme_id_ns) != NVME_IDENTIFY_DATA_SIZE);
b60503ba drivers/block/nvme.c Matthew Wilcox 2011-01-20 216 BUILD_BUG_ON(sizeof(struct nvme_lba_range_type) != 64);
6ecec745 drivers/block/nvme.c Keith Busch 2012-09-26 217 BUILD_BUG_ON(sizeof(struct nvme_smart_log) != 512);
f9f38e33 drivers/nvme/host/pci.c Helen Koike 2017-04-10 218 BUILD_BUG_ON(sizeof(struct nvme_dbbuf) != 64);
f9f38e33 drivers/nvme/host/pci.c Helen Koike 2017-04-10 219 }
f9f38e33 drivers/nvme/host/pci.c Helen Koike 2017-04-10 220

:::::: The code at line 206 was first introduced by commit
:::::: b60503ba432b16fc84442a84e29a7aad2c0c363d NVMe: New driver

:::::: TO: Matthew Wilcox <[email protected]>
:::::: CC: Matthew Wilcox <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Attachments:

(No filename) (10.61 kB)
.config.gz (29.22 kB)
Download all attachments

2018-12-24 08:07:07

by Ashish Mhetre

[permalink] [raw]

Subject: Re: [PATCH] scatterlist: Update size type to support greater then 4GB size.

We don't have a real use-case for this. Understanding the consequences,
we are questioning the patch in downstream itself.
Please ignore this patch for now.

On 12/12/18 1:36 PM, Christoph Hellwig wrote:
> scatterlist elements longer than 4GB sound odd. Please submit it
> in a series with your actual user so that we can help figuring out
> if it really makes sense or if there is a better way to solve your
> problem.
>
> As is this patch will massively increase the memory usage for all users
> of struct scatterlist, so you better have a really good reason.
>

2018-12-12 08:07:00

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH] scatterlist: Update size type to support greater then 4GB size.

scatterlist elements longer than 4GB sound odd. Please submit it
in a series with your actual user so that we can help figuring out
if it really makes sense or if there is a better way to solve your
problem.

As is this patch will massively increase the memory usage for all users
of struct scatterlist, so you better have a really good reason.

2018-12-12 07:52:10

by Ard Biesheuvel

[permalink] [raw]

Subject: Re: [PATCH] scatterlist: Update size type to support greater then 4GB size.

On Wed, 12 Dec 2018 at 07:25, Ashish Mhetre <[email protected]> wrote:
>
> From: Krishna Reddy <[email protected]>
>
> In the cases where greater than 4GB allocations are required, current
> definition of scatterlist doesn't support it. For example, Tegra devices
> have more than 4GB of system memory available. So they are expected to
> support larger allocation requests.
> This patch updates the type of scatterlist members to support creating
> scatterlist for allocations of size greater than 4GB size.

Can you explain what the use case is? We have had systems with more
than 4 GB for a while now, so where does this new requirement come
from?

Also, you are changing 'length' to size_t and 'offset' to unsigned
long. What is the rationale behind that? Did you consider 32-bit
architectures with PAE at all?

> Updated the files that are necessary to fix compilation issues with updated
> type of variables.
>
> With definition of scatterlist changed in this patch, size of sg has
> increased from 28 bytes to 40 bytes because of which allocations in nvme
> driver are crossing order-0( as sizeof(struct scatterlist) is used in nvme
> driver allocations ) i.e. they are not able to fit into single page.
>
> Recently a patch to limit the nvme allocations to order-0 is mergerd.
> 'commit 943e942e6266f22babee5efeb00f8f672fbff5bd ("nvme-pci: limit
> max IO size and segments to avoid high order allocations")'
> Because of that patch, WARN log is getting printed in our case as
> definition of scatterlist has changed. Alloc size of nvme is calculated as
> NVME_MAX_SEGS * sizeof(struct scatterlist). As sizeof(struct scatterlist)
> has changed from 28 bytes to 40 bytes, so updating NVME_MAX_SEGS from 127
> to 88 to correspond to original nvme alloc size value.
>
> Signed-off-by: Krishna Reddy <[email protected]>
> Signed-off-by: Ashish Mhetre <[email protected]>
> ---
> crypto/shash.c | 2 +-
> drivers/ata/libata-sff.c | 2 +-
> drivers/mmc/host/sdhci.c | 2 +-
> drivers/mmc/host/toshsd.c | 2 +-
> drivers/mmc/host/usdhi6rol0.c | 14 +++++++-------
> drivers/nvme/host/pci.c | 8 ++++----
> include/linux/nvme.h | 2 +-
> include/linux/scatterlist.h | 8 ++++----
> 8 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/crypto/shash.c b/crypto/shash.c
> index d21f04d..434970e 100644
> --- a/crypto/shash.c
> +++ b/crypto/shash.c
> @@ -298,7 +298,7 @@ int shash_ahash_digest(struct ahash_request *req, struct shash_desc *desc)
>
> if (nbytes &&
> (sg = req->src, offset = sg->offset,
> - nbytes < min(sg->length, ((unsigned int)(PAGE_SIZE)) - offset))) {
> + nbytes < min(sg->length, ((size_t)(PAGE_SIZE)) - offset))) {
> void *data;
>
> data = kmap_atomic(sg_page(sg));
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index c5ea0fc..675def6 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -810,7 +810,7 @@ static int __atapi_pio_bytes(struct ata_queued_cmd *qc, unsigned int bytes)
> offset %= PAGE_SIZE;
>
> /* don't overrun current sg */
> - count = min(sg->length - qc->cursg_ofs, bytes);
> + count = min(sg->length - qc->cursg_ofs, (size_t)bytes);
>
> /* don't cross page boundaries */
> count = min(count, (unsigned int)PAGE_SIZE - offset);
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 99bdae5..bd84c0c 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1025,7 +1025,7 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
> if (unlikely(length_mask | offset_mask)) {
> for_each_sg(data->sg, sg, data->sg_len, i) {
> if (sg->length & length_mask) {
> - DBG("Reverting to PIO because of transfer size (%d)\n",
> + DBG("Reverting to PIO because of transfer size (%zd)\n",
> sg->length);
> host->flags &= ~SDHCI_REQ_USE_DMA;
> break;
> diff --git a/drivers/mmc/host/toshsd.c b/drivers/mmc/host/toshsd.c
> index dd961c5..af00936 100644
> --- a/drivers/mmc/host/toshsd.c
> +++ b/drivers/mmc/host/toshsd.c
> @@ -479,7 +479,7 @@ static void toshsd_start_data(struct toshsd_host *host, struct mmc_data *data)
> {
> unsigned int flags = SG_MITER_ATOMIC;
>
> - dev_dbg(&host->pdev->dev, "setup data transfer: blocksize %08x nr_blocks %d, offset: %08x\n",
> + dev_dbg(&host->pdev->dev, "setup data transfer: blocksize %08x nr_blocks %d, offset: %08lx\n",
> data->blksz, data->blocks, data->sg->offset);
>
> host->data = data;
> diff --git a/drivers/mmc/host/usdhi6rol0.c b/drivers/mmc/host/usdhi6rol0.c
> index cd8b1b9..5fce5ff 100644
> --- a/drivers/mmc/host/usdhi6rol0.c
> +++ b/drivers/mmc/host/usdhi6rol0.c
> @@ -316,7 +316,7 @@ static void usdhi6_blk_bounce(struct usdhi6_host *host,
> struct mmc_data *data = host->mrq->data;
> size_t blk_head = host->head_len;
>
> - dev_dbg(mmc_dev(host->mmc), "%s(): CMD%u of %u SG: %ux%u @ 0x%x\n",
> + dev_dbg(mmc_dev(host->mmc), "%s(): CMD%u of %u SG: %ux%u @ 0x%lx\n",
> __func__, host->mrq->cmd->opcode, data->sg_len,
> data->blksz, data->blocks, sg->offset);
>
> @@ -360,7 +360,7 @@ static void *usdhi6_sg_map(struct usdhi6_host *host)
>
> WARN(host->pg.page, "%p not properly unmapped!\n", host->pg.page);
> if (WARN(sg_dma_len(sg) % data->blksz,
> - "SG size %u isn't a multiple of block size %u\n",
> + "SG size %zu isn't a multiple of block size %u\n",
> sg_dma_len(sg), data->blksz))
> return NULL;
>
> @@ -383,7 +383,7 @@ static void *usdhi6_sg_map(struct usdhi6_host *host)
> else
> host->blk_page = host->pg.mapped;
>
> - dev_dbg(mmc_dev(host->mmc), "Mapped %p (%lx) at %p + %u for CMD%u @ 0x%p\n",
> + dev_dbg(mmc_dev(host->mmc), "Mapped %p (%lx) at %p + %lu for CMD%u @ 0x%p\n",
> host->pg.page, page_to_pfn(host->pg.page), host->pg.mapped,
> sg->offset, host->mrq->cmd->opcode, host->mrq);
>
> @@ -492,7 +492,7 @@ static void usdhi6_sg_advance(struct usdhi6_host *host)
> host->sg = next;
>
> if (WARN(next && sg_dma_len(next) % data->blksz,
> - "SG size %u isn't a multiple of block size %u\n",
> + "SG size %zu isn't a multiple of block size %u\n",
> sg_dma_len(next), data->blksz))
> data->error = -EINVAL;
>
> @@ -1044,7 +1044,7 @@ static int usdhi6_rq_start(struct usdhi6_host *host)
> (data->blksz % 4 ||
> data->sg->offset % 4))
> dev_dbg(mmc_dev(host->mmc),
> - "Bad SG of %u: %ux%u @ %u\n", data->sg_len,
> + "Bad SG of %u: %ux%u @ %lu\n", data->sg_len,
> data->blksz, data->blocks, data->sg->offset);
>
> /* Enable DMA for USDHI6_MIN_DMA bytes or more */
> @@ -1056,7 +1056,7 @@ static int usdhi6_rq_start(struct usdhi6_host *host)
> usdhi6_write(host, USDHI6_CC_EXT_MODE, USDHI6_CC_EXT_MODE_SDRW);
>
> dev_dbg(mmc_dev(host->mmc),
> - "%s(): request opcode %u, %u blocks of %u bytes in %u segments, %s %s @+0x%x%s\n",
> + "%s(): request opcode %u, %u blocks of %u bytes in %u segments, %s %s @+0x%lx%s\n",
> __func__, cmd->opcode, data->blocks, data->blksz,
> data->sg_len, use_dma ? "DMA" : "PIO",
> data->flags & MMC_DATA_READ ? "read" : "write",
> @@ -1704,7 +1704,7 @@ static void usdhi6_timeout_work(struct work_struct *work)
> case USDHI6_WAIT_FOR_WRITE:
> sg = host->sg ?: data->sg;
> dev_dbg(mmc_dev(host->mmc),
> - "%c: page #%u @ +0x%zx %ux%u in SG%u. Current SG %u bytes @ %u\n",
> + "%c: page #%u @ +0x%zx %ux%u in SG%u. Current SG %zu bytes @ %lu\n",
> data->flags & MMC_DATA_READ ? 'R' : 'W', host->page_idx,
> host->offset, data->blocks, data->blksz, data->sg_len,
> sg_dma_len(sg), sg->offset);
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index d668682..57ef89d 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -43,7 +43,7 @@
> * require an sg allocation that needs more than a page of data.
> */
> #define NVME_MAX_KB_SZ 4096
> -#define NVME_MAX_SEGS 127
> +#define NVME_MAX_SEGS 88
>
> static int use_threaded_interrupts;
> module_param(use_threaded_interrupts, int, 0);
> @@ -552,8 +552,8 @@ static void nvme_print_sgl(struct scatterlist *sgl, int nents)
>
> for_each_sg(sgl, sg, nents, i) {
> dma_addr_t phys = sg_phys(sg);
> - pr_warn("sg[%d] phys_addr:%pad offset:%d length:%d "
> - "dma_address:%pad dma_length:%d\n",
> + pr_warn("sg[%d] phys_addr:%pad offset:%lu length:%zu "
> + "dma_address:%pad dma_length:%zu\n",
> i, &phys, sg->offset, sg->length, &sg_dma_address(sg),
> sg_dma_len(sg));
> }
> @@ -566,7 +566,7 @@ static blk_status_t nvme_pci_setup_prps(struct nvme_dev *dev,
> struct dma_pool *pool;
> int length = blk_rq_payload_bytes(req);
> struct scatterlist *sg = iod->sg;
> - int dma_len = sg_dma_len(sg);
> + u64 dma_len = sg_dma_len(sg);
> u64 dma_addr = sg_dma_address(sg);
> u32 page_size = dev->ctrl.page_size;
> int offset = dma_addr & (page_size - 1);
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index 68e91ef..0a07a29 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -587,7 +587,7 @@ enum {
>
> struct nvme_sgl_desc {
> __le64 addr;
> - __le32 length;
> + __le64 length;
> __u8 rsvd[3];
> __u8 type;
> };
> diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
> index 093aa57..f6d3482 100644
> --- a/include/linux/scatterlist.h
> +++ b/include/linux/scatterlist.h
> @@ -10,11 +10,11 @@
>
> struct scatterlist {
> unsigned long page_link;
> - unsigned int offset;
> - unsigned int length;
> + unsigned long offset;
> + size_t length;
> dma_addr_t dma_address;
> #ifdef CONFIG_NEED_SG_DMA_LENGTH
> - unsigned int dma_length;
> + size_t dma_length;
> #endif
> };
>
> @@ -114,7 +114,7 @@ static inline void sg_assign_page(struct scatterlist *sg, struct page *page)
> *
> **/
> static inline void sg_set_page(struct scatterlist *sg, struct page *page,
> - unsigned int len, unsigned int offset)
> + size_t len, unsigned long offset)
> {
> sg_assign_page(sg, page);
> sg->offset = offset;
> --
> 2.7.4
>