Re: [PATCH v3] mm/memory_hotplug: refrain from adding memory into an impossible node

From: David Hildenbrand <david@redhat.com>
To: "Verma, Vishal L" <vishal.l.verma@intel.com>,
	"mhocko@kernel.org" <mhocko@kernel.org>
Cc: "Williams, Dan J" <dan.j.williams@intel.com>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"linux-nvdimm@lists.01.org" <linux-nvdimm@lists.01.org>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>
Subject: Re: [PATCH v3] mm/memory_hotplug: refrain from adding memory into an impossible node
Date: Thu, 16 Apr 2020 18:16:36 +0200	[thread overview]
Message-ID: <16b04f11-f0d7-08e0-ac8c-72b1bd66f1c3@redhat.com> (raw)
In-Reply-To: <4c806f6dc2bdb78dd3fb46814bc119df2815f8c2.camel@intel.com>

On 16.04.20 18:13, Verma, Vishal L wrote:
> On Thu, 2020-04-16 at 08:19 +0200, Michal Hocko wrote:
>> On Wed 15-04-20 20:32:00, Verma, Vishal L wrote:
>>>>
>>>> I really do not like this. Why should we try to be clever and change the
>>>> node id requested by the caller? I would just stick with node_possible
>>>> check and be done with this.
>>>
>>> Hi Michal,
>>>
>>> Being clever allows us to still use the memory even if it is in a non-
>>> optimal configuration. Failing here leaves the user no path to add this
>>> memory until the firmware is fixed. It is the tradeoff between some
>>> usability vs. how loud we want to be for the failure.
>>
>> Doing that papers over something that is clearly a FW issue and makes
>> it "my performance is suboptimal" deal with it OS problem.  Really, is
>> this something we have to care about. Your changelog talks about a Qemu
>> misconfiguration which is trivial to fix. Has this ever been observed
>> with a real HW?
>>
> Well - more of a qemu bug I think - I can share the details, but it just
> looked like it was producing a bogus SRAT. I think it is plausible that
> such a firmware bug can happen out in the wild. The NFIT tables would
> just need to reference a 'proximity domain' that the SRAT hasn't
> previously described, and hotplug will happily go add memory from the
> NFIT and the backing node related data structures would be missing.
> 
> I'm not too opposed to erroring out, so long as we are ok with the fact
> that we will leave some memory stranded until there's a firmware fix.

So let's reject it and print a warning, so we know it's a thing. If this
actually shows up often in real live, we have good evidence that we
should tolerate buggy firmwares instead of warning/rejecting.

(rejecting from inside add_memory() still makes sense IMHO)

-- 
Thanks,

David / dhildenb