Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Malloc for path's buffers instead of stack #5888

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

grom72
Copy link
Contributor

@grom72 grom72 commented Oct 24, 2023

Decrease the amount of stack memory usage by allocating the path buffer on the heap using the Malloc() function.

Requires:

Signed-off-by: Tomasz Gromadzki tomasz.gromadzki@intel.com


This change is Reviewable

@grom72 grom72 added libpmemobj src/libpmemobj DAOS DAOS-related sprint goal This pull request is part of the ongoing sprint labels Oct 24, 2023
@grom72 grom72 force-pushed the limited-PATH_MAX branch 3 times, most recently from f0bc8f1 to 9e55da3 Compare October 24, 2023 10:25
@grom72 grom72 requested review from janekmi and osalyk October 24, 2023 10:34
Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r1, 1 of 3 files at r2, 8 of 8 files at r3, all commit messages.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @grom72 and @osalyk)

a discussion (no related file):
I understand that having stack stats generation on the same branch is convenient for you but it is not for the reviewers nor necessary.



src/core/out.c line 192 at r3 (raw file):

		}
		Free(log_file_pid);
	}

It is outrageous. This piece of code has three possible error cases and each of them prints its error message differently. Please sort it out or at least do not introduce the third one.

Code quote:

	if ((log_file = os_getenv(log_file_var)) != NULL &&
				log_file[0] != '\0') {

		/* reserve more than enough space for a PID + '\0' */
		char *log_file_pid;
		log_file_pid = Malloc(PATH_MAX);
		if (log_file_pid == NULL) {
			fprintf(stderr, "out_init !Malloc\n");
			abort();
		}
		size_t len = strlen(log_file);
		if (len > 0 && log_file[len - 1] == '-') {
			if (util_snprintf(log_file_pid, PATH_MAX, "%s%d",
					log_file, getpid()) < 0) {
				ERR("snprintf: %d", errno);
				Free(log_file_pid);
				abort();
			}
			log_file = log_file_pid;
		}

		if ((Out_fp = os_fopen(log_file, "w")) == NULL) {
			char buff[UTIL_MAX_ERR_MSG];
			util_strerror(errno, buff, UTIL_MAX_ERR_MSG);
			fprintf(stderr, "Error (%s): %s=%s: %s\n",
				log_prefix, log_file_var,
				log_file, buff);
			Free(log_file_pid);
			abort();
		}
		Free(log_file_pid);
	}

src/core/out.c line 199 at r3 (raw file):

#ifdef DEBUG
	static char namepath[PATH_MAX];

I do not know why this one was static but assuming its value has to survive between function calls I assume it is not allocated on the stack, or is it?

Code quote:

static char namepath[PATH_MAX];

src/core/out.c line 209 at r3 (raw file):

#ifdef DEBUG
	char *namepath;
	namepath = Malloc(PATH_MAX);

Suggestion:

	char *namepath = Malloc(PATH_MAX);

src/core/out.c line 215 at r3 (raw file):

	}
	LOG(1, "pid %d: program: %s", getpid(),
		util_getexecname(namepath, PATH_MAX));

Code snippet:

Free(namepath);

src/libpmem2/auto_flush_linux.c line 94 at r3 (raw file):

	int cpu_cache = 0;

	domain_path  = Malloc(PATH_MAX);

Suggestion:

domain_path = Malloc(PATH_MAX * sizeof(char));

src/libpmem2/deep_flush_linux.c line 37 at r3 (raw file):

	char rbuf[2];

	deep_flush_path  = Malloc(PATH_MAX);

Please apply consistently.

Note: Just a single space around the assignment sign (=).

Suggestion:

deep_flush_path = Malloc(PATH_MAX * sizeof(char));

src/libpmem2/deep_flush_linux.c line 40 at r3 (raw file):

	if (deep_flush_path == NULL) {
		ERR("!Malloc");
		ret = -1;

Suggestion:

ret = PMEM2_E_ERRNO;

src/libpmem2/deep_flush_linux.c line 81 at r3 (raw file):

end:
	if (deep_flush_fd > -1)

Suggestion:

deep_flush_fd != -1

src/libpmem2/pmem2_utils_linux.c line 47 at r3 (raw file):

	spath = Malloc(PATH_MAX);
	if (spath == NULL) {
		errno = ENOMEM;

Unnecessary. Please see other comments.


src/libpmem2/pmem2_utils_linux.c line 68 at r3 (raw file):

	npath = Malloc(PATH_MAX);
	if (npath == NULL) {
		errno = ENOMEM;

.


src/libpmem2/pmem2_utils_linux.c line 94 at r3 (raw file):

		Free(spath);
	if (npath)
		Free(npath);

Free in the reverse order they were created.

Suggestion:

	if (npath)
		Free(npath);
	if (spath)
		Free(spath);

src/libpmem2/region_namespace_ndctl.c line 40 at r3 (raw file):
No need to set errno by yourself.

On error, these functions return NULL and set errno.

Ref: https://man7.org/linux/man-pages/man3/malloc.3.html


src/libpmem2/region_namespace_ndctl.c line 43 at r3 (raw file):

		ERR("!Malloc");
		return PMEM2_E_ERRNO;

Unnecessary empty line.


src/libpmem2/region_namespace_ndctl.c line 65 at r3 (raw file):

	LOG(4, "found matching device: %s", path);
end:

Suggestion:

	LOG(4, "found matching device: %s", path);

end:

src/libpmem2/region_namespace_ndctl.c line 92 at r3 (raw file):

	path = Malloc(PATH_MAX);
	if (path == NULL) {
		errno = ENOMEM;

.

Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @janekmi and @osalyk)

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

I understand that having stack stats generation on the same branch is convenient for you but it is not for the reviewers nor necessary.

Shall we keep stats files up-to-date?


@grom72 grom72 force-pushed the limited-PATH_MAX branch 2 times, most recently from bd6e8de to de0c594 Compare October 24, 2023 11:58
Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 14 files reviewed, 16 unresolved discussions (waiting on @janekmi and @osalyk)


src/core/out.c line 192 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It is outrageous. This piece of code has three possible error cases and each of them prints its error message differently. Please sort it out or at least do not introduce the third one.

Done.


src/core/out.c line 199 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I do not know why this one was static but assuming its value has to survive between function calls I assume it is not allocated on the stack, or is it?

Can we assume that out_init will be called once at a time?


src/core/out.c line 209 at r3 (raw file):

#ifdef DEBUG
	char *namepath;
	namepath = Malloc(PATH_MAX);

Done.
I mean use static variable here.


src/core/out.c line 215 at r3 (raw file):

	}
	LOG(1, "pid %d: program: %s", getpid(),
		util_getexecname(namepath, PATH_MAX));

Done.
Rollback to the original solution with static.


src/libpmem2/deep_flush_linux.c line 37 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please apply consistently.

Note: Just a single space around the assignment sign (=).

Done.


src/libpmem2/deep_flush_linux.c line 40 at r3 (raw file):

	if (deep_flush_path == NULL) {
		ERR("!Malloc");
		ret = -1;

Done.

@grom72 grom72 force-pushed the limited-PATH_MAX branch 2 times, most recently from 0cf4069 to 1210c10 Compare October 24, 2023 12:21
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #5888 (04a984a) into master (a08e391) will decrease coverage by 2.36%.
Report is 1 commits behind head on master.
The diff coverage is 22.53%.

❗ Current head 04a984a differs from pull request most recent head 9dfd9c0. Consider uploading reports for the commit 9dfd9c0 to get more accurate results

@@            Coverage Diff             @@
##           master    #5888      +/-   ##
==========================================
- Coverage   71.00%   68.65%   -2.36%     
==========================================
  Files         131      131              
  Lines       19172    19725     +553     
  Branches     3192     3265      +73     
==========================================
- Hits        13614    13543      -71     
- Misses       5558     6182     +624     

@grom72 grom72 force-pushed the limited-PATH_MAX branch 2 times, most recently from 5e632fb to 9176a8d Compare October 24, 2023 13:52
Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r1, 7 of 12 files at r4, 2 of 9 files at r5, 1 of 1 files at r6, 4 of 6 files at r7, 3 of 3 files at r8, 1 of 1 files at r9.
Reviewable status: 10 of 14 files reviewed, 16 unresolved discussions (waiting on @janekmi and @osalyk)


src/libpmem2/auto_flush_linux.c line 94 at r3 (raw file):

	int cpu_cache = 0;

	domain_path  = Malloc(PATH_MAX);

Done.


src/libpmem2/deep_flush_linux.c line 81 at r3 (raw file):

end:
	if (deep_flush_fd > -1)

It is opposite to the previous condition ....<0


src/libpmem2/pmem2_utils_linux.c line 47 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Unnecessary. Please see other comments.

Done.


src/libpmem2/pmem2_utils_linux.c line 68 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.


src/libpmem2/pmem2_utils_linux.c line 94 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Free in the reverse order they were created.

Done.


src/libpmem2/region_namespace_ndctl.c line 40 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

No need to set errno by yourself.

On error, these functions return NULL and set errno.

Ref: https://man7.org/linux/man-pages/man3/malloc.3.html

Done.


src/libpmem2/region_namespace_ndctl.c line 43 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Unnecessary empty line.

Done.


src/libpmem2/region_namespace_ndctl.c line 65 at r3 (raw file):

	LOG(4, "found matching device: %s", path);
end:

Done.


src/libpmem2/region_namespace_ndctl.c line 92 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.

Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: 10 of 14 files reviewed, 16 unresolved discussions (waiting on @janekmi and @osalyk)

Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com>
@grom72 grom72 marked this pull request as draft October 30, 2023 11:53
@janekmi janekmi removed the sprint goal This pull request is part of the ongoing sprint label Oct 31, 2023
Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r10, 3 of 3 files at r11, all commit messages.
Reviewable status: 11 of 14 files reviewed, 16 unresolved discussions (waiting on @janekmi and @osalyk)

Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 9 files at r5.
Reviewable status: 12 of 14 files reviewed, 16 unresolved discussions (waiting on @janekmi and @osalyk)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DAOS DAOS-related libpmemobj src/libpmemobj
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants