Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 12 May 2023 12:04:11 +0200
From:   Andrew Jones <ajones@ventanamicro.com>
To:     Amma Lee <lixiaoyun@binary-semi.com>
Cc:     paul.walmsley@sifive.com, palmer@dabbelt.com,
        aou@eecs.berkeley.edu, conor@kernel.org,
        linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
        xiezx@binary-semi.com
Subject: Re: [PATCH v2] riscv: optimize ELF relocation function in riscv
Message-ID: <20230512-a73001a0f4cfcf5f0e68d898@orel>
References: <1683881513-18730-1-git-send-email-lixiaoyun@binary-semi.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1683881513-18730-1-git-send-email-lixiaoyun@binary-semi.com>
Precedence: bulk

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@binary-semi.com>
> Reviewed-by: Conor Dooley <conor@kernel.org>
> 
> ---
>  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*/
> +
> +			/*

whitespace issue here

> +			* 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++) {
> -			/* 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;
>  				}
> -				
> -				
> -				for (j = 0; j < j_idx; j++){ 
> +
> +				/*

also here

> +				* 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)

previous

> +				* 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);
> -				
> -				
> +
> +

While fixing this whitespace, we could remove the redundant blank line too.

>  					/* Find the corresponding HI20 relocation entry */
>  					if (hi20_loc == sym->st_value
>  						&& (hi20_type == R_RISCV_PCREL_HI20
> @@ -447,36 +463,37 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
>  						unsigned long hi20_sym_val =
>  							hi20_sym->st_value
>  							+ rel[j].r_addend;
> -					
> -				
> +
> +

also here

>  						/* Calculate lo12 */
>  						size_t offset = hi20_sym_val - hi20_loc;
> +						/* Calculate lo12 */

stray copy+pasted line?

>  						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;
> -				
> +

need to just remove this blank line

>  						}
>  						hi20 = (offset + 0x800) & 0xfffff000;
>  						lo12 = offset - hi20;
>  						v = lo12;
> -					
> +

same here

>  						break;
>  					}
>  				}
> -				
> -				if (j == j_idx)
> -				{
> +
> +				if (j == j_idx) {
>  					pr_err(
>  						"%s: Can not find HI20 relocation information\n",
>  						me->name);
>  					return -EINVAL;
>  				}
> -				
> -				
> +
> +

should just remove both the blank lines above

>  			}
> -			
> +
> +			/* Record the previous j-loop end index */
>  			j_idx = j;

Huh... what did I miss? I went through the whole patch but only see
formatting changes (and more opportunity for formatting changes).
Where's the actual change?

Thanks,
drew