From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id DD8B3C433F5 for ; Wed, 29 Dec 2021 12:21:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date: Message-ID:From:References:CC:To:Subject:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Owner; bh=D/+zVeHMrT1PjH9Kvo3mK5nZQDpdkC4ZQk2zJ6E4PB0=; b=x3DPZHYF5gd7ujBQ9LzJ4iIRN+ EBAoJbqBg4A+56/YPoph+01uSEkVSuJyaZsDSVhx0dlPQfTgB4swFXunHCAWYFooZ8IbEzTFlJmhP DztlMYZbZiSoagpFAUlyOtOpvUpaV4Shb57RcEacwyaiycarVr6qI4qjnUMlGys1Hghe+wJWNG8fs N4dy7k1Nb8KIloXVU4ilRIPe/GsA2Me+2Ne5RmgqmNElvY4nq4W9sYV9sPc/oALVG63rk4Jh4fADO A/w60Znd14wbus3Uam9c1VQhnBajbECPzXaajntH3sICWTOeW5H5V+Zo1qr3PRm5kwiDZJ4WKj6Mj 4R71GfRA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1n2Xvu-002jsD-2S; Wed, 29 Dec 2021 12:19:54 +0000 Received: from szxga08-in.huawei.com ([45.249.212.255]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1n2Xvn-002joi-Em; Wed, 29 Dec 2021 12:19:49 +0000 Received: from dggpemm500022.china.huawei.com (unknown [172.30.72.56]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4JP9P34mBfz1DJjH; Wed, 29 Dec 2021 20:16:23 +0800 (CST) Received: from dggpemm500006.china.huawei.com (7.185.36.236) by dggpemm500022.china.huawei.com (7.185.36.162) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.20; Wed, 29 Dec 2021 20:19:41 +0800 Received: from [10.174.178.55] (10.174.178.55) by dggpemm500006.china.huawei.com (7.185.36.236) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.20; Wed, 29 Dec 2021 20:19:40 +0800 Subject: Re: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code To: Dave Young CC: Borislav Petkov , Thomas Gleixner , "Ingo Molnar" , , "H . Peter Anvin" , , Baoquan He , Vivek Goyal , Eric Biederman , , Catalin Marinas , "Will Deacon" , , Rob Herring , Frank Rowand , , Jonathan Corbet , , Randy Dunlap , Feng Zhou , Kefeng Wang , "Chen Zhou" , John Donnelly References: <20211228132612.1860-1-thunder.leizhen@huawei.com> <20211228132612.1860-3-thunder.leizhen@huawei.com> From: "Leizhen (ThunderTown)" Message-ID: Date: Wed, 29 Dec 2021 20:19:39 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-Originating-IP: [10.174.178.55] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) To dggpemm500006.china.huawei.com (7.185.36.236) X-CFilter-Loop: Reflected X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211229_041947_897051_142054BC X-CRM114-Status: GOOD ( 35.78 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 2021/12/29 15:27, Dave Young wrote: > On 12/29/21 at 10:27am, Leizhen (ThunderTown) wrote: >> >> >> On 2021/12/29 0:13, Borislav Petkov wrote: >>> On Tue, Dec 28, 2021 at 09:26:01PM +0800, Zhen Lei wrote: >>>> Use parse_crashkernel_high_low() to bring the parsing of >>>> "crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they >>>> are strongly dependent, make code logic clear and more readable. >>>> >>>> Suggested-by: Borislav Petkov >>> >>> Yeah, doesn't look like something I suggested... >>> >>>> @@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void) >>>> /* crashkernel=XM */ >>>> ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base); >>>> if (ret != 0 || crash_size <= 0) { >>>> - /* crashkernel=X,high */ >>>> - ret = parse_crashkernel_high(boot_command_line, total_mem, >>>> - &crash_size, &crash_base); >>>> - if (ret != 0 || crash_size <= 0) >>>> + /* crashkernel=X,high and possible crashkernel=Y,low */ >>>> + ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size); >>> >>> So this calls parse_crashkernel() and when that one fails, it calls this >>> new weird parse high/low helper you added. >>> >>> But then all three end up in the same __parse_crashkernel() worker >>> function which seems to do the actual parsing. >>> >>> What I suggested and what would be real clean is if the arches would >>> simply call a *single* >>> >>> parse_crashkernel() >>> >>> function and when that one returns, *all* crashkernel= options would >>> have been parsed properly, low, high, middle crashkernel, whatever... >>> and the caller would know what crash kernel needs to be allocated. >>> >>> Then each arch can do its memory allocations and checks based on that >>> parsed data and decide to allocate or bail. >> >> However, only x86 currently supports "crashkernel=X,high" and "crashkernel=Y,low", and arm64 >> will also support it. It is not supported on other architectures. So changing parse_crashkernel() >> is not appropriate unless a new function is introduced. But naming this new function isn't easy, >> and the name parse_crashkernel_in_order() that I've named before doesn't seem to be good. >> Of course, we can also consider changing parse_crashkernel() to another name, then use >> parse_crashkernel() to parse all possible "crashkernel=" options in order, but this will cause >> other architectures to change as well. > > Hi, I did not follow up all discussions, but if the only difference is > about the low -> high fallback, I think you can add another argument in > parse_crashkernel(..., *fallback_high), and doing some changes in > __parse_crashkernel() before it returns. But since there are two > many arguments, you could need a wrapper struct for crashkernel_param if > needed. A wrapper struct is needed. Because at least two new output parameters need to be added: 1. high, to indicate whether "crashkernel=X,high" is specified 2. low_size, to save the memory size specified by "crashkernel=Y,low" > > If you do not want to touch other arches, another function maybe > something like parse_crashkernel_fallback() for x86 and arm64 to use. > > But I may not get all the context, please ignore if this is not the > case. I agree that calling parse_crash_kernel* in the > reserve_crashkernel funtions looks not good though. > > OTOH there are bunch of other logics in param parsing code, > eg. determin the final size and offset etc. To split the logic out more > things need to be done, eg. firstly parsing function just get all the > inputs from cmdline string eg. an array of struct crashkernel_param with > mem_range, expected size, expected offset, high, or low, and do not do > any other things. Then pass these parsed inputs to another function to > determine the final crash_size and crash_base, that part can be arch > specific somehow. Yes, this way makes the code logic clear. But there's a bit of trouble with "crashkernel=range1:size1[,range2:size2,...][@offset]", the array size is dynamic. > > So I think you can unify the parse_crashkernel* in x86 first with just > one function. And leave the further improvements to later work. But > let's see how Boris think about this. > >> >>> >>> So it is getting there but it needs more surgery... >>> >>> Thx. >>> >> >> -- >> Regards, >> Zhen Lei >> > > Thanks > Dave > > . > -- Regards, Zhen Lei _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel