From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:400e:c00::233; helo=mail-pf0-x233.google.com; envelope-from=heyi.guo@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pf0-x233.google.com (mail-pf0-x233.google.com [IPv6:2607:f8b0:400e:c00::233]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A2B7D20954CB4 for ; Mon, 26 Feb 2018 16:42:29 -0800 (PST) Received: by mail-pf0-x233.google.com with SMTP id 17so7252235pfw.11 for ; Mon, 26 Feb 2018 16:48:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:date:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=j7E8Yf2PrtWtJRKC5qROVIxYT/e2JcS8m5e/apxF1/c=; b=Af+Y3lO0XIX6mPCx6uU/Lvp/PRGSPrRjzJYFIbT6O7egk28jasaZ4goblf8mha/DRN s5opRQjKOTq5fBBNrP8dPUCWfv2c2n9yw5jPU/LoDZAtQ/BGYZOtL43gwNXOZLYkKPpA 2JarGubJfomhFwD7+8kDH+bDB3rZ/OaClr4wE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:date:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=j7E8Yf2PrtWtJRKC5qROVIxYT/e2JcS8m5e/apxF1/c=; b=LE+p/FJH7Zih7SjCbX8HrLkejEmSJK+rMebHyaYXU59eedFeIrprii5T2HkFKRzY7W 4g1rBcS0vjhGNJMRZCq4yYvRHpOM0aV41EmzEvjHjqWFTcIOxhBSvHNUMj7My3vbAzkd s3VtNU1/FA4XhAWR+MD9eAAU9QtVBGctBJdqYQQlcIycsAZJVVVzTglIz/EwVIenWQyL UY8oduM8VwDrsByQGpA26eo2k82jxrNacxh9mKItfJdt5xFNUuhLygyrPT53pmJb0ndE w4P2LPrrtLZZbCEiPYQx3oF7bP/n1SFOkqWb1hvS9CrYiZfjNsgd7gli1Cf3zVOHPzJU oLHg== X-Gm-Message-State: APf1xPCZr3r07qcqPoVFOExVUwg8cpyfDNAmuwrlLpLMoquYVF1Hr5t5 iSt+MRae0NdSEPjToCCVyUBohQ== X-Google-Smtp-Source: AH8x225SdbMe8b2LRBufgDVBn6MeJElF+YFQKsh86v7uN6wexwpfEHogumb+NKxo7fqkRXBPJFnHWA== X-Received: by 10.98.21.200 with SMTP id 191mr11604684pfv.166.1519692514486; Mon, 26 Feb 2018 16:48:34 -0800 (PST) Received: from SZX1000114654 ([104.237.91.49]) by smtp.gmail.com with ESMTPSA id u10sm1295164pfh.57.2018.02.26.16.48.32 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 26 Feb 2018 16:48:33 -0800 (PST) From: Guo Heyi X-Google-Original-From: Guo Heyi Date: Tue, 27 Feb 2018 08:48:30 +0800 To: Laszlo Ersek Cc: Heyi Guo , edk2-devel@lists.01.org, Ruiyu Ni , Eric Dong , Star Zeng Message-ID: <20180227004830.GA2261@SZX1000114654> References: <1519633779-130687-1-git-send-email-heyi.guo@linaro.org> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Subject: Re: [PATCH 1/1] MdeModulePkg/UefiBootManagerLib: limit recursive call depth X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 27 Feb 2018 00:42:30 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi Laszlo, I agree the current patch makes the code ugly, and turning the logic into a normal loop should be the perfect solution. If Ray also agrees on it, I can try to do that. Thanks and regards, Heyi On Mon, Feb 26, 2018 at 05:23:29PM +0100, Laszlo Ersek wrote: > 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 > > Cc: Star Zeng > > Cc: Eric Dong > > Cc: Ruiyu Ni > > --- > > 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 ( > > >