linux-kernel - Re: [PATCH 2/3] mm: don't rely on system state to detect hot-plug operations

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 14 Sep 2020 10:39:49 +0200
From:   Laurent Dufour <ldufour@...ux.ibm.com>
To:     Oscar Salvador <osalvador@...e.de>,
        David Hildenbrand <david@...hat.com>
Cc:     akpm@...ux-foundation.org, mhocko@...nel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-mm@...ck.org, "Rafael J . Wysocki" <rafael@...nel.org>,
        nathanl@...ux.ibm.com, cheloha@...ux.ibm.com,
        Tony Luck <tony.luck@...el.com>,
        Fenghua Yu <fenghua.yu@...el.com>, linux-ia64@...r.kernel.org,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        Michal Hocko <mhocko@...e.com>
Subject: Re: [PATCH 2/3] mm: don't rely on system state to detect hot-plug
 operations

Le 14/09/2020 à 10:19, Oscar Salvador a écrit :
> On Mon, Sep 14, 2020 at 09:57:46AM +0200, David Hildenbrand wrote:
>>>   /* register memory section under specified node if it spans that node */
>>> +struct rmsun_args {
>>> +	int nid;
>>> +	enum memplug_context context;
>>> +};
> 
> Uhmf, that is a not so descriptive name.

I do agree, but didn't have a better idea.
Anyway this will disappear since the choosen direction is to have 2 callbacks.

> 
>> Instead of handling this in register_mem_sect_under_node(), I
>> think it would be better two have two separate
>> register_mem_sect_under_node() implementations.
>>
>> static int register_mem_sect_under_node_hotplug(struct memory_block *mem_blk,
>> 						void *arg)
>> {
>> 	const int nid = *(int *)arg;
>> 	int ret;
>>
>> 	/* Hotplugged memory has no holes and belongs to a single node. */
>> 	mem_blk->nid = nid;
>> 	ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
>> 				       &mem_blk->dev.kobj,
>> 				       kobject_name(&mem_blk->dev.kobj));
>> 	if (ret)
>> 		returnr et;
>> 	return sysfs_create_link_nowarn(&mem_blk->dev.kobj,
>> 					&node_devices[nid]->dev.kobj,
>> 					kobject_name(&node_devices[nid]->dev.kobj));
>>
>> }
>>
>> Cleaner, right? :) No unnecessary checks.
> 
> I tend to agree here, I like more a simplistic version for hotplug.
> 
>> One could argue if link_mem_section_hotplug() would be better than passing around the context.
> 
> I am not sure if I would duplicate the code there.
> We could just pass the pointer of the function we want to call to
> link_mem_sections? either register_mem_sect_under_node_hotplug or
> register_mem_sect_under_node_early?
> Would not that be clean and clear enough?

That would expose the register_mem_sect_under_node*() prototype to the caller.

I'm wondering if that would be cleaner than passing a MEMPLUG_* constant value 
to link_mem_sections() and let it choose the right callback.