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
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(>pdev->dev, "setup data transfer: blocksize %08x
> nr_blocks %d, offset: %08x\n",
> + dev_dbg(>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
> +++
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.
