| Message ID | 1688355132-62933-1-git-send-email-lixiaoyun@binary-semi.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | riscv: optimize ELF relocation function in riscv | expand |
On Mon, Jul 03, 2023 at 11:32:12AM +0800, Amma Lee wrote: > The patch can optimize the running times of insmod command by modify ELF > relocation function. > In the 5.10 and latest kernel, when install the riscv ELF drivers 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 architecture handle R_RISCV_HI20 and R_RISCV_LO20 > type items relocation function in the arch\riscv\kernel\module.c and > find that there are two-loops in the function. If we modify the begin > number in the second for-loops iteration, we could save significant time > for installation. We install the same 3+MB driver could just need 2s. > > Signed-off-by: Amma Lee <lixiaoyun@binary-semi.com> > --- > arch/riscv/kernel/module.c | 67 +++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 64 insertions(+), 3 deletions(-) > I guess this is a v3 of [1]? But there's no change log here to know what's different. [1] 1683881513-18730-1-git-send-email-lixiaoyun@binary-semi.com Thanks, drew
On Mon, Jul 03, 2023 at 12:35:15PM +0200, Andrew Jones wrote: > On Mon, Jul 03, 2023 at 11:32:12AM +0800, Amma Lee wrote: > > The patch can optimize the running times of insmod command by modify ELF > > relocation function. > > In the 5.10 and latest kernel, when install the riscv ELF drivers 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 architecture handle R_RISCV_HI20 and R_RISCV_LO20 > > type items relocation function in the arch\riscv\kernel\module.c and > > find that there are two-loops in the function. If we modify the begin > > number in the second for-loops iteration, we could save significant time > > for installation. We install the same 3+MB driver could just need 2s. > > > > Signed-off-by: Amma Lee <lixiaoyun@binary-semi.com> > > --- > > arch/riscv/kernel/module.c | 67 +++++++++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 64 insertions(+), 3 deletions(-) > > > > I guess this is a v3 of [1]? But there's no change log here to know > what's different. > > [1] 1683881513-18730-1-git-send-email-lixiaoyun@binary-semi.com It's also still got the checkpatch issues (and possibly others) that were pointed out previously. Cheers, Conor. Also, when applying the patch: warning: arch/riscv/kernel/module.c has type 100644, expected 100755
On 25/07/2023 10:18, 李筱云(李筱云) wrote: > Hi Conor, >>On Mon, Jul 03, 2023 at 12:35:15PM +0200, Andrew Jones wrote: > >> On Mon, Jul 03, 2023 at 11:32:12AM +0800, Amma Lee wrote: > >> > The patch can optimize the running times of insmod command by modify ELF > >> > relocation function. > >> > In the 5.10 and latest kernel, when install the riscv ELF drivers 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 architecture handle R_RISCV_HI20 and R_RISCV_LO20 > >> > type items relocation function in the arch\riscv\kernel\module.c and > >> > find that there are two-loops in the function. If we modify the begin > >> > number in the second for-loops iteration, we could save significant time > >> > for installation. We install the same 3+MB driver could just need 2s. > >> > > >> > Signed-off-by: Amma Lee <lixiaoyun@binary-semi.com> > >> > --- > >> > arch/riscv/kernel/module.c | 67 +++++++++++++++++++++++++++++++++++++++++++--- > >> > 1 file changed, 64 insertions(+), 3 deletions(-) > >> > > >> > >> I guess this is a v3 of [1]? But there's no change log here to know > >> what's different. > >> > >> [1] 1683881513-18730-1-git-send-email-lixiaoyun@binary-semi.com > > > >It's also still got the checkpatch issues (and possibly others) that >>were pointed out previously. > > I'm sorry for the duplicate emails. Because we released this patch many times, the first > patch was based on 5.10. Later we verified this issue on kernel 6.4, then we released a new > patch based on the 6.4 kernel. Since we never got any reply, we don't know how to proceed > with this patch. > > BR, > Amma > > ------------------------------------------------------------------ > 发件人：Conor Dooley <conor.dooley@microchip.com> > 发送时间：2023年7月3日(星期一) 18:48 > 收件人：Andrew Jones <ajones@ventanamicro.com> > 抄 送：李筱云(李筱云) <lixiaoyun@binary-semi.com>; paul.walmsley <paul.walmsley@sifive.com>; palmer <palmer@dabbelt.com>; aou <aou@eecs.berkeley.edu>; 谢振新(谢振新) <xiezx@binary-semi.com>; linux-riscv <linux-riscv@lists.infradead.org>; linux-kernel <linux-kernel@vger.kernel.org> > 主 题：Re: [PATCH] riscv: optimize ELF relocation function in riscv > > On Mon, Jul 03, 2023 at 12:35:15PM +0200, Andrew Jones wrote: > > On Mon, Jul 03, 2023 at 11:32:12AM +0800, Amma Lee wrote: > > > The patch can optimize the running times of insmod command by modify ELF > > > relocation function. > > > In the 5.10 and latest kernel, when install the riscv ELF drivers 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 architecture handle R_RISCV_HI20 and R_RISCV_LO20 > > > type items relocation function in the arch\riscv\kernel\module.c and > > > find that there are two-loops in the function. If we modify the begin > > > number in the second for-loops iteration, we could save significant time > > > for installation. We install the same 3+MB driver could just need 2s. > > > > > > Signed-off-by: Amma Lee <lixiaoyun@binary-semi.com> > > > --- > > > arch/riscv/kernel/module.c | 67 +++++++++++++++++++++++++++++++++++++++++++--- > > > 1 file changed, 64 insertions(+), 3 deletions(-) > > > > > > > I guess this is a v3 of [1]? But there's no change log here to know > > what's different. > > > > [1] 1683881513-18730-1-git-send-email-lixiaoyun@binary-semi.com > > It's also still got the checkpatch issues (and possibly others) that > were pointed out previously. > > Cheers, > Conor. > > Also, when applying the patch: > warning: arch/riscv/kernel/module.c has type 100644, expected 100755 > > Firstly, please no html mails. The mailing list will reject them :/ There was no changelog to indicate what is different from the prior submissions, nor were the issues pointed out by checkpatch resolved. It's considered impolite to post another version of a patch, without addressing comments that were pointed out on the previous submission. Please fix up the things pointed out by checkpatch (and please run it with the --strict argument) and resubmit a v4, that contains a changelog under the --- line, explaining what is different between this patch & v4. Thanks, Conor.
diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c index 7c651d5..b8df144 100755 --- a/arch/riscv/kernel/module.c +++ b/arch/riscv/kernel/module.c @@ -345,13 +345,13 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab, int (*handler)(struct module *me, u32 *location, Elf_Addr v); Elf_Sym *sym; u32 *location; - unsigned int i, type; + unsigned int i, type, j_idx; Elf_Addr v; int res; pr_debug("Applying relocate section %u to %u\n", relsec, sechdrs[relsec].sh_info); - + j_idx = 0; for (i = 0; i < sechdrs[relsec].sh_size / sizeof(*rel); i++) { /* This is where to make the change */ location = (void *)sechdrs[sechdrs[relsec].sh_info].sh_addr @@ -386,7 +386,15 @@ 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; - for (j = 0; j < sechdrs[relsec].sh_size / sizeof(*rel); j++) { + /*In the second for-loops, each traversal for j is + * 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++) { unsigned long hi20_loc = sechdrs[sechdrs[relsec].sh_info].sh_addr + rel[j].r_offset; @@ -420,11 +428,64 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab, } } if (j == sechdrs[relsec].sh_size / sizeof(*rel)) { + if (j_idx == 0) { pr_err( "%s: Can not find HI20 relocation information\n", me->name); return -EINVAL; +} + + /*If the last j-loop have been traversed to the + * maximum value but never match the + * corresponding symbol relocation item, the + * j-loop will execute the second loop which + * is begin from 0 to the prerious index (j_idx) + * unless the previous j_idx == 0; + * */ + for (j = 0; j < j_idx; j++) { + unsigned long hi20_loc = + sechdrs[sechdrs[relsec].sh_info].sh_addr + + rel[j].r_offset; + u32 hi20_type = ELF_RISCV_R_TYPE(rel[j].r_info); + + /* Find the corresponding HI20 relocation entry */ + if (hi20_loc == sym->st_value + && (hi20_type == R_RISCV_PCREL_HI20 + || hi20_type == R_RISCV_GOT_HI20)) { + s32 hi20, lo12; + Elf_Sym *hi20_sym = + (Elf_Sym *)sechdrs[symindex].sh_addr + + ELF_RISCV_R_SYM(rel[j].r_info); + unsigned long hi20_sym_val = + hi20_sym->st_value + + rel[j].r_addend; + + /* Calculate lo12 */ + size_t offset = hi20_sym_val - hi20_loc; + if (IS_ENABLED(CONFIG_MODULE_SECTIONS) + && hi20_type == R_RISCV_GOT_HI20) { + offset = module_emit_got_entry( + me, hi20_sym_val); + offset = offset - hi20_loc; + } + hi20 = (offset + 0x800) & 0xfffff000; + lo12 = offset - hi20; + v = lo12; + + break; + } } + + if (j == j_idx) { + pr_err( + "%s: Can not find HI20 relocation information\n", + me->name); + return -EINVAL; + } + } + + /* Record the previous j-loop end index */ + j_idx = j; } res = handler(me, location, v);
The patch can optimize the running times of insmod command by modify ELF relocation function. In the 5.10 and latest kernel, when install the riscv ELF drivers 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 architecture handle R_RISCV_HI20 and R_RISCV_LO20 type items relocation function in the arch\riscv\kernel\module.c and find that there are two-loops in the function. If we modify the begin number in the second for-loops iteration, we could save significant time for installation. We install the same 3+MB driver could just need 2s. Signed-off-by: Amma Lee <lixiaoyun@binary-semi.com> --- arch/riscv/kernel/module.c | 67 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 64 insertions(+), 3 deletions(-)