From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::234; helo=mail-it0-x234.google.com; envelope-from=heyi.guo@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x234.google.com (mail-it0-x234.google.com [IPv6:2607:f8b0:4001:c0b::234]) (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 01F17222DE146 for ; Tue, 27 Feb 2018 02:33:37 -0800 (PST) Received: by mail-it0-x234.google.com with SMTP id l187so14749471ith.4 for ; Tue, 27 Feb 2018 02:39:44 -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:content-transfer-encoding:in-reply-to :user-agent; bh=yJKW6FH1jYyn6JmQWox6G0eUSDZ6DdGiFarvk88gBH4=; b=TNfZ/teXIDBYK8nQMkc3j4pPHJirH31lwHjAfo6t04rmCzGiBIfZ0ZqHrvbHirw1DT 3k+847d0WDq/OPrfdNQj2GGkGvv5UPySMLQpJl6ljR0gXXgWuiM3HAblIzj0BdojwRvN Xlh6Xedc3LhASXpSxS0UBkwnBQ2vhc6ZENARs= 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:content-transfer-encoding :in-reply-to:user-agent; bh=yJKW6FH1jYyn6JmQWox6G0eUSDZ6DdGiFarvk88gBH4=; b=juSi4c8sfpto0JNpJar9weDwwXOuKKFx6MPRTO7HBpgOsPg3Pc8B9EbetAAcaymB7h d+6fdb8QRZhcBOPB+OnBwUWyWP1rE3Z7Ey1O3ByCb4h1TVWL96SXceXT85OHym8lPglc WsPEAKkpdR6NbGs/kCwhHsqAaAusg1ugsiuG/3LaJSZ0hHVyMjjpvTo3H8H3d0uYmuLw qslV5kZIlxy+yHVqYbMV+b3eb6Cwm6qwahViFIPPS0MtTU1fsuZgc9aVxUuZeBxUm+Wb 0aMYujIbQOChMdb8FUdej7jasGy2SsxFO3cajvDzsoXw3hyPEtTVKcSwcgg7N+ZxoOEv ixxg== X-Gm-Message-State: APf1xPA83fJUm8aJZvpQ+Gye1DkYUB8u+PpPgRD68qNOEsOLRqgrtq9L u/RIA5EXEr72DbTeSEhByffo5g== X-Google-Smtp-Source: AH8x225jGUQEQ+ftcdxU+IoyGjlbf0kBE2PkGKetyLY2dXvOb2AL06bHp64DYuRm72UNdqtiF+DB6A== X-Received: by 10.36.28.208 with SMTP id c199mr16768502itc.53.1519727983096; Tue, 27 Feb 2018 02:39:43 -0800 (PST) Received: from SZX1000114654 ([104.237.91.49]) by smtp.gmail.com with ESMTPSA id u132sm6945235itf.0.2018.02.27.02.39.40 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 27 Feb 2018 02:39:42 -0800 (PST) From: Guo Heyi X-Google-Original-From: Guo Heyi Date: Tue, 27 Feb 2018 18:39:38 +0800 To: Laszlo Ersek Cc: "Ni, Ruiyu" , Guo Heyi , edk2-devel@lists.01.org, Star Zeng , Eric Dong Message-ID: <20180227103938.GC3918@SZX1000114654> References: <1519633779-130687-1-git-send-email-heyi.guo@linaro.org> <20180227004830.GA2261@SZX1000114654> <169e8bb8-00c5-43ec-094a-28079a5fa1d1@redhat.com> MIME-Version: 1.0 In-Reply-To: <169e8bb8-00c5-43ec-094a-28079a5fa1d1@redhat.com> 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 10:33:38 -0000 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit Thanks Ray and Laszlo, I will create v2 according to your comments. Regards, Heyi On Tue, Feb 27, 2018 at 11:29:18AM +0100, Laszlo Ersek wrote: > On 02/27/18 06:48, Ni, Ruiyu wrote: > > On 2/27/2018 8:48 AM, Guo Heyi wrote: > >> 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). > > > > How about to just remove the global variable? > > I prefer to change BmRepairAllControllers in the following prototype: > > VOID > > BmRepairAllControllers ( > >   UINTN  ReconnectRepairCount > >   ); > > And start to call this like BmRepairAllControllers (0). > > > > I am neutral between recursive call and while loop. > > But I am afraid such a big change may introduce some bugs. > > And I also like to move the DEBUG_CODE to before: > > if (ReconnectRequired) { > >   BmRepairAllControllers (ReconnectRepairCount + 1); > > } > > So that we can dump the health info for every reconnect repair. > > Sure, that too works for me. > > Thanks! > Laszlo