Messages in this thread | | | Date | Wed, 9 Aug 2023 22:57:40 +0530 | Subject | Re: [PATCH v7 8/8] kbuild: modinst: do modules_install step by step | From | Shreenidhi Shedi <> |
| |
On 08/08/23 00:14, Masahiro Yamada wrote: > On Mon, Aug 7, 2023 at 5:08 PM Shreenidhi Shedi <yesshedi@gmail.com> wrote: >> >> On 07/08/23 01:02, Masahiro Yamada wrote: >>> On Fri, Jun 23, 2023 at 11:54 PM Shreenidhi Shedi <yesshedi@gmail.com> wrote: >>>> >>>> Currently Makefile.modinst does three tasks on each module built: >>>> - Install modules >>>> - Sign modules >>>> - Compress modules >>>> >>>> All the above tasks happen from a single place. >>>> >>>> This patch divides this task further and uses a different makefile for >>>> each task. >>>> Signing module logic is completely refactored and everything happens >>>> from a shell script now. >>>> >>>> Signed-off-by: Shreenidhi Shedi <yesshedi@gmail.com> >>> >>> >>> This patch is bad in multiple ways. >>> >>> 1. Break "make modules_sign" >> >> Correct, somehow I missed it. I will fix it. >> I'm using below command to test sign only option. Please let me know if >> I should use something else. >> >> make modules_sign modules_sign_only=1 INSTALL_MOD_PATH=$PWD/tmp -j8 >> >>> 2. Serialize the installation steps, that is, works less efficiently >> >> Even in the existing system it happens in serially. > > The existing code runs in parallel. > > 1. Copy the module "foo.ko" to the destination > 2. Sign the module "bar.ko" > 3. Compress the module "baz.ko" > > Those three have no dependency among them, so > should be able to run in parallel. > > Your code serializes 1 -> 2 -> 3 > > > >> And the existing >> method takes more time than the proposed version. >> >> root@ph5dev:~/linux-6.3.5 # ./test.sh orig >> >> real 0m14.699s >> user 0m55.519s >> sys 0m9.036s >> >> root@ph5dev:~/linux-6.3.5 # ./test.sh new >> >> real 0m13.327s >> user 0m46.885s >> sys 0m6.770s >> >> Here is my test script. >> ``` >> #!/bin/bash >> >> set -e >> >> if [ "$1" != "new" ] && [ "$1" != "orig" ]; then >> echo "invalid arg, ($0 [orig|new])" >&2 >> exit 1 >> fi >> >> rm -rf $PWD/tmp >> >> s="scripts/sign-file.c" >> m="scripts/Makefile.modinst" >> fns=($s $m) >> >> for f in ${fns[@]}; do >> cp $f.$1 $f >> done >> >> cd scripts >> gcc -o sign-file sign-file.c -lcrypto >> cd - >> >> time make modules_install INSTALL_MOD_PATH=$PWD/tmp -s -j$(nproc) >> ``` >> >>> 3. Increase code without adding any benefits. >>> Agree with increased code but this change is one step closer to Unix >> philosophy, do one thing well wrt modules_install. > > > I do not understand why "closer to Unix philosophy"? > > You are adding extra/unnecessary complexity. > > Currently, the parallel job is managed by Make's job server. > > You are introducing another way of parallel execution > in scripts/signfile.sh > (and you completely ignored -j <jobs> option to Make, > and always spawned $(nproc) threads). > > > Leave the parallel execution GNU Make. > That is how Kbuild works _properly_. > > > > >>> There is no good reason to do these chang >I hope the data I provided above to your 2nd point provides evidence >> that this fix is improving existing system. Please take a look again. > > > I saw it. I re-confirmed this is not an improvement. Thanks for the data. > > As I replied to the other thread, my measurement did not show an > attractive result. > https://lore.kernel.org/lkml/CAK7LNATNRchNoj0Y6sdb+_81xwV3kAX57-w5q2zew-q8RyzJVg@mail.gmail.com/T/#m8234fc76e631363fbe6bdfb6e45ef6727fc48e80 > > >> >>> NACK. >> >> Hi Masahiro Yamada, >> >> Replies inline above. >> >> Please correct me if my understanding is wrong. Thanks a lot for your >> time and patience. Have a nice time ahead. > > > I must let you know you are misunderstanding > the meaning of NACK. > > > NACK means: > "I do not like it. Please do not submit it any more". > > > > -- > Best Regards > Masahiro Yamada
Thanks for all the advice and your time, I'm getting used to this process. I dropped Kbuild changes from my patch series v9. PTAL.
Have a nice time ahead.
-- Shedi
| |