Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.117.161 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.117.161; helo=mail.nvidia.com; pr=C
Message-ID: <80e01fa9-28c0-37e8-57f8-5bb4ce9a9db7@nvidia.com>
Date:   Tue, 20 Jun 2023 14:54:31 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH] mm/memory_hotplug.c: don't fail hot unplug quite so
 eagerly
Content-Language: en-US
To:     David Hildenbrand <david@redhat.com>,
        Oscar Salvador <osalvador@suse.de>
CC:     Andrew Morton <akpm@linux-foundation.org>,
        LKML <linux-kernel@vger.kernel.org>, <linux-mm@kvack.org>
References: <20230620011719.155379-1-jhubbard@nvidia.com>
 <ed83df65-f785-7077-ddd0-4e53d6fa6056@redhat.com>
From:   John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <ed83df65-f785-7077-ddd0-4e53d6fa6056@redhat.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Jun 2023 21:54:48.9259
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 083c0afe-4c01-4f04-230a-08db71d8f802
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.117.161];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: DM6NAM11FT053.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR12MB8978
X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 6/20/23 00:12, David Hildenbrand wrote:
> On 20.06.23 03:17, John Hubbard wrote:
>> mm/memory_hotplug.c: don't fail hot unplug quite so eagerly
>>
>> Some device drivers add memory to the system via memory hotplug. When
>> the driver is unloaded, that memory is hot-unplugged.
> 
> Which interfaces are they using to add/remove memory?

It's coming in from the kernel driver, like this:

offline_and_remove_memory()
     walk_memory_blocks()
         try_offline_memory_block()
             device_offline()
                 memory_subsys_offline()
                     offline_pages()

...and the above is getting invoked as part of killing a user space
process that was helping (for performance reasons) holding the device
nodes open. That triggers a final close of the file descriptors and
leads to tearing down the driver. The teardown succeeds even though
the memory was not offlined, and now everything is, to use a technical
term, "stuck". :)

More below...

> 
>>
>> However, memory hot unplug can fail. And these days, it fails a little
>> too easily, with respect to the above case. Specifically, if a signal is
>> pending on the process, hot unplug fails. This leads directly to: the
>> user must reboot the machine in order to unload the driver, and
>> therefore the device is unusable until the machine is rebooted.
> 
> Why can't they retry in user space when offlining fails with -EINTR, or re-trigger driver unloading?

If someone uses "kill -9" to kill that process, then we get here,
because user space cannot trap that signal.


...
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1879,12 +1879,6 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>>       do {
>>           pfn = start_pfn;
>>           do {
>> -            if (signal_pending(current)) {
>> -                ret = -EINTR;
>> -                reason = "signal backoff";
>> -                goto failed_removal_isolated;
>> -            }
>> -
>>               cond_resched();
>>               ret = scan_movable_pages(pfn, end_pfn, &pfn);
> 
> No, we can't remove that. It's documented behavior that exists precisely for that reason:
> 
> https://docs.kernel.org/admin-guide/mm/memory-hotplug.html#id21
> 
> "
> When offlining is triggered from user space, the offlining context can be terminated by sending a fatal signal. A timeout based offlining can easily be implemented via:
> 
> % timeout $TIMEOUT offline_block | failure_handling
> "
> 
> Otherwise, there is no way to stop an userspace-triggered offline operation that loops forever in the kernel.

OK yes, I see.

> 
> I guess switching to fatal_signal_pending() might help to some degree, it should keep the timeout trick working.
> 
> But it wouldn't help in your case because where root kills arbitrary processes. I'm not sure if that is something we should be paying attention to.
> 

Right. I think it would be more accurate perhaps, but it wouldn't help
this particular complaint.

Perhaps it is reasonable to claim that, "well, kill -9 *means* that you
end up here!" :) And the above patch clearly is not the way to go, but...

...what about discerning between "user initiated offline_pages" and
"offline pages as part of a driver shutdown/unload"?

thanks,
-- 
John Hubbard
NVIDIA