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 X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A8144C43331 for ; Sat, 28 Mar 2020 02:56:09 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 38D10206F6 for ; Sat, 28 Mar 2020 02:56:09 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="OlqhpasY" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 38D10206F6 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 9613E6B0010; Fri, 27 Mar 2020 22:56:08 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 911FB6B0032; Fri, 27 Mar 2020 22:56:08 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7DAA26B0036; Fri, 27 Mar 2020 22:56:08 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0249.hostedemail.com [216.40.44.249]) by kanga.kvack.org (Postfix) with ESMTP id 66CC86B0010 for ; Fri, 27 Mar 2020 22:56:08 -0400 (EDT) Received: from smtpin18.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 71D5F180AD811 for ; Sat, 28 Mar 2020 02:56:08 +0000 (UTC) X-FDA: 76643256816.18.dock40_187767c9ab53c X-HE-Tag: dock40_187767c9ab53c X-Filterd-Recvd-Size: 7604 Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by imf47.hostedemail.com (Postfix) with ESMTP for ; Sat, 28 Mar 2020 02:56:07 +0000 (UTC) Received: by mail-wm1-f65.google.com with SMTP id c187so13746190wme.1 for ; Fri, 27 Mar 2020 19:56:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:reply-to:references:mime-version :content-disposition:in-reply-to:user-agent; bh=ZtEvNjpln4ORM2p45tfUuN3WjyJ390+vYelXRedBehY=; b=OlqhpasYqO12u3f1CNhykVEkyBwd/9NdjNjZOCCc6zz2kBJcYUi8AHVKyLGxjfVo0+ a1URseFx/zmG/c8QQhiK+xoltfUTAZ4kIptJaOJWFTYYYuX3POGgpyOw22IB8rKldg+h Q0tkqIlRTBmqTy6jQ67h2qotJ9h3vRtce2TB8QrdbjQM96qs+8/pIYl8PYpUqywc74sh 4gKBCxsDRLecKIerY+fJDSz8gRQsNsG6QO4PeR99yZdz95C87ks59hbkbWvwFDRq72qN pERGeh9ZPGL0sNphcsc8R5CFEM0nm4Elw23ZJn9QknGS1Vsavji1RJn2/YhsfaRQmF9V Iezw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:reply-to :references:mime-version:content-disposition:in-reply-to:user-agent; bh=ZtEvNjpln4ORM2p45tfUuN3WjyJ390+vYelXRedBehY=; b=GmOvd74RliETMQM2HrsWLCuctW2UpImBhdgJDKTSNuzYSbXqrvmt6v/r6Hfb8XD7VV YWuzX0T3BN/g9VSgBRBQtxJzcButOi1MzxG3kz+m14gL7qs2CX0jCA+M9hUjdsskwRio ZqRnZ0kiYBr5x3ouGbBUvgSb9J2LGgrvwRtoyNcC8b1w/ys+PtyjUMp+kpL8vBh9jR2x mrhe+ZJvHOBfapkIclcUJFeSwvWTr9BZHGz9Y/+x1zZBZKOsD8lu41erTFu3P2AA1ZlB +pi2A9XHwnldt8JZWoXacTK4XXEFK2vPj/lQ1yOYtFvGaeqi8/30muytfw9B340yFzxT 4fXw== X-Gm-Message-State: ANhLgQ3511UfaSYmG4hAlBcV8k2AEEMjXNHMRgaW+NuNUWcRFjeekaOG A9SMKFq2DsgQwPbhk22WmJ4= X-Google-Smtp-Source: ADFU+vsoe26Zu2+g296FpXaN9yZnaNizDQJVFea+gwg0Pg/xM7ohnt8e5fIJ5/Cky9nAOkvXVBfkig== X-Received: by 2002:a1c:a757:: with SMTP id q84mr1698667wme.146.1585364166459; Fri, 27 Mar 2020 19:56:06 -0700 (PDT) Received: from localhost ([185.92.221.13]) by smtp.gmail.com with ESMTPSA id a186sm10352904wmh.33.2020.03.27.19.56.05 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 27 Mar 2020 19:56:05 -0700 (PDT) Date: Sat, 28 Mar 2020 02:56:05 +0000 From: Wei Yang To: John Hubbard Cc: Wei Yang , akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, jgg@ziepe.ca, david@redhat.com Subject: Re: [Patch v2 2/2] mm/page_alloc.c: define node_order with all zero Message-ID: <20200328025605.cpnbnavl27pphr7g@master> Reply-To: Wei Yang References: <20200327220121.27823-1-richard.weiyang@gmail.com> <20200327220121.27823-2-richard.weiyang@gmail.com> <4c9d8138-d379-810f-64e7-0d018ed019df@nvidia.com> <20200328002616.kjtf7dpkqbugyzi2@master> <97a6bf40-792b-6216-d35b-691027c85aad@nvidia.com> <20200328011031.olsaehwdev2gqdsn@master> <40facd34-40b2-0925-90ca-a4c53fc520e8@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <40facd34-40b2-0925-90ca-a4c53fc520e8@nvidia.com> User-Agent: NeoMutt/20170113 (1.7.2) X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Fri, Mar 27, 2020 at 06:28:36PM -0700, John Hubbard wrote: >On 3/27/20 6:10 PM, Wei Yang wrote: >... >> > It's not just about preserving the value. Sometimes it's about stack space. >> > Here's the trade-offs for static variables within a function: >> > >> > Advantages of static variables within a function (compared to non-static >> > variables, also within a function): >> > ----------------------------------- >> > >> > * Doesn't use any of the scarce kernel stack space >> > * Preserves values (not always necessarily and advantage) >> > >> > Disadvantages: >> > ----------------------------------- >> > >> > * Removes basic thread safety: multiple threads can no longer independently >> > call the function without getting interaction, and generally that means >> > data corruption. >> > >> > So here, I suspect that the original motivation was probably to conserve stack >> > space, and the author likely observed that there was no concurrency to worry >> > about: the function was only being called by one thread at a time. Given those >> > constraints (which I haven't confirmed just yet, btw), a static function variable >> > fits well. >> > >> > > >> > > My suggestion is to remove the static and define it {0} instead of memset >> > > every time. Is my understanding correct here? >> > >> > >> > Not completely: >> > >> > a) First of all, "instead of memset every time" is a misconception, because >> > there is still a memset happening every time with {0}. It's just that the >> > compiler silently writes that code for you, and you don't see it on the >> > screen. But it's still there. >> > >> > b) Switching away from a static to an on-stack variable requires that you first >> > verify that stack space is not an issue. Or, if you determine that this >> > function needs the per-thread isolation that a non-static variable provides, >> > then you can switch to either an on-stack variable, or a *alloc() function. >> > >> >> I think you get some point. While one more question about stack and static. If >> one function is thread safe, which factor determines whether we choose on >> stack value or static? Any reference size? It looks currently we don't have a >> guide line for this. >> > > >There's not really any general guideline, but applying the points above (plus keeping >in mind that kernel stack space is quite small) to each case, you'll come to a good >answer. > >In this case, if we really are only ever calling this function in one thread at a time, >then it's probably best to let the "conserve stack space" point win. Which leads to >just leaving the code nearly as-is. The only thing left to do would be to (optionally, >because this is an exceedingly minor point) delete the arguably misleading "= {0}" part. >And as Jason points out, doing so also moves node_order into .bss : > >diff --git a/mm/page_alloc.c b/mm/page_alloc.c >index 4bd35eb83d34..cb4b07458249 100644 >--- a/mm/page_alloc.c >+++ b/mm/page_alloc.c >@@ -5607,7 +5607,7 @@ static void build_thisnode_zonelists(pg_data_t *pgdat) > static void build_zonelists(pg_data_t *pgdat) > { >- static int node_order[MAX_NUMNODES] = {0}; This is what I added, so I would drop this one. >+ static int node_order[MAX_NUMNODES]; > int node, load, nr_nodes = 0; > nodemask_t used_mask = NODE_MASK_NONE; > int local_node, prev_node; > > > >Further note: On my current testing .config, I've got MAX_NUMNODES set to 64, which makes >256 bytes required for node_order array. 256 bytes on a 16KB stack is a little bit above >my mental watermark for "that's too much in today's kernels". > Thanks for your explanation. I would keep this in mind. Now I have one more question, hope it won't sound silly. (16KB / 256) = 64, this means if each function call takes 256 space on stack, the max call depth is 64. So how deep a kernel function call would be? or expected to be? Also because of the limit space on stack, recursive function is not welcome in kernel neither. Am I right? > >thanks, >-- >John Hubbard >NVIDIA > -- Wei Yang Help you, Help me