Detect HAVE_LARGE_STACKS at compile time #12350

kev009 · 2021-07-11T19:56:55Z

Signed-off-by: Kevin Bowling kbowling@FreeBSD.org

Motivation and Context

On FreeBSD, this OZFS change seems to regress performance of sequential workloads (find /, indexing a mail spool, etc) handily versus the previous implementation. I am unsure if the conditions of b58986e apply equally to FreeBSD stack usage, but we have 16K stacks on common platforms so detect and set HAVE_LARGE_STACKS when it is the case.

Discussed with: @mjguzik

CC @allanjude @amotin

Description

How Has This Been Tested?

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:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

allanjude

Reviewed By: Allan Jude allanjude@freebsd.org

allanjude · 2021-07-11T20:59:23Z

Thanks for looking into this. Do you happen to have any numbers to show how much of a difference this makes?

config/kernel-config-defined.m4

module/Makefile.bsd

amotin · 2021-07-13T01:52:40Z

Thinking about it again, why would it need to be done at configure or even make? If we already have the defines, why not just check them in some header(s)? Why complicate?

kev009 · 2021-07-13T01:55:41Z

Feel free to do it another way, I don't know this code very well.

amotin · 2021-07-13T02:38:10Z

I don't mean anything complicated. Not sure if there is a better header, may be @behlendorf propose one, but I am thinking about something like this, plus equivalent for Linux:

diff --git a/sys/contrib/openzfs/include/os/freebsd/zfs/sys/zfs_context_os.h b/sys/contrib/openzfs/include/os/freebsd/zfs/sys/zfs_context_os.h
index 8dbe907d098..a32eb52c53c 100644
--- a/sys/contrib/openzfs/include/os/freebsd/zfs/sys/zfs_context_os.h
+++ b/sys/contrib/openzfs/include/os/freebsd/zfs/sys/zfs_context_os.h
@@ -41,6 +41,10 @@
 #include <sys/ccompat.h>
 #include <linux/types.h>
 
+#if KSTACK_PAGES * PAGE_SIZE >= 16384
+#define        HAVE_LARGE_STACKS       1
+#endif
+
 #define        cond_resched()          kern_yield(PRI_USER)
 
 #define        taskq_create_sysdc(a, b, d, e, p, dc, f) \
diff --git a/sys/contrib/openzfs/lib/libspl/include/os/freebsd/sys/zfs_context_os.h b/sys/contrib/openzfs/lib/libspl/include/os/freebsd/sys/zfs_context_os.h
index f5a136d2212..0e9cfd77ddc 100644
--- a/sys/contrib/openzfs/lib/libspl/include/os/freebsd/sys/zfs_context_os.h
+++ b/sys/contrib/openzfs/lib/libspl/include/os/freebsd/sys/zfs_context_os.h
@@ -30,5 +30,6 @@
 #define        ZFS_CONTEXT_OS_H_
 
+#define        HAVE_LARGE_STACKS       1
 #define        ZFS_EXPORTS_PATH        "/etc/zfs/exports"
 
 #endif

behlendorf · 2021-07-13T15:39:18Z

I think either approach should work fine, although doing the check as KSTACK_PAGES * PAGE_SIZE >= 16384 on FreeBSD seems like it would be more robust. On Linux we should be able to move that check out of autoconf and in to the openzfs/include/os/linux/zfs/sys/zfs_context_os.h header. Note that in user space we already always set HAVE_LARGE_STACKS=1 in config/Rules.am.

ghost

Great! Just the checkstyle whitespace nits to fix :)

Signed-off-by: Kevin Bowling <kbowling@FreeBSD.org>

kev009 · 2021-07-15T20:16:12Z

I'd also like this merged back to 2.1, is there anything I can do to help with that?

Move HAVE_LARGE_STACKS definitions to header and set when appropriate. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Allan Jude <allan@klarasystems.com> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Signed-off-by: Kevin Bowling <kbowling@FreeBSD.org> Closes #12350

Move HAVE_LARGE_STACKS definitions to header and set when appropriate. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Allan Jude <allan@klarasystems.com> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Signed-off-by: Kevin Bowling <kbowling@FreeBSD.org> Closes openzfs#12350

Move HAVE_LARGE_STACKS definitions to header and set when appropriate. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Allan Jude <allan@klarasystems.com> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Signed-off-by: Kevin Bowling <kbowling@FreeBSD.org> Closes #12350

Move HAVE_LARGE_STACKS definitions to header and set when appropriate. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Allan Jude <allan@klarasystems.com> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Signed-off-by: Kevin Bowling <kbowling@FreeBSD.org> Closes openzfs#12350

allanjude approved these changes Jul 11, 2021

View reviewed changes

amotin reviewed Jul 12, 2021

View reviewed changes

config/kernel-config-defined.m4 Outdated Show resolved Hide resolved

ghost added this to In progress in FreeBSD via automation Jul 12, 2021

ghost added the Status: Code Review Needed Ready for review and testing label Jul 12, 2021

kev009 force-pushed the fbsd-large-stack branch 3 times, most recently from 06ca1a0 to 12c7e8e Compare July 12, 2021 21:28

amotin reviewed Jul 12, 2021

View reviewed changes

config/kernel-config-defined.m4 Outdated Show resolved Hide resolved

module/Makefile.bsd Outdated Show resolved Hide resolved

kev009 force-pushed the fbsd-large-stack branch 2 times, most recently from c4b5e1c to d46a5fd Compare July 13, 2021 00:53

kev009 force-pushed the fbsd-large-stack branch from d46a5fd to f42eb1e Compare July 13, 2021 18:28

kev009 changed the title ~~FreeBSD: Detect and define HAVE_LARGE_STACKS~~ Detect HAVE_LARGE_STACKS at compile time Jul 13, 2021

amotin approved these changes Jul 13, 2021

View reviewed changes

ghost approved these changes Jul 13, 2021

View reviewed changes

Detect HAVE_LARGE_STACKS at compile time

5305056

Signed-off-by: Kevin Bowling <kbowling@FreeBSD.org>

kev009 force-pushed the fbsd-large-stack branch from f42eb1e to 5305056 Compare July 13, 2021 20:01

behlendorf approved these changes Jul 13, 2021

View reviewed changes

behlendorf assigned tonynguien Jul 13, 2021

kev009 mentioned this pull request Jul 13, 2021

FreeBSD: enable spa_thread #10749

Closed

12 tasks

tonynguien added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jul 15, 2021

tonynguien merged commit ca14e08 into openzfs:master Jul 16, 2021

FreeBSD automation moved this from In progress to Done Jul 16, 2021

kev009 deleted the fbsd-large-stack branch July 16, 2021 21:02

kev009 mentioned this pull request Jul 16, 2021

[2.1] Detect HAVE_LARGE_STACKS at compile time #12384

Merged

13 tasks

kev009 mentioned this pull request Sep 23, 2021

[2.1] Detect HAVE_LARGE_STACKS at compile time #12584

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect HAVE_LARGE_STACKS at compile time #12350

Detect HAVE_LARGE_STACKS at compile time #12350

kev009 commented Jul 11, 2021 •

edited

allanjude left a comment

allanjude commented Jul 11, 2021

amotin commented Jul 13, 2021

kev009 commented Jul 13, 2021

amotin commented Jul 13, 2021 •

edited

behlendorf commented Jul 13, 2021

ghost left a comment

kev009 commented Jul 15, 2021

Detect HAVE_LARGE_STACKS at compile time #12350

Detect HAVE_LARGE_STACKS at compile time #12350

Conversation

kev009 commented Jul 11, 2021 • edited

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

allanjude left a comment

Choose a reason for hiding this comment

allanjude commented Jul 11, 2021

amotin commented Jul 13, 2021

kev009 commented Jul 13, 2021

amotin commented Jul 13, 2021 • edited

behlendorf commented Jul 13, 2021

ghost left a comment

Choose a reason for hiding this comment

kev009 commented Jul 15, 2021

kev009 commented Jul 11, 2021 •

edited

amotin commented Jul 13, 2021 •

edited