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

[Masahiro Yamada]

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