| Message ID | 20211116150624.7720-1-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [PATCH-4.16] arm/efi: Improve performance requesting filesystem handle | expand |
Luca Fancellu writes ("[PATCH-4.16] arm/efi: Improve performance requesting filesystem handle"):
> Currently, the code used to handle and possibly load from the filesystem
> modules defined in the DT is allocating and closing the filesystem handle
> for each module to be loaded.
>
> To improve the performance, the filesystem handle pointer is passed
> through the call stack, requested when it's needed only once and closed
> if it was allocated.
Thsnks for submitting this formally. The clarity is very helpful.
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> Justification for integration in 4.16:
> Upside: Fixes a performance issue only on arm that would be really useful
> for ARM users using the release, no functional changes.
The size of this perf issue is not discussed here. At this stage of
the release I will usually give release acks for fixes to serious
problems. Does this meet that test ? I think probably not.
Thanks to Bertrand who, when I asked on IRC, confirmed that this perf
issue only affects users of the new feature in a1743fc3a9 (so is not a
regression), and the perf issue is only a consquenceof 9bc9fff04ba0
which was itself a fix to something wrong with a1743fc3a9. It seems
that one interpretation is that the problem here is simply a
less-than-optimal new feature.
> Downside: It's affecting the EFI boot path (only on ARM).
> Risk: Risk is low given the small changes.
I did find this risk analysis at all convincing, I'm afraid.
So I'm afraid I'm saying "no".
Ian.
On Tue, 16 Nov 2021, Luca Fancellu wrote: > Currently, the code used to handle and possibly load from the filesystem > modules defined in the DT is allocating and closing the filesystem handle > for each module to be loaded. > > To improve the performance, the filesystem handle pointer is passed > through the call stack, requested when it's needed only once and closed > if it was allocated. > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> This is great, thanks Luca! Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > Justification for integration in 4.16: > Upside: Fixes a performance issue only on arm that would be really useful > for ARM users using the release, no functional changes. > Downside: It's affecting the EFI boot path (only on ARM). > Risk: Risk is low given the small changes. As mentioned on IRC to Ian, the reason I said I'd be happy to see it in 4.16 is that it is addressing the leftover open issue from the original patch which was committed a bit too quickly (if you recall you asked me if I thought it should be reverted). But now at this stage it is hard to disagree that it should go in when the window reopens. So I think we can queue it in the Xen on ARM temporary for-next branch. > Tested in this configurations: > - Bootloader loads modules and specify them as multiboot modules in DT: > * combination of Dom0, DomUs, Dom0 and DomUs > - DT specifies multiboot modules in DT using xen,uefi-binary property: > * combination of Dom0, DomUs, Dom0 and DomUs > - Bootloader loads a Dom0 module and appends it as multiboot module in DT, > other multiboot modules are listed for DomUs using xen,uefi-binary > - No multiboot modules in DT and no kernel entry in cfg file: > * proper error thrown > --- > xen/arch/arm/efi/efi-boot.h | 33 +++++++++++++++++++++------------ > 1 file changed, 21 insertions(+), 12 deletions(-) > > diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h > index 458cfbbed4..c4ed412845 100644 > --- a/xen/arch/arm/efi/efi-boot.h > +++ b/xen/arch/arm/efi/efi-boot.h > @@ -45,14 +45,17 @@ void __flush_dcache_area(const void *vaddr, unsigned long size); > static int get_module_file_index(const char *name, unsigned int name_len); > static void PrintMessage(const CHAR16 *s); > static int allocate_module_file(EFI_LOADED_IMAGE *loaded_image, > + EFI_FILE_HANDLE *dir_handle, > const char *name, > unsigned int name_len); > static int handle_module_node(EFI_LOADED_IMAGE *loaded_image, > + EFI_FILE_HANDLE *dir_handle, > int module_node_offset, > int reg_addr_cells, > int reg_size_cells, > bool is_domu_module); > static int handle_dom0less_domain_node(EFI_LOADED_IMAGE *loaded_image, > + EFI_FILE_HANDLE *dir_handle, > int domain_node); > static int efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image); > > @@ -648,10 +651,10 @@ static void __init PrintMessage(const CHAR16 *s) > * index of the file in the modules array or a negative number on error. > */ > static int __init allocate_module_file(EFI_LOADED_IMAGE *loaded_image, > + EFI_FILE_HANDLE *dir_handle, > const char *name, > unsigned int name_len) > { > - EFI_FILE_HANDLE dir_handle; > module_name *file_name; > CHAR16 *fname; > union string module_name; > @@ -686,12 +689,11 @@ static int __init allocate_module_file(EFI_LOADED_IMAGE *loaded_image, > file_name->name_len = name_len; > > /* Get the file system interface. */ > - dir_handle = get_parent_handle(loaded_image, &fname); > + if ( !*dir_handle ) > + *dir_handle = get_parent_handle(loaded_image, &fname); > > /* Load the binary in memory */ > - read_file(dir_handle, s2w(&module_name), &module_binary, NULL); > - > - dir_handle->Close(dir_handle); > + read_file(*dir_handle, s2w(&module_name), &module_binary, NULL); > > /* Save address and size */ > file_name->addr = module_binary.addr; > @@ -712,6 +714,7 @@ static int __init allocate_module_file(EFI_LOADED_IMAGE *loaded_image, > * Returns 1 if module is multiboot,module, 0 if not, < 0 on error > */ > static int __init handle_module_node(EFI_LOADED_IMAGE *loaded_image, > + EFI_FILE_HANDLE *dir_handle, > int module_node_offset, > int reg_addr_cells, > int reg_size_cells, > @@ -744,8 +747,8 @@ static int __init handle_module_node(EFI_LOADED_IMAGE *loaded_image, > file_idx = get_module_file_index(uefi_name_prop, uefi_name_len); > if ( file_idx < 0 ) > { > - file_idx = allocate_module_file(loaded_image, uefi_name_prop, > - uefi_name_len); > + file_idx = allocate_module_file(loaded_image, dir_handle, > + uefi_name_prop, uefi_name_len); > if ( file_idx < 0 ) > return file_idx; > } > @@ -812,6 +815,7 @@ static int __init handle_module_node(EFI_LOADED_IMAGE *loaded_image, > * Returns number of multiboot,module found or negative number on error. > */ > static int __init handle_dom0less_domain_node(EFI_LOADED_IMAGE *loaded_image, > + EFI_FILE_HANDLE *dir_handle, > int domain_node) > { > int module_node, addr_cells, size_cells, len; > @@ -842,8 +846,8 @@ static int __init handle_dom0less_domain_node(EFI_LOADED_IMAGE *loaded_image, > module_node > 0; > module_node = fdt_next_subnode(fdt, module_node) ) > { > - int ret = handle_module_node(loaded_image, module_node, addr_cells, > - size_cells, true); > + int ret = handle_module_node(loaded_image, dir_handle, module_node, > + addr_cells, size_cells, true); > if ( ret < 0 ) > return ret; > > @@ -862,6 +866,7 @@ static int __init efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image) > { > int chosen, node, addr_len, size_len; > unsigned int i = 0, modules_found = 0; > + EFI_FILE_HANDLE dir_handle = NULL; > > /* Check for the chosen node in the current DTB */ > chosen = setup_chosen_node(fdt, &addr_len, &size_len); > @@ -881,20 +886,24 @@ static int __init efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image) > if ( !fdt_node_check_compatible(fdt, node, "xen,domain") ) > { > /* Found a node with compatible xen,domain; handle this node. */ > - ret = handle_dom0less_domain_node(loaded_image, node); > + ret = handle_dom0less_domain_node(loaded_image, &dir_handle, node); > if ( ret < 0 ) > return ERROR_DT_MODULE_DOMU; > } > else > { > - ret = handle_module_node(loaded_image, node, addr_len, size_len, > - false); > + ret = handle_module_node(loaded_image, &dir_handle, node, addr_len, > + size_len, false); > if ( ret < 0 ) > return ERROR_DT_MODULE_DOM0; > } > modules_found += ret; > } > > + /* dir_handle can be allocated in allocate_module_file, free it if exists */ > + if ( dir_handle ) > + dir_handle->Close(dir_handle); > + > /* Free boot modules file names if any */ > for ( ; i < modules_idx; i++ ) > { > -- > 2.17.1 >
Hi, On 16/11/2021 20:38, Stefano Stabellini wrote: > On Tue, 16 Nov 2021, Luca Fancellu wrote: > So I think we can queue it in the Xen on ARM temporary for-next branch. I have pushed it to for-next/4.17 on both my repo and gitlab (hopefully this triggered a CI run). Cheers, > > >> Tested in this configurations: >> - Bootloader loads modules and specify them as multiboot modules in DT: >> * combination of Dom0, DomUs, Dom0 and DomUs >> - DT specifies multiboot modules in DT using xen,uefi-binary property: >> * combination of Dom0, DomUs, Dom0 and DomUs >> - Bootloader loads a Dom0 module and appends it as multiboot module in DT, >> other multiboot modules are listed for DomUs using xen,uefi-binary >> - No multiboot modules in DT and no kernel entry in cfg file: >> * proper error thrown >> --- >> xen/arch/arm/efi/efi-boot.h | 33 +++++++++++++++++++++------------ >> 1 file changed, 21 insertions(+), 12 deletions(-) >> >> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h >> index 458cfbbed4..c4ed412845 100644 >> --- a/xen/arch/arm/efi/efi-boot.h >> +++ b/xen/arch/arm/efi/efi-boot.h >> @@ -45,14 +45,17 @@ void __flush_dcache_area(const void *vaddr, unsigned long size); >> static int get_module_file_index(const char *name, unsigned int name_len); >> static void PrintMessage(const CHAR16 *s); >> static int allocate_module_file(EFI_LOADED_IMAGE *loaded_image, >> + EFI_FILE_HANDLE *dir_handle, >> const char *name, >> unsigned int name_len); >> static int handle_module_node(EFI_LOADED_IMAGE *loaded_image, >> + EFI_FILE_HANDLE *dir_handle, >> int module_node_offset, >> int reg_addr_cells, >> int reg_size_cells, >> bool is_domu_module); >> static int handle_dom0less_domain_node(EFI_LOADED_IMAGE *loaded_image, >> + EFI_FILE_HANDLE *dir_handle, >> int domain_node); >> static int efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image); >> >> @@ -648,10 +651,10 @@ static void __init PrintMessage(const CHAR16 *s) >> * index of the file in the modules array or a negative number on error. >> */ >> static int __init allocate_module_file(EFI_LOADED_IMAGE *loaded_image, >> + EFI_FILE_HANDLE *dir_handle, >> const char *name, >> unsigned int name_len) >> { >> - EFI_FILE_HANDLE dir_handle; >> module_name *file_name; >> CHAR16 *fname; >> union string module_name; >> @@ -686,12 +689,11 @@ static int __init allocate_module_file(EFI_LOADED_IMAGE *loaded_image, >> file_name->name_len = name_len; >> >> /* Get the file system interface. */ >> - dir_handle = get_parent_handle(loaded_image, &fname); >> + if ( !*dir_handle ) >> + *dir_handle = get_parent_handle(loaded_image, &fname); >> >> /* Load the binary in memory */ >> - read_file(dir_handle, s2w(&module_name), &module_binary, NULL); >> - >> - dir_handle->Close(dir_handle); >> + read_file(*dir_handle, s2w(&module_name), &module_binary, NULL); >> >> /* Save address and size */ >> file_name->addr = module_binary.addr; >> @@ -712,6 +714,7 @@ static int __init allocate_module_file(EFI_LOADED_IMAGE *loaded_image, >> * Returns 1 if module is multiboot,module, 0 if not, < 0 on error >> */ >> static int __init handle_module_node(EFI_LOADED_IMAGE *loaded_image, >> + EFI_FILE_HANDLE *dir_handle, >> int module_node_offset, >> int reg_addr_cells, >> int reg_size_cells, >> @@ -744,8 +747,8 @@ static int __init handle_module_node(EFI_LOADED_IMAGE *loaded_image, >> file_idx = get_module_file_index(uefi_name_prop, uefi_name_len); >> if ( file_idx < 0 ) >> { >> - file_idx = allocate_module_file(loaded_image, uefi_name_prop, >> - uefi_name_len); >> + file_idx = allocate_module_file(loaded_image, dir_handle, >> + uefi_name_prop, uefi_name_len); >> if ( file_idx < 0 ) >> return file_idx; >> } >> @@ -812,6 +815,7 @@ static int __init handle_module_node(EFI_LOADED_IMAGE *loaded_image, >> * Returns number of multiboot,module found or negative number on error. >> */ >> static int __init handle_dom0less_domain_node(EFI_LOADED_IMAGE *loaded_image, >> + EFI_FILE_HANDLE *dir_handle, >> int domain_node) >> { >> int module_node, addr_cells, size_cells, len; >> @@ -842,8 +846,8 @@ static int __init handle_dom0less_domain_node(EFI_LOADED_IMAGE *loaded_image, >> module_node > 0; >> module_node = fdt_next_subnode(fdt, module_node) ) >> { >> - int ret = handle_module_node(loaded_image, module_node, addr_cells, >> - size_cells, true); >> + int ret = handle_module_node(loaded_image, dir_handle, module_node, >> + addr_cells, size_cells, true); >> if ( ret < 0 ) >> return ret; >> >> @@ -862,6 +866,7 @@ static int __init efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image) >> { >> int chosen, node, addr_len, size_len; >> unsigned int i = 0, modules_found = 0; >> + EFI_FILE_HANDLE dir_handle = NULL; >> >> /* Check for the chosen node in the current DTB */ >> chosen = setup_chosen_node(fdt, &addr_len, &size_len); >> @@ -881,20 +886,24 @@ static int __init efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image) >> if ( !fdt_node_check_compatible(fdt, node, "xen,domain") ) >> { >> /* Found a node with compatible xen,domain; handle this node. */ >> - ret = handle_dom0less_domain_node(loaded_image, node); >> + ret = handle_dom0less_domain_node(loaded_image, &dir_handle, node); >> if ( ret < 0 ) >> return ERROR_DT_MODULE_DOMU; >> } >> else >> { >> - ret = handle_module_node(loaded_image, node, addr_len, size_len, >> - false); >> + ret = handle_module_node(loaded_image, &dir_handle, node, addr_len, >> + size_len, false); >> if ( ret < 0 ) >> return ERROR_DT_MODULE_DOM0; >> } >> modules_found += ret; >> } >> >> + /* dir_handle can be allocated in allocate_module_file, free it if exists */ >> + if ( dir_handle ) >> + dir_handle->Close(dir_handle); >> + >> /* Free boot modules file names if any */ >> for ( ; i < modules_idx; i++ ) >> { >> -- >> 2.17.1 >>
On Wed, 17 Nov 2021, Julien Grall wrote: > On 16/11/2021 20:38, Stefano Stabellini wrote: > > On Tue, 16 Nov 2021, Luca Fancellu wrote: > > So I think we can queue it in the Xen on ARM temporary for-next branch. > > I have pushed it to for-next/4.17 on both my repo and gitlab (hopefully this > triggered a CI run). Thanks, and yes it triggered the following run: https://gitlab.com/xen-project/fusa/xen-integration/-/pipelines/411093550 which passed. FYI if you want a quick link to check the latest runs for xen-integration, this is what I use: https://gitlab.com/xen-project/fusa/xen-integration/-/pipelines (I really prefer the pipeline view because it is too easy to get lost with the full list of individual jobs.)
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h index 458cfbbed4..c4ed412845 100644 --- a/xen/arch/arm/efi/efi-boot.h +++ b/xen/arch/arm/efi/efi-boot.h @@ -45,14 +45,17 @@ void __flush_dcache_area(const void *vaddr, unsigned long size); static int get_module_file_index(const char *name, unsigned int name_len); static void PrintMessage(const CHAR16 *s); static int allocate_module_file(EFI_LOADED_IMAGE *loaded_image, + EFI_FILE_HANDLE *dir_handle, const char *name, unsigned int name_len); static int handle_module_node(EFI_LOADED_IMAGE *loaded_image, + EFI_FILE_HANDLE *dir_handle, int module_node_offset, int reg_addr_cells, int reg_size_cells, bool is_domu_module); static int handle_dom0less_domain_node(EFI_LOADED_IMAGE *loaded_image, + EFI_FILE_HANDLE *dir_handle, int domain_node); static int efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image); @@ -648,10 +651,10 @@ static void __init PrintMessage(const CHAR16 *s) * index of the file in the modules array or a negative number on error. */ static int __init allocate_module_file(EFI_LOADED_IMAGE *loaded_image, + EFI_FILE_HANDLE *dir_handle, const char *name, unsigned int name_len) { - EFI_FILE_HANDLE dir_handle; module_name *file_name; CHAR16 *fname; union string module_name; @@ -686,12 +689,11 @@ static int __init allocate_module_file(EFI_LOADED_IMAGE *loaded_image, file_name->name_len = name_len; /* Get the file system interface. */ - dir_handle = get_parent_handle(loaded_image, &fname); + if ( !*dir_handle ) + *dir_handle = get_parent_handle(loaded_image, &fname); /* Load the binary in memory */ - read_file(dir_handle, s2w(&module_name), &module_binary, NULL); - - dir_handle->Close(dir_handle); + read_file(*dir_handle, s2w(&module_name), &module_binary, NULL); /* Save address and size */ file_name->addr = module_binary.addr; @@ -712,6 +714,7 @@ static int __init allocate_module_file(EFI_LOADED_IMAGE *loaded_image, * Returns 1 if module is multiboot,module, 0 if not, < 0 on error */ static int __init handle_module_node(EFI_LOADED_IMAGE *loaded_image, + EFI_FILE_HANDLE *dir_handle, int module_node_offset, int reg_addr_cells, int reg_size_cells, @@ -744,8 +747,8 @@ static int __init handle_module_node(EFI_LOADED_IMAGE *loaded_image, file_idx = get_module_file_index(uefi_name_prop, uefi_name_len); if ( file_idx < 0 ) { - file_idx = allocate_module_file(loaded_image, uefi_name_prop, - uefi_name_len); + file_idx = allocate_module_file(loaded_image, dir_handle, + uefi_name_prop, uefi_name_len); if ( file_idx < 0 ) return file_idx; } @@ -812,6 +815,7 @@ static int __init handle_module_node(EFI_LOADED_IMAGE *loaded_image, * Returns number of multiboot,module found or negative number on error. */ static int __init handle_dom0less_domain_node(EFI_LOADED_IMAGE *loaded_image, + EFI_FILE_HANDLE *dir_handle, int domain_node) { int module_node, addr_cells, size_cells, len; @@ -842,8 +846,8 @@ static int __init handle_dom0less_domain_node(EFI_LOADED_IMAGE *loaded_image, module_node > 0; module_node = fdt_next_subnode(fdt, module_node) ) { - int ret = handle_module_node(loaded_image, module_node, addr_cells, - size_cells, true); + int ret = handle_module_node(loaded_image, dir_handle, module_node, + addr_cells, size_cells, true); if ( ret < 0 ) return ret; @@ -862,6 +866,7 @@ static int __init efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image) { int chosen, node, addr_len, size_len; unsigned int i = 0, modules_found = 0; + EFI_FILE_HANDLE dir_handle = NULL; /* Check for the chosen node in the current DTB */ chosen = setup_chosen_node(fdt, &addr_len, &size_len); @@ -881,20 +886,24 @@ static int __init efi_check_dt_boot(EFI_LOADED_IMAGE *loaded_image) if ( !fdt_node_check_compatible(fdt, node, "xen,domain") ) { /* Found a node with compatible xen,domain; handle this node. */ - ret = handle_dom0less_domain_node(loaded_image, node); + ret = handle_dom0less_domain_node(loaded_image, &dir_handle, node); if ( ret < 0 ) return ERROR_DT_MODULE_DOMU; } else { - ret = handle_module_node(loaded_image, node, addr_len, size_len, - false); + ret = handle_module_node(loaded_image, &dir_handle, node, addr_len, + size_len, false); if ( ret < 0 ) return ERROR_DT_MODULE_DOM0; } modules_found += ret; } + /* dir_handle can be allocated in allocate_module_file, free it if exists */ + if ( dir_handle ) + dir_handle->Close(dir_handle); + /* Free boot modules file names if any */ for ( ; i < modules_idx; i++ ) {
Currently, the code used to handle and possibly load from the filesystem modules defined in the DT is allocating and closing the filesystem handle for each module to be loaded. To improve the performance, the filesystem handle pointer is passed through the call stack, requested when it's needed only once and closed if it was allocated. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- Justification for integration in 4.16: Upside: Fixes a performance issue only on arm that would be really useful for ARM users using the release, no functional changes. Downside: It's affecting the EFI boot path (only on ARM). Risk: Risk is low given the small changes. Tested in this configurations: - Bootloader loads modules and specify them as multiboot modules in DT: * combination of Dom0, DomUs, Dom0 and DomUs - DT specifies multiboot modules in DT using xen,uefi-binary property: * combination of Dom0, DomUs, Dom0 and DomUs - Bootloader loads a Dom0 module and appends it as multiboot module in DT, other multiboot modules are listed for DomUs using xen,uefi-binary - No multiboot modules in DT and no kernel entry in cfg file: * proper error thrown --- xen/arch/arm/efi/efi-boot.h | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-)