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.
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-x000-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/init.h:5:0, from drivers/nvme//host/fabrics.c:15: drivers/nvme//host/fabrics.c: In function 'nvmf_exit': >> include/linux/compiler.h:372:38: error: call to '__compiletime_assert_1149' >> declared with attribute error: BUILD_BUG_ON failed: sizeof(struct >> nvmf_connect_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/fabrics.c:1149:2: note: in expansion of macro >> 'BUILD_BUG_ON' BUILD_BUG_ON(sizeof(struct nvmf_connect_command) != 64); ^~~~ -- In file included from include/linux/init.h:5:0, from drivers/nvme/host/fabrics.c:15: drivers/nvme/host/fabrics.c: In function 'nvmf_exit': >> include/linux/compiler.h:372:38: error: call to '__compiletime_assert_1149' >> declared with attribute error: BUILD_BUG_ON failed: sizeof(struct >> nvmf_connect_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/fabrics.c:1149:2: note: in expansion of macro 'BUILD_BUG_ON' BUILD_BUG_ON(sizeof(struct nvmf_connect_command) != 64); ^~~~ vim +/__compiletime_assert_1149 +372 include/linux/compiler.h 9a8ab1c3 Daniel Santos 2013-02-21 358 9a8ab1c3 Daniel Santos 2013-02-21 359 #define _compiletime_assert(condition, msg, prefix, suffix) \ 9a8ab1c3 Daniel Santos 2013-02-21 360 __compiletime_assert(condition, msg, prefix, suffix) 9a8ab1c3 Daniel Santos 2013-02-21 361 9a8ab1c3 Daniel Santos 2013-02-21 362 /** 9a8ab1c3 Daniel Santos 2013-02-21 363 * compiletime_assert - break build and emit msg if condition is false 9a8ab1c3 Daniel Santos 2013-02-21 364 * @condition: a compile-time constant condition to check 9a8ab1c3 Daniel Santos 2013-02-21 365 * @msg: a message to emit if condition is false 9a8ab1c3 Daniel Santos 2013-02-21 366 * 9a8ab1c3 Daniel Santos 2013-02-21 367 * In tradition of POSIX assert, this macro will break the build if the 9a8ab1c3 Daniel Santos 2013-02-21 368 * supplied condition is *false*, emitting the supplied error message if the 9a8ab1c3 Daniel Santos 2013-02-21 369 * compiler has support to do so. 9a8ab1c3 Daniel Santos 2013-02-21 370 */ 9a8ab1c3 Daniel Santos 2013-02-21 371 #define compiletime_assert(condition, msg) \ 9a8ab1c3 Daniel Santos 2013-02-21 @372 _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__) 9a8ab1c3 Daniel Santos 2013-02-21 373 :: The code at line 372 was first introduced by commit :: 9a8ab1c39970a4938a72d94e6fd13be88a797590 bug.h, compiler.h: introduce compiletime_assert & BUILD_BUG_ON_MSG :: TO: Daniel Santos :: CC: Linus Torvalds --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipe
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__)
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.
Re: [PATCH] scatterlist: Update size type to support greater then 4GB size.
On Wed, 12 Dec 2018 at 07:25, Ashish Mhetre wrote:
>
> From: Krishna Reddy
>
> 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
> Signed-off-by: Ashish Mhetre
> ---
> 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/
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.
Re: [PATCH] scatterlist: Update size type to support greater then 4GB size.
struct nvme_sgl_desc {
__le64 addr;
- __le32 length;
+ __le64 length;
__u8rsvd[3];
__u8type;
};
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...
Re: [PATCH] scatterlist: Update size type to support greater then 4GB size.
struct nvme_sgl_desc {
__le64 addr;
- __le32 length;
+ __le64 length;
__u8rsvd[3];
__u8type;
};
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.
Re: [PATCH] scatterlist: Update size type to support greater then 4GB size.
From: Ashish Mhetre
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;
> __u8rsvd[3];
> __u8type;
> };
Isn't this a device or protocol defined datastructure? You can't just
change it like this.
[PATCH] scatterlist: Update size type to support greater then 4GB size.
From: Krishna Reddy
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
Signed-off-by: Ashish Mhetre
---
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(ho
