Refactor `CONFIG_SPE` check on Linux/powerpc #14623

Low-power · 2023-03-14T07:31:09Z

Motivation and Context

Commit 5401472 adds a check to call enable_kernel_spe and disable_kernel_spe only if CONFIG_SPE is defined. This introduces redundant implementations of kfpu_begin() and kfpu_end().

Description

Refactor this check in a way similar to what CONFIG_ALTIVEC and CONFIG_VSX are checked in #14591, in order to remove redundant codes.

How Has This Been Tested?

Tested on my POWER7-based machine without CONFIG_SPE. May need more testings on machines with CONFIG_SPE.

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.

Low-power · 2023-03-14T13:02:34Z

Hold on!

When testing with CONFIG_SPE=y, I found an severe issue in this change.

Working on fixes.

mcmilk · 2023-03-14T13:14:40Z

Hold on!

When testing with CONFIG_SPE=y, I found an severe issue in this change.

Working on fixes.

What can go wrong, it's a simple ifdef ... when it's wrong, it was also wrong before.

Signed-off-by: WHR <msl0000023508@gmail.com>

Commit 5401472 adds a check to call enable_kernel_spe and disable_kernel_spe only if CONFIG_SPE is defined. Refactor this check in a way similar to what CONFIG_ALTIVEC and CONFIG_VSX are checked, in order to remove redundant kfpu_begin() and kfpu_end() implementations. Signed-off-by: WHR <msl0000023508@gmail.com>

Low-power · 2023-03-14T13:31:14Z

The semicolons after the functions calls are missing from #14591. I have no idea how it didn't trigger a build failure when I testing #14591 for myself.

Has added a separated commit (6b81956) to fix it.

it was also wrong before.

You are right.

mcmilk · 2023-03-14T14:10:52Z

I like this code snippet more:

#define kfpu_begin()                            \
        {                                       \
                preempt_disable();              \
#ifdef  CONFIG_ALTIVEC
                enable_kernel_altivec();
#endif
#ifdef  CONFIG_VSX
                enable_kernel_vsx();
#endif
#ifdef CONFIG_SPE
                enable_kernel_spe();
#endif
        }
#define kfpu_end()                              \
        {                                       \
#ifdef CONFIG_SPE
                disable_kernel_spe();           \
#endif
#ifdef  CONFIG_VSX
                disable_kernel_vsx();           \
#endif
#ifdef  CONFIG_ALTIVEC
                disable_kernel_altivec();       \
#endif
                preempt_enable();               \
        }
#endif

Edit: Some tabs an backslashes are not set ... but you see what I mean ;-)

Low-power · 2023-03-14T14:40:42Z

I like this code snippet more:

But #if didn't work with continued lines.

mcmilk · 2023-03-14T16:24:13Z

I like this code snippet more:

But #if didn't work with continued lines.

YES, you are right, and inlining would be bad for performance :-/

Can we take the ; to the define like this:

#ifdef  CONFIG_ALTIVEC
#define ENABLE_KERNEL_ALTIVEC   enable_kernel_altivec();
#define DISABLE_KERNEL_ALTIVEC  disable_kernel_altivec();
#else
#define ENABLE_KERNEL_ALTIVEC
#define DISABLE_KERNEL_ALTIVEC
#endif

#ifdef  CONFIG_VSX
#define ENABLE_KERNEL_VSX       enable_kernel_vsx();
#define DISABLE_KERNEL_VSX      disable_kernel_vsx();
#else
#define ENABLE_KERNEL_VSX
#define DISABLE_KERNEL_VSX
#endif

#ifdef  CONFIG_SPE
#define ENABLE_KERNEL_SPE       enable_kernel_spe();
#define DISABLE_KERNEL_SPE      disable_kernel_spe();
#else
#define ENABLE_KERNEL_SPE
#define DISABLE_KERNEL_SPE
#endif

#define kfpu_begin()                            \
        {                                       \
                preempt_disable();              \
                ENABLE_KERNEL_ALTIVEC           \
                ENABLE_KERNEL_VSX               \
                ENABLE_KERNEL_SPE               \
        }

#define kfpu_end()                              \
        {                                       \
                DISABLE_KERNEL_SPE              \
                DISABLE_KERNEL_VSX              \
                DISABLE_KERNEL_ALTIVEC          \
                preempt_enable();               \
        }

Low-power · 2023-03-14T16:39:44Z

Can we take the ; to the define like this:

This should also be ok, I think.

mcmilk · 2023-03-14T21:48:11Z

include/os/linux/kernel/linux/simd_powerpc.h

-#else /* CONFIG_SPE */
+#ifdef	CONFIG_SPE
+#define	ENABLE_KERNEL_SPE	enable_kernel_spe()
+#define	DISABLE_KERNEL_SPE	disable_kernel_spe()


From:

#define ENABLE_KERNEL_SPE enable_kernel_spe() #define DISABLE_KERNEL_SPE disable_kernel_spe()

To:

#define ENABLE_KERNEL_SPE enable_kernel_spe(); #define DISABLE_KERNEL_SPE disable_kernel_spe();

Then I will approve ;)

Why the preference for including the ; in the #define? This seems like it should be purely cosmetic, and our usual convention is to not hide it in the macro. Though I don't feel strongly about it.

Yes, it's just cosmetic. My variant just defines it all away - which seems nicer to me.
But when there is a usual convention which does does not hide the ; - then we should follow this convention.

@Low-power - sorry for the noise ;-)

mcmilk · 2023-03-14T21:49:44Z

include/os/linux/kernel/linux/simd_powerpc.h

-		ENABLE_KERNEL_VSX		\
+		ENABLE_KERNEL_ALTIVEC;		\
+		ENABLE_KERNEL_VSX;		\
+		ENABLE_KERNEL_SPE;		\


And here the other way:
ENABLE_KERNEL_SPE; --> ENABLE_KERNEL_SPE

Low-power · 2023-03-15T04:57:52Z

Before I going to amend my commits, I would like to ask, are you sure this is better?

I mean, by searching the source tree, the only occurrences of #define with trailing ; are some pseudo instructions for assembler:

whr@debian:~/src/zfs$ grep -E '#[[:space:]]*define[[:space:]]+[A-Za-z0-9_]+[[:space:]].+;$' --include "*.[ch]" -r *
include/os/linux/spl/sys/ia32/asm_linkage.h:#define	D16	data16;
include/os/linux/spl/sys/ia32/asm_linkage.h:#define	A16	addr16;
include/os/linux/spl/sys/ia32/asm_linkage.h:#define	D16	.byte	0x66;
include/os/linux/spl/sys/ia32/asm_linkage.h:#define	A16	.byte	0x67;
include/os/freebsd/spl/sys/ia32/asm_linkage.h:#define	D16	data16;
include/os/freebsd/spl/sys/ia32/asm_linkage.h:#define	A16	addr16;
include/os/freebsd/spl/sys/ia32/asm_linkage.h:#define	D16	.byte	0x66;
include/os/freebsd/spl/sys/ia32/asm_linkage.h:#define	A16	.byte	0x67;
lib/libspl/include/os/linux/sys/ia32/asm_linkage.h:#define	D16	data16;
lib/libspl/include/os/linux/sys/ia32/asm_linkage.h:#define	A16	addr16;
lib/libspl/include/os/linux/sys/ia32/asm_linkage.h:#define	D16	.byte	0x66;
lib/libspl/include/os/linux/sys/ia32/asm_linkage.h:#define	A16	.byte	0x67;
lib/libspl/include/os/freebsd/sys/ia32/asm_linkage.h:#define	D16	data16;
lib/libspl/include/os/freebsd/sys/ia32/asm_linkage.h:#define	A16	addr16;
lib/libspl/include/os/freebsd/sys/ia32/asm_linkage.h:#define	D16	.byte	0x66;
lib/libspl/include/os/freebsd/sys/ia32/asm_linkage.h:#define	A16	.byte	0x67;

Low-power · 2023-03-15T05:19:06Z

Tested with Linux 5.15.96 and 6.2.1.

mcmilk · 2023-03-15T06:17:14Z

Before I going to amend my commits, I would like to ask, are you sure this is better?

Sorry ... if you want, you can rebase to the first variant. Both variants are fine and fix the ppc64 code path.

Low-power · 2023-03-15T13:42:59Z

Well, if I change the grep(1) command to

grep -E '#[[:space:]]*define[[:space:]]+[A-Za-z0-9_(,)]+[[:space:]].+;$' --include "*.[ch]" -r *

it will give some more results.
While some results are indeed questionable (will producing redundant ;), some other such as CALLB_CPR_ASSERT in include/os/freebsd/spl/sys/callb.h, are written to hide ; in the macro.

And I also found the similar style in your recent commit fe6a7b7 (macro MEMORY_BARRIER).

I added those macros simply because the impossibility of using #if inside a single #define, so I think as long as it looks clear, either style is fine.

Anyway if the other style is desired, it should still be available on 0ed5911.

Commit 5401472 adds a check to call enable_kernel_spe and disable_kernel_spe only if CONFIG_SPE is defined. Refactor this check in a way similar to what CONFIG_ALTIVEC and CONFIG_VSX are checked, in order to remove redundant kfpu_begin() and kfpu_end() implementations. Reviewed-by: Tino Reichardt <milky-zfs@mcmilk.de> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: WHR <msl0000023508@gmail.com> Closes #14623

Reviewed-by: Tino Reichardt <milky-zfs@mcmilk.de> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: WHR <msl0000023508@gmail.com> Closes openzfs#14623

Commit 5401472 adds a check to call enable_kernel_spe and disable_kernel_spe only if CONFIG_SPE is defined. Refactor this check in a way similar to what CONFIG_ALTIVEC and CONFIG_VSX are checked, in order to remove redundant kfpu_begin() and kfpu_end() implementations. Reviewed-by: Tino Reichardt <milky-zfs@mcmilk.de> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: WHR <msl0000023508@gmail.com> Closes openzfs#14623

Reviewed-by: Tino Reichardt <milky-zfs@mcmilk.de> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: WHR <msl0000023508@gmail.com> Closes openzfs#14623

Commit 5401472 adds a check to call enable_kernel_spe and disable_kernel_spe only if CONFIG_SPE is defined. Refactor this check in a way similar to what CONFIG_ALTIVEC and CONFIG_VSX are checked, in order to remove redundant kfpu_begin() and kfpu_end() implementations. Reviewed-by: Tino Reichardt <milky-zfs@mcmilk.de> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: WHR <msl0000023508@gmail.com> Closes openzfs#14623

Reviewed-by: Tino Reichardt <milky-zfs@mcmilk.de> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: WHR <msl0000023508@gmail.com> Closes openzfs#14623

Commit 5401472 adds a check to call enable_kernel_spe and disable_kernel_spe only if CONFIG_SPE is defined. Refactor this check in a way similar to what CONFIG_ALTIVEC and CONFIG_VSX are checked, in order to remove redundant kfpu_begin() and kfpu_end() implementations. Reviewed-by: Tino Reichardt <milky-zfs@mcmilk.de> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: WHR <msl0000023508@gmail.com> Closes openzfs#14623

Reviewed-by: Tino Reichardt <milky-zfs@mcmilk.de> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: WHR <msl0000023508@gmail.com> Closes openzfs#14623

Commit 5401472 adds a check to call enable_kernel_spe and disable_kernel_spe only if CONFIG_SPE is defined. Refactor this check in a way similar to what CONFIG_ALTIVEC and CONFIG_VSX are checked, in order to remove redundant kfpu_begin() and kfpu_end() implementations. Reviewed-by: Tino Reichardt <milky-zfs@mcmilk.de> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: WHR <msl0000023508@gmail.com> Closes openzfs#14623

mcmilk approved these changes Mar 14, 2023

View reviewed changes

Low-power added 2 commits March 14, 2023 21:24

Fix missing semicolons in commit 1f196e3

a4021d0

Signed-off-by: WHR <msl0000023508@gmail.com>

Low-power force-pushed the linux-powerpc-spe-refactor branch from 592bed1 to 0ed5911 Compare March 14, 2023 13:29

Low-power requested a review from mcmilk March 14, 2023 13:32

behlendorf approved these changes Mar 14, 2023

View reviewed changes

behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Mar 14, 2023

mcmilk suggested changes Mar 14, 2023

View reviewed changes

Low-power force-pushed the linux-powerpc-spe-refactor branch from 0ed5911 to 69ff0e1 Compare March 15, 2023 05:13

mcmilk approved these changes Mar 15, 2023

View reviewed changes

behlendorf closed this in f31b0d4 Mar 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `CONFIG_SPE` check on Linux/powerpc #14623

Refactor `CONFIG_SPE` check on Linux/powerpc #14623

Low-power commented Mar 14, 2023

Low-power commented Mar 14, 2023

mcmilk commented Mar 14, 2023

Low-power commented Mar 14, 2023

mcmilk commented Mar 14, 2023 •

edited

Loading

Low-power commented Mar 14, 2023

mcmilk commented Mar 14, 2023

Low-power commented Mar 14, 2023

mcmilk Mar 14, 2023

behlendorf Mar 14, 2023

mcmilk Mar 15, 2023

mcmilk Mar 14, 2023

Low-power commented Mar 15, 2023 •

edited

Loading

Low-power commented Mar 15, 2023 •

edited

Loading

mcmilk commented Mar 15, 2023

Low-power commented Mar 15, 2023 •

edited

Loading

Refactor CONFIG_SPE check on Linux/powerpc #14623

Refactor CONFIG_SPE check on Linux/powerpc #14623

Conversation

Low-power commented Mar 14, 2023

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

Low-power commented Mar 14, 2023

mcmilk commented Mar 14, 2023

Low-power commented Mar 14, 2023

mcmilk commented Mar 14, 2023 • edited Loading

Low-power commented Mar 14, 2023

mcmilk commented Mar 14, 2023

Low-power commented Mar 14, 2023

mcmilk Mar 14, 2023

Choose a reason for hiding this comment

behlendorf Mar 14, 2023

Choose a reason for hiding this comment

mcmilk Mar 15, 2023

Choose a reason for hiding this comment

mcmilk Mar 14, 2023

Choose a reason for hiding this comment

Low-power commented Mar 15, 2023 • edited Loading

Low-power commented Mar 15, 2023 • edited Loading

mcmilk commented Mar 15, 2023

Low-power commented Mar 15, 2023 • edited Loading

Refactor `CONFIG_SPE` check on Linux/powerpc #14623

Refactor `CONFIG_SPE` check on Linux/powerpc #14623

mcmilk commented Mar 14, 2023 •

edited

Loading

Low-power commented Mar 15, 2023 •

edited

Loading

Low-power commented Mar 15, 2023 •

edited

Loading

Low-power commented Mar 15, 2023 •

edited

Loading