LKML Archive on lore.kernel.org help / color / mirror / Atom feed
From: Michal Hocko <email@example.com> To: Andrea Reale <firstname.lastname@example.org> Cc: "Rafael J. Wysocki" <email@example.com>, "firstname.lastname@example.org" <email@example.com>, Linux Kernel Mailing List <firstname.lastname@example.org>, Linux Memory Management List <email@example.com>, firstname.lastname@example.org, email@example.com, Mark Rutland <firstname.lastname@example.org>, email@example.com, Will Deacon <firstname.lastname@example.org>, email@example.com, Catalin Marinas <firstname.lastname@example.org>, Rafael Wysocki <email@example.com>, ACPI Devel Maling List <firstname.lastname@example.org> Subject: Re: [PATCH v2 2/5] mm: memory_hotplug: Remove assumption on memory state before hotremove Date: Fri, 24 Nov 2017 19:17:41 +0100 [thread overview] Message-ID: <email@example.com> (raw) In-Reply-To: <20171124155458.GC1966@samekh> On Fri 24-11-17 15:54:59, Andrea Reale wrote: > On Fri 24 Nov 2017, 16:43, Michal Hocko wrote: > > On Fri 24-11-17 14:49:17, Andrea Reale wrote: > > > Hi Rafael, > > > > > > On Fri 24 Nov 2017, 15:39, Rafael J. Wysocki wrote: > > > > On Fri, Nov 24, 2017 at 11:22 AM, Andrea Reale <firstname.lastname@example.org> wrote: > > > > > Resending the patch adding linux-acpi in CC, as suggested by Rafael. > > > > > Everyone else: apologies for the noise. > > > > > > > > > > Commit 242831eb15a0 ("Memory hotplug / ACPI: Simplify memory removal") > > > > > introduced an assumption whereas when control > > > > > reaches remove_memory the corresponding memory has been already > > > > > offlined. In that case, the acpi_memhotplug was making sure that > > > > > the assumption held. > > > > > This assumption, however, is not necessarily true if offlining > > > > > and removal are not done by the same "controller" (for example, > > > > > when first offlining via sysfs). > > > > > > > > > > Removing this assumption for the generic remove_memory code > > > > > and moving it in the specific acpi_memhotplug code. This is > > > > > a dependency for the software-aided arm64 offlining and removal > > > > > process. > > > > > > > > > > Signed-off-by: Andrea Reale <email@example.com> > > > > > Signed-off-by: Maciej Bielski <firstname.lastname@example.org> > > > > > --- > > > > > drivers/acpi/acpi_memhotplug.c | 2 +- > > > > > include/linux/memory_hotplug.h | 9 ++++++--- > > > > > mm/memory_hotplug.c | 13 +++++++++---- > > > > > 3 files changed, 16 insertions(+), 8 deletions(-) > > > > > > > > > > diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c > > > > > index 6b0d3ef..b0126a0 100644 > > > > > --- a/drivers/acpi/acpi_memhotplug.c > > > > > +++ b/drivers/acpi/acpi_memhotplug.c > > > > > @@ -282,7 +282,7 @@ static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device) > > > > > nid = memory_add_physaddr_to_nid(info->start_addr); > > > > > > > > > > acpi_unbind_memory_blocks(info); > > > > > - remove_memory(nid, info->start_addr, info->length); > > > > > + BUG_ON(remove_memory(nid, info->start_addr, info->length)); > > > > > > > > Why does this have to be BUG_ON()? Is it really necessary to kill the > > > > system here? > > > > > > Actually, I hoped you would help me understand that: that BUG() call was introduced > > > by yourself in Commit 242831eb15a0 ("Memory hotplug / ACPI: Simplify memory removal") > > > in memory_hoptlug.c:remove_memory()). > > > > > > Just reading at that commit my understanding was that you were assuming > > > that acpi_memory_remove_memory() have already done the job of offlining > > > the target memory, so there would be a bug if that wasn't the case. > > > > > > In my case, that assumption did not hold and I found that it might not > > > hold for other platforms that do not use ACPI. In fact, the purpose of > > > this patch is to move this assumption out of the generic hotplug code > > > and move it to ACPI code where it originated. > > > > remove_memory failure is basically impossible to handle AFAIR. The > > original code to BUG in remove_memory is ugly as hell and we do not want > > to spread that out of that function. Instead we really want to get rid > > of it. > > Today, BUG() is called even in the simple case where remove fails > because the section we are removing is not offline. You cannot hotremove memory which is still online. This is what caller should enforce. This is too late to handle the failure. At least for ACPI. > I cannot see any need to > BUG() in such a case: an error code seems more than sufficient to me. I do not rememeber details but AFAIR ACPI is in a deferred (kworker) context here and cannot simply communicate error code down the road. I agree that we should be able to simply return an error but what is the actual error condition that might happen here? > This is why this patch removes the BUG() call when the "offline" check > fails from the generic code. As I've said we should simply get rid of BUG rather than move it around. > It moves it back to the ACPI call, where the assumption > originated. Honestlly, I cannot tell if it makes sense to BUG() there: > I have nothing against removing it from ACPI hotplug too, but > I don't know enough to feel free to change the acpi semantics myself, so I > moved it there to keep the original behavior unchanged for x86 code. Heh, yeah that is an easier path for sure. I would prefer sorting this out ;) Not that I would enforce that, though. My concern is that the previous hotplug development followed this "I do not understand exactly so I will simply put my on top of existing code" mantra and it ended up in a huge mess. > In this arm64 hot-remove port, offline and remove are done in two separate > steps, and is conceivable that an user tries erroneusly to remove some > section that he forgot to offline first: in that case, with the patch, > remove will just report an erro without BUGing. As I've said it is the caller to enforce that. > Is my reasoning flawed? I wouldn't say flawed but this is a low-level call that should already happen in a reasonable context. -- Michal Hocko SUSE Labs
next prev parent reply other threads:[~2017-11-24 18:17 UTC|newest] Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-11-23 11:13 [PATCH v2 0/5] Memory hotplug support for arm64 - complete patchset v2 Andrea Reale 2017-11-23 11:13 ` [PATCH v2 1/5] mm: memory_hotplug: Memory hotplug (add) support for arm64 Maciej Bielski 2017-11-24 5:55 ` Arun KS 2017-11-24 9:42 ` Andrea Reale 2017-11-24 10:53 ` Maciej Bielski 2017-11-26 6:58 ` Arun KS 2017-11-27 15:19 ` Robin Murphy 2017-11-27 16:39 ` Maciej Bielski 2017-11-27 17:11 ` Andrea Reale 2017-11-23 11:14 ` [PATCH v2 2/5] mm: memory_hotplug: Remove assumption on memory state before hotremove Andrea Reale 2017-11-23 22:18 ` Rafael J. Wysocki 2017-11-24 14:39 ` Rafael J. Wysocki 2017-11-24 14:49 ` Andrea Reale 2017-11-24 15:43 ` Michal Hocko 2017-11-24 15:54 ` Andrea Reale 2017-11-24 18:17 ` Michal Hocko [this message] 2017-11-29 1:20 ` joeyli 2017-11-30 9:47 ` Michal Hocko 2017-11-27 15:20 ` Robin Murphy 2017-11-27 17:44 ` Andrea Reale 2017-11-29 0:49 ` joeyli 2017-11-29 1:52 ` joeyli 2017-12-04 11:28 ` Andrea Reale 2017-12-04 14:05 ` Rafael J. Wysocki 2017-11-23 11:14 ` [PATCH v2 3/5] mm: memory_hotplug: memblock to track partially removed vmemmap mem Andrea Reale 2017-11-27 15:20 ` Robin Murphy 2017-11-27 17:38 ` Andrea Reale 2017-11-30 14:51 ` Michal Hocko 2017-12-04 11:49 ` Andrea Reale 2017-12-04 12:32 ` Michal Hocko 2017-12-04 12:42 ` Andrea Reale 2017-12-04 12:48 ` Michal Hocko 2017-11-23 11:14 ` [PATCH v2 4/5] mm: memory_hotplug: Add memory hotremove probe device Andrea Reale 2017-11-24 10:35 ` zhong jiang 2017-11-24 10:44 ` Andrea Reale 2017-11-24 12:17 ` zhong jiang 2017-11-24 14:29 ` Andrea Reale 2017-12-04 17:50 ` Reza Arbab 2017-11-27 15:33 ` Robin Murphy 2017-11-27 17:14 ` Andrea Reale 2017-11-30 14:49 ` Michal Hocko 2017-12-04 11:51 ` Andrea Reale 2017-12-04 12:33 ` Michal Hocko 2017-12-04 12:44 ` Andrea Reale 2017-11-23 11:15 ` [PATCH v2 5/5] mm: memory-hotplug: Add memory hot remove support for arm64 Andrea Reale 2017-11-23 16:02 ` [PATCH v2 0/5] Memory hotplug support for arm64 - complete patchset v2 Michal Hocko 2017-11-23 17:33 ` Andrea Reale 2017-11-30 14:57 ` Michal Hocko 2017-12-04 11:34 ` Andrea Reale
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).