|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH-4.16] arm/efi: Improve performance requesting filesystem handle
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@xxxxxxx>
This is great, thanks Luca!
Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> 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
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |