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

[Shreenidhi Shedi <yesshedi@gmail.com>]

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