[PATCH v1 0/2] Reduce memory usage of FDT utils

[PATCH v1 0/2] Reduce memory usage of FDT utils

[Andreas Gnau]

This patch series reduces the memory usage of fdt{get,put,dump} and
related tools. This is especially relevant when using the utilities on
embedded devices to process FIT images, where the actual image payload
is stored behind the FDT blob in the same file.

The first patch optimises how much is read from the file, while the
second patch optimises the allocation strategy.

[PATCH v1 1/2] util: Only read files until fdt_totalsize

[Andreas Gnau]

When reading files, it is actually only necessary to read the amount of
bytes specified in the FDT header. Any trailing data that is not part of
the FDT is not relevant to the operation of any of the utilities and is
discarded anyways. Stop reading when reaching either fdt_totalsize bytes
or end-of-file.

Stopping reading before end-of-file saves a considerable amount of
memory when using FIT (Flattened Image Tree) [1] images with "external
data" which are used by bootloaders such as Das U-Boot. In case of FIT
images, the FDT is a few kilobytes, while the actual file can be several
megabytes, which is quite a significant difference in memory usage.

[1] https://fitspec.osfw.foundation/

Signed-off-by: Andreas Gnau <andreas.gnau@iopsys.eu>
---
 util.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/util.c b/util.c
index 507f0120cd13..093303078d5f 100644
--- a/util.c
+++ b/util.c
@@ -249,6 +249,7 @@ int utilfdt_read_err(const char *filename, char **buffp, size_t *len)
 	char *buf = NULL;
 	size_t bufsize = 1024, offset = 0;
 	int ret = 0;
+	uint32_t totalsize = UINT32_MAX;
 
 	*buffp = NULL;
 	if (strcmp(filename, "-") != 0) {
@@ -257,7 +258,7 @@ int utilfdt_read_err(const char *filename, char **buffp, size_t *len)
 			return errno;
 	}
 
-	/* Loop until we have read everything */
+	/* Loop until we have read until the end of file or end of FDT */
 	buf = xmalloc(bufsize);
 	do {
 		/* Expand the buffer to hold the next chunk */
@@ -272,14 +273,24 @@ int utilfdt_read_err(const char *filename, char **buffp, size_t *len)
 			break;
 		}
 		offset += ret;
-	} while (ret != 0);
+
+		/* assumes that the size is always stored in V1 part */
+		if (totalsize == UINT32_MAX && offset >= FDT_V1_SIZE) {
+			totalsize = fdt_totalsize(buf);
+			if (totalsize <= 0)
+				/* read file up to INT32_MAX in case of errors */
+				totalsize = UINT32_MAX;
+		}
+	} while (ret != 0 && offset < totalsize);
 
 	/* Clean up, including closing stdin; return errno on error */
 	close(fd);
-	if (ret)
+	if (ret <= 0)
 		free(buf);
-	else
+	else {
 		*buffp = buf;
+		ret = 0;
+	}
 	if (len)
 		*len = bufsize;
 	return ret;
-- 
2.46.1

Re: [PATCH v1 1/2] util: Only read files until fdt_totalsize

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3464 bytes --]

On Tue, Sep 24, 2024 at 04:50:51PM +0200, Andreas Gnau wrote:
> When reading files, it is actually only necessary to read the amount of
> bytes specified in the FDT header. Any trailing data that is not part of
> the FDT is not relevant to the operation of any of the utilities and is
> discarded anyways. Stop reading when reaching either fdt_totalsize bytes
> or end-of-file.
> 
> Stopping reading before end-of-file saves a considerable amount of
> memory when using FIT (Flattened Image Tree) [1] images with "external
> data" which are used by bootloaders such as Das U-Boot. In case of FIT
> images, the FDT is a few kilobytes, while the actual file can be several
> megabytes, which is quite a significant difference in memory usage.
> 
> [1] https://fitspec.osfw.foundation/
> 
> Signed-off-by: Andreas Gnau <andreas.gnau@iopsys.eu>

The idea is good, but the approach needs some work.

First, utilfdt_read_err() wasn't intended to be specific to loading a
dtb, although that happens to be all we use it for.  So I'd prefer to
see this replace by a new function named to indicate it's specifically
for loading dtbs.

Secondly, folding the totalsize logic into the existing loop is
unnecessarily complex.  If we're basing the load size on the totalsize
field, there's a much more straightforward approach:

 1. Load the (v1) header into a temporary buffer
 2. Check the magic number (to see if it really is a dtb)
 3. Allocate a final buffer based on totalsize, copy the header into it
 4. Read the remaining (totalsize - header) bytes into the buffer

Both (1) and (4) can be done with a classic "read_all()" helper to
deal with short read()s.

> ---
>  util.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/util.c b/util.c
> index 507f0120cd13..093303078d5f 100644
> --- a/util.c
> +++ b/util.c
> @@ -249,6 +249,7 @@ int utilfdt_read_err(const char *filename, char **buffp, size_t *len)
>  	char *buf = NULL;
>  	size_t bufsize = 1024, offset = 0;
>  	int ret = 0;
> +	uint32_t totalsize = UINT32_MAX;
>  
>  	*buffp = NULL;
>  	if (strcmp(filename, "-") != 0) {
> @@ -257,7 +258,7 @@ int utilfdt_read_err(const char *filename, char **buffp, size_t *len)
>  			return errno;
>  	}
>  
> -	/* Loop until we have read everything */
> +	/* Loop until we have read until the end of file or end of FDT */
>  	buf = xmalloc(bufsize);
>  	do {
>  		/* Expand the buffer to hold the next chunk */
> @@ -272,14 +273,24 @@ int utilfdt_read_err(const char *filename, char **buffp, size_t *len)
>  			break;
>  		}
>  		offset += ret;
> -	} while (ret != 0);
> +
> +		/* assumes that the size is always stored in V1 part */
> +		if (totalsize == UINT32_MAX && offset >= FDT_V1_SIZE) {
> +			totalsize = fdt_totalsize(buf);
> +			if (totalsize <= 0)
> +				/* read file up to INT32_MAX in case of errors */
> +				totalsize = UINT32_MAX;
> +		}
> +	} while (ret != 0 && offset < totalsize);
>  
>  	/* Clean up, including closing stdin; return errno on error */
>  	close(fd);
> -	if (ret)
> +	if (ret <= 0)
>  		free(buf);
> -	else
> +	else {
>  		*buffp = buf;
> +		ret = 0;
> +	}
>  	if (len)
>  		*len = bufsize;
>  	return ret;

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v1 2/2] util: Once size is known, allocate fdt_totalsize

[Andreas Gnau]

Instead of growing the buffer when reading a file by a factor of 2,
try to allocate the whole size of the FDT once the size is known after
having read the header. While the size is not known, use the existing
strategy of doubling the buffer's size.

Since the initial buffer size is 1024, which is greater than
FDT_V1_SIZE (and any later version header sizes such as FDT_V17_SIZE),
this reduces the number of allocations to two (one for the initial
buffer and one additional one for the full size of the FDT. The total
size of the buffer will be reduced as well by up to
(fdt_totalsize / 2) - 1 bytes.

Signed-off-by: Andreas Gnau <andreas.gnau@iopsys.eu>
---
 util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util.c b/util.c
index 093303078d5f..c5873e91608e 100644
--- a/util.c
+++ b/util.c
@@ -263,7 +263,7 @@ int utilfdt_read_err(const char *filename, char **buffp, size_t *len)
 	do {
 		/* Expand the buffer to hold the next chunk */
 		if (offset == bufsize) {
-			bufsize *= 2;
+			bufsize = (totalsize != UINT32_MAX) ? totalsize : 2 * bufsize;
 			buf = xrealloc(buf, bufsize);
 		}
 
-- 
2.46.1