2 comments below.
Reviewed-by: Jaben Carsey <jaben.car...@intel.com>
> -----Original Message-----
> From: Qiu, Shumin
> Sent: Sunday, April 10, 2016 5:55 AM
> To: edk2-devel@lists.01.org
> Cc: Qiu, Shumin <shumin....@intel.com>; Carsey, Jaben
> <jaben.car...@intel.com>; Ni, Ruiyu <ruiyu...@intel.com>
> Subject: [PATCH] ShellPkg : Cache the environment variable into memory to
> enhance the performance.
> Importance: High
>
> Currently UEFI Shell reads variable storage to get the environment
> variables every time running a new command. And reading(writing)
> UEFI variables is a high cost operation on most platforms. In order
> to enhance the performance this patch read the variable storage once
> and cache the environment variables in memory. Every further 'set'
> command will save the variable not only to Shell cache, but also the
> flash variable storage.
>
> Cc: Jaben Carsey <jaben.car...@intel.com>
> Cc: Ruiyu Ni <ruiyu...@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Qiu Shumin <shumin....@intel.com>
> ---
> ShellPkg/Application/Shell/Shell.c | 4 +
> ShellPkg/Application/Shell/ShellEnvVar.c | 175
> ++++++++++++++++++++++++++++-
> ShellPkg/Application/Shell/ShellEnvVar.h | 82 +++++++++++++-
> ShellPkg/Application/Shell/ShellProtocol.c | 101 ++++++++++-------
> 4 files changed, 317 insertions(+), 45 deletions(-)
>
> diff --git a/ShellPkg/Application/Shell/Shell.c
> b/ShellPkg/Application/Shell/Shell.c
> index bd695a4..12daff9 100644
> --- a/ShellPkg/Application/Shell/Shell.c
> +++ b/ShellPkg/Application/Shell/Shell.c
> @@ -445,6 +445,8 @@ UefiMain (
> Status = CommandInit();
> ASSERT_EFI_ERROR(Status);
>
> + Status = ShellInitEnvVarList ();
> +
> //
> // Check the command line
> //
> @@ -702,6 +704,8 @@ FreeResources:
> DEBUG_CODE(ShellInfoObject.ConsoleInfo = NULL;);
> }
>
> + ShellFreeEnvVarList ();
> +
> if (ShellCommandGetExit()) {
> return ((EFI_STATUS)ShellCommandGetExitCode());
> }
> diff --git a/ShellPkg/Application/Shell/ShellEnvVar.c
> b/ShellPkg/Application/Shell/ShellEnvVar.c
> index a8f177e..d2c721d 100644
> --- a/ShellPkg/Application/Shell/ShellEnvVar.c
> +++ b/ShellPkg/Application/Shell/ShellEnvVar.c
> @@ -1,7 +1,7 @@
> /** @file
> function declarations for shell environment functions.
>
> - Copyright (c) 2009 - 2015, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.<BR>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD
> License
> which accompanies this distribution. The full text of the license may be
> found at
> @@ -17,6 +17,11 @@
> #define INIT_NAME_BUFFER_SIZE 128
> #define INIT_DATA_BUFFER_SIZE 1024
>
> +//
> +// The list is used to cache the environment variables.
> +//
> +ENV_VAR_LIST gShellEnvVarList;
> +
> /**
> Reports whether an environment variable is Volatile or Non-Volatile.
>
> @@ -379,3 +384,171 @@ SetEnvironmentVariables(
> //
> return (SetEnvironmentVariableList(&VarList->Link));
> }
> +
> +/**
> + Find an environment variable in the gShellEnvVarList.
> +
> + @param Key The name of the environment variable.
> + @param Value The value of the environment variable, the buffer
> + shoule be freed by the caller.
> + @param ValueSize The size in bytes of the environment variable
> + including the tailing CHAR_NELL.
> + @param Atts The attributes of the variable.
> +
> + @retval EFI_SUCCESS The command executed successfully.
> + @retval EFI_NOT_FOUND The environment variable is not found in
> + gShellEnvVarList.
> +
> +**/
> +EFI_STATUS
> +ShellFindEnvVarInList (
> + IN CONST CHAR16 *Key,
> + OUT CHAR16 **Value,
> + OUT UINTN *ValueSize,
> + OUT UINT32 *Atts OPTIONAL
> + )
> +{
> + ENV_VAR_LIST *Node;
> +
> + if (Key == NULL || Value == NULL || ValueSize == NULL) {
> + return SHELL_INVALID_PARAMETER;
> + }
> +
> + for ( Node = (ENV_VAR_LIST*)GetFirstNode(&gShellEnvVarList.Link)
> + ; !IsNull(&gShellEnvVarList.Link, &Node->Link)
> + ; Node = (ENV_VAR_LIST*)GetNextNode(&gShellEnvVarList.Link, &Node-
> >Link)
> + ){
> + if (Node->Key != NULL && StrCmp(Key, Node->Key) == 0) {
> + *Value = AllocateCopyPool(StrSize(Node->Val), Node->Val);
> + *ValueSize = StrSize(Node->Val);
> + if (Atts != NULL) {
> + *Atts = Node->Atts;
> + }
> + return EFI_SUCCESS;
> + }
> + }
> +
> + return EFI_NOT_FOUND;
> +}
> +
> +/**
> + Add an environment variable into gShellEnvVarList.
> +
> + @param Key The name of the environment variable.
> + @param Value The value of environment variable.
> + @param ValueSize The size in bytes of the environment variable
> + including the tailing CHAR_NULL
> + @param Atts The attributes of the variable.
> +
> +**/
> +VOID
> +ShellAddEnvVarToList (
> + IN CONST CHAR16 *Key,
> + IN CONST CHAR16 *Value,
> + IN UINTN ValueSize,
> + IN UINT32 Atts
> + )
> +{
> + ENV_VAR_LIST *Node;
> +
> + if (Key == NULL || Value == NULL || ValueSize == 0) {
> + return;
> + }
> +
Please add a comment for what this loop here does. (something like "update the
variable if it exists")
> + for ( Node = (ENV_VAR_LIST*)GetFirstNode(&gShellEnvVarList.Link)
> + ; !IsNull(&gShellEnvVarList.Link, &Node->Link)
> + ; Node = (ENV_VAR_LIST*)GetNextNode(&gShellEnvVarList.Link, &Node-
> >Link)
> + ){
> + if (Node->Key != NULL && StrCmp(Key, Node->Key) == 0) {
> + Node->Atts = Atts;
> + SHELL_FREE_NON_NULL(Node->Val);
> + Node->Val = AllocateZeroPool (ValueSize);
> + ASSERT (Node->Val != NULL);
> + CopyMem(Node->Val, Value, ValueSize);
> + return;
> + }
> + }
> +
> + //
> + // If the environment varialbe key doesn't exist in list.
> + //
> + Node = (ENV_VAR_LIST*)AllocateZeroPool (sizeof(ENV_VAR_LIST));
> + ASSERT (Node != NULL);
> + Node->Key = AllocateCopyPool(StrSize(Key), Key);
> + ASSERT (Node->Key != NULL);
> + Node->Val = AllocateCopyPool(ValueSize, Value);
> + ASSERT (Node->Val != NULL);
> + InsertTailList(&gShellEnvVarList.Link, &Node->Link);
Why is Node->Atts not assigned here? I think it should be.
> +
> + return;
> +}
> +
> +/**
> + Remove a specified environment variable in gShellEnvVarList.
> +
> + @param Key The name of the environment variable.
> +
> + @retval EFI_SUCCESS The command executed successfully.
> + @retval EFI_NOT_FOUND The environment variable is not found in
> + gShellEnvVarList.
> +**/
> +EFI_STATUS
> +ShellRemvoeEnvVarFromList (
> + IN CONST CHAR16 *Key
> + )
> +{
> + ENV_VAR_LIST *Node;
> +
> + if (Key == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + for ( Node = (ENV_VAR_LIST*)GetFirstNode(&gShellEnvVarList.Link)
> + ; !IsNull(&gShellEnvVarList.Link, &Node->Link)
> + ; Node = (ENV_VAR_LIST*)GetNextNode(&gShellEnvVarList.Link, &Node-
> >Link)
> + ){
> + if (Node->Key != NULL && StrCmp(Key, Node->Key) == 0) {
> + SHELL_FREE_NON_NULL(Node->Key);
> + SHELL_FREE_NON_NULL(Node->Val);
> + RemoveEntryList(&Node->Link);
> + SHELL_FREE_NON_NULL(Node);
> + return EFI_SUCCESS;
> + }
> + }
> +
> + return EFI_NOT_FOUND;
> +}
> +
> +/**
> + Initialize the gShellEnvVarList and cache all Shell-Guid-based environment
> + variables.
> +
> +**/
> +EFI_STATUS
> +ShellInitEnvVarList (
> + VOID
> + )
> +{
> + EFI_STATUS Status;
> +
> + InitializeListHead(&gShellEnvVarList.Link);
> + Status = GetEnvironmentVariableList (&gShellEnvVarList.Link);
> +
> + return Status;
> +}
> +
> +/**
> + Destructe the gShellEnvVarList.
> +
> +**/
> +VOID
> +ShellFreeEnvVarList (
> + VOID
> + )
> +{
> + FreeEnvironmentVariableList (&gShellEnvVarList.Link);
> + InitializeListHead(&gShellEnvVarList.Link);
> +
> + return;
> +}
> +
> diff --git a/ShellPkg/Application/Shell/ShellEnvVar.h
> b/ShellPkg/Application/Shell/ShellEnvVar.h
> index ab3d916..dd88b29 100644
> --- a/ShellPkg/Application/Shell/ShellEnvVar.h
> +++ b/ShellPkg/Application/Shell/ShellEnvVar.h
> @@ -6,7 +6,7 @@
> //#include <Library/UefiRuntimeServicesTableLib.h>
>
>
> - Copyright (c) 2009 - 2010, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.<BR>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD
> License
> which accompanies this distribution. The full text of the license may be
> found at
> @@ -27,6 +27,12 @@ typedef struct {
> UINT32 Atts;
> } ENV_VAR_LIST;
>
> +//
> +// The list is used to cache the environment variables.
> +//
> +extern ENV_VAR_LIST gShellEnvVarList;
> +
> +
> /**
> Reports whether an environment variable is Volatile or Non-Volatile
>
> @@ -206,5 +212,79 @@ FreeEnvironmentVariableList(
> IN LIST_ENTRY *List
> );
>
> +/**
> + Find an environment variable in the gShellEnvVarList.
> +
> + @param Key The name of the environment variable.
> + @param Value The value of the environment variable, the buffer
> + shoule be freed by the caller.
> + @param ValueSize The size in bytes of the environment variable
> + including the tailing CHAR_NULL.
> + @param Atts The attributes of the variable.
> +
> + @retval EFI_SUCCESS The command executed successfully.
> + @retval EFI_NOT_FOUND The environment variable is not found in
> + gShellEnvVarList.
> +
> +**/
> +EFI_STATUS
> +ShellFindEnvVarInList (
> + IN CONST CHAR16 *Key,
> + OUT CHAR16 **Value,
> + OUT UINTN *ValueSize,
> + OUT UINT32 *Atts OPTIONAL
> + );
> +
> +/**
> + Add an environment variable into gShellEnvVarList.
> +
> + @param Key The name of the environment variable.
> + @param Value The value of environment variable.
> + @param ValueSize The size in bytes of the environment variable
> + including the tailing CHAR_NELL
> + @param Atts The attributes of the variable.
> +
> +**/
> +VOID
> +ShellAddEnvVarToList (
> + IN CONST CHAR16 *Key,
> + IN CONST CHAR16 *Value,
> + IN UINTN ValueSize,
> + IN UINT32 Atts
> + );
> +
> +/**
> + Remove a specified environment variable in gShellEnvVarList.
> +
> + @param Key The name of the environment variable.
> +
> + @retval EFI_SUCCESS The command executed successfully.
> + @retval EFI_NOT_FOUND The environment variable is not found in
> + gShellEnvVarList.
> +**/
> +EFI_STATUS
> +ShellRemvoeEnvVarFromList (
> + IN CONST CHAR16 *Key
> + );
> +
> +/**
> + Initialize the gShellEnvVarList and cache all Shell-Guid-based environment
> + variables.
> +
> +**/
> +EFI_STATUS
> +ShellInitEnvVarList (
> + VOID
> + );
> +
> +/**
> + Destructe the gShellEnvVarList.
> +
> +**/
> +VOID
> +ShellFreeEnvVarList (
> + VOID
> + );
> +
> #endif //_SHELL_ENVIRONMENT_VARIABLE_HEADER_
>
> diff --git a/ShellPkg/Application/Shell/ShellProtocol.c
> b/ShellPkg/Application/Shell/ShellProtocol.c
> index e55b5e9..17c3002 100644
> --- a/ShellPkg/Application/Shell/ShellProtocol.c
> +++ b/ShellPkg/Application/Shell/ShellProtocol.c
> @@ -2691,7 +2691,6 @@ EfiShellGetEnvEx(
> EFI_STATUS Status;
> VOID *Buffer;
> UINTN Size;
> - LIST_ENTRY List;
> ENV_VAR_LIST *Node;
> CHAR16 *CurrentWriteLocation;
>
> @@ -2699,21 +2698,13 @@ EfiShellGetEnvEx(
> Buffer = NULL;
>
> if (Name == NULL) {
> - //
> - // Get all our environment variables
> - //
> - InitializeListHead(&List);
> - Status = GetEnvironmentVariableList(&List);
> - if (EFI_ERROR(Status)){
> - return (NULL);
> - }
>
> //
> // Build the semi-colon delimited list. (2 passes)
> //
> - for ( Node = (ENV_VAR_LIST*)GetFirstNode(&List)
> - ; !IsNull(&List, &Node->Link)
> - ; Node = (ENV_VAR_LIST*)GetNextNode(&List, &Node->Link)
> + for ( Node = (ENV_VAR_LIST*)GetFirstNode(&gShellEnvVarList.Link)
> + ; !IsNull(&gShellEnvVarList.Link, &Node->Link)
> + ; Node = (ENV_VAR_LIST*)GetNextNode(&gShellEnvVarList.Link, &Node-
> >Link)
> ){
> ASSERT(Node->Key != NULL);
> Size += StrSize(Node->Key);
> @@ -2723,16 +2714,13 @@ EfiShellGetEnvEx(
>
> Buffer = AllocateZeroPool(Size);
> if (Buffer == NULL) {
> - if (!IsListEmpty (&List)) {
> - FreeEnvironmentVariableList(&List);
> - }
> return (NULL);
> }
> CurrentWriteLocation = (CHAR16*)Buffer;
>
> - for ( Node = (ENV_VAR_LIST*)GetFirstNode(&List)
> - ; !IsNull(&List, &Node->Link)
> - ; Node = (ENV_VAR_LIST*)GetNextNode(&List, &Node->Link)
> + for ( Node = (ENV_VAR_LIST*)GetFirstNode(&gShellEnvVarList.Link)
> + ; !IsNull(&gShellEnvVarList.Link, &Node->Link)
> + ; Node = (ENV_VAR_LIST*)GetNextNode(&gShellEnvVarList.Link, &Node-
> >Link)
> ){
> ASSERT(Node->Key != NULL);
> StrCpyS( CurrentWriteLocation,
> @@ -2742,37 +2730,43 @@ EfiShellGetEnvEx(
> CurrentWriteLocation += StrLen(CurrentWriteLocation) + 1;
> }
>
> - //
> - // Free the list...
> - //
> - if (!IsListEmpty (&List)) {
> - FreeEnvironmentVariableList(&List);
> - }
> } else {
> //
> // We are doing a specific environment variable
> //
> + Status = ShellFindEnvVarInList(Name, (CHAR16**)&Buffer, &Size,
> Attributes);
>
> - //
> - // get the size we need for this EnvVariable
> - //
> - Status = SHELL_GET_ENVIRONMENT_VARIABLE_AND_ATTRIBUTES(Name,
> Attributes, &Size, Buffer);
> - if (Status == EFI_BUFFER_TOO_SMALL) {
> + if (EFI_ERROR(Status)){
> //
> - // Allocate the space and recall the get function
> + // get the size we need for this EnvVariable
> //
> - Buffer = AllocateZeroPool(Size);
> Status = SHELL_GET_ENVIRONMENT_VARIABLE_AND_ATTRIBUTES(Name,
> Attributes, &Size, Buffer);
> - }
> - //
> - // we didnt get it (might not exist)
> - // free the memory if we allocated any and return NULL
> - //
> - if (EFI_ERROR(Status)) {
> - if (Buffer != NULL) {
> - FreePool(Buffer);
> + if (Status == EFI_BUFFER_TOO_SMALL) {
> + //
> + // Allocate the space and recall the get function
> + //
> + Buffer = AllocateZeroPool(Size);
> + Status = SHELL_GET_ENVIRONMENT_VARIABLE_AND_ATTRIBUTES(Name,
> Attributes, &Size, Buffer);
> + }
> + //
> + // we didnt get it (might not exist)
> + // free the memory if we allocated any and return NULL
> + //
> + if (EFI_ERROR(Status)) {
> + if (Buffer != NULL) {
> + FreePool(Buffer);
> + }
> + return (NULL);
> + } else {
> + //
> + // If we did not find the environment variable in the
> gShellEnvVarList
> + // but get it from UEFI variable storage successfully then we need
> update
> + // the gShellEnvVarList.
> + //
> + ShellFreeEnvVarList ();
> + Status = ShellInitEnvVarList ();
> + ASSERT (Status == EFI_SUCCESS);
> }
> - return (NULL);
> }
> }
>
> @@ -2832,14 +2826,35 @@ InternalEfiShellSetEnv(
> IN BOOLEAN Volatile
> )
> {
> + EFI_STATUS Status;
> + UINT32 Atts;
> +
> + Atts = 0x0;
> +
> if (Value == NULL || StrLen(Value) == 0) {
> - return (SHELL_DELETE_ENVIRONMENT_VARIABLE(Name));
> + Status = SHELL_DELETE_ENVIRONMENT_VARIABLE(Name);
> + if (!EFI_ERROR(Status)) {
> + ShellRemvoeEnvVarFromList(Name);
> + }
> + return Status;
> } else {
> SHELL_DELETE_ENVIRONMENT_VARIABLE(Name);
> if (Volatile) {
> - return (SHELL_SET_ENVIRONMENT_VARIABLE_V(Name, StrSize(Value),
> Value));
> + Status = SHELL_SET_ENVIRONMENT_VARIABLE_V(Name, StrSize(Value),
> Value);
> + if (!EFI_ERROR(Status)) {
> + Atts &= ~EFI_VARIABLE_NON_VOLATILE;
> + Atts |= EFI_VARIABLE_BOOTSERVICE_ACCESS;
> + ShellAddEnvVarToList(Name, Value, StrSize(Value), Atts);
> + }
> + return Status;
> } else {
> - return (SHELL_SET_ENVIRONMENT_VARIABLE_NV(Name, StrSize(Value),
> Value));
> + Status = SHELL_SET_ENVIRONMENT_VARIABLE_NV(Name, StrSize(Value),
> Value);
> + if (!EFI_ERROR(Status)) {
> + Atts |= EFI_VARIABLE_NON_VOLATILE;
> + Atts |= EFI_VARIABLE_BOOTSERVICE_ACCESS;
> + ShellAddEnvVarToList(Name, Value, StrSize(Value), Atts);
> + }
> + return Status;
> }
> }
> }
> --
> 2.7.1.windows.2
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
