| Message ID | cover.1691788036.git.falcon@tinylab.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | tools/nolibc: fix up size inflat regression | expand |
On Sat, Aug 12, 2023 at 05:49:36AM +0800, Zhangjin Wu wrote: > After these two patches, will send the proposed my_syscall() patchset > tomorrow, it can even further reduce more type conversions and therefore > reduce more binary bytes, here is a preview of the testing result: > > // with the coming my_syscall() patchset, sys_* from functionsn to macros > i386: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 19250 > x86_64: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 21733 (...) > It can also shrink the whole sys.h from 1171 lines to around 738 lines. Please, Zhangjin, please. Let's stop constantly speaking about potential future improvements when the present is broken. It needlessly adds a lot of noise in the discussion and tends to encourage you to explore areas that are incompatible with what is required to fix the breakage, and very likely steers your approach to fixes in a direction that you think is compatible with such future paths. But as long as existing code is broken you cannot speculate on how better the next iteration will be, because it's built on a broken basis. And I would like to remind that the *only* reason for the current breakage is this attempt to save even more code lines, that was not a requirement at all in the first place! Sure it can be fine to remove code when possible, but not at the cost of trying to force squares to enter round holes like this. The reality is that *some* syscalls are different and *some* archs are different, and these differences have to be taken into account, and if we keep exceptions it's fine. So let's only speak about this later once the issue is completely solved. Thanks, Willy
Hi, Willy > On Sat, Aug 12, 2023 at 05:49:36AM +0800, Zhangjin Wu wrote: > > After these two patches, will send the proposed my_syscall() patchset > > tomorrow, it can even further reduce more type conversions and therefore > > reduce more binary bytes, here is a preview of the testing result: > > > > // with the coming my_syscall() patchset, sys_* from functionsn to macros > > i386: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 19250 > > x86_64: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 21733 > (...) > > It can also shrink the whole sys.h from 1171 lines to around 738 lines. > > Please, Zhangjin, please. Let's stop constantly speaking about potential > future improvements when the present is broken. It needlessly adds a lot > of noise in the discussion and tends to encourage you to explore areas > that are incompatible with what is required to fix the breakage, and > very likely steers your approach to fixes in a direction that you think > is compatible with such future paths. But as long as existing code is > broken you cannot speculate on how better the next iteration will be, > because it's built on a broken basis. And I would like to remind that > the *only* reason for the current breakage is this attempt to save even > more code lines, that was not a requirement at all in the first place! > Sure it can be fine to remove code when possible, but not at the cost of > trying to force squares to enter round holes like this. The reality is > that *some* syscalls are different and *some* archs are different, and > these differences have to be taken into account, and if we keep exceptions > it's fine. > Agree very much, that's why I didn't send the new patchset but only send these two ones about size inflate regression, I don't want to discuss more than one issue at a time either (and you also have shared this idea several times) ;-) The progress and preview data here is only because the patch 1/2 [1] is an important preparation of the new patchset, the data here is more or less providing a selling point why we need patch 1/2, I have explained it in this reply [2]. Of course, we can roll them back directly, and If we do need sys_brk/mmap return 'long', we can revert the rolling-back and apply patch 1/2. [PATCH v6 1/2] tools/nolibc: let sys_brk, sys_mmap and sys_mmap2 return long > So let's only speak about this later once the issue is completely solved. > Ok, it is the right direction. Best regards, Zhangjin --- [1]: https://lore.kernel.org/lkml/82b584cbda5cee8d5318986644a2a64ba749a098.1691788036.git.falcon@tinylab.org/ [2]: https://lore.kernel.org/lkml/20230813132620.19411-1-falcon@tinylab.org/ > Thanks, > Willy
Hi, Willy As we discussed in v5, I have proposed a my_syscall() macro, it can convert all of the sys_* functions to macros, and such macros can simply preserve input types from library routines and inherit the 'long' return type from my_syscall<N>. As a result, our __sysret() helper will only require to accept integer types and therefore we can simply revert it to our old sign comparison version (but as macro). I have already prepared a series of my_syscall() patchset but it includes several not that simple patches, before sending it for review, to directly solve the __sysret() issue at first, it is better to only convert the current three sys_* functions to return 'long' instead of pointer, which will make things easier. Here is the testing result on all archs (except loongarch) with Arnd's gcc 13.2.0, before testing it, we'd better apply the CROSS_COMPILE patchset [1] manually: // before $ ARCHS="i386 x86_64 arm64 arm mips ppc ppc64 ppc64le riscv s390" $ for arch in ${ARCHS[@]}; do printf "%9s: " $arch; make run-user XARCH=$arch 2>/dev/null | grep status | tr '\n' ' '; \ size nolibc-test | tail -1 | tr '\t' ' ' | tr -s ' ' | cut -d ' ' -f2; done i386: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 19654 x86_64: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 22337 arm64: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 26292 arm: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 23140 mips: 160 test(s): 157 passed, 3 skipped, 0 failed => status: warning 23164 ppc: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 26812 ppc64: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 27380 ppc64le: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 28004 riscv: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 22062 s390: 160 test(s): 157 passed, 3 skipped, 0 failed => status: warning 22592 // after $ for arch in ${ARCHS[@]}; do printf "%9s: " $arch; make run-user XARCH=$arch 2>/dev/null | grep status | tr '\n' ' '; \ size nolibc-test | tail -1 | tr '\t' ' ' | tr -s ' ' | cut -d ' ' -f2; done i386: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 19502 x86_64: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 22000 arm64: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 25860 arm: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 23108 mips: 160 test(s): 157 passed, 3 skipped, 0 failed => status: warning 22908 ppc: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 26616 ppc64: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 27192 ppc64le: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 27816 riscv: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 21790 s390: 160 test(s): 157 passed, 3 skipped, 0 failed => status: warning 22184 // compare i386: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 19654 -> 19502 x86_64: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 22337 -> 22000 arm64: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 26292 -> 25860 arm: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 23140 -> 23108 mips: 160 test(s): 157 passed, 3 skipped, 0 failed => status: warning 23164 -> 22908 ppc: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 26812 -> 26616 ppc64: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 27380 -> 27192 ppc64le: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 28004 -> 27816 riscv: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 22062 -> 21790 s390: 160 test(s): 157 passed, 3 skipped, 0 failed => status: warning 22592 -> 22184 After these two patches, will send the proposed my_syscall() patchset tomorrow, it can even further reduce more type conversions and therefore reduce more binary bytes, here is a preview of the testing result: // with the coming my_syscall() patchset, sys_* from functionsn to macros i386: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 19250 x86_64: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 21733 arm64: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 25804 arm: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 22828 mips: 160 test(s): 157 passed, 3 skipped, 0 failed => status: warning 22740 ppc: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 26376 ppc64: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 26752 ppc64le: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 27360 riscv: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 21746 s390: 160 test(s): 157 passed, 3 skipped, 0 failed => status: warning 21928 // compare: __sysret() function -> __sysret() macro -> sys_* macros i386: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 19654 -> 19502 -> 19250 x86_64: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 22337 -> 22000 -> 21733 arm64: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 26292 -> 25860 -> 25804 arm: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 23140 -> 23108 -> 22828 mips: 160 test(s): 157 passed, 3 skipped, 0 failed => status: warning 23164 -> 22908 -> 22740 ppc: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 26812 -> 26616 -> 26376 ppc64: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 27380 -> 27192 -> 26752 ppc64le: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 28004 -> 27816 -> 27360 riscv: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 22062 -> 21790 -> 21746 s390: 160 test(s): 157 passed, 3 skipped, 0 failed => status: warning 22592 -> 22184 -> 21928 It can also shrink the whole sys.h from 1171 lines to around 738 lines. Changes from v5 --> v6: * The method introduced in v5 works but it is too complex ;-) * Convert the return type of sys_brk/mmap/mmap2 from pointer to 'long' (like my_syscall<N> does), after this, all of the sys_* functions return integer. * Restore __sysret() helper to sign comparison as originally, but also use macro instead of inline function to avoid useless input type and return type conversion. Changes from v4 --> v5: * Use __typeof__((arg) + 0) to lose the 'const' flag for old gcc versions. * Import the famous __is_constexpr() macro from kernel side and add a __is_pointer() macro based on it. (David, to avoid introduce extra discuss on the prove-in-use __is_constexpr macro, this patch uses the original version instead of your suggested version, more info here: https://lore.kernel.org/lkml/20220131204357.1133674-1-keescook@chromium.org/) * Use __builtin_choose_expr() to merge two comparisons to share the same errno setting code and the -1L assignment code. Changes from v3 --> v4: * fix up a new warning about 'ret < 0' when the input arg type is (void *) Changes from v2 --> v3: * define a __GXX_HAS_AUTO_TYPE_WITH_CONST_SUPPORT for gcc >= 11.0 (ABI_VERSION >= 1016) * split __sysret() to two versions by the macro instead of a mixed unified and unreadable version * use shorter __ret instead of __sysret_arg Changes from v1 --> v2: * fix up argument with 'const' in the type * support "void *" argument Best regards, Zhangjin Wu --- v5: https://lore.kernel.org/lkml/b6ff2684f557f6ce00151905990643e651391614.1691437328.git.falcon@tinylab.org/ v4: https://lore.kernel.org/lkml/a4084f7fac7a89f861b5582774bc7a98634d1e76.1691392805.git.falcon@tinylab.org/ v3: https://lore.kernel.org/lkml/8eaab5da2dcbba42e3f3efc2ae686a22c95f84f0.1691386601.git.falcon@tinylab.org/ v2: https://lore.kernel.org/lkml/95fe3e732f455fab653fe1427118d905e4d04257.1691339836.git.falcon@tinylab.org/ v1: https://lore.kernel.org/lkml/20230806131921.52453-1-falcon@tinylab.org/ [1]: https://lore.kernel.org/lkml/cover.1691783604.git.falcon@tinylab.org/ Zhangjin Wu (2): tools/nolibc: let sys_brk, sys_mmap and sys_mmap2 return long tools/nolibc: fix up size inflate regression tools/include/nolibc/arch-s390.h | 4 +-- tools/include/nolibc/sys.h | 43 +++++++++++++------------------- 2 files changed, 20 insertions(+), 27 deletions(-)