[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230512-morbidly-supremacy-9cdb519107d1@wendy>
Date: Fri, 12 May 2023 10:03:19 +0100
From: Conor Dooley <conor.dooley@...rochip.com>
To: Amma Lee <lixiaoyun@...ary-semi.com>
CC: <paul.walmsley@...ive.com>, <palmer@...belt.com>,
<aou@...s.berkeley.edu>, <conor@...nel.org>,
<linux-riscv@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
<xiezx@...ary-semi.com>
Subject: Re: [PATCH v2] riscv: optimize ELF relocation function in riscv
Hey Amma,
On Fri, May 12, 2023 at 04:51:53PM +0800, Amma Lee wrote:
> The patch can optimize the running times of "insmod" command by modify ELF
> relocation function.
> When install the riscv ELF driver which contains multiple
> symbol table items to be relocated, kernel takes a lot of time to
> execute the relocation. For example, we install a 3+MB driver need 180+s.
> We focus on the riscv kernel handle R_RISCV_HI20 and R_RISCV_LO12 type
> items relocation function and find that there are two for-loops in this
> function. If we modify the begin number in the second for-loops iteration,
> we could save significant time for installation. We install the 3+MB
> driver could just need 2s.
>
> Signed-off-by: Amma Lee <lixiaoyun@...ary-semi.com>
> Reviewed-by: Conor Dooley <conor@...nel.org>
I did not provide this tag. Do not add tags that you were not explicitly
given by people.
Also, you didn't actually even implement all of the things I pointed out
either :(
>
> ---
> arch/riscv/kernel/module.c | 53 ++++++++++++++++++++++++++++++----------------
> 1 file changed, 35 insertions(+), 18 deletions(-)
>
> diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
> index 55507b0..1683c1d 100755
> --- a/arch/riscv/kernel/module.c
> +++ b/arch/riscv/kernel/module.c
> @@ -385,9 +385,17 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
>
> if (type == R_RISCV_PCREL_LO12_I || type == R_RISCV_PCREL_LO12_S) {
> unsigned int j;
> - /*Modify the j for-loops begin number from last iterates end value*/
> +
> + /*
> + * In the second for-loops, each traversal for j is
^^
Comment in passing here, we align the *s in comments.
> + * starts from 0 to the symbol table item index which
> + * is detected. By the tool "readelf", we find that all
> + * the symbol table items about R_RISCV_PCREL_HI20 type
> + * are incrementally added in order. It means that we
> + * could interate the j with the previous loop end
> + * value(j_idx) as the begin number in the next loop;
> + */
> for (j = j_idx; j < sechdrs[relsec].sh_size / sizeof(*rel); j++) {
> - /* Modify end */
> unsigned long hi20_loc =
> sechdrs[sechdrs[relsec].sh_info].sh_addr
> + rel[j].r_offset;
> @@ -420,22 +428,30 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
> break;
> }
> }
> +
> if (j == sechdrs[relsec].sh_size / sizeof(*rel)) {
> - if(j_idx == 0){
> + if (j_idx == 0) {
> pr_err(
> "%s: Can not find HI20 relocation information\n",
> me->name);
> return -EINVAL;
> }
> -
> -
^^ and this is one of the checkpatch complaints I mentioned last time,
no double blank lines please.
Also, this version of the patch doesn't actually apply for me to
v6.4-rc1 anymore, but the file has not been modified in upstream since
29/01. Not too sure what you have used as the base for this as a result
:/
Thanks,
Conor.
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists