| Message ID | 20250409154356.423512-1-visitorckw@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce parity_odd() and refactor redundant parity code | expand |
On Wed, Apr 09, 2025 at 11:43:43PM +0800, Kuan-Wei Chiu wrote: > Several parts of the kernel contain open-coded and redundant > implementations of parity calculation. This patch series introduces > a unified helper, parity_odd(), to simplify and standardize these > cases. > > The first patch renames parity8() to parity_odd(), changes its argument Alright, if it's an extension of the area of applicability, it should be renamed to just parity(). I already shared a table that summarized the drivers authors' view on that, and they clearly prefer not to add the suffix - 13 vs 2. The __builtin_parity() doesn't care of suffix as well. https://lore.kernel.org/all/Z9GtcNJie8TRKywZ@thinkpad/ Yes, the argument that boolean function should explain itself sounds correct, but in this case, comment on top of the function looks enough to me. The existing codebase doesn't care about the suffix as well. If no strong preference, let's just pick a short and sweet name? > type from u8 to u64 for broader applicability, and updates its return > type from int to bool to make its usage and return semantics more > intuitive-returning true for odd parity and false for even parity. It > also adds __attribute_const__ to enable compiler optimizations. That's correct and nice, but can you support it with a bloat-o-meter's before/after and/or asm snippets? I also think it worth to be a separate patch, preferably the last patch in the series. > While more efficient implementations may exist, further optimization is > postponed until a use case in performance-critical paths arises. > > Subsequent patches refactor various kernel components to replace > open-coded parity logic with the new helper, reducing code duplication > and improving consistency. > > Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com> > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> > --- > > To H. Peter: > I understand your preference for a parity8/16/32/64() style interface, > and I agree that such a design would better accommodate potential > arch-specific implementations. However, I suspect there are very few, > if any, users who care about the performance of parity calculations > enough to warrant such optimizations. So my inclination is to defer any > arch-specific or optimized implementations until we see parity_odd() > being used in hot paths. > > Changes in v4: > - Rename parity8() to parity_odd(). > - Change the argument type from u8 to u64. > - Use a single parity_odd() function. > > Changes in v3: > - Avoid using __builtin_parity. > - Change return type to bool. > - Drop parity() macro. > - Change parityXX() << y to !!parityXX() << y. > > Changes in v2: > - Provide fallback functions for __builtin_parity() when the compiler > decides not to inline it > - Use __builtin_parity() when no architecture-specific implementation > is available > - Optimize for constant folding when val is a compile-time constant > - Add a generic parity() macro > - Drop the x86 bootflag conversion patch since it has been merged into > the tip tree > > v3: https://lore.kernel.org/lkml/20250306162541.2633025-1-visitorckw@gmail.com/ > v1: https://lore.kernel.org/lkml/20250223164217.2139331-1-visitorckw@gmail.com/ > v2: https://lore.kernel.org/lkml/20250301142409.2513835-1-visitorckw@gmail.com/ > > Kuan-Wei Chiu (13): > bitops: Change parity8() to parity_odd() with u64 input and bool > return type > media: media/test_drivers: Replace open-coded parity calculation with > parity_odd() > media: pci: cx18-av-vbi: Replace open-coded parity calculation with > parity_odd() > media: saa7115: Replace open-coded parity calculation with > parity_odd() > serial: max3100: Replace open-coded parity calculation with > parity_odd() > lib/bch: Replace open-coded parity calculation with parity_odd() > Input: joystick - Replace open-coded parity calculation with > parity_odd() > net: ethernet: oa_tc6: Replace open-coded parity calculation with > parity_odd() > wifi: brcm80211: Replace open-coded parity calculation with > parity_odd() > drm/bridge: dw-hdmi: Replace open-coded parity calculation with > parity_odd() > mtd: ssfdc: Replace open-coded parity calculation with parity_odd() > fsi: i2cr: Replace open-coded parity calculation with parity_odd() > nfp: bpf: Replace open-coded parity calculation with parity_odd() > > arch/x86/kernel/bootflag.c | 4 +-- > drivers/fsi/fsi-master-i2cr.c | 20 +++------------ > .../drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 8 ++---- > drivers/hwmon/spd5118.c | 2 +- > drivers/i3c/master/dw-i3c-master.c | 2 +- > drivers/i3c/master/i3c-master-cdns.c | 2 +- > drivers/i3c/master/mipi-i3c-hci/dat_v1.c | 2 +- > drivers/input/joystick/grip_mp.c | 17 ++----------- > drivers/input/joystick/sidewinder.c | 25 ++++--------------- > drivers/media/i2c/saa7115.c | 12 ++------- > drivers/media/pci/cx18/cx18-av-vbi.c | 12 ++------- > .../media/test-drivers/vivid/vivid-vbi-gen.c | 8 ++---- > drivers/mtd/ssfdc.c | 20 +++------------ > drivers/net/ethernet/netronome/nfp/nfp_asm.c | 7 +----- > drivers/net/ethernet/oa_tc6.c | 19 +++----------- > .../broadcom/brcm80211/brcmsmac/dma.c | 18 ++----------- > drivers/tty/serial/max3100.c | 3 ++- > include/linux/bitops.h | 19 ++++++++------ > lib/bch.c | 14 +---------- > 19 files changed, 49 insertions(+), 165 deletions(-) OK, now it looks like a nice consolidation and simplification of code base. Thanks for the work. Thanks, Yury
On Wed, Apr 09, 2025 at 12:54:35PM -0400, Yury Norov wrote: > On Wed, Apr 09, 2025 at 11:43:43PM +0800, Kuan-Wei Chiu wrote: > > Several parts of the kernel contain open-coded and redundant > > implementations of parity calculation. This patch series introduces > > a unified helper, parity_odd(), to simplify and standardize these > > cases. > > > > The first patch renames parity8() to parity_odd(), changes its argument > > Alright, if it's an extension of the area of applicability, it should be > renamed to just parity(). I already shared a table that summarized the > drivers authors' view on that, and they clearly prefer not to add the > suffix - 13 vs 2. The __builtin_parity() doesn't care of suffix as well. > > https://lore.kernel.org/all/Z9GtcNJie8TRKywZ@thinkpad/ > > Yes, the argument that boolean function should explain itself sounds > correct, but in this case, comment on top of the function looks enough > to me. > > The existing codebase doesn't care about the suffix as well. If no > strong preference, let's just pick a short and sweet name? > I don't have a strong preference for the name, but if I had to guess the return value from the function prototype, I would intuitively expect an int to return "0 for even and 1 for odd," and a bool to return "true for even, false for odd." I recall Jiri and Jacob shared similar thoughts, which is why I felt adding _odd could provide better clarity. However, I agree that if the kernel doc comment is clear, it might not be a big issue. But David previously mentioned that he doesn't want to rely on checking the function's documentation every time while reading the code. Regardless, I'm flexible as long as we all reach a consensus on the naming. > > type from u8 to u64 for broader applicability, and updates its return > > type from int to bool to make its usage and return semantics more > > intuitive-returning true for odd parity and false for even parity. It > > also adds __attribute_const__ to enable compiler optimizations. > > That's correct and nice, but can you support it with a bloat-o-meter's > before/after and/or asm snippets? I also think it worth to be a separate > patch, preferably the last patch in the series. > I quickly tested it with the x86 defconfig, and it appears that the generated code doesn't change. I forgot who requested the addition during the review process, but I initially thought it would either improve the generated code or leave it unchanged without significantly increasing the source code size. However, if there's no actual difference in the generated code, maybe let's just remove it? Regards, Kuan-Wei > > While more efficient implementations may exist, further optimization is > > postponed until a use case in performance-critical paths arises. > > > > Subsequent patches refactor various kernel components to replace > > open-coded parity logic with the new helper, reducing code duplication > > and improving consistency. > > > > Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com> > > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> > > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> > > --- > > > > To H. Peter: > > I understand your preference for a parity8/16/32/64() style interface, > > and I agree that such a design would better accommodate potential > > arch-specific implementations. However, I suspect there are very few, > > if any, users who care about the performance of parity calculations > > enough to warrant such optimizations. So my inclination is to defer any > > arch-specific or optimized implementations until we see parity_odd() > > being used in hot paths. > > > > Changes in v4: > > - Rename parity8() to parity_odd(). > > - Change the argument type from u8 to u64. > > - Use a single parity_odd() function. > > > > Changes in v3: > > - Avoid using __builtin_parity. > > - Change return type to bool. > > - Drop parity() macro. > > - Change parityXX() << y to !!parityXX() << y. > > > > Changes in v2: > > - Provide fallback functions for __builtin_parity() when the compiler > > decides not to inline it > > - Use __builtin_parity() when no architecture-specific implementation > > is available > > - Optimize for constant folding when val is a compile-time constant > > - Add a generic parity() macro > > - Drop the x86 bootflag conversion patch since it has been merged into > > the tip tree > > > > v3: https://lore.kernel.org/lkml/20250306162541.2633025-1-visitorckw@gmail.com/ > > v1: https://lore.kernel.org/lkml/20250223164217.2139331-1-visitorckw@gmail.com/ > > v2: https://lore.kernel.org/lkml/20250301142409.2513835-1-visitorckw@gmail.com/ > > > > Kuan-Wei Chiu (13): > > bitops: Change parity8() to parity_odd() with u64 input and bool > > return type > > media: media/test_drivers: Replace open-coded parity calculation with > > parity_odd() > > media: pci: cx18-av-vbi: Replace open-coded parity calculation with > > parity_odd() > > media: saa7115: Replace open-coded parity calculation with > > parity_odd() > > serial: max3100: Replace open-coded parity calculation with > > parity_odd() > > lib/bch: Replace open-coded parity calculation with parity_odd() > > Input: joystick - Replace open-coded parity calculation with > > parity_odd() > > net: ethernet: oa_tc6: Replace open-coded parity calculation with > > parity_odd() > > wifi: brcm80211: Replace open-coded parity calculation with > > parity_odd() > > drm/bridge: dw-hdmi: Replace open-coded parity calculation with > > parity_odd() > > mtd: ssfdc: Replace open-coded parity calculation with parity_odd() > > fsi: i2cr: Replace open-coded parity calculation with parity_odd() > > nfp: bpf: Replace open-coded parity calculation with parity_odd() > > > > arch/x86/kernel/bootflag.c | 4 +-- > > drivers/fsi/fsi-master-i2cr.c | 20 +++------------ > > .../drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 8 ++---- > > drivers/hwmon/spd5118.c | 2 +- > > drivers/i3c/master/dw-i3c-master.c | 2 +- > > drivers/i3c/master/i3c-master-cdns.c | 2 +- > > drivers/i3c/master/mipi-i3c-hci/dat_v1.c | 2 +- > > drivers/input/joystick/grip_mp.c | 17 ++----------- > > drivers/input/joystick/sidewinder.c | 25 ++++--------------- > > drivers/media/i2c/saa7115.c | 12 ++------- > > drivers/media/pci/cx18/cx18-av-vbi.c | 12 ++------- > > .../media/test-drivers/vivid/vivid-vbi-gen.c | 8 ++---- > > drivers/mtd/ssfdc.c | 20 +++------------ > > drivers/net/ethernet/netronome/nfp/nfp_asm.c | 7 +----- > > drivers/net/ethernet/oa_tc6.c | 19 +++----------- > > .../broadcom/brcm80211/brcmsmac/dma.c | 18 ++----------- > > drivers/tty/serial/max3100.c | 3 ++- > > include/linux/bitops.h | 19 ++++++++------ > > lib/bch.c | 14 +---------- > > 19 files changed, 49 insertions(+), 165 deletions(-) > > OK, now it looks like a nice consolidation and simplification of code > base. Thanks for the work. > > Thanks, > Yury
On Thu, Apr 10, 2025 at 02:15:30AM +0800, Kuan-Wei Chiu wrote: > On Wed, Apr 09, 2025 at 12:54:35PM -0400, Yury Norov wrote: > > On Wed, Apr 09, 2025 at 11:43:43PM +0800, Kuan-Wei Chiu wrote: > > > Several parts of the kernel contain open-coded and redundant > > > implementations of parity calculation. This patch series introduces > > > a unified helper, parity_odd(), to simplify and standardize these > > > cases. > > > > > > The first patch renames parity8() to parity_odd(), changes its argument > > > > Alright, if it's an extension of the area of applicability, it should be > > renamed to just parity(). I already shared a table that summarized the > > drivers authors' view on that, and they clearly prefer not to add the > > suffix - 13 vs 2. The __builtin_parity() doesn't care of suffix as well. > > > > https://lore.kernel.org/all/Z9GtcNJie8TRKywZ@thinkpad/ > > > > Yes, the argument that boolean function should explain itself sounds > > correct, but in this case, comment on top of the function looks enough > > to me. > > > > The existing codebase doesn't care about the suffix as well. If no > > strong preference, let's just pick a short and sweet name? > > > I don't have a strong preference for the name, but if I had to guess > the return value from the function prototype, I would intuitively > expect an int to return "0 for even and 1 for odd," and a bool to > return "true for even, false for odd." I recall Jiri and Jacob shared > similar thoughts, which is why I felt adding _odd could provide better > clarity. I think they said they are convinced that parity should return 1 for odd because of folding and __builtin_parity() arguments. > However, I agree that if the kernel doc comment is clear, it might not > be a big issue. But David previously mentioned that he doesn't want to > rely on checking the function's documentation every time while reading > the code. He's wrong. Kernel engineers _must_ read documentation, regardless. > Regardless, I'm flexible as long as we all reach a consensus on the > naming. > > > > type from u8 to u64 for broader applicability, and updates its return > > > type from int to bool to make its usage and return semantics more > > > intuitive-returning true for odd parity and false for even parity. It > > > also adds __attribute_const__ to enable compiler optimizations. > > > > That's correct and nice, but can you support it with a bloat-o-meter's > > before/after and/or asm snippets? I also think it worth to be a separate > > patch, preferably the last patch in the series. > > > I quickly tested it with the x86 defconfig, and it appears that the > generated code doesn't change. I forgot who requested the addition > during the review process, but I initially thought it would either > improve the generated code or leave it unchanged without significantly > increasing the source code size. That's what I actually expected, but was shy to guess openly. :). It's hard to imagine how compiler may improve code generation in this case... This attribute is used when there's an asm block, or some non-trivial function call. In this case, the function is self-consistent and makes no calls. And you see, const annotation raises more questions than solves problems. Let's drop it. Thanks, Yury
On 4/9/25 11:33, Yury Norov wrote: >>> >> I don't have a strong preference for the name, but if I had to guess >> the return value from the function prototype, I would intuitively >> expect an int to return "0 for even and 1 for odd," and a bool to >> return "true for even, false for odd." I recall Jiri and Jacob shared >> similar thoughts, which is why I felt adding _odd could provide better >> clarity. > > I think they said they are convinced that parity should return 1 for > odd because of folding and __builtin_parity() arguments. > And for bool, 0 == false, and 1 == true. In fact, the *definitions* for false and true in C (but not C++) is: <stdbool.h>: typedef _Bool bool; #define false 0 #define true 1 If someone wants to make more clear, it would be better to put "typedef bool bit_t" in a common header, but that personally seems ridiculous to me. >>>> type from u8 to u64 for broader applicability, and updates its return >>>> type from int to bool to make its usage and return semantics more >>>> intuitive-returning true for odd parity and false for even parity. It >>>> also adds __attribute_const__ to enable compiler optimizations. >>> >>> That's correct and nice, but can you support it with a bloat-o-meter's >>> before/after and/or asm snippets? I also think it worth to be a separate >>> patch, preferably the last patch in the series. >>> >> I quickly tested it with the x86 defconfig, and it appears that the >> generated code doesn't change. I forgot who requested the addition >> during the review process, but I initially thought it would either >> improve the generated code or leave it unchanged without significantly >> increasing the source code size. > > That's what I actually expected, but was shy to guess openly. :). It's > hard to imagine how compiler may improve code generation in this case... > > This attribute is used when there's an asm block, or some non-trivial > function call. In this case, the function is self-consistent and makes > no calls. And you see, const annotation raises more questions than > solves problems. Let's drop it. Ah yes; one of the quirks about gcc asm is that an asm is implicitly assumed "const" (with no memory operands) or "pure" (with memory operands) unless declared volatile or given an explicit "memory" clobber. So yes, the compiler can most definitely derive the constness from the form of the function even in the variable case. I would still like to see __builtin_parity() being used as an architecture opt-in; it can, of course, also be unconditionally used in the constant case. So in the end one of these two become my preferred implementation, and I really don't think it is very complicated: #ifndef use_builtin_parity #define use_builtin_parity(x) __builtin_constant_p(x) #endif static inline bool parity8(u8 val) { if (use_builtin_parity(val)) return __builtin_parity(val); val ^= val >> 4; return (0x6996 >> (val & 0xf)) & 1; } static inline bool parity16(u16 val) { if (use_builtin_parity(val)) return __builtin_parity(val); return parity8(val ^ (val >> 8)); } static inline bool parity32(u32 val) { if (use_builtin_parity(val)) return __builtin_parity(val); return parity16(val ^ (val >> 16)); } static inline bool parity64(u64 val) { if (use_builtin_parity(val)) return __builtin_parityll(val); return parity32(val ^ (val >> 32)); } This means that an architecture -- in particular, x86 -- can still ask to use __builtin_parity*() directly. It means that architectures on which __builtin_parity*() produces bad code should either complain to the gcc/clang team and have it fixed, or we can add additional mechanism for them to override the implementation at that time. The alternative is to stop worrying about overengineering, and just do it once and for all: #ifndef parity8 static inline bool parity8(u8 val) { val ^= val >> 4; return (0x6996 >> (val & 0xf)) & 1; } #endif #ifndef parity16 static inline bool parity16(u16 val) { return parity8(val ^ (val >> 8)); } #endif #ifndef parity32 static inline bool parity32(u32 val) { return parity16(val ^ (val >> 16)); } #endif #ifndef parity64 static inline bool parity64(u64 val) { return parity32(val ^ (val >> 32)); } #endif In either case, instead of packing the cascade into one function, make good use of it. In the latter case, __builtin_constant_p() isn't necessary as it puts the onus on the architecture to separate out const and non-const cases, if it matters -- which it doesn't if the architecture simply wants to use __builtin_parity: #define parity8(x) ((bool) __builtin_parity((u8)(x))) #define parity16(x) ((bool) __builtin_parity((u16)(x))) #define parity32(x) ((bool) __builtin_parity((u32)(x))) #define parity64(x) ((bool) __builtin_parityll((u64)(x))) As stated before, I don't really see that the parity function itself would be very suitable for a generic helper, but if it were to, then using the "standard" macro construct for it would seem to be the better option. (And I would be very much in favor of not open-coding the helper everywhere but to macroize it; effectively creating a C++ template equivalent. It is out of scope for this project, though.) -hpa
On Wed, Apr 09, 2025 at 07:09:28PM -0700, H. Peter Anvin wrote: > On 4/9/25 11:33, Yury Norov wrote: > > > > > > > I don't have a strong preference for the name, but if I had to guess > > > the return value from the function prototype, I would intuitively > > > expect an int to return "0 for even and 1 for odd," and a bool to > > > return "true for even, false for odd." I recall Jiri and Jacob shared > > > similar thoughts, which is why I felt adding _odd could provide better > > > clarity. > > > > I think they said they are convinced that parity should return 1 for > > odd because of folding and __builtin_parity() arguments. > > > > And for bool, 0 == false, and 1 == true. In fact, the *definitions* for > false and true in C (but not C++) is: > > <stdbool.h>: > typedef _Bool bool; > #define false 0 > #define true 1 > > If someone wants to make more clear, it would be better to put "typedef bool > bit_t" in a common header, but that personally seems ridiculous to me. > >>>> type from u8 to u64 for broader applicability, and updates its return > > > > > type from int to bool to make its usage and return semantics more > > > > > intuitive-returning true for odd parity and false for even parity. It > > > > > also adds __attribute_const__ to enable compiler optimizations. > > > > > > > > That's correct and nice, but can you support it with a bloat-o-meter's > > > > before/after and/or asm snippets? I also think it worth to be a separate > > > > patch, preferably the last patch in the series. > > > > > > > I quickly tested it with the x86 defconfig, and it appears that the > > > generated code doesn't change. I forgot who requested the addition > > > during the review process, but I initially thought it would either > > > improve the generated code or leave it unchanged without significantly > > > increasing the source code size. > > > > That's what I actually expected, but was shy to guess openly. :). It's > > hard to imagine how compiler may improve code generation in this case... > > > > This attribute is used when there's an asm block, or some non-trivial > > function call. In this case, the function is self-consistent and makes > > no calls. And you see, const annotation raises more questions than > > solves problems. Let's drop it. > > Ah yes; one of the quirks about gcc asm is that an asm is implicitly assumed > "const" (with no memory operands) or "pure" (with memory operands) unless > declared volatile or given an explicit "memory" clobber. > > So yes, the compiler can most definitely derive the constness from the form > of the function even in the variable case. > > I would still like to see __builtin_parity() being used as an architecture > opt-in; it can, of course, also be unconditionally used in the constant > case. > > So in the end one of these two become my preferred implementation, and I > really don't think it is very complicated: > > #ifndef use_builtin_parity > #define use_builtin_parity(x) __builtin_constant_p(x) > #endif > > static inline bool parity8(u8 val) > { > if (use_builtin_parity(val)) > return __builtin_parity(val); > val ^= val >> 4; > return (0x6996 >> (val & 0xf)) & 1; > } > > static inline bool parity16(u16 val) > { > if (use_builtin_parity(val)) > return __builtin_parity(val); > return parity8(val ^ (val >> 8)); > } > > static inline bool parity32(u32 val) > { > if (use_builtin_parity(val)) > return __builtin_parity(val); > return parity16(val ^ (val >> 16)); > } > > static inline bool parity64(u64 val) > { > if (use_builtin_parity(val)) > return __builtin_parityll(val); > return parity32(val ^ (val >> 32)); > } > > This means that an architecture -- in particular, x86 -- can still ask to > use __builtin_parity*() directly. It means that architectures on which > __builtin_parity*() produces bad code should either complain to the > gcc/clang team and have it fixed, or we can add additional mechanism for > them to override the implementation at that time. > > The alternative is to stop worrying about overengineering, and just do it > once and for all: > > #ifndef parity8 > static inline bool parity8(u8 val) > { > val ^= val >> 4; > return (0x6996 >> (val & 0xf)) & 1; > } > #endif > > #ifndef parity16 > static inline bool parity16(u16 val) > { > return parity8(val ^ (val >> 8)); > } > #endif > > #ifndef parity32 > static inline bool parity32(u32 val) > { > return parity16(val ^ (val >> 16)); > } > #endif > > #ifndef parity64 > static inline bool parity64(u64 val) > { > return parity32(val ^ (val >> 32)); > } > #endif > > In either case, instead of packing the cascade into one function, make good > use of it. > > In the latter case, __builtin_constant_p() isn't necessary as it puts the > onus on the architecture to separate out const and non-const cases, if it > matters -- which it doesn't if the architecture simply wants to use > __builtin_parity: > > #define parity8(x) ((bool) __builtin_parity((u8)(x))) > #define parity16(x) ((bool) __builtin_parity((u16)(x))) > #define parity32(x) ((bool) __builtin_parity((u32)(x))) > #define parity64(x) ((bool) __builtin_parityll((u64)(x))) > > As stated before, I don't really see that the parity function itself would > be very suitable for a generic helper, but if it were to, then using the > "standard" macro construct for it would seem to be the better option. > > (And I would be very much in favor of not open-coding the helper everywhere > but to macroize it; effectively creating a C++ template equivalent. It is > out of scope for this project, though.) > IIUC, you prefer using the parity8/16/32/64() interface with __builtin_parity(), regardless of whether there are users on the hot path? If the maintainer has no concerns about maintenance burden, I also lean toward this interface. While I don't think the implementation is particularly complex, I'm not the maintainer, so I'd rather not argue about the maintenance aspect. Also, to be clear, I don't think I'm the right person to provide evidence that performance or code generation matters to any user. OTOH, If we do end up going with one of the two approaches, since bitops.h is included (directly or indirectly) by many files while parity is only used in fewer than 20, perhaps we should move the parity-related code to a separate parity.h. It doesn't necessarily have to be maintained by Yury - it could be someone else, or me. Regards, Kuan-Wei