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 6ABD7C77B7D for ; Tue, 18 Apr 2023 18:30:56 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B40238E0003; Tue, 18 Apr 2023 14:30:55 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AF13A8E0001; Tue, 18 Apr 2023 14:30:55 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9B7E88E0003; Tue, 18 Apr 2023 14:30:55 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 8970A8E0001 for ; Tue, 18 Apr 2023 14:30:55 -0400 (EDT) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 4DB791A03BA for ; Tue, 18 Apr 2023 18:30:55 +0000 (UTC) X-FDA: 80695353270.18.8DE4B9D Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) by imf01.hostedemail.com (Postfix) with ESMTP id 98EBD40026 for ; Tue, 18 Apr 2023 18:30:50 +0000 (UTC) Authentication-Results: imf01.hostedemail.com; dkim=pass header.d=infradead.org header.s=bombadil.20210309 header.b="jc0q0X/S"; spf=none (imf01.hostedemail.com: domain of mcgrof@infradead.org has no SPF policy when checking 198.137.202.133) smtp.mailfrom=mcgrof@infradead.org; dmarc=fail reason="No valid SPF, DKIM not aligned (relaxed)" header.from=kernel.org (policy=none) ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1681842652; h=from:from:sender: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=Y/wE/R9Dcq9B6NYciIuhBA/N45s6bRLmpBHThTjOxQg=; b=XFc6fWMVaFLd5MNAkck8dBpmRRw4pkfAoNbwoveaWsUW+7dpC+Vattdu3k/G/Acnk4/wti VbjY4AeFvmaZfFT/OCBKGVyQU7srX7dz2NJ4ZJ1lYL0pGfdJZXztLsyj9sp303GkWk3I3P dKJ+hgCtE6zCNu1MKLkEmS+LP85JH+A= ARC-Authentication-Results: i=1; imf01.hostedemail.com; dkim=pass header.d=infradead.org header.s=bombadil.20210309 header.b="jc0q0X/S"; spf=none (imf01.hostedemail.com: domain of mcgrof@infradead.org has no SPF policy when checking 198.137.202.133) smtp.mailfrom=mcgrof@infradead.org; dmarc=fail reason="No valid SPF, DKIM not aligned (relaxed)" header.from=kernel.org (policy=none) ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1681842652; a=rsa-sha256; cv=none; b=I0FwAEobQBZeqowqc8gMJF5hQehNWqe9PfVVH4O74i+H30dPFAtK094Ybxgc+NRwCHXZu+ SEE+Q+3EXb+UDJNWi+wldrMBW/pbKYneW4G/5ykB8wDqF4P9nuSIBowJPTDorsIklMuwHd TsWGIbyJuYYmT9Gq19Nc8YEMlBxDoVE= DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20210309; h=Sender:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=Y/wE/R9Dcq9B6NYciIuhBA/N45s6bRLmpBHThTjOxQg=; b=jc0q0X/Stu0gCYHzxjw2YLMSYK g+4WCw/KVSVM7FtcywTLV3RGc6XplWnEgKfSo1u2tqgex5fiqgYrAgUQ7yeeveB70AXx3fWbE9wS0 T6EdB8GrRaz7IhwZ7cl23Z1grJoRtjp96PL8oV63XpK/YTv/FWlwuH4KCE/iljkVQZEkEL+eBMDxC IhSL0b8zBkUVqVFgqg7N+saruwD3wcr0/6rswZ+fGmL6Y7YoFwZp6NIy1V/+aMxMyz3nOnZLORiv3 hhkyTmjiFTwA/qjEub0if7Bj/1PwzybfOo1akaTMPztJAYfzrwYTHL9mSop0tEsiUt+vRVOHTZOdg VcXV7aeA==; Received: from mcgrof by bombadil.infradead.org with local (Exim 4.96 #2 (Red Hat Linux)) id 1poq68-0034Zx-1b; Tue, 18 Apr 2023 18:30:36 +0000 Date: Tue, 18 Apr 2023 11:30:36 -0700 From: Luis Chamberlain To: Petr Pavlu Cc: christophe.leroy@csgroup.eu, tglx@linutronix.de, peterz@infradead.org, song@kernel.org, rppt@kernel.org, dave@stgolabs.net, willy@infradead.org, vbabka@suse.cz, mhocko@suse.com, dave.hansen@linux.intel.com, colin.i.king@gmail.com, jim.cromie@gmail.com, catalin.marinas@arm.com, jbaron@akamai.com, rick.p.edgecombe@intel.com, david@redhat.com, patches@lists.linux.dev, linux-modules@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, pmladek@suse.com, prarit@redhat.com, torvalds@linux-foundation.org, gregkh@linuxfoundation.org, rafael@kernel.org Subject: Re: [PATCH v3 3/4] module: add debug stats to help identify memory pressure Message-ID: References: <20230414050836.1984746-1-mcgrof@kernel.org> <20230414050836.1984746-4-mcgrof@kernel.org> <6110982a-bb68-c88b-6fd1-24d2c49d9fd7@suse.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6110982a-bb68-c88b-6fd1-24d2c49d9fd7@suse.com> X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 98EBD40026 X-Rspam-User: X-Stat-Signature: yc3e18qqiiks5giji4d57urwx6io6cih X-HE-Tag: 1681842650-682301 X-HE-Meta: U2FsdGVkX1+n2X7QVYNUOCR4qHvBHQDtqBF2LXD4LnS+aqz+kyj3azRDxkvMK2uB5CoHssyKTorU+dnZdUEC3mxk6CaSlGVTf/9Tb0RbZ1ir1tAGGGmLEDkZ2ZtnDllur25h8YRxXQh7L50q6V5ZIsC8ymwd7t9r/PBtP7HI8V75ooq3mYMIho1MK9LVaEn9NY9TjdQwgoqaszlzbrTBggiGMLiqwM6gdqirJ3Q1JDnqqTZPHjmHPZP2XAd0JWglvUdGZlzsA4lL+R5Yy3q3XNupi4XZpfzgB+cwLCW74KDtTtShFMBDAp9vvQF7BYu+w3OqCCtKDANYPxdrboKzOWKiVs/mcAPDFxSpv5RfaHIbK0Se6SlmGMxqG53/kI7f/JNuAaM3BBKGFtY44UtpH8F5NRDejnN4FSTG9aQoR3dNeMb8r2ItjEG1higkYUJjaZcKwqbwDT6IQLi/boPHhl3qBZeS4kpNoSw1iCKDbBbcPmlWR/Emeuvk0lvQuTbc4MGDvDvGDyaRBq7cY027XB53gKIMvyUXXIpSdxuLaqVkLryqubYq71N4269SjUjgKOR3TELT5MNoIO2lW2U0rAZ+qRI/WUPnxqYt1IywXWct2s9yjQMwwyRY7cNPUvMgTxIWS47XfLIoAI1PjqTBDBvEy/OvAUNpVdHVz3q7CKJ0r9MQhDlmKryLvgd2EDm9uqO+GBNieBV9gmu/j0Juo6yXLl9oj631sg4NellwYQIzjoBW/RGoCYuhcW2RFIPLf203/z1VDEc/ca8SZQjPO7Sc5SLiVUA+iiuBHPkvzSKmDh19SOs++E/s2jzCfl9ubRgtogcFdDFYuUvESqrNs4nB4QbNdBN4NVYfbdTB00g8jXrpYQv8ST+Ju60CCql2+SvWK10PRBssEcIoFmxThMHZSyXuBtL1lglLKH1BPZczpXq3ZkuezzLckkn4JCn7Yr9Wlgd5f9Aw8+kcN8j iJoe0+0E rXX8oTok5D1ECn8hAdl/pLG8040MryKI8E3VdxAm3TtmXEhq6X3EF1C8ZXe0AKj4ai/KvrsVMWoL7oc9f3hcDm1kqXT90ft8n+B5/HN08a4dbHS7gxphrNZBsm3FAjYTAZZH1hFO1lxNGBwQt3gaW+bcC7oaRy0ukSdNhjRQvkql6R61t/hanHeZq31Brs1CoEKY591/RrSQxDmBJxL9Nx4cqOhdhNvAZr8rFofQjdHSfOyq+JQo8l/MS8RcbzoE6sHyzwE2d4DdmFwGj9DOdquBVtuKzcg3iorggBU/lLOvB0sys94yf9Rrc/xS9qGoTP96uOmz7Syla2qRHzFXLvTt5l5t0JGx0fKMD9m0y9mpj9dtzChg42tfGrib5cXoRyCdMtdeWq5J5myc= 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 Mon, Apr 17, 2023 at 01:18:14PM +0200, Petr Pavlu wrote: > On 4/14/23 07:08, Luis Chamberlain wrote: <-- Petr's spell checking --> > Note that there are plenty of other typos in the added comments and > documentation. Please review them with a spell checker. Yes I am terrible at that, I've now integrated a spell checker into my workflow. Fixed all these, thanks. > > @@ -2500,6 +2503,18 @@ static noinline int do_init_module(struct module *mod) > > { > > int ret = 0; > > struct mod_initfree *freeinit; > > +#if defined(CONFIG_MODULE_STATS) > > + unsigned int text_size = 0, total_size = 0; > > + > > + for_each_mod_mem_type(type) { > > + const struct module_memory *mod_mem = &mod->mem[type]; > > + if (mod_mem->size) { > > + total_size += mod_mem->size; > > + if (type == MOD_TEXT || type == MOD_INIT_TEXT) > > + text_size += mod->mem[type].size; > > 'text_size += mod_mem->size;' would be simpler. Sure. > > +extern struct dentry *mod_debugfs_root; > > Files kernel/module/stats.c and kernel/module/tracking.c both add this extern > declaration. Can it be moved to kernel/module/internal.h? Sure. > > +#if defined(CONFIG_MODULE_DECOMPRESS) > > + if (flags & MODULE_INIT_COMPRESSED_FILE) > > + atomic_long_add(info->compressed_len, &invalid_mod_byte); > > Variable invalid_mod_byte is not declared, should be invalid_mod_bytes. Arnd already sent a fix for that, thanks. > > +int try_add_failed_module(const char *name, size_t len, enum fail_dup_mod_reason reason) > > Function try_add_failed_module() is only called from > module_patient_check_exists() which always passes in a NUL-terminated string. > The len parameter could be then dropped and the comparison in > try_add_failed_module() could simply use strcmp(). Sure, did that. > Indentation in try_add_failed_module() uses spaces instead of tabs in a few > places. Fixed. > > + size = MAX_PREAMBLE + min((unsigned int)(floads + fbecoming) * MAX_BYTES_PER_MOD, > > + (unsigned int) MAX_FAILED_MOD_PRINT * MAX_BYTES_PER_MOD); > > Using > 'size = MAX_PREAMBLE + min((unsigned int)(floads + fbecoming), (unsigned int)MAX_FAILED_MOD_PRINT) * MAX_BYTES_PER_MOD;' > is a bit simpler and avoids any theoretical overflow of > '(floads + fbecoming) * MAX_BYTES_PER_MOD'. Sure. > > + len += scnprintf(buf + len, size - len, "%25s\t%15s\t%25s\n", > > + "module-name", "How-many-times", "Reason"); > > "module-name" -> "Module-name" OK sure. > Function module_stats_init() requires mod_debugfs_root being initialized which > is done in module_debugfs_init(). Both functions are recorded to be called via > module_init(). Just to make sure, is their ordering guaranteed in some way? Link order takes care of that and main.o goes first. > mod_debugfs_root is initialized in module_debugfs_init() only if > CONFIG_MODULE_DEBUG is set. However, my reading is that feature > CONFIG_MODULE_UNLOAD_TAINT_TRACKING is orthogonal to it and doesn't require > CONFIG_MODULE_DEBUG, so it looks this change breaks this tracking? Ah yes We need a bool CONFIG_MODULE_DEBUGFS which is selected by those that need it. Added. Luis