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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 32122C433F5 for ; Fri, 4 Mar 2022 03:49:05 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id BFFC08D0002; Thu, 3 Mar 2022 22:49:04 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id BDFD68D0001; Thu, 3 Mar 2022 22:49:04 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A9DF38D0002; Thu, 3 Mar 2022 22:49:04 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (relay.hostedemail.com [64.99.140.27]) by kanga.kvack.org (Postfix) with ESMTP id 972C68D0001 for ; Thu, 3 Mar 2022 22:49:04 -0500 (EST) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 57ADB61341 for ; Fri, 4 Mar 2022 03:49:04 +0000 (UTC) X-FDA: 79205323008.06.A9A3137 Received: from mail-oo1-f67.google.com (mail-oo1-f67.google.com [209.85.161.67]) by imf09.hostedemail.com (Postfix) with ESMTP id 8541D140004 for ; Fri, 4 Mar 2022 03:49:03 +0000 (UTC) Received: by mail-oo1-f67.google.com with SMTP id s203-20020a4a3bd4000000b003191c2dcbe8so8111089oos.9 for ; Thu, 03 Mar 2022 19:49:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=owY1s88VHqFIqmWlN/4fq6tMIh7jqxjAFgGkmfxykCc=; b=be6F6DUnBVqm6V8lf6rJEZlRStZRBrvf/NOPkoEFbfXxUE3/hQ6TPD10pbNkQro36n X7XWV7VX7RwlfhlghkxS6z4KqHuuY6n0xSK59EAzGhcks76Rg7ENd/tmaL+ivQxo6q6q ck/39o8bSNRHz6V1//cnQYt9KvTBcQP4LFOST9HXS6FRW1HMZ2cn1rf+8v0JbDm2J6D5 31dag7spo5VRAo6cPZIsMJM0lFhYVVp6oYxAAXmAOUw2lBTQacPvVqmzt//+4pxDRIDO hmEjg+t8xH7tRnRUjjr9UNmNassjZplIQ892Svbf0c1+cOv+Rs0PpGaRI4rqRnG94XkE Tb1g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=owY1s88VHqFIqmWlN/4fq6tMIh7jqxjAFgGkmfxykCc=; b=BL99Qtg/plM0weSIlExSDJwJ8R9PQcm4ihf6Jbu7Xv63uMt5ZLjz6ds/bigD63i0W9 HqkFBEfuf6yTOERUtaXte2nikTgcKCc3DSaHgi0WA7huagX14cs7xvIwHsdJbJXi7RYB pYb2kN6OBvcVo1PUS1ed7MLJ/PWmzXOCC1okGKWVe/gXX5Z/Ew6JD1KnagUTRd2TDpAO ZBdmdAKTqX18vPgS4YzvhwT2dNfDSa0tXr+ju9+nGLqf2Ug8k4ySHvOMy5KBmA9idPcy +nxsoSclAHgyqdZr1wZN13C+clKRw5AE/li+KQAhKfI436kcMhvyVxt4DRQMY/nvyy9/ FNVQ== X-Gm-Message-State: AOAM53265Zp+055zABkHcj51dPEf27PeoISdYxsgo0HJAu8ZhYuKs40B HTeAZC6ZseiEpDG3dvcT5ivfH1vw3FxbBeMn X-Google-Smtp-Source: ABdhPJzFlaLTCsMtGn2cwRd0MGGVF6lbESQ/hRdBkkUekh8Das9Mmy1uSMMmXl5QuWqGAAT+m8xkgQ== X-Received: by 2002:a17:90b:1bca:b0:1bf:1a17:beda with SMTP id oa10-20020a17090b1bca00b001bf1a17bedamr4491359pjb.215.1646362280440; Thu, 03 Mar 2022 18:51:20 -0800 (PST) Received: from ubuntu.huawei.com ([119.3.119.19]) by smtp.googlemail.com with ESMTPSA id w5-20020a056a0014c500b004f3a5535431sm4143781pfu.4.2022.03.03.18.51.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 03 Mar 2022 18:51:19 -0800 (PST) From: Xiaomeng Tong To: torvalds@linux-foundation.org Cc: arnd@arndb.de, gregkh@linuxfoundation.org, jakobkoschel@gmail.com, jannh@google.com, keescook@chromium.org, linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, netdev@vger.kernel.org, xiam0nd.tong@gmail.com Subject: Re: [PATCH 2/6] list: add new MACROs to make iterator invisiable outside the loop Date: Fri, 4 Mar 2022 10:51:09 +0800 Message-Id: <20220304025109.15501-1-xiam0nd.tong@gmail.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: References: X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 8541D140004 X-Stat-Signature: wdb1a8qj5jw495i3xzit8z3drkok7633 Authentication-Results: imf09.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=be6F6DUn; spf=pass (imf09.hostedemail.com: domain of xiam0nd.tong@gmail.com designates 209.85.161.67 as permitted sender) smtp.mailfrom=xiam0nd.tong@gmail.com; dmarc=pass (policy=none) header.from=gmail.com X-HE-Tag: 1646365743-935958 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000155, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: First of all, thank you very much for your patient reply and valuable comments. This is a great inspiration to me. > On Mon, Feb 28, 2022 at 11:59 PM Xiaomeng Tong wrote: > > > > +#define list_for_each_entry_inside(pos, type, head, member) \ > > So as mentioned in another thread, I actually tried exactly this. > > And it was horrendous. > > It's _technically_ probably a very nice solution, but Yes, I always think it is a perfect solution _technically_, since you first proposed in the thread of Jakob's first subject. > > - it means that the already *good* cases are the ones that are > penalized by having to change Yes, but it also kills potential risks that one day somebody mistakely uses iterator after the loop in this already *good* cases, as it removed the original declare of pos and any use-after-loop will be catched by compiler. > > - the syntax of the thing becomes absolutely nasty > > which means that _practially_ it's exactly the wrong thing to do. > > Just as an example, this is a random current "good user" in kernel/exit.c: > > - list_for_each_entry_safe(p, n, dead, ptrace_entry) { > + list_for_each_entry_safe_inside(p, n, struct task_struct, > dead, ptrace_entry) { > > and while some of the effects are nice (no need to declare p/n ahead > of time), just look at how nasty that line is. > > Basically every single use will result in an over-long line. The above > example has minimal indentation, almost minimal variable names (to the > point of not being very descriptive at all), and one of the most basic > kernel structure types. And it still ended up 87 columns wide. > > And no, the answer to that is not "do it on multiple lines then". > That is just even worse. Two avoid multiple lines, there are some mitigations: 1. use a shorter macro name: (add 2 chars) list_for_each_entry_i instead of list_for_each_entry_inside 2. using a shorter type passing to the macro: (add 3 chars) + #define t struct sram_bank_info - list_for_each_entry(pos, head, member) { + list_for_each_entry_i(pos, t, head, member) { 3. restore all name back to list_for_each_entry after everything is done: (minus 2 chars) Although we need replace all the use of list_for_each_entry* (15000+) with list_for_each_entry*_i, the work can be done gradually rather than all at once. We can incrementally replace these callers until all these in the kernel are completely updated with *_i* one. At that time, we can just remove the implements of origin macros and rename the *_i* macro back to the origin name just in one single patch. 4. As you mentioned, the "safe" version of list_for_each_entry do not need "n" argument anymore with the help of -std=gnu11. (minus 3 chars) Thus, after all mitigations applied, the "safe" version adds *no* chars to columns wide, and other version adds 3 chars totally, which is acceptable to me. > > So I really think this is a major step in the wrong direction. Maybe yes or maybe no. Before the list_for_each_entry_inside way, I have tried something like "typeof(pos) pos" way as and before you proposed in the thread of Jakob's second subject, to avoid any changes to callers of the macros. But it also has potential problems. see my previous reply to you here: https://lore.kernel.org/lkml/20220302093106.8402-1-xiam0nd.tong@gmail.com/ > > We should strive for the *bad* cases to have to do extra work, and > even there we should really strive for legibility. Indeed, there are many "multiple lines" problems in the current kernel code, for example (drivers/dma/iop-adma.c): list_for_each_entry_from(grp_iter, &iop_chan->chain, chain_node) { > > Now, I think that "safe" version in particular can be simplified: > there's no reason to give the "n" variable a name. Now that we can > (with -stc=gnu11) just declare our own variables in the for-loop, the > need for that externally visible 'next' declaration just goes away. > > So three of those 87 columns are pointless and should be removed. The > macro can just internally decare 'n' like it always wanted (but > couldn't do due to legacy C language syntax restrictions). Great, this does reduce three chars. and i will look into other versions. > > But even with that fixed, it's still a very cumbersome line. With other mitigations mentioned above, the addition to line will be acceptable. > > Note how the old syntax was "only" 60 characters - long but still > quite legible (and would have space for two more levels of indentation > without even hitting 80 characters). And that was _despute_ having to > have that 'n' declaration. > > And yes, the old syntax does require that > > struct task_struct *p, *n; > > line to declare the types, but that really is not a huge burden, and > is not complicated. It's just another "variables of the right type" > line (and as mentioned, the 'n' part has always been a C syntax > annoyance). Yes, that really is not a huge burden, so is the mitigation 2 mentioned above which defining a shorter type passing to the macro, to shorten the new line. > > Linus -- Xiaomeng Tong