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

Ignore too large stack in case of dsl_deadlist_merge #14524

Merged
merged 1 commit into from
Feb 27, 2023

Conversation

oshogbo
Copy link
Contributor

@oshogbo oshogbo commented Feb 24, 2023

Motivation and Context

In the case of a regular compilation, the compiler raises a warning for a dsl_deadlist_merge function. However, in debug build this can generate an error.

For this single function, let's ignore the limit of stack size.

Signed-off-by: Mariusz Zaborski mariusz.zaborski@klarasystems.com
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.

Description

For this single function override the compiler option of stack size.

How Has This Been Tested?

I have built a project using gcc and clang.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@amotin
Copy link
Member

amotin commented Feb 24, 2023

Does it tell how big is the frame? Can it be fixed instead by some noinline's?

@oshogbo
Copy link
Contributor Author

oshogbo commented Feb 24, 2023

In debug:

error: the frame size of 1040 bytes is larger than 1024 bytes

@amotin
Copy link
Member

amotin commented Feb 24, 2023

error: the frame size of 1040 bytes is larger than 1024 bytes

That is quite a lot for kernel. Would be good to make sure we've done everything we could to reduce it.

@behlendorf
Copy link
Contributor

It looks like the two zap_attribute_t locals za, and pza are responsible for over half of the usage. Let's just relocate them to the heap.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Feb 25, 2023
In the case of a regular compilation, the compiler
raises a warning for a dsl_deadlist_merge function, that
the stack size is to large. In debug build this can
generate an error.

Move to large structores to heap.

Signed-off-by: Mariusz Zaborski <mariusz.zaborski@klarasystems.com>
Sponsored-by:	Klara, Inc.
Sponsored-by:	Wasabi Technology, Inc.
@@ -875,28 +875,31 @@ dsl_deadlist_merge(dsl_deadlist_t *dl, uint64_t obj, dmu_tx_t *tx)
return;
}

za = kmem_alloc(sizeof (*za), KM_SLEEP);
pza = kmem_alloc(sizeof (*pza), KM_SLEEP);
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be more efficient to do a single allocation like this:

za = kmem_alloc(sizeof (*za) * 2, KM_SLEEP);
pza = &za[1];

Then we would free the memory by calling kmem_free(za, sizeof (*za) * 2); at the end of the function.

That said, I do not feel strongly about this, so I am fine with merging this either way. This is purely a suggestion for a way to minimize the runtime overhead of using dynamic memory to reduce stack space usage that stood out to me.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Feb 27, 2023
@behlendorf behlendorf merged commit 3b9309a into openzfs:master Feb 27, 2023
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 3, 2023
In the case of a regular compilation, the compiler
raises a warning for a dsl_deadlist_merge function, that
the stack size is to large. In debug build this can
generate an error.

Move large structures to heap.

Reviewed-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Mariusz Zaborski <mariusz.zaborski@klarasystems.com>
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Closes openzfs#14524
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 1, 2023
In the case of a regular compilation, the compiler
raises a warning for a dsl_deadlist_merge function, that
the stack size is to large. In debug build this can
generate an error.

Move large structures to heap.

Reviewed-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Mariusz Zaborski <mariusz.zaborski@klarasystems.com>
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Closes openzfs#14524
behlendorf pushed a commit that referenced this pull request Jun 1, 2023
In the case of a regular compilation, the compiler
raises a warning for a dsl_deadlist_merge function, that
the stack size is to large. In debug build this can
generate an error.

Move large structures to heap.

Reviewed-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Mariusz Zaborski <mariusz.zaborski@klarasystems.com>
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Closes #14524
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants