From:   Song Liu <songliubraving@meta.com>
To:     Christophe Leroy <christophe.leroy@csgroup.eu>
CC:     Song Liu <song@kernel.org>,
        "linux-modules@vger.kernel.org" <linux-modules@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "hch@lst.de" <hch@lst.de>, Kernel Team <kernel-team@meta.com>,
        Luis Chamberlain <mcgrof@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH v10] module: replace module_layout with module_memory
Thread-Topic: [PATCH v10] module: replace module_layout with module_memory
Thread-Index: AQHZOosWgrFk94mbpUiAn23Fr3awBK7FVgEAgABAnoA=
Date:   Wed, 8 Feb 2023 21:39:54 +0000
Message-ID: <1574471D-D1A6-4720-A57D-626204E8E746@fb.com>
References: <20230207002802.2514802-1-song@kernel.org>
 <b40ec330-8c9e-0265-19b9-d82b516c95c1@csgroup.eu>
In-Reply-To: <b40ec330-8c9e-0265-19b9-d82b516c95c1@csgroup.eu>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?ixsvsb6PVTyOev5H44IpihjygFLFKxBpHCRTFtCp1rJmKok6Gkn06HOc0j4a?=
 =?us-ascii?Q?Toftp3hOCUzrutMxeduvGZewbYEOCKlpb7h98U4Jo++xZ2Jn78n7uM4LNVjx?=
 =?us-ascii?Q?l0mP12EC98QzgQUII58oe9mbWrez0pf1AbipjrLy+rY8CWvsjFYLX0EGAj52?=
 =?us-ascii?Q?rjZk6R7eJgwGPNCaPO9zb06NHkE8HP3l+IuXlxS8K6li/lNlEX0CJ932qupW?=
 =?us-ascii?Q?J2xeqPXQL4YU12Y2hphQbdb7y6/T+NqBH2n9SjqHWRzGwcWtZ629XboiYOFp?=
 =?us-ascii?Q?TWpc0GZGTdfVCigjrOjf9NeNS5RDvGCzvfxMpJsv+2GPXrupHtwhxHcTIv+t?=
 =?us-ascii?Q?OSTHu3FS1U4RfZWJfAy1mkICBmzADmkHnQ9h2aaymqOEPsmgE5YlYa4YBdNH?=
 =?us-ascii?Q?rinv1tEmk6iray73IMdq7JBa1MJwsMy5N2eMzyOjQG1NjKxomXCEI5pUSJlX?=
 =?us-ascii?Q?64OTpwrDPR6JkZ4w1sPz/FkxlpMsqezGwzQUJfb6FsbK9IMiWxibRd5fU96P?=
 =?us-ascii?Q?m4WdB7v7qsiaBm5/ozbJlDEk3GjLF2qQ3+85hDZOlkReHTNsOEjPRhXBtb4T?=
 =?us-ascii?Q?NAMRuFW5Xs60KhDRvbIxLlFu3emTdWNN6sj+KqpBBiMSeU5vt5oBBfNsCe9Z?=
 =?us-ascii?Q?d31mqvXd6npNZ44Fek6mM4Z8n0vjuYCPz+MiaEKVEe4Gra8V2NVMN0yHGs3W?=
 =?us-ascii?Q?MES36HTLyLIhAUo8UpJ92XAcTaf3bTU1buWD2lkC8R/ka5xu6ew/dnmqLJrs?=
 =?us-ascii?Q?Its18cBc2Nx1HeCETvoSukXVW117gP14MdOgnA9BehcSvt32j/DkXkCq8x+B?=
 =?us-ascii?Q?VHX3YmsWX9E8iF8mPMvsP6uB1Lv4aQJj5LVakXBTM11XcpvZV+nL7f3OJlkH?=
 =?us-ascii?Q?e9zwylQKdHheA2rpmVvx2yUc2mRzIA/Yzc0bIKHQ2ozmJZdWwwY/08vEPKCc?=
 =?us-ascii?Q?5qXfM3I9rjRelRatzhUWWsS+b+nb9HL94GTmwNoYsUI1BDB7lC+a/BPyV95s?=
 =?us-ascii?Q?W6jHOwhP55qJBJL5+e/curqIkclqVMq5ofl+ElKsCp3iALy57VWaWUBeBrmO?=
 =?us-ascii?Q?7lAZFM3lVluUfqm53fPKYcCODFENwUhrT0BD1BmeWif/q4MDuWgHPdpQqWYO?=
 =?us-ascii?Q?nUF6UEwUEEToulgsd1Nk3PmsOdCESjLpAvbhJ3addgXM1Jd5j/4YR4b5nE1F?=
 =?us-ascii?Q?qFLsw2o6jd1H6g0+vCNuvgpz7U/2uGHDcJ2JtoEJiOGguaiX3MMX4fqt+k+S?=
 =?us-ascii?Q?rTwWgJ+RgY9vSoN5lI6bzsowQaPlMuTAmwqXSVZ4onUZQa8cFYQQhVkwBk/A?=
 =?us-ascii?Q?ubKp4M8F6wVGtoUEBEuqEN9rbLPiMZVi27Mlut0mkYKmwEc3Ji3gqZ8Yz7Ba?=
 =?us-ascii?Q?pFfAaNquxjAgIdmEk3SllWV4BJicyr0FEDUhhs7LxcWn61H4VwdL03M1YEt0?=
 =?us-ascii?Q?EhvCUt6QxiFF1ln8YABk5EHiagHLjpmcj0onEUVYUOOPuVaDqewV2UDpRijm?=
 =?us-ascii?Q?MiAUuuXhXNSMB2u+dOF+C8+Mpr7cYtzGTtYIoRPPwj7G8wtLcuxp6jqpJf4y?=
 =?us-ascii?Q?6s3f9emAGbMcZL1qdrA/5soQylfSIzLB5L+cjF5TVErd2dpXoCj0FfzcAEgM?=
 =?us-ascii?Q?aQPlFrLw+f01a4cNENRsIT6xLMBfeSUiMRMdsbabtiuc?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <08718219D5FF8B4CB9BD406D0A35437C@namprd15.prod.outlook.com>
X-OriginatorOrg: meta.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SA1PR15MB5109.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b084c99e-ca00-4fb0-67cc-08db0a1d043d
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Feb 2023 21:39:54.4363
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: w3+koXKmtM7V9SHR0aPIBwNZ4Tlczr5Q4EEpqQQGZEOfzqt3qoxZfR6wRQhK1P2QyniY990baU4JOCc7cjpLFQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR15MB4349
X-Proofpoint-ORIG-GUID: Gfdw9gm1DuyasNit-SRuOhGg98urmldm
X-Proofpoint-GUID: Gfdw9gm1DuyasNit-SRuOhGg98urmldm
X-Proofpoint-UnRewURL: 0 URL was un-rewritten
MIME-Version: 1.0
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.122.1
 definitions=2023-02-08_09,2023-02-08_02,2022-06-22_01
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Feb 8, 2023, at 9:48 AM, Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

[...]

>> diff --git a/arch/arc/kernel/unwind.c b/arch/arc/kernel/unwind.c
>> index 200270a94558..933451f4494f 100644
>> --- a/arch/arc/kernel/unwind.c
>> +++ b/arch/arc/kernel/unwind.c
>> @@ -369,6 +369,8 @@ void *unwind_add_table(struct module *module, const void *table_start,
>>         unsigned long table_size)
>>  {
>>   struct unwind_table *table;
>> + struct module_memory *mod_mem_core_text;
>> + struct module_memory *mod_mem_init_text;
> 
> This function is small (35 lines) so no need to have so big names for 
> local functions, see 
> https://docs.kernel.org/process/coding-style.html#naming
> 
> struct module_memory *core_text;
> struct module_memory *init_text;

Will fix. 

[...]

>> 
>> 
>>  /*
>> - * Bounds of module text, for speeding up __module_address.
>> + * Bounds of module memory, for speeding up __module_address.
>>   * Protected by module_mutex.
>>   */
>> -static void __mod_update_bounds(void *base, unsigned int size, struct mod_tree_root *tree)
>> +static void __mod_update_bounds(enum mod_mem_type type __maybe_unused, void *base,
>> + unsigned int size, struct mod_tree_root *tree)
>>  {
>>   unsigned long min = (unsigned long)base;
>>   unsigned long max = min + size;
>> 
>> +#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
> 
> A #ifdef shouldn't be required. You can use IS_ENABLED() instead:

Will fix. 

> 
> 
> 
>> + if (mod_mem_type_is_core_data(type)) {
> 
> if (IS_ENABLED(CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC) &&
>    mod_mem_type_is_core_data(type))

[...]

>> - switch (m) {
>> - case 0: /* executable */
>> - mod->core_layout.size = strict_align(mod->core_layout.size);
> 
> Where is the strict alignment done now ?

AFAICT, each of these memory regions are allocated separately, 
so they are always page aligned, no? 

> 
>> - mod->core_layout.text_size = mod->core_layout.size;
>> - break;
>> - case 1: /* RO: text and ro-data */
>> - mod->data_layout.size = strict_align(mod->data_layout.size);
>> - mod->data_layout.ro_size = mod->data_layout.size;
>> - break;
>> - case 2: /* RO after init */
>> - mod->data_layout.size = strict_align(mod->data_layout.size);
>> - mod->data_layout.ro_after_init_size = mod->data_layout.size;
>> - break;
>> - case 4: /* whole core */
>> - mod->data_layout.size = strict_align(mod->data_layout.size);
>> - break;
>> - }
>> - }

[...]

> 
>> 
>>   if (shdr->sh_type != SHT_NOBITS)
>>   memcpy(dest, (void *)shdr->sh_addr, shdr->sh_size);
> 
>> @@ -3060,20 +3091,21 @@ bool is_module_address(unsigned long addr)
>>  struct module *__module_address(unsigned long addr)
>>  {
>>   struct module *mod;
>> - struct mod_tree_root *tree;
>> 
>>   if (addr >= mod_tree.addr_min && addr <= mod_tree.addr_max)
>> - tree = &mod_tree;
>> + goto lookup;
>> +
>>  #ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
> 
> Can we try to avoid that #ifdef ?
> I know that means getting data_addr_min and data_addr_max alwyas 
> existing, maybe through an unnamed union or a macro or a static inline 
> helper ?

IIUC, we want __module_address() to be as fast as possible. So #ifdef
is probably the best solution here?

Thanks,
Song

> 
>> - else if (addr >= mod_data_tree.addr_min && addr <= mod_data_tree.addr_max)
>> - tree = &mod_data_tree;
>> + if (addr >= mod_tree.data_addr_min && addr <= mod_tree.data_addr_max)
>> + goto lookup;
>>  #endif
>> - else
>> - return NULL;
>> 
>> + return NULL;
>> +
>> +lookup:
>>   module_assert_mutex_or_preempt();
>> 
>> - mod = mod_find(addr, tree);
>> + mod = mod_find(addr, &mod_tree);
>>   if (mod) {
>>   BUG_ON(!within_module(addr, mod));
>>   if (mod->state == MODULE_STATE_UNFORMED)