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 8D88AC0015E for ; Sun, 18 Jun 2023 22:33:02 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7CA8C8D0002; Sun, 18 Jun 2023 18:33:01 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 77A5A8D0001; Sun, 18 Jun 2023 18:33:01 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 669368D0002; Sun, 18 Jun 2023 18:33:01 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 559598D0001 for ; Sun, 18 Jun 2023 18:33:01 -0400 (EDT) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 208F4AF797 for ; Sun, 18 Jun 2023 22:33:01 +0000 (UTC) X-FDA: 80917320162.27.818ED50 Received: from galois.linutronix.de (Galois.linutronix.de [193.142.43.55]) by imf07.hostedemail.com (Postfix) with ESMTP id 5D44240005 for ; Sun, 18 Jun 2023 22:32:58 +0000 (UTC) Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=linutronix.de header.s=2020 header.b=bdpHhvr9; dkim=pass header.d=linutronix.de header.s=2020e header.b=ZuJVVOhs; dmarc=pass (policy=none) header.from=linutronix.de; spf=pass (imf07.hostedemail.com: domain of tglx@linutronix.de designates 193.142.43.55 as permitted sender) smtp.mailfrom=tglx@linutronix.de ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1687127578; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=5eocF6oVYznFl/DEwITHqQ7TMn26EFf0pZbwEDCTK84=; b=hdclSEza+Y79BvFsSTcX9w+H5gh8JJqB0FWP+4Nh9c1VrRV29dUSqfgRvS4CE2osm/sFbb Im1J7nR+J3FHte5pvGk+Q+nbHOwl8rDKumMgeO/YDALZhpnws5oNhHfwsYjuBpMqBLa9qn 1+7YcRbWKgqkzJgeAfnMlu2OIlME4gY= ARC-Authentication-Results: i=1; imf07.hostedemail.com; dkim=pass header.d=linutronix.de header.s=2020 header.b=bdpHhvr9; dkim=pass header.d=linutronix.de header.s=2020e header.b=ZuJVVOhs; dmarc=pass (policy=none) header.from=linutronix.de; spf=pass (imf07.hostedemail.com: domain of tglx@linutronix.de designates 193.142.43.55 as permitted sender) smtp.mailfrom=tglx@linutronix.de ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1687127578; a=rsa-sha256; cv=none; b=FHTRmaZFZP2Pc2UUEKrUgQcbq2ouGNU2sRrQpvD7dshL47pStoHFE6FQZtLxnPzxxVmjGD xb4QzZ8Ly6yfkHyD9KumkbOTCWr39XOHiDkLnNjb5lMBBdFFsjeoujtqGGSDbFgdFOL042 1ARt5m8595qxWM9+5tR8FE0rv+Hgzr4= From: Thomas Gleixner DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1687127575; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=5eocF6oVYznFl/DEwITHqQ7TMn26EFf0pZbwEDCTK84=; b=bdpHhvr9Hn5s8M8pBt6h0RlUGOZ1Lo4/y28rwu0yVR7JZSlAq39/46BcKsilIUinfOPogU qmKwez+jgzyMGsHhHmUjh9dF2vQKfcfzUpnC430yDZMMldnMWI1vYw06jpKeElXhmTcntG yypvYR4iRqVpOziVhZdqJC7qgQJw5KA81XnRrk5f520jV+sacnO/fQhXBIQnrLmfO/2M+r YFxwYhKBguqFcAkoCz4x6FEz5SiwhD5VV9/U1fv8TVeo3fA0XlwfdM8MGzqwB21MFiONgp HFCCVB4X260bSKBTYLft5JXmqXu6l7oYYyLOqT0toHcgXAbIR5XTU+7H5xAo3g== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1687127575; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=5eocF6oVYznFl/DEwITHqQ7TMn26EFf0pZbwEDCTK84=; b=ZuJVVOhsCR9WDQQO84QX2oPc5VTYDEn0agA6nGmpVtt6s52bacZEJ4aPLDh/ary7zlNGrA JmqYVkKOO6bt5CBA== To: Mike Rapoport , linux-kernel@vger.kernel.org Cc: Andrew Morton , Catalin Marinas , Christophe Leroy , "David S. Miller" , Dinh Nguyen , Heiko Carstens , Helge Deller , Huacai Chen , Kent Overstreet , Luis Chamberlain , Mark Rutland , Michael Ellerman , Mike Rapoport , Nadav Amit , "Naveen N. Rao" , Palmer Dabbelt , Puranjay Mohan , Rick Edgecombe , Russell King , Song Liu , Steven Rostedt , Thomas Bogendoerfer , Will Deacon , bpf@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mips@vger.kernel.org, linux-mm@kvack.org, linux-modules@vger.kernel.org, linux-parisc@vger.kernel.org, linux-riscv@lists.infradead.org, linux-s390@vger.kernel.org, linux-trace-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, loongarch@lists.linux.dev, netdev@vger.kernel.org, sparclinux@vger.kernel.org, x86@kernel.org Subject: Re: [PATCH v2 06/12] mm/execmem: introduce execmem_data_alloc() In-Reply-To: <20230616085038.4121892-7-rppt@kernel.org> References: <20230616085038.4121892-1-rppt@kernel.org> <20230616085038.4121892-7-rppt@kernel.org> Date: Mon, 19 Jun 2023 00:32:55 +0200 Message-ID: <87jzw0qu3s.ffs@tglx> MIME-Version: 1.0 Content-Type: text/plain X-Rspamd-Queue-Id: 5D44240005 X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: kiop7dssjjomndyhnmg1sen8ptd3ot71 X-HE-Tag: 1687127578-432912 X-HE-Meta: U2FsdGVkX1/X0703k0yefKBHMW/nqDjJuBs+uHo0/VQqwbYbPBZpm0jd6xFMkeduIh8iT+Qrdx7V7tpXl2dwny9lWrrKg2USPXmeYq0Ghkd7irMIt7HX5pWSZ1iRLv+soZ8EAt8sEh553yXish75+LqXShTY+vCqp/Tj3WoRgjSy5Dy8fTpyMLwPhrFDT0OUSxyj7slW3mfq1231OqtJ9DWqFC1f2MQ7vKAwFErww+8kFApMjY2xMn6SpTyW4j8UKAVR6YhmLfqRoQ1XCWuHZund68x3xWVDnaLxPunGq9zATOmWX9+uAUijM0p2kS0XNv30bnsSqJaT7Is0bshAKAEnFxrUKg+gD6yjYgHUV+4H+QPYy3ZtIAEMzQ93WhK6GXX0VB3NVeQYdzjpmg/+EvVArk2NuQbdXzC2H0BMXKs2iZ1ULguI2O1UUMu+lIbQJEY72i2R+BOhQNtTH0QHxwiSlzAx3zPOE4Lb9Vs9SQ3826uK0X/fa+Gq6GSJJ/ZDzZB+6WELuo7/2N0jYH/WkaFAcoTU02milI5n5kS5olas8FHwS8L8HHpgDPOnMlkppUEw41bs0lNn4DcRXBZBiX2HcP8TMChPJjVRDwkzt4mNIOH5eyM/t42tlSa26HAItxd5VItwcnBBn0eTMeWSm2bzFBnnS15ZzmPIH7dWdjIkor1qniPd/3kiRdbrJdfKip6mPAWo8xz/o5L78p7wC2FGq5B8FDfpEnhf2NDJ35lHoaQgFICEiJYMcNgyNhS73vf3AnIZP72pg9wTr7xB6ngc8AaoijUSz7mrNibJIxWGlFFesyWW/Aq8P6spo/XtExQm2Km9IH4OeW6CS6y9uOyRnmhBhSLyzSLLVPgvL45vLSmTBz5W5hhIIFecbpbIpuOhlej1XeOTMx7RFRCaG4UUmUeFo3f+rg5fBBAZEBjeHkGRbB7/2WLray0KIm02GUt95uNS4pdpJEGsJL5 gL/b9Xzr lnb6vlRtYw6dlX7MhNf/h8H2vvgJZ8TTAUgOtrqf56Ahf2ZbxtlgnAX5gxSvxBQXUCV65C/uT4vFkoINGJeIj24xij8qbJMm4HW9a9ZsZY+xlDAbS+n0wviD9Z4Q/W7hovuLDUT36PtvdM/A/5EQcYkVTlBEZl9tlUuziwJiXVwa/BUR76iVFToc9MfEnxPWuHd7BsJXp1x3uzWJY9f+gE5cy71GxxKyBwWlZq4ms9RAkL0E= 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: Mike! Sorry for being late on this ... On Fri, Jun 16 2023 at 11:50, Mike Rapoport wrote: > > +void *execmem_data_alloc(size_t size) > +{ > + unsigned long start = execmem_params.modules.data.start; > + unsigned long end = execmem_params.modules.data.end; > + pgprot_t pgprot = execmem_params.modules.data.pgprot; > + unsigned int align = execmem_params.modules.data.alignment; > + unsigned long fallback_start = execmem_params.modules.data.fallback_start; > + unsigned long fallback_end = execmem_params.modules.data.fallback_end; > + bool kasan = execmem_params.modules.flags & EXECMEM_KASAN_SHADOW; While I know for sure that you read up on the discussion I had with Song about data structures, it seems you completely failed to understand it. > + return execmem_alloc(size, start, end, align, pgprot, > + fallback_start, fallback_end, kasan); Having _seven_ intermediate variables to fill _eight_ arguments of a function instead of handing in @size and a proper struct pointer is tasteless and disgusting at best. Six out of those seven parameters are from: execmem_params.module.data while the KASAN shadow part is retrieved from execmem_params.module.flags So what prevents you from having a uniform data structure, which is extensible and decribes _all_ types of allocations? Absolutely nothing. The flags part can either be in the type dependend part or you make the type configs an array as I had suggested originally and then execmem_alloc() becomes: void *execmem_alloc(type, size) and static inline void *execmem_data_alloc(size_t size) { return execmem_alloc(EXECMEM_TYPE_DATA, size); } which gets the type independent parts from @execmem_param. Just read through your own series and watch the evolution of execmem_alloc(): static void *execmem_alloc(size_t size) static void *execmem_alloc(size_t size, unsigned long start, unsigned long end, unsigned int align, pgprot_t pgprot) static void *execmem_alloc(size_t len, unsigned long start, unsigned long end, unsigned int align, pgprot_t pgprot, unsigned long fallback_start, unsigned long fallback_end, bool kasan) In a month from now this function will have _ten_ parameters and tons of horrible wrappers which convert an already existing data structure into individual function arguments. Seriously? If you want this function to be [ab]used outside of the exec_param configuration space for whatever non-sensical reasons then this still can be either: void *execmem_alloc(params, type, size) static inline void *execmem_data_alloc(size_t size) { return execmem_alloc(&exec_param, EXECMEM_TYPE_DATA, size); } or void *execmem_alloc(type_params, size); static inline void *execmem_data_alloc(size_t size) { return execmem_alloc(&exec_param.data, size); } which both allows you to provide alternative params, right? Coming back to my conversation with Song: "Bad programmers worry about the code. Good programmers worry about data structures and their relationships." You might want to reread: https://lore.kernel.org/r/87lenuukj0.ffs@tglx and the subsequent thread. The fact that my suggestions had a 'mod_' namespace prefix does not make any of my points moot. Song did an extremly good job in abstracting things out, but you decided to ditch his ground work instead of building on it and keeping the good parts. That's beyond sad. Worse, you went the full 'not invented here' path with an outcome which is even worse than the original hackery from Song which started the above referenced thread. I don't know what caused you to decide that ad hoc hackery is better than proper data structure based design patterns. I actually don't want to know. As much as my voice counts: NAK-ed-by: Thomas Gleixner Thanks, tglx