[PATCH] tools: copyfile: use 64k instead of 512 buffer

[PATCH] tools: copyfile: use 64k instead of 512 buffer

[Ahelenia Ziemiańska]

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

This is an incredible pessimisation:
mkimage took >200ms (and 49489 writes (of which 49456 512)),
now it takes  110ms (and   419 writes (of which   386 64k)).

sendfile is much more appropriate for this and is done in one syscall,
but doesn't bring any significant speedups over 64k r/w
at the 13M size ranges, so there's no need to introduce
	#if __linux__
	while((size = sendfile(fd_dst, fd_src, NULL, 128 * 1024 * 1024)) > 0)
		;
	if(size != -1) {
		ret = 0;
		goto out;
	}
	#endif

Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
---
 tools/fit_common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/fit_common.c b/tools/fit_common.c
index 2d417d47..373fab6a 100644
--- a/tools/fit_common.c
+++ b/tools/fit_common.c
@@ -145,14 +145,14 @@ int copyfile(const char *src, const char *dst)
 		goto out;
 	}
 
-	buf = calloc(1, 512);
+	buf = calloc(1, 64 * 1024);
 	if (!buf) {
 		printf("Can't allocate buffer to copy file\n");
 		goto out;
 	}
 
 	while (1) {
-		size = read(fd_src, buf, 512);
+		size = read(fd_src, buf, 64 * 1024);
 		if (size < 0) {
 			printf("Can't read file %s\n", src);
 			goto out;
-- 
2.39.2

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

Re: [PATCH] tools: copyfile: use 64k instead of 512 buffer

[Dragan Simic]

Hello Ahelenia,

Please see my comments below.

On 2024-03-20 14:08, Ahelenia Ziemiańska wrote:
> This is an incredible pessimisation:

s/pessimisation/optimization/

> mkimage took >200ms (and 49489 writes (of which 49456 512)),
> now it takes  110ms (and   419 writes (of which   386 64k)).
> 
> sendfile is much more appropriate for this and is done in one syscall,
> but doesn't bring any significant speedups over 64k r/w
> at the 13M size ranges, so there's no need to introduce
> 	#if __linux__
> 	while((size = sendfile(fd_dst, fd_src, NULL, 128 * 1024 * 1024)) > 0)
> 		;
> 	if(size != -1) {
> 		ret = 0;
> 		goto out;
> 	}
> 	#endif
> 
> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>

Looking good to me.  With the small nitpick above and
a suggestion below,

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

> ---
>  tools/fit_common.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/fit_common.c b/tools/fit_common.c
> index 2d417d47..373fab6a 100644
> --- a/tools/fit_common.c
> +++ b/tools/fit_common.c
> @@ -145,14 +145,14 @@ int copyfile(const char *src, const char *dst)
>  		goto out;
>  	}
> 
> -	buf = calloc(1, 512);
> +	buf = calloc(1, 64 * 1024);
>  	if (!buf) {
>  		printf("Can't allocate buffer to copy file\n");
>  		goto out;
>  	}
> 
>  	while (1) {
> -		size = read(fd_src, buf, 512);
> +		size = read(fd_src, buf, 64 * 1024);

Perhaps this would be a good opportunity to introduce a new
#define for 64 * 1024 as the new buffer size.

>  		if (size < 0) {
>  			printf("Can't read file %s\n", src);
>  			goto out;

[PATCH v2] tools: copyfile: use 64k instead of 512 buffer

[Ahelenia Ziemiańska]

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

This is a trivial but significant optimisation:
mkimage took >200ms (and 49489 writes (of which 49456 512)),
now it takes  110ms (and   419 writes (of which   386 64k)).

sendfile is much more appropriate for this and is done in one syscall,
but doesn't bring any significant speedups over 64k r/w
at the 13M size ranges, so there's no need to introduce
	#if __linux__
	while((size = sendfile(fd_dst, fd_src, NULL, 128 * 1024 * 1024)) > 0)
		;
	if(size != -1) {
		ret = 0;
		goto out;
	}
	#endif

Also lift up the buffer size to a macro.

Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Reviewed-by: Dragan Simic <dsimic@manjaro.org>
---
 tools/fit_common.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/fit_common.c b/tools/fit_common.c
index 2d417d47..1df4785c 100644
--- a/tools/fit_common.c
+++ b/tools/fit_common.c
@@ -127,6 +127,7 @@ err:
 
 int copyfile(const char *src, const char *dst)
 {
+#define copyfile_bufsize (64 * 1024)
 	int fd_src = -1, fd_dst = -1;
 	void *buf = NULL;
 	ssize_t size;
@@ -145,14 +146,14 @@ int copyfile(const char *src, const char *dst)
 		goto out;
 	}
 
-	buf = calloc(1, 512);
+	buf = calloc(1, copyfile_bufsize);
 	if (!buf) {
 		printf("Can't allocate buffer to copy file\n");
 		goto out;
 	}
 
 	while (1) {
-		size = read(fd_src, buf, 512);
+		size = read(fd_src, buf, copyfile_bufsize);
 		if (size < 0) {
 			printf("Can't read file %s\n", src);
 			goto out;
-- 
2.39.2

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

Re: [PATCH v2] tools: copyfile: use 64k instead of 512 buffer

[Dragan Simic]

Hello Ahelenia,

Please see my comments below.

On 2024-03-21 19:29, Ahelenia Ziemiańska wrote:
> This is a trivial but significant optimisation:

s/optimisation/optimization/

> mkimage took >200ms (and 49489 writes (of which 49456 512)),
> now it takes  110ms (and   419 writes (of which   386 64k)).
> 
> sendfile is much more appropriate for this and is done in one syscall,
> but doesn't bring any significant speedups over 64k r/w
> at the 13M size ranges, so there's no need to introduce
> 	#if __linux__
> 	while((size = sendfile(fd_dst, fd_src, NULL, 128 * 1024 * 1024)) > 0)
> 		;
> 	if(size != -1) {
> 		ret = 0;
> 		goto out;
> 	}
> 	#endif
> 
> Also lift up the buffer size to a macro.

s/lift up/extract/
s/macro/constant/

> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
> Reviewed-by: Dragan Simic <dsimic@manjaro.org>
> ---
>  tools/fit_common.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/fit_common.c b/tools/fit_common.c
> index 2d417d47..1df4785c 100644
> --- a/tools/fit_common.c
> +++ b/tools/fit_common.c
> @@ -127,6 +127,7 @@ err:
> 
>  int copyfile(const char *src, const char *dst)
>  {
> +#define copyfile_bufsize (64 * 1024)

This is not the right place for such a preprocessor directive.
Instead, it should be placed at the start of the file.

Also, it should use all uppercase letters.

>  	int fd_src = -1, fd_dst = -1;
>  	void *buf = NULL;
>  	ssize_t size;
> @@ -145,14 +146,14 @@ int copyfile(const char *src, const char *dst)
>  		goto out;
>  	}
> 
> -	buf = calloc(1, 512);
> +	buf = calloc(1, copyfile_bufsize);
>  	if (!buf) {
>  		printf("Can't allocate buffer to copy file\n");
>  		goto out;
>  	}
> 
>  	while (1) {
> -		size = read(fd_src, buf, 512);
> +		size = read(fd_src, buf, copyfile_bufsize);
>  		if (size < 0) {
>  			printf("Can't read file %s\n", src);
>  			goto out;

[PATCH v3] tools: copyfile: use 64k instead of 512 buffer

[Ahelenia Ziemiańska]

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

This is a trivial but significant optimization:
mkimage took >200ms (and 49489 writes (of which 49456 512)),
now it takes  110ms (and   419 writes (of which   386 64k)).

sendfile is much more appropriate for this and is done in one syscall,
but doesn't bring any significant speedups over 64k r/w
at the 13M size ranges, so there's no need to introduce
	#if __linux__
	while((size = sendfile(fd_dst, fd_src, NULL, 128 * 1024 * 1024)) > 0)
		;
	if(size != -1) {
		ret = 0;
		goto out;
	}
	#endif

Also extract the buffer size to a constant.

Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Reviewed-by: Dragan Simic <dsimic@manjaro.org>
---
On Thu, Mar 21, 2024 at 08:49:52PM +0100, Dragan Simic wrote:
> On 2024-03-21 19:29, Ahelenia Ziemiańska wrote:
> > This is a trivial but significant optimisation:
> s/optimisation/optimization/
This seems to run counter to precedent of not doing americans'
imperialism for them for free
(I see -ise/-ize in free variation even in my 100-deep checkout).

> > +#define copyfile_bufsize (64 * 1024)
> This is not the right place for such a preprocessor directive.
> Instead, it should be placed at the start of the file.
>
> Also, it should use all uppercase letters.
FTR, neither of these points seem to be universal;
I modelled this after tools/mtk_image.c (also cf. scripts/kconfig/expr.c),
but there are at least 25 other function-local macros
(as in git grep -B1 '^#define' | grep -c -A1 -e '-{' returns 27).

Also kinda weird to explicitly request a macro
only to review it back to a constant, but whatever.

 tools/fit_common.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/fit_common.c b/tools/fit_common.c
index 2d417d47..37066203 100644
--- a/tools/fit_common.c
+++ b/tools/fit_common.c
@@ -129,6 +129,7 @@ int copyfile(const char *src, const char *dst)
 {
 	int fd_src = -1, fd_dst = -1;
 	void *buf = NULL;
+	const size_t bufsize = 64 * 1024;
 	ssize_t size;
 	size_t count;
 	int ret = -1;
@@ -145,14 +146,14 @@ int copyfile(const char *src, const char *dst)
 		goto out;
 	}
 
-	buf = calloc(1, 512);
+	buf = calloc(1, bufsize);
 	if (!buf) {
 		printf("Can't allocate buffer to copy file\n");
 		goto out;
 	}
 
 	while (1) {
-		size = read(fd_src, buf, 512);
+		size = read(fd_src, buf, bufsize);
 		if (size < 0) {
 			printf("Can't read file %s\n", src);
 			goto out;
-- 
2.39.2

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

Re: [PATCH v3] tools: copyfile: use 64k instead of 512 buffer

[Dragan Simic]

On 2024-03-21 21:37, Ahelenia Ziemiańska wrote:
> This is a trivial but significant optimization:
> mkimage took >200ms (and 49489 writes (of which 49456 512)),
> now it takes  110ms (and   419 writes (of which   386 64k)).
> 
> sendfile is much more appropriate for this and is done in one syscall,
> but doesn't bring any significant speedups over 64k r/w
> at the 13M size ranges, so there's no need to introduce
> 	#if __linux__
> 	while((size = sendfile(fd_dst, fd_src, NULL, 128 * 1024 * 1024)) > 0)
> 		;
> 	if(size != -1) {
> 		ret = 0;
> 		goto out;
> 	}
> 	#endif
> 
> Also extract the buffer size to a constant.
> 
> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
> Reviewed-by: Dragan Simic <dsimic@manjaro.org>

Please, remove my Reviewed-by tag from this version of the patch,
because it no longer applies.

> ---
> On Thu, Mar 21, 2024 at 08:49:52PM +0100, Dragan Simic wrote:
>> On 2024-03-21 19:29, Ahelenia Ziemiańska wrote:
>> > This is a trivial but significant optimisation:
>> s/optimisation/optimization/
> This seems to run counter to precedent of not doing americans'
> imperialism for them for free
> (I see -ise/-ize in free variation even in my 100-deep checkout).

Well, that's politics, which I have no interest about.

>> > +#define copyfile_bufsize (64 * 1024)
>> This is not the right place for such a preprocessor directive.
>> Instead, it should be placed at the start of the file.
>> 
>> Also, it should use all uppercase letters.
> FTR, neither of these points seem to be universal;
> I modelled this after tools/mtk_image.c (also cf. 
> scripts/kconfig/expr.c),
> but there are at least 25 other function-local macros
> (as in git grep -B1 '^#define' | grep -c -A1 -e '-{' returns 27).

AFAICT, scripts/kconfig/expr.c could use come cleanups.

> Also kinda weird to explicitly request a macro
> only to review it back to a constant, but whatever.

Well, it isn't weird.  See, the instances of a preprocessor #define
directive are replaced with its definition before the source code
reaches the compiler, making it, in this case, a bit easier for the
compiler to do its job.

The way you wrote it below, the compiler will almost surely optimize
out the constant variable and replace it with the literal number in
the generated object code, but that puts unnecessary burden on the
compiler, which can be avoided by feeding it with the literal number,
using a preprocessor directive.

>  tools/fit_common.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/fit_common.c b/tools/fit_common.c
> index 2d417d47..37066203 100644
> --- a/tools/fit_common.c
> +++ b/tools/fit_common.c
> @@ -129,6 +129,7 @@ int copyfile(const char *src, const char *dst)
>  {
>  	int fd_src = -1, fd_dst = -1;
>  	void *buf = NULL;
> +	const size_t bufsize = 64 * 1024;
>  	ssize_t size;
>  	size_t count;
>  	int ret = -1;
> @@ -145,14 +146,14 @@ int copyfile(const char *src, const char *dst)
>  		goto out;
>  	}
> 
> -	buf = calloc(1, 512);
> +	buf = calloc(1, bufsize);
>  	if (!buf) {
>  		printf("Can't allocate buffer to copy file\n");
>  		goto out;
>  	}
> 
>  	while (1) {
> -		size = read(fd_src, buf, 512);
> +		size = read(fd_src, buf, bufsize);
>  		if (size < 0) {
>  			printf("Can't read file %s\n", src);
>  			goto out;

Re: [PATCH v3] tools: copyfile: use 64k instead of 512 buffer

[Dragan Simic]

Hello Ahelenia,

Just checking, have you forgotten about this patch?


On 2024-03-21 21:50, Dragan Simic wrote:
> On 2024-03-21 21:37, Ahelenia Ziemiańska wrote:
>> This is a trivial but significant optimization:
>> mkimage took >200ms (and 49489 writes (of which 49456 512)),
>> now it takes  110ms (and   419 writes (of which   386 64k)).
>> 
>> sendfile is much more appropriate for this and is done in one syscall,
>> but doesn't bring any significant speedups over 64k r/w
>> at the 13M size ranges, so there's no need to introduce
>> 	#if __linux__
>> 	while((size = sendfile(fd_dst, fd_src, NULL, 128 * 1024 * 1024)) > 0)
>> 		;
>> 	if(size != -1) {
>> 		ret = 0;
>> 		goto out;
>> 	}
>> 	#endif
>> 
>> Also extract the buffer size to a constant.
>> 
>> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
>> Reviewed-by: Dragan Simic <dsimic@manjaro.org>
> 
> Please, remove my Reviewed-by tag from this version of the patch,
> because it no longer applies.
> 
>> ---
>> On Thu, Mar 21, 2024 at 08:49:52PM +0100, Dragan Simic wrote:
>>> On 2024-03-21 19:29, Ahelenia Ziemiańska wrote:
>>> > This is a trivial but significant optimisation:
>>> s/optimisation/optimization/
>> This seems to run counter to precedent of not doing americans'
>> imperialism for them for free
>> (I see -ise/-ize in free variation even in my 100-deep checkout).
> 
> Well, that's politics, which I have no interest about.
> 
>>> > +#define copyfile_bufsize (64 * 1024)
>>> This is not the right place for such a preprocessor directive.
>>> Instead, it should be placed at the start of the file.
>>> 
>>> Also, it should use all uppercase letters.
>> FTR, neither of these points seem to be universal;
>> I modelled this after tools/mtk_image.c (also cf. 
>> scripts/kconfig/expr.c),
>> but there are at least 25 other function-local macros
>> (as in git grep -B1 '^#define' | grep -c -A1 -e '-{' returns 27).
> 
> AFAICT, scripts/kconfig/expr.c could use come cleanups.
> 
>> Also kinda weird to explicitly request a macro
>> only to review it back to a constant, but whatever.
> 
> Well, it isn't weird.  See, the instances of a preprocessor #define
> directive are replaced with its definition before the source code
> reaches the compiler, making it, in this case, a bit easier for the
> compiler to do its job.
> 
> The way you wrote it below, the compiler will almost surely optimize
> out the constant variable and replace it with the literal number in
> the generated object code, but that puts unnecessary burden on the
> compiler, which can be avoided by feeding it with the literal number,
> using a preprocessor directive.
> 
>>  tools/fit_common.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/tools/fit_common.c b/tools/fit_common.c
>> index 2d417d47..37066203 100644
>> --- a/tools/fit_common.c
>> +++ b/tools/fit_common.c
>> @@ -129,6 +129,7 @@ int copyfile(const char *src, const char *dst)
>>  {
>>  	int fd_src = -1, fd_dst = -1;
>>  	void *buf = NULL;
>> +	const size_t bufsize = 64 * 1024;
>>  	ssize_t size;
>>  	size_t count;
>>  	int ret = -1;
>> @@ -145,14 +146,14 @@ int copyfile(const char *src, const char *dst)
>>  		goto out;
>>  	}
>> 
>> -	buf = calloc(1, 512);
>> +	buf = calloc(1, bufsize);
>>  	if (!buf) {
>>  		printf("Can't allocate buffer to copy file\n");
>>  		goto out;
>>  	}
>> 
>>  	while (1) {
>> -		size = read(fd_src, buf, 512);
>> +		size = read(fd_src, buf, bufsize);
>>  		if (size < 0) {
>>  			printf("Can't read file %s\n", src);
>>  			goto out;

[PATCH v4] tools: copyfile: use 64k instead of 512 buffer

[Ahelenia Ziemiańska]

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

This is a trivial but significant optimization:
mkimage took >200ms (and 49489 writes (of which 49456 512)),
now it takes  110ms (and   419 writes (of which   386 64k)).

sendfile is much more appropriate for this and is done in one syscall,
but doesn't bring any significant speedups over 64k r/w
at the 13M size ranges, so there's no need to introduce
	#if __linux__
	while((size = sendfile(fd_dst, fd_src, NULL, 128 * 1024 * 1024)) > 0)
		;
	if(size != -1) {
		ret = 0;
		goto out;
	}
	#endif

Also extract the buffer size to a macro.

Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
---
 tools/fit_common.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/fit_common.c b/tools/fit_common.c
index 2d417d47..d1cde16c 100644
--- a/tools/fit_common.c
+++ b/tools/fit_common.c
@@ -23,6 +23,8 @@
 #include <image.h>
 #include <u-boot/crc.h>
 
+#define COPYFILE_BUFSIZE (64 * 1024)
+
 void fit_print_header(const void *fit, struct image_tool_params *params)
 {
 	fit_print_contents(fit);
@@ -145,14 +147,14 @@ int copyfile(const char *src, const char *dst)
 		goto out;
 	}
 
-	buf = calloc(1, 512);
+	buf = calloc(1, COPYFILE_BUFSIZE);
 	if (!buf) {
 		printf("Can't allocate buffer to copy file\n");
 		goto out;
 	}
 
 	while (1) {
-		size = read(fd_src, buf, 512);
+		size = read(fd_src, buf, COPYFILE_BUFSIZE);
 		if (size < 0) {
 			printf("Can't read file %s\n", src);
 			goto out;
-- 
2.39.2

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

Re: [PATCH v4] tools: copyfile: use 64k instead of 512 buffer

[Dragan Simic]

On 2024-04-09 14:14, Ahelenia Ziemiańska wrote:
> This is a trivial but significant optimization:
> mkimage took >200ms (and 49489 writes (of which 49456 512)),
> now it takes  110ms (and   419 writes (of which   386 64k)).
> 
> sendfile is much more appropriate for this and is done in one syscall,
> but doesn't bring any significant speedups over 64k r/w
> at the 13M size ranges, so there's no need to introduce
> 	#if __linux__
> 	while((size = sendfile(fd_dst, fd_src, NULL, 128 * 1024 * 1024)) > 0)
> 		;
> 	if(size != -1) {
> 		ret = 0;
> 		goto out;
> 	}
> 	#endif
> 
> Also extract the buffer size to a macro.
> 
> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>

Looking good to me, thanks for the v4.

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

> ---
>  tools/fit_common.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/fit_common.c b/tools/fit_common.c
> index 2d417d47..d1cde16c 100644
> --- a/tools/fit_common.c
> +++ b/tools/fit_common.c
> @@ -23,6 +23,8 @@
>  #include <image.h>
>  #include <u-boot/crc.h>
> 
> +#define COPYFILE_BUFSIZE (64 * 1024)
> +
>  void fit_print_header(const void *fit, struct image_tool_params 
> *params)
>  {
>  	fit_print_contents(fit);
> @@ -145,14 +147,14 @@ int copyfile(const char *src, const char *dst)
>  		goto out;
>  	}
> 
> -	buf = calloc(1, 512);
> +	buf = calloc(1, COPYFILE_BUFSIZE);
>  	if (!buf) {
>  		printf("Can't allocate buffer to copy file\n");
>  		goto out;
>  	}
> 
>  	while (1) {
> -		size = read(fd_src, buf, 512);
> +		size = read(fd_src, buf, COPYFILE_BUFSIZE);
>  		if (size < 0) {
>  			printf("Can't read file %s\n", src);
>  			goto out;

Re: [PATCH v4] tools: copyfile: use 64k instead of 512 buffer

[Tom Rini]

On Tue, 09 Apr 2024 14:14:34 +0200, Ahelenia Ziemiańska wrote:

> This is a trivial but significant optimization:
> mkimage took >200ms (and 49489 writes (of which 49456 512)),
> now it takes  110ms (and   419 writes (of which   386 64k)).
> 
> sendfile is much more appropriate for this and is done in one syscall,
> but doesn't bring any significant speedups over 64k r/w
> at the 13M size ranges, so there's no need to introduce
> 	#if __linux__
> 	while((size = sendfile(fd_dst, fd_src, NULL, 128 * 1024 * 1024)) > 0)
> 		;
> 	if(size != -1) {
> 		ret = 0;
> 		goto out;
> 	}
> 	#endif
> 
> [...]

Applied to u-boot/master, thanks!

-- 
Tom