public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Laszlo Ersek <lersek@redhat.com>
To: Heyi Guo <heyi.guo@linaro.org>, edk2-devel@lists.01.org
Cc: Ruiyu Ni <ruiyu.ni@intel.com>, Eric Dong <eric.dong@intel.com>,
	Star Zeng <star.zeng@intel.com>
Subject: Re: [PATCH 1/1] MdeModulePkg/UefiBootManagerLib: limit recursive call depth
Date: Mon, 26 Feb 2018 17:23:29 +0100	[thread overview]
Message-ID: <b3e02a30-217e-4b03-dde0-ee064cd12706@redhat.com> (raw)
In-Reply-To: <1519633779-130687-1-git-send-email-heyi.guo@linaro.org>

On 02/26/18 09:29, Heyi Guo wrote:
> Function BmRepairAllControllers may recursively call itself if some
> driver health protocol returns EfiDriverHealthStatusReconnectRequired.
> However, driver health protocol of some buggy third party driver may
> always return such status even after one and another reconnect. The
> endless iteration will cause stack overflow and then system exception,
> and it may be not easy to find that the exception is actually caused
> by stack overflow.
> 
> So we limit the number of reconnect retry to 10 to improve code
> robustness.

Not really my place to comment on this, but how about removing the
recursion entirely, and turning the logic into a normal (iterative) loop
instead?

(1) Rename the current function to:

STATIC
VOID
BmRepairAllControllersWorker (
  OUT BOOLEAN *ReconnectRequired,
  OUT BOOLEAN *RebootRequired
  );


(2) The worker function should end right before

  if (ReconnectRequired) {
    BmRepairAllControllers ();
  }


(3) The worker function should not contain

  RebootRequired    = FALSE;
  ReconnectRequired = FALSE;

Such initialization should be left to the caller.


(4) The worker function should be called in a loop from a new
BmRepairAllControllers() function, like this:

  Reconnect = 0;
  RebootRequired = FALSE;
  do {
    ReconnectRequired = FALSE;
    BmRepairAllControllersWorker (&ReconnectRequired, &RebootRequired);
    ++Reconnect;
  } while (ReconnectRequired && Reconnect < MAX_RECONNECT_REPAIR);

  DEBUG_CODE (...);

  if (RebootRequired) {
    ...
  }


In addition to eliminating the shoddy recursive call (and the shoddier
global counter, ewww :) ), this would fix the following other warts with
the code:

- When a nested call chain is unwound, we currently dump a series of
"driver health info" lists (assuming no reboot is required), in the
DEBUG_CODE section. I would argue that we should do that only once, at
the end. (Even if we have to do it multiple times, it can be moved into
the worker function, to the end.)

- It seems to be sufficient to accumulate RebootRequired into one
variable (i.e. not multiple instances of the same local variable on the
stack) and to act upon it at the very end.


Feel free to ignore my comments -- I just think we should be moving in
the opposite direction; that is, away from recursion (especially from
recursion combined with global variables -- that's one difficult pattern
to reason about).

Thanks
Laszlo

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> ---
>  MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h     |  6 ++++++
>  MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c | 17 ++++++++++++++++-
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
> index 25a1d522fe84..9aa86b096525 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
> @@ -108,6 +108,12 @@ CHAR16 *
>  #define BM_OPTION_NAME_LEN                          sizeof ("PlatformRecovery####")
>  extern CHAR16  *mBmLoadOptionName[];
>  
> +//
> +// Maximum number of reconnect retry to repair controller; it is to limit the
> +// number of recursive call of BmRepairAllControllers.
> +//
> +#define MAX_RECONNECT_REPAIR                        10
> +
>  /**
>    Visitor function to be called by BmForEachVariable for each variable
>    in variable storage.
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c b/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c
> index ddcee8b0676f..30d70f32af84 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c
> @@ -26,6 +26,12 @@ GLOBAL_REMOVE_IF_UNREFERENCED
>      L"Reboot Required"
>      };
>  
> +//
> +// Counter of reconnect retry to repair controller; it is to limit the
> +// number of recursive call of BmRepairAllControllers.
> +//
> +STATIC UINTN mReconnectRepairCount;
> +
>  /**
>    Return the controller name.
>  
> @@ -549,7 +555,16 @@ BmRepairAllControllers (
>  
>  
>    if (ReconnectRequired) {
> -    BmRepairAllControllers ();
> +    if (mReconnectRepairCount < MAX_RECONNECT_REPAIR) {
> +      mReconnectRepairCount++;
> +      BmRepairAllControllers ();
> +    } else {
> +      DEBUG ((DEBUG_ERROR, "[%a:%d] Repair failed after %d retries.\n",
> +        __FUNCTION__, __LINE__, mReconnectRepairCount));
> +      // Reset counter so that it will not affect calling
> +      // BmRepairAllControllers() somewhere else
> +      mReconnectRepairCount = 0;
> +    }
>    }
>  
>    DEBUG_CODE (
> 



  parent reply	other threads:[~2018-02-26 16:17 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-26  8:29 [PATCH 1/1] MdeModulePkg/UefiBootManagerLib: limit recursive call depth Heyi Guo
2018-02-26  8:56 ` Wang, Sunny (HPS SW)
2018-02-26 11:34   ` Guo Heyi
2018-02-27  2:47     ` Wang, Sunny (HPS SW)
2018-02-26 16:23 ` Laszlo Ersek [this message]
2018-02-27  0:48   ` Guo Heyi
2018-02-27  5:48     ` Ni, Ruiyu
2018-02-27 10:29       ` Laszlo Ersek
2018-02-27 10:39         ` Guo Heyi
  -- strict thread matches above, loose matches on Subject: below --
2018-03-01  2:39 Heyi Guo
2018-03-01  2:43 ` Guo Heyi
2018-03-01  4:46 ` Ni, Ruiyu
2018-03-07  1:54   ` Guo Heyi
2018-03-08  2:53     ` Ni, Ruiyu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b3e02a30-217e-4b03-dde0-ee064cd12706@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox