On Mon, 31 Jul 2017 19:58:30 +0200 Gerald Schaefer <gerald.schaefer@xxxxxxxxxx> wrote: > On Mon, 31 Jul 2017 17:53:50 +0200 > Michal Hocko <mhocko@xxxxxxxxxx> wrote: > > > On Mon 31-07-17 17:04:59, Gerald Schaefer wrote: > > > On Mon, 31 Jul 2017 14:53:19 +0200 > > > Michal Hocko <mhocko@xxxxxxxxxx> wrote: > > > > > > > On Mon 31-07-17 14:35:21, Gerald Schaefer wrote: > > > > > On Fri, 28 Jul 2017 14:19:41 +0200 > > > > > Michal Hocko <mhocko@xxxxxxxxxx> wrote: > > > > > > > > > > > On Thu 27-07-17 08:56:52, Michal Hocko wrote: > > > > > > > On Wed 26-07-17 17:06:59, Jerome Glisse wrote: > > > > > > > [...] > > > > > > > > This does not seems to be an opt-in change ie if i am reading patch 3 > > > > > > > > correctly if an altmap is not provided to __add_pages() you fallback > > > > > > > > to allocating from begining of zone. This will not work with HMM ie > > > > > > > > device private memory. So at very least i would like to see some way > > > > > > > > to opt-out of this. Maybe a new argument like bool forbid_altmap ? > > > > > > > > > > > > > > OK, I see! I will think about how to make a sane api for that. > > > > > > > > > > > > This is what I came up with. s390 guys mentioned that I cannot simply > > > > > > use the new range at this stage yet. This will need probably some other > > > > > > changes but I guess we want an opt-in approach with an arch veto in general. > > > > > > > > > > > > So what do you think about the following? Only x86 is update now and I > > > > > > will split it into two parts but the idea should be clear at least. > > > > > > > > > > This looks good, and the kernel will also boot again on s390 when applied > > > > > on top of the other 5 patches (plus adding the s390 part here). > > > > > > > > Thanks for testing Gerald! I am still undecided whether the arch code > > > > should veto MHP_RANGE_ACCESSIBLE if it cannot be supported or just set > > > > it when it is supported. My last post did the later but the first one > > > > sounds like a more clear API to me. I will keep thinking about it. > > > > > > > > Anyway, did you have any chance to consider mapping the new physical > > > > memory range inside arch_add_memory rather than during online on s390? > > > > > > Well, it still looks like we cannot do w/o splitting up add_memory(): > > > 1) (only) set up section map during our initial memory detection, w/o > > > allocating struct pages, so that the sysfs entries get created also for > > > our offline memory (or else we have no way to online it later) > > > 2) set up vmemmap and allocate struct pages with your new altmap approach > > > during our MEM_GOING_ONLINE callback, because only now the memory is really > > > accessible > > > > As I've tried to mentioned in my other response. This is not possible > > because there are memory hotplug usecases which never do an explicit > > online. > > Of course the default behaviour should not change, we only need an option > to do the "2-stage-approach". E.g. we would call __add_pages() from our > MEM_GOING_ONLINE handler, and not from arch_add_memory() as before, but > then we would need some way to add memory sections (for generating sysfs > memory blocks) only, without allocating struct pages. See also below. > > > > > I am sorry to ask again. But why exactly cannot we make the range > > accessible from arch_add_memory on s390? > > We have no acpi or other events to indicate new memory, both online and > offline memory needs to be (hypervisor) defined upfront, and then we want > to be able to use memory hotplug for ballooning during runtime. > > Making the range accessible is equivalent to a hypervisor call that assigns > the memory to the guest. The problem with arch_add_memory() is now that > this gets called from add_memory(), which we call during initial memory > detection for the offline memory ranges. At that time, assigning all > offline memory to the guest, and thus making it accessible, would break > the ballooning usecase (even if it is still offline in Linux, the > hypervisor could not use it for other guests any more). > > The main difference to other architectures is that we can not simply > call add_memory() (and thus arch_add_memory()) at the time when the > offline memory is actually supposed to get online (e.g. triggered by acpi). > We rather need to somehow make sure that the offline memory is detected > early, and sysfs entries are created for it, so that it can be set online > later on demand. > > Maybe our design to use add_memory() for offline ranges during memory > detection was wrong, or overkill, since we actually only need to establish > a memory section, if I understood the sysfs code right. But I currently > see no other way to make sure that we get the sysfs attributes. And of > course the presence of users that work on offline struct pages, like > valid_zones, is also not helpful. > > > > > > Besides the obvious problem that this would need a new interface, there is > > > also the problem that (at least) show_valid_zones() in drivers/base/memory.c > > > operates on struct pages from _offline_ memory, for its page_zone() checks. > > > This will not work well if we have no struct pages for offline memory ... > > > > Yes. > > > > > BTW, the latter may also be a issue with your rework on any architecture. > > > Not sure if I understood it correctly, but the situation on s390 (i.e. > > > having offline memory blocks visible in sysfs) should be similar to > > > the scenario on x86, when you plug in memory, set it online in the acpi > > > handler, and then manually set it offline again via sysfs. Now the > > > memory is still visible in sysfs, and reading the valid_zones attribute > > > will trigger an access to struct pages for that memory. What if this > > > memory is now physically removed, in a race with such a struct page > > > access? > > > > The memmap goes away together with the whole section tear down. And we > > shouldn't have any users of any struct page by that time. Memblock sysfs > > should be down as well. I will go and double check whether there are any > > possible races. > > I was thinking of someone pulling out a DIMM whose range was (manually) > set offline before. It looks like (arch_)remove_memory() is not triggered > directly on setting it offline, but rather by an acpi event, probably after > physical memory removal. Order here a little bit different, first and ACPI event sent and processed by kernel (including removing ranges from sysfs) and only then kernel should call ACPI _EJ0 method on DIMM device when there shouldn't be any users left nor races should happen. > And that would mean that a user could just read > sysfs valid_zones in a loop, after setting it offline and before the > physical removal, thereby accessing struct pages in the offline range, > which would then race with the physical DIMM removal. > > However, as you can see, s390 memory hotplug works in a special way, > so I may have gotten the wrong picture of how it works on "normal" > architectures :-) > > Regards, > Gerald > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx"> email@xxxxxxxxx </a>