Re: [PATCH v7 8/8] kbuild: modinst: do modules_install step by step

[Shreenidhi Shedi]

On 08/08/23 00:14, Masahiro Yamada wrote:
> On Mon, Aug 7, 2023 at 5:08 PM Shreenidhi Shedi <yesshedi@xxxxxxxxx> wrote:
> > On 07/08/23 01:02, Masahiro Yamada wrote:
> > > On Fri, Jun 23, 2023 at 11:54 PM Shreenidhi Shedi <yesshedi@xxxxxxxxx> 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@xxxxxxxxx>
> > >
> > >
> > > 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@xxxxxxxxxxxxxx/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