| Message ID | 20230801044116.10674-8-aneesh.kumar@linux.ibm.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Add support for memmap on memory feature on ppc64 | expand |
On Tue 01-08-23 10:11:16, Aneesh Kumar K.V wrote: > Allow updating memmap_on_memory mode after the kernel boot. Memory > hotplug done after the mode update will use the new mmemap_on_memory > value. Well, this is a user space kABI extension and as such you should spend more words about the usecase. Why we could live with this static and now need dynamic?
On 8/1/23 2:28 PM, Michal Hocko wrote: > On Tue 01-08-23 10:11:16, Aneesh Kumar K.V wrote: >> Allow updating memmap_on_memory mode after the kernel boot. Memory >> hotplug done after the mode update will use the new mmemap_on_memory >> value. > > Well, this is a user space kABI extension and as such you should spend > more words about the usecase. Why we could live with this static and now > need dynamic? > This enables easy testing of memmap_on_memory feature without a kernel reboot. I also expect people wanting to use that when they find dax kmem memory online failing because of struct page allocation failures[1]. User could reboot back with memmap_on_memory=y kernel parameter. But being able to enable it via sysfs makes the feature much more useful. -aneesh
On Tue 01-08-23 14:58:29, Aneesh Kumar K V wrote: > On 8/1/23 2:28 PM, Michal Hocko wrote: > > On Tue 01-08-23 10:11:16, Aneesh Kumar K.V wrote: > >> Allow updating memmap_on_memory mode after the kernel boot. Memory > >> hotplug done after the mode update will use the new mmemap_on_memory > >> value. > > > > Well, this is a user space kABI extension and as such you should spend > > more words about the usecase. Why we could live with this static and now > > need dynamic? > > > > This enables easy testing of memmap_on_memory feature without a kernel reboot. Testing alone is rather weak argument to be honest. > I also expect people wanting to use that when they find dax kmem memory online > failing because of struct page allocation failures[1]. User could reboot back with > memmap_on_memory=y kernel parameter. But being able to enable it via sysfs makes > the feature much more useful. Sure it can be useful but that holds for any feature, right. The main question is whether this is worth maintaing. The current implementation seems rather trivial which is an argument to have it but are there any risks long term? Have you evaluated a potential long term maintenance cost? There is no easy way to go back and disable it later on without breaking some userspace. All that should be in the changelog!
On 8/1/23 4:20 PM, Michal Hocko wrote: > On Tue 01-08-23 14:58:29, Aneesh Kumar K V wrote: >> On 8/1/23 2:28 PM, Michal Hocko wrote: >>> On Tue 01-08-23 10:11:16, Aneesh Kumar K.V wrote: >>>> Allow updating memmap_on_memory mode after the kernel boot. Memory >>>> hotplug done after the mode update will use the new mmemap_on_memory >>>> value. >>> >>> Well, this is a user space kABI extension and as such you should spend >>> more words about the usecase. Why we could live with this static and now >>> need dynamic? >>> >> >> This enables easy testing of memmap_on_memory feature without a kernel reboot. > > Testing alone is rather weak argument to be honest. > >> I also expect people wanting to use that when they find dax kmem memory online >> failing because of struct page allocation failures[1]. User could reboot back with >> memmap_on_memory=y kernel parameter. But being able to enable it via sysfs makes >> the feature much more useful. > > Sure it can be useful but that holds for any feature, right. The main > question is whether this is worth maintaing. The current implementation > seems rather trivial which is an argument to have it but are there any > risks long term? Have you evaluated a potential long term maintenance > cost? There is no easy way to go back and disable it later on without > breaking some userspace. > > All that should be in the changelog! I updated it as below. mm/memory_hotplug: Enable runtime update of memmap_on_memory parameter Allow updating memmap_on_memory mode after the kernel boot. Memory hotplug done after the mode update will use the new mmemap_on_memory value. It is now possible to test the memmap_on_memory feature easily without the need for a kernel reboot. Additionally, for those encountering struct page allocation failures while using dax kmem memory online, this feature may prove useful. Instead of rebooting with the memmap_on_memory=y kernel parameter, users can now enable it via sysfs, which greatly enhances its usefulness. Making the necessary changes to support runtime updates is a simple change that should not affect the addition of new features or result in long-term maintenance problems.
On Wed 02-08-23 10:15:04, Aneesh Kumar K V wrote: > On 8/1/23 4:20 PM, Michal Hocko wrote: > > On Tue 01-08-23 14:58:29, Aneesh Kumar K V wrote: > >> On 8/1/23 2:28 PM, Michal Hocko wrote: > >>> On Tue 01-08-23 10:11:16, Aneesh Kumar K.V wrote: > >>>> Allow updating memmap_on_memory mode after the kernel boot. Memory > >>>> hotplug done after the mode update will use the new mmemap_on_memory > >>>> value. > >>> > >>> Well, this is a user space kABI extension and as such you should spend > >>> more words about the usecase. Why we could live with this static and now > >>> need dynamic? > >>> > >> > >> This enables easy testing of memmap_on_memory feature without a kernel reboot. > > > > Testing alone is rather weak argument to be honest. > > > >> I also expect people wanting to use that when they find dax kmem memory online > >> failing because of struct page allocation failures[1]. User could reboot back with > >> memmap_on_memory=y kernel parameter. But being able to enable it via sysfs makes > >> the feature much more useful. > > > > Sure it can be useful but that holds for any feature, right. The main > > question is whether this is worth maintaing. The current implementation > > seems rather trivial which is an argument to have it but are there any > > risks long term? Have you evaluated a potential long term maintenance > > cost? There is no easy way to go back and disable it later on without > > breaking some userspace. > > > > All that should be in the changelog! > > I updated it as below. > > mm/memory_hotplug: Enable runtime update of memmap_on_memory parameter > > Allow updating memmap_on_memory mode after the kernel boot. Memory > hotplug done after the mode update will use the new mmemap_on_memory > value. > > It is now possible to test the memmap_on_memory feature easily without > the need for a kernel reboot. Additionally, for those encountering > struct page allocation failures while using dax kmem memory online, this > feature may prove useful. Instead of rebooting with the > memmap_on_memory=y kernel parameter, users can now enable it via sysfs, > which greatly enhances its usefulness. I do not really see a solid argument why rebooting is really a problem TBH. Also is the global policy knob really the right fit for existing hotplug usecases? In other words, if we really want to make memmap_on_memory more flexible would it make more sense to have it per memory block property instead (the global knob being the default if admin doesn't specify it differently).
On 02.08.23 17:50, Michal Hocko wrote: > On Wed 02-08-23 10:15:04, Aneesh Kumar K V wrote: >> On 8/1/23 4:20 PM, Michal Hocko wrote: >>> On Tue 01-08-23 14:58:29, Aneesh Kumar K V wrote: >>>> On 8/1/23 2:28 PM, Michal Hocko wrote: >>>>> On Tue 01-08-23 10:11:16, Aneesh Kumar K.V wrote: >>>>>> Allow updating memmap_on_memory mode after the kernel boot. Memory >>>>>> hotplug done after the mode update will use the new mmemap_on_memory >>>>>> value. >>>>> >>>>> Well, this is a user space kABI extension and as such you should spend >>>>> more words about the usecase. Why we could live with this static and now >>>>> need dynamic? >>>>> >>>> >>>> This enables easy testing of memmap_on_memory feature without a kernel reboot. >>> >>> Testing alone is rather weak argument to be honest. >>> >>>> I also expect people wanting to use that when they find dax kmem memory online >>>> failing because of struct page allocation failures[1]. User could reboot back with >>>> memmap_on_memory=y kernel parameter. But being able to enable it via sysfs makes >>>> the feature much more useful. >>> >>> Sure it can be useful but that holds for any feature, right. The main >>> question is whether this is worth maintaing. The current implementation >>> seems rather trivial which is an argument to have it but are there any >>> risks long term? Have you evaluated a potential long term maintenance >>> cost? There is no easy way to go back and disable it later on without >>> breaking some userspace. >>> >>> All that should be in the changelog! >> >> I updated it as below. >> >> mm/memory_hotplug: Enable runtime update of memmap_on_memory parameter >> >> Allow updating memmap_on_memory mode after the kernel boot. Memory >> hotplug done after the mode update will use the new mmemap_on_memory >> value. >> >> It is now possible to test the memmap_on_memory feature easily without >> the need for a kernel reboot. Additionally, for those encountering >> struct page allocation failures while using dax kmem memory online, this >> feature may prove useful. Instead of rebooting with the >> memmap_on_memory=y kernel parameter, users can now enable it via sysfs, >> which greatly enhances its usefulness. > > > I do not really see a solid argument why rebooting is really a problem > TBH. Also is the global policy knob really the right fit for existing > hotplug usecases? In other words, if we really want to make > memmap_on_memory more flexible would it make more sense to have it per > memory block property instead (the global knob being the default if > admin doesn't specify it differently). Per memory block isn't possible, due to the creation order. Also, I think it's not the right approach. I thought about driver toggles. At least for dax/kmem people are looking into that: https://lkml.kernel.org/r/20230801-vv-kmem_memmap-v3-2-406e9aaf5689@intel.com Where that can also be toggled per device.
On 8/2/23 9:24 PM, David Hildenbrand wrote: > On 02.08.23 17:50, Michal Hocko wrote: >> On Wed 02-08-23 10:15:04, Aneesh Kumar K V wrote: >>> On 8/1/23 4:20 PM, Michal Hocko wrote: >>>> On Tue 01-08-23 14:58:29, Aneesh Kumar K V wrote: >>>>> On 8/1/23 2:28 PM, Michal Hocko wrote: >>>>>> On Tue 01-08-23 10:11:16, Aneesh Kumar K.V wrote: >>>>>>> Allow updating memmap_on_memory mode after the kernel boot. Memory >>>>>>> hotplug done after the mode update will use the new mmemap_on_memory >>>>>>> value. >>>>>> >>>>>> Well, this is a user space kABI extension and as such you should spend >>>>>> more words about the usecase. Why we could live with this static and now >>>>>> need dynamic? >>>>>> >>>>> >>>>> This enables easy testing of memmap_on_memory feature without a kernel reboot. >>>> >>>> Testing alone is rather weak argument to be honest. >>>> >>>>> I also expect people wanting to use that when they find dax kmem memory online >>>>> failing because of struct page allocation failures[1]. User could reboot back with >>>>> memmap_on_memory=y kernel parameter. But being able to enable it via sysfs makes >>>>> the feature much more useful. >>>> >>>> Sure it can be useful but that holds for any feature, right. The main >>>> question is whether this is worth maintaing. The current implementation >>>> seems rather trivial which is an argument to have it but are there any >>>> risks long term? Have you evaluated a potential long term maintenance >>>> cost? There is no easy way to go back and disable it later on without >>>> breaking some userspace. >>>> >>>> All that should be in the changelog! >>> >>> I updated it as below. >>> >>> mm/memory_hotplug: Enable runtime update of memmap_on_memory parameter >>> >>> Allow updating memmap_on_memory mode after the kernel boot. Memory >>> hotplug done after the mode update will use the new mmemap_on_memory >>> value. >>> >>> It is now possible to test the memmap_on_memory feature easily without >>> the need for a kernel reboot. Additionally, for those encountering >>> struct page allocation failures while using dax kmem memory online, this >>> feature may prove useful. Instead of rebooting with the >>> memmap_on_memory=y kernel parameter, users can now enable it via sysfs, >>> which greatly enhances its usefulness. >> >> >> I do not really see a solid argument why rebooting is really a problem >> TBH. Also is the global policy knob really the right fit for existing >> hotplug usecases? In other words, if we really want to make >> memmap_on_memory more flexible would it make more sense to have it per >> memory block property instead (the global knob being the default if >> admin doesn't specify it differently). > > Per memory block isn't possible, due to the creation order. Also, I think it's not the right approach. > > I thought about driver toggles. At least for dax/kmem people are looking into that: > > https://lkml.kernel.org/r/20230801-vv-kmem_memmap-v3-2-406e9aaf5689@intel.com > > Where that can also be toggled per device. > That still is dependent on the global memmap_on_memory enabled right? We could make the dax facility independent of the global feature toggle? -aneesh
On Wed, 2023-08-02 at 21:27 +0530, Aneesh Kumar K V wrote: > On 8/2/23 9:24 PM, David Hildenbrand wrote: > > On 02.08.23 17:50, Michal Hocko wrote: > > > On Wed 02-08-23 10:15:04, Aneesh Kumar K V wrote: > > > > On 8/1/23 4:20 PM, Michal Hocko wrote: > > > > > On Tue 01-08-23 14:58:29, Aneesh Kumar K V wrote: > > > > > > On 8/1/23 2:28 PM, Michal Hocko wrote: > > > > > > > On Tue 01-08-23 10:11:16, Aneesh Kumar K.V wrote: > > > > > > > > Allow updating memmap_on_memory mode after the kernel boot. Memory > > > > > > > > hotplug done after the mode update will use the new mmemap_on_memory > > > > > > > > value. > > > > > > > > > > > > > > Well, this is a user space kABI extension and as such you should spend > > > > > > > more words about the usecase. Why we could live with this static and now > > > > > > > need dynamic? > > > > > > > > > > > > > > > > > > > This enables easy testing of memmap_on_memory feature without a kernel reboot. > > > > > > > > > > Testing alone is rather weak argument to be honest. > > > > > > > > > > > I also expect people wanting to use that when they find dax kmem memory online > > > > > > failing because of struct page allocation failures[1]. User could reboot back with > > > > > > memmap_on_memory=y kernel parameter. But being able to enable it via sysfs makes > > > > > > the feature much more useful. > > > > > > > > > > Sure it can be useful but that holds for any feature, right. The main > > > > > question is whether this is worth maintaing. The current implementation > > > > > seems rather trivial which is an argument to have it but are there any > > > > > risks long term? Have you evaluated a potential long term maintenance > > > > > cost? There is no easy way to go back and disable it later on without > > > > > breaking some userspace. > > > > > > > > > > All that should be in the changelog! > > > > > > > > I updated it as below. > > > > > > > > mm/memory_hotplug: Enable runtime update of memmap_on_memory parameter > > > > > > > > Allow updating memmap_on_memory mode after the kernel boot. Memory > > > > hotplug done after the mode update will use the new mmemap_on_memory > > > > value. > > > > > > > > It is now possible to test the memmap_on_memory feature easily without > > > > the need for a kernel reboot. Additionally, for those encountering > > > > struct page allocation failures while using dax kmem memory online, this > > > > feature may prove useful. Instead of rebooting with the > > > > memmap_on_memory=y kernel parameter, users can now enable it via sysfs, > > > > which greatly enhances its usefulness. > > > > > > > > > I do not really see a solid argument why rebooting is really a problem > > > TBH. Also is the global policy knob really the right fit for existing > > > hotplug usecases? In other words, if we really want to make > > > memmap_on_memory more flexible would it make more sense to have it per > > > memory block property instead (the global knob being the default if > > > admin doesn't specify it differently). > > > > Per memory block isn't possible, due to the creation order. Also, I think it's not the right approach. > > > > I thought about driver toggles. At least for dax/kmem people are looking into that: > > > > https://lkml.kernel.org/r/20230801-vv-kmem_memmap-v3-2-406e9aaf5689@intel.com > > > > Where that can also be toggled per device. > > > > That still is dependent on the global memmap_on_memory enabled right? We could make the dax facility independent of the > global feature toggle? Correct - the latest version of those David linked does depend on the global memmap_on_memory param. Since kmem's behavior for dax devices is set up to be turned on / off dynamically via sysfs, it would be nice to have a similar facility for this flag. I did try the making dax independent of memmap_on_memory approach in my first posting: https://lore.kernel.org/nvdimm/20230613-vv-kmem_memmap-v1-1-f6de9c6af2c6@intel.com/ We can certainly revisit that if it looks more suitable.
On Wed 02-08-23 17:54:04, David Hildenbrand wrote: > On 02.08.23 17:50, Michal Hocko wrote: > > On Wed 02-08-23 10:15:04, Aneesh Kumar K V wrote: > > > On 8/1/23 4:20 PM, Michal Hocko wrote: > > > > On Tue 01-08-23 14:58:29, Aneesh Kumar K V wrote: > > > > > On 8/1/23 2:28 PM, Michal Hocko wrote: > > > > > > On Tue 01-08-23 10:11:16, Aneesh Kumar K.V wrote: > > > > > > > Allow updating memmap_on_memory mode after the kernel boot. Memory > > > > > > > hotplug done after the mode update will use the new mmemap_on_memory > > > > > > > value. > > > > > > > > > > > > Well, this is a user space kABI extension and as such you should spend > > > > > > more words about the usecase. Why we could live with this static and now > > > > > > need dynamic? > > > > > > > > > > > > > > > > This enables easy testing of memmap_on_memory feature without a kernel reboot. > > > > > > > > Testing alone is rather weak argument to be honest. > > > > > > > > > I also expect people wanting to use that when they find dax kmem memory online > > > > > failing because of struct page allocation failures[1]. User could reboot back with > > > > > memmap_on_memory=y kernel parameter. But being able to enable it via sysfs makes > > > > > the feature much more useful. > > > > > > > > Sure it can be useful but that holds for any feature, right. The main > > > > question is whether this is worth maintaing. The current implementation > > > > seems rather trivial which is an argument to have it but are there any > > > > risks long term? Have you evaluated a potential long term maintenance > > > > cost? There is no easy way to go back and disable it later on without > > > > breaking some userspace. > > > > > > > > All that should be in the changelog! > > > > > > I updated it as below. > > > > > > mm/memory_hotplug: Enable runtime update of memmap_on_memory parameter > > > > > > Allow updating memmap_on_memory mode after the kernel boot. Memory > > > hotplug done after the mode update will use the new mmemap_on_memory > > > value. > > > > > > It is now possible to test the memmap_on_memory feature easily without > > > the need for a kernel reboot. Additionally, for those encountering > > > struct page allocation failures while using dax kmem memory online, this > > > feature may prove useful. Instead of rebooting with the > > > memmap_on_memory=y kernel parameter, users can now enable it via sysfs, > > > which greatly enhances its usefulness. > > > > > > I do not really see a solid argument why rebooting is really a problem > > TBH. Also is the global policy knob really the right fit for existing > > hotplug usecases? In other words, if we really want to make > > memmap_on_memory more flexible would it make more sense to have it per > > memory block property instead (the global knob being the default if > > admin doesn't specify it differently). > > Per memory block isn't possible, due to the creation order. I am not sure I follow. Could you elaborate more? > Also, I think it's not the right approach. > > I thought about driver toggles. At least for dax/kmem people are looking > into that: > > https://lkml.kernel.org/r/20230801-vv-kmem_memmap-v3-2-406e9aaf5689@intel.com > > Where that can also be toggled per device. Per device flag makes sense to me as well. But what we should do about hotplugged memory with a backing device (like regular RAM). I can see some reason to have large hotplugged memory ranges to be self vmemap hosted while smaller blocks to be backed by external vmemmaps.
On Wed 02-08-23 18:59:04, Michal Hocko wrote: > On Wed 02-08-23 17:54:04, David Hildenbrand wrote: > > On 02.08.23 17:50, Michal Hocko wrote: > > > On Wed 02-08-23 10:15:04, Aneesh Kumar K V wrote: > > > > On 8/1/23 4:20 PM, Michal Hocko wrote: > > > > > On Tue 01-08-23 14:58:29, Aneesh Kumar K V wrote: > > > > > > On 8/1/23 2:28 PM, Michal Hocko wrote: > > > > > > > On Tue 01-08-23 10:11:16, Aneesh Kumar K.V wrote: > > > > > > > > Allow updating memmap_on_memory mode after the kernel boot. Memory > > > > > > > > hotplug done after the mode update will use the new mmemap_on_memory > > > > > > > > value. > > > > > > > > > > > > > > Well, this is a user space kABI extension and as such you should spend > > > > > > > more words about the usecase. Why we could live with this static and now > > > > > > > need dynamic? > > > > > > > > > > > > > > > > > > > This enables easy testing of memmap_on_memory feature without a kernel reboot. > > > > > > > > > > Testing alone is rather weak argument to be honest. > > > > > > > > > > > I also expect people wanting to use that when they find dax kmem memory online > > > > > > failing because of struct page allocation failures[1]. User could reboot back with > > > > > > memmap_on_memory=y kernel parameter. But being able to enable it via sysfs makes > > > > > > the feature much more useful. > > > > > > > > > > Sure it can be useful but that holds for any feature, right. The main > > > > > question is whether this is worth maintaing. The current implementation > > > > > seems rather trivial which is an argument to have it but are there any > > > > > risks long term? Have you evaluated a potential long term maintenance > > > > > cost? There is no easy way to go back and disable it later on without > > > > > breaking some userspace. > > > > > > > > > > All that should be in the changelog! > > > > > > > > I updated it as below. > > > > > > > > mm/memory_hotplug: Enable runtime update of memmap_on_memory parameter > > > > > > > > Allow updating memmap_on_memory mode after the kernel boot. Memory > > > > hotplug done after the mode update will use the new mmemap_on_memory > > > > value. > > > > > > > > It is now possible to test the memmap_on_memory feature easily without > > > > the need for a kernel reboot. Additionally, for those encountering > > > > struct page allocation failures while using dax kmem memory online, this > > > > feature may prove useful. Instead of rebooting with the > > > > memmap_on_memory=y kernel parameter, users can now enable it via sysfs, > > > > which greatly enhances its usefulness. > > > > > > > > > I do not really see a solid argument why rebooting is really a problem > > > TBH. Also is the global policy knob really the right fit for existing > > > hotplug usecases? In other words, if we really want to make > > > memmap_on_memory more flexible would it make more sense to have it per > > > memory block property instead (the global knob being the default if > > > admin doesn't specify it differently). > > > > Per memory block isn't possible, due to the creation order. > > I am not sure I follow. Could you elaborate more? Must have been a tired brain. Now I see what you mean of course. vmemmap is allocated at the time the range is registered and therefore memory block sysfs created. So you are right that it is too late in some sense but I still believe that this would be doable. The memmap_on_memory file would be readable only when the block is offline and it would reallocate vmemmap on the change. Makes sense? Are there any risks? Maybe pfn walkers?
On 03.08.23 10:52, Michal Hocko wrote: > On Wed 02-08-23 18:59:04, Michal Hocko wrote: >> On Wed 02-08-23 17:54:04, David Hildenbrand wrote: >>> On 02.08.23 17:50, Michal Hocko wrote: >>>> On Wed 02-08-23 10:15:04, Aneesh Kumar K V wrote: >>>>> On 8/1/23 4:20 PM, Michal Hocko wrote: >>>>>> On Tue 01-08-23 14:58:29, Aneesh Kumar K V wrote: >>>>>>> On 8/1/23 2:28 PM, Michal Hocko wrote: >>>>>>>> On Tue 01-08-23 10:11:16, Aneesh Kumar K.V wrote: >>>>>>>>> Allow updating memmap_on_memory mode after the kernel boot. Memory >>>>>>>>> hotplug done after the mode update will use the new mmemap_on_memory >>>>>>>>> value. >>>>>>>> >>>>>>>> Well, this is a user space kABI extension and as such you should spend >>>>>>>> more words about the usecase. Why we could live with this static and now >>>>>>>> need dynamic? >>>>>>>> >>>>>>> >>>>>>> This enables easy testing of memmap_on_memory feature without a kernel reboot. >>>>>> >>>>>> Testing alone is rather weak argument to be honest. >>>>>> >>>>>>> I also expect people wanting to use that when they find dax kmem memory online >>>>>>> failing because of struct page allocation failures[1]. User could reboot back with >>>>>>> memmap_on_memory=y kernel parameter. But being able to enable it via sysfs makes >>>>>>> the feature much more useful. >>>>>> >>>>>> Sure it can be useful but that holds for any feature, right. The main >>>>>> question is whether this is worth maintaing. The current implementation >>>>>> seems rather trivial which is an argument to have it but are there any >>>>>> risks long term? Have you evaluated a potential long term maintenance >>>>>> cost? There is no easy way to go back and disable it later on without >>>>>> breaking some userspace. >>>>>> >>>>>> All that should be in the changelog! >>>>> >>>>> I updated it as below. >>>>> >>>>> mm/memory_hotplug: Enable runtime update of memmap_on_memory parameter >>>>> >>>>> Allow updating memmap_on_memory mode after the kernel boot. Memory >>>>> hotplug done after the mode update will use the new mmemap_on_memory >>>>> value. >>>>> >>>>> It is now possible to test the memmap_on_memory feature easily without >>>>> the need for a kernel reboot. Additionally, for those encountering >>>>> struct page allocation failures while using dax kmem memory online, this >>>>> feature may prove useful. Instead of rebooting with the >>>>> memmap_on_memory=y kernel parameter, users can now enable it via sysfs, >>>>> which greatly enhances its usefulness. >>>> >>>> >>>> I do not really see a solid argument why rebooting is really a problem >>>> TBH. Also is the global policy knob really the right fit for existing >>>> hotplug usecases? In other words, if we really want to make >>>> memmap_on_memory more flexible would it make more sense to have it per >>>> memory block property instead (the global knob being the default if >>>> admin doesn't specify it differently). >>> >>> Per memory block isn't possible, due to the creation order. >> >> I am not sure I follow. Could you elaborate more? > > Must have been a tired brain. Now I see what you mean of course. vmemmap > is allocated at the time the range is registered and therefore memory > block sysfs created. So you are right that it is too late in some sense > but I still believe that this would be doable. The memmap_on_memory file Exactly. > would be readable only when the block is offline and it would reallocate > vmemmap on the change. Makes sense? Are there any risks? Maybe pfn > walkers? The question is: is it of any real value such that it would be worth the cost and risk? One of the primary reasons for memmap_on_memory is that *memory hotplug* succeeds even in low-memory situations (including, low on ZONE_NORMAL situations). So you want that behavior already when hotplugging such devices. While there might be value to relocate it later, I'm not sure if that is really worth it, and it does not solve the main use case. For dax/kmem memory hotplug this is controlled by user space, so user space can easily configure it. For DIMMs it's just a hotplug event that triggers all that in the kernel, and one has to plan ahead (e.g., set the global kernel parameter). Besides, devices like virtio-mem really cannot universally support memmap_on_memory.
On Thu 03-08-23 11:24:08, David Hildenbrand wrote: [...] > > would be readable only when the block is offline and it would reallocate > > vmemmap on the change. Makes sense? Are there any risks? Maybe pfn > > walkers? > > The question is: is it of any real value such that it would be worth the > cost and risk? > > > One of the primary reasons for memmap_on_memory is that *memory hotplug* > succeeds even in low-memory situations (including, low on ZONE_NORMAL > situations). One usecase I would have in mind is a mix of smaller and larger memory blocks. For larger ones you want to have memmap_on_memory in general because they do not eat memory from outside but small(er) ones might be more tricky because now you can add a lot of blocks that would be internally fragmented to prevent larger allocations to form. > So you want that behavior already when hotplugging such > devices. While there might be value to relocate it later, I'm not sure if > that is really worth it, and it does not solve the main use case. Is it worth it? TBH I am not sure same as I am not sure the global default should be writable after boot. If we want to make it more dynamic we should however talk about the proper layer this is implemented on.
On 8/3/23 5:00 PM, Michal Hocko wrote: > On Thu 03-08-23 11:24:08, David Hildenbrand wrote: > [...] >>> would be readable only when the block is offline and it would reallocate >>> vmemmap on the change. Makes sense? Are there any risks? Maybe pfn >>> walkers? >> >> The question is: is it of any real value such that it would be worth the >> cost and risk? >> >> >> One of the primary reasons for memmap_on_memory is that *memory hotplug* >> succeeds even in low-memory situations (including, low on ZONE_NORMAL >> situations). > > One usecase I would have in mind is a mix of smaller and larger memory > blocks. For larger ones you want to have memmap_on_memory in general > because they do not eat memory from outside but small(er) ones might be > more tricky because now you can add a lot of blocks that would be > internally fragmented to prevent larger allocations to form. > I guess that closely aligns with device memory and being able to add device memory via dax/kmem using a larger memory block size. We can then make sure we enable the memmap_on_memory feature at the device level for this device memory. Do you see a need for firmware-managed memory to be hotplugged in with different memory block sizes? >> So you want that behavior already when hotplugging such >> devices. While there might be value to relocate it later, I'm not sure if >> that is really worth it, and it does not solve the main use case. > > Is it worth it? TBH I am not sure same as I am not sure the global > default should be writable after boot. If we want to make it more > dynamic we should however talk about the proper layer this is > implemented on. -aneesh
On Sat 05-08-23 19:54:23, Aneesh Kumar K V wrote: [...] > Do you see a need for firmware-managed memory to be hotplugged in with > different memory block sizes? In short. Yes. Slightly longer, a fixed size memory block semantic is just standing in the way and I would even argue it is actively harmful. Just have a look at ridicously small memory blocks on ppc. I do understand that it makes some sense to be aligned to the memory model (so sparsmem section aligned). In an ideal world, memory hotplug v2 interface (if we ever go that path) should be physical memory range based.
On 07.08.23 14:27, Michal Hocko wrote: > On Sat 05-08-23 19:54:23, Aneesh Kumar K V wrote: > [...] >> Do you see a need for firmware-managed memory to be hotplugged in with >> different memory block sizes? > > In short. Yes. Slightly longer, a fixed size memory block semantic is > just standing in the way and I would even argue it is actively harmful. > Just have a look at ridicously small memory blocks on ppc. I do > understand that it makes some sense to be aligned to the memory model > (so sparsmem section aligned). In an ideal world, memory hotplug v2 > interface (if we ever go that path) should be physical memory range based. Yes, we discussed that a couple of times already (and so far nobody cared to implement any of that). Small memory block sizes are very beneficial for use cases like PPC dlar, virtio-mem, hyperv-balloon, ... essentially in most virtual environments where you might want to add/remove memory in very small granularity. I don't see that changing any time soon. Rather the opposite. Small memory block sizes are suboptimal for large machines where you might never end up removing such memory (boot memory), or when dealing with devices that can only be removed in one piece (DIMM/kmem). We already have memory groups in place to model that. For the latter it might be beneficial to have memory blocks of larger size that correspond to the physical memory ranges. That might also make a memmap (re-)configuration easier. Not sure if that is standing in any way or is harmful, though.
On 03.08.23 13:30, Michal Hocko wrote: > On Thu 03-08-23 11:24:08, David Hildenbrand wrote: > [...] >>> would be readable only when the block is offline and it would reallocate >>> vmemmap on the change. Makes sense? Are there any risks? Maybe pfn >>> walkers? >> >> The question is: is it of any real value such that it would be worth the >> cost and risk? >> >> >> One of the primary reasons for memmap_on_memory is that *memory hotplug* >> succeeds even in low-memory situations (including, low on ZONE_NORMAL >> situations). Sorry for the late reply, I'm busy with 100 other things. > > One usecase I would have in mind is a mix of smaller and larger memory > blocks. For larger ones you want to have memmap_on_memory in general > because they do not eat memory from outside but small(er) ones might be > more tricky because now you can add a lot of blocks that would be > internally fragmented to prevent larger allocations to form. Okay, I see what you mean. The internal fragmentation might become an issue at some point: for x86-64 with 128 MiB blocks / 2 MiB THP it's not a real issue right now. For a arm64 64k with 512 MiB blocks and 512 MiB THP / hugelb it could be one. I recall discussing that with Oscar back when he added memmap_on_memory, where we also discussed the variable-sized memory blocks to avoid such internal fragmentation. For small ones you probably want to only use memmap_on_memory when unavoidable: for example, when adding without memmap_on_memory would fail / already failed. Possibly some later memmap relocation might make sense in some scenarios. > >> So you want that behavior already when hotplugging such >> devices. While there might be value to relocate it later, I'm not sure if >> that is really worth it, and it does not solve the main use case. > > Is it worth it? TBH I am not sure same as I am not sure the global > default should be writable after boot. If we want to make it more > dynamic we should however talk about the proper layer this is > implemented on. Agreed.
On 07.08.23 14:41, David Hildenbrand wrote: > On 07.08.23 14:27, Michal Hocko wrote: >> On Sat 05-08-23 19:54:23, Aneesh Kumar K V wrote: >> [...] >>> Do you see a need for firmware-managed memory to be hotplugged in with >>> different memory block sizes? >> >> In short. Yes. Slightly longer, a fixed size memory block semantic is >> just standing in the way and I would even argue it is actively harmful. >> Just have a look at ridicously small memory blocks on ppc. I do >> understand that it makes some sense to be aligned to the memory model >> (so sparsmem section aligned). In an ideal world, memory hotplug v2 >> interface (if we ever go that path) should be physical memory range based. > > Yes, we discussed that a couple of times already (and so far nobody > cared to implement any of that). > > Small memory block sizes are very beneficial for use cases like PPC > dlar, virtio-mem, hyperv-balloon, ... essentially in most virtual > environments where you might want to add/remove memory in very small > granularity. I don't see that changing any time soon. Rather the opposite. > > Small memory block sizes are suboptimal for large machines where you > might never end up removing such memory (boot memory), or when dealing > with devices that can only be removed in one piece (DIMM/kmem). We > already have memory groups in place to model that. > > For the latter it might be beneficial to have memory blocks of larger > size that correspond to the physical memory ranges. That might also make > a memmap (re-)configuration easier. > > Not sure if that is standing in any way or is harmful, though. > Just because I thought of something right now, I'll share it, maybe it makes sense. Assume when we get add_memory*(MHP_MEMMAP_ON_MEMORY) and it is enabled by the admin: 1) We create a single altmap at the beginning of the memory 2) We create the existing fixed-size memory block devices, but flag them to be part of a single "altmap" unit. 3) Whenever we trigger offlining of a single such memory block, we offline *all* memory blocks belonging to that altmap, essentially using a single offline_pages() call and updating all memory block states accordingly. 4) Whenever we trigger onlining of a single such memory block, we online *all* memory blocks belonging to that altmap, using a single online_pages() call. 5) We fail remove_memory() if it doesn't cover the same (altmap) range. So we can avoid having a memory block v2 (and all that comes with that ...) for now and still get that altmap stuff sorted out. As that altmap behavior can be controlled by the admin, we should be fine for now. I think all memory notifiers should already be able to handle bigger granularity, but it would be easy to check. Some internal things might require a bit of tweaking. Just a thought.
On 8/8/23 12:05 AM, David Hildenbrand wrote: > On 07.08.23 14:41, David Hildenbrand wrote: >> On 07.08.23 14:27, Michal Hocko wrote: >>> On Sat 05-08-23 19:54:23, Aneesh Kumar K V wrote: >>> [...] >>>> Do you see a need for firmware-managed memory to be hotplugged in with >>>> different memory block sizes? >>> >>> In short. Yes. Slightly longer, a fixed size memory block semantic is >>> just standing in the way and I would even argue it is actively harmful. >>> Just have a look at ridicously small memory blocks on ppc. I do >>> understand that it makes some sense to be aligned to the memory model >>> (so sparsmem section aligned). In an ideal world, memory hotplug v2 >>> interface (if we ever go that path) should be physical memory range based. >> >> Yes, we discussed that a couple of times already (and so far nobody >> cared to implement any of that). >> >> Small memory block sizes are very beneficial for use cases like PPC >> dlar, virtio-mem, hyperv-balloon, ... essentially in most virtual >> environments where you might want to add/remove memory in very small >> granularity. I don't see that changing any time soon. Rather the opposite. >> >> Small memory block sizes are suboptimal for large machines where you >> might never end up removing such memory (boot memory), or when dealing >> with devices that can only be removed in one piece (DIMM/kmem). We >> already have memory groups in place to model that. >> >> For the latter it might be beneficial to have memory blocks of larger >> size that correspond to the physical memory ranges. That might also make >> a memmap (re-)configuration easier. >> >> Not sure if that is standing in any way or is harmful, though. >> > > Just because I thought of something right now, I'll share it, maybe it makes sense. > > Assume when we get add_memory*(MHP_MEMMAP_ON_MEMORY) and it is enabled by the admin: > > 1) We create a single altmap at the beginning of the memory > > 2) We create the existing fixed-size memory block devices, but flag them > to be part of a single "altmap" unit. > > 3) Whenever we trigger offlining of a single such memory block, we > offline *all* memory blocks belonging to that altmap, essentially > using a single offline_pages() call and updating all memory block > states accordingly. > > 4) Whenever we trigger onlining of a single such memory block, we > online *all* memory blocks belonging to that altmap, using a single > online_pages() call. > > 5) We fail remove_memory() if it doesn't cover the same (altmap) range. > > So we can avoid having a memory block v2 (and all that comes with that ...) for now and still get that altmap stuff sorted out. As that altmap behavior can be controlled by the admin, we should be fine for now. > > I think all memory notifiers should already be able to handle bigger granularity, but it would be easy to check. Some internal things might require a bit of tweaking. > > Just a thought. > W.r.t enabling memmap_on_memory for ppc64, I guess I will drop patch 7 and resend the series so that Andrew can pick rest of the patches? -aneesh
On 8/8/23 12:05 AM, David Hildenbrand wrote: > On 07.08.23 14:41, David Hildenbrand wrote: >> On 07.08.23 14:27, Michal Hocko wrote: >>> On Sat 05-08-23 19:54:23, Aneesh Kumar K V wrote: >>> [...] >>>> Do you see a need for firmware-managed memory to be hotplugged in with >>>> different memory block sizes? >>> >>> In short. Yes. Slightly longer, a fixed size memory block semantic is >>> just standing in the way and I would even argue it is actively harmful. >>> Just have a look at ridicously small memory blocks on ppc. I do >>> understand that it makes some sense to be aligned to the memory model >>> (so sparsmem section aligned). In an ideal world, memory hotplug v2 >>> interface (if we ever go that path) should be physical memory range based. >> >> Yes, we discussed that a couple of times already (and so far nobody >> cared to implement any of that). >> >> Small memory block sizes are very beneficial for use cases like PPC >> dlar, virtio-mem, hyperv-balloon, ... essentially in most virtual >> environments where you might want to add/remove memory in very small >> granularity. I don't see that changing any time soon. Rather the opposite. >> >> Small memory block sizes are suboptimal for large machines where you >> might never end up removing such memory (boot memory), or when dealing >> with devices that can only be removed in one piece (DIMM/kmem). We >> already have memory groups in place to model that. >> >> For the latter it might be beneficial to have memory blocks of larger >> size that correspond to the physical memory ranges. That might also make >> a memmap (re-)configuration easier. >> >> Not sure if that is standing in any way or is harmful, though. >> > > Just because I thought of something right now, I'll share it, maybe it makes sense. > > Assume when we get add_memory*(MHP_MEMMAP_ON_MEMORY) and it is enabled by the admin: > > 1) We create a single altmap at the beginning of the memory > > 2) We create the existing fixed-size memory block devices, but flag them > to be part of a single "altmap" unit. > > 3) Whenever we trigger offlining of a single such memory block, we > offline *all* memory blocks belonging to that altmap, essentially > using a single offline_pages() call and updating all memory block > states accordingly. > > 4) Whenever we trigger onlining of a single such memory block, we > online *all* memory blocks belonging to that altmap, using a single > online_pages() call. > > 5) We fail remove_memory() if it doesn't cover the same (altmap) range. > > So we can avoid having a memory block v2 (and all that comes with that ...) for now and still get that altmap stuff sorted out. As that altmap behavior can be controlled by the admin, we should be fine for now. > > I think all memory notifiers should already be able to handle bigger granularity, but it would be easy to check. Some internal things might require a bit of tweaking. > We can look at the possibility of using the altmap space reserved for a namespace (via option -M dev) for allocating struct page memory even with dax/kmem. -aneesh
On 08.08.23 08:29, Aneesh Kumar K V wrote: > On 8/8/23 12:05 AM, David Hildenbrand wrote: >> On 07.08.23 14:41, David Hildenbrand wrote: >>> On 07.08.23 14:27, Michal Hocko wrote: >>>> On Sat 05-08-23 19:54:23, Aneesh Kumar K V wrote: >>>> [...] >>>>> Do you see a need for firmware-managed memory to be hotplugged in with >>>>> different memory block sizes? >>>> >>>> In short. Yes. Slightly longer, a fixed size memory block semantic is >>>> just standing in the way and I would even argue it is actively harmful. >>>> Just have a look at ridicously small memory blocks on ppc. I do >>>> understand that it makes some sense to be aligned to the memory model >>>> (so sparsmem section aligned). In an ideal world, memory hotplug v2 >>>> interface (if we ever go that path) should be physical memory range based. >>> >>> Yes, we discussed that a couple of times already (and so far nobody >>> cared to implement any of that). >>> >>> Small memory block sizes are very beneficial for use cases like PPC >>> dlar, virtio-mem, hyperv-balloon, ... essentially in most virtual >>> environments where you might want to add/remove memory in very small >>> granularity. I don't see that changing any time soon. Rather the opposite. >>> >>> Small memory block sizes are suboptimal for large machines where you >>> might never end up removing such memory (boot memory), or when dealing >>> with devices that can only be removed in one piece (DIMM/kmem). We >>> already have memory groups in place to model that. >>> >>> For the latter it might be beneficial to have memory blocks of larger >>> size that correspond to the physical memory ranges. That might also make >>> a memmap (re-)configuration easier. >>> >>> Not sure if that is standing in any way or is harmful, though. >>> >> >> Just because I thought of something right now, I'll share it, maybe it makes sense. >> >> Assume when we get add_memory*(MHP_MEMMAP_ON_MEMORY) and it is enabled by the admin: >> >> 1) We create a single altmap at the beginning of the memory >> >> 2) We create the existing fixed-size memory block devices, but flag them >> to be part of a single "altmap" unit. >> >> 3) Whenever we trigger offlining of a single such memory block, we >> offline *all* memory blocks belonging to that altmap, essentially >> using a single offline_pages() call and updating all memory block >> states accordingly. >> >> 4) Whenever we trigger onlining of a single such memory block, we >> online *all* memory blocks belonging to that altmap, using a single >> online_pages() call. >> >> 5) We fail remove_memory() if it doesn't cover the same (altmap) range. >> >> So we can avoid having a memory block v2 (and all that comes with that ...) for now and still get that altmap stuff sorted out. As that altmap behavior can be controlled by the admin, we should be fine for now. >> >> I think all memory notifiers should already be able to handle bigger granularity, but it would be easy to check. Some internal things might require a bit of tweaking. >> > > We can look at the possibility of using the altmap space reserved for a namespace (via option -M dev) for allocating struct page memory even with dax/kmem. Right, an alternative would also be for the caller to pass in the altmap. Individual memory blocks can then get onlined/offlined as is. One issue might be, how to get that altmap considered online memory (e.g., pfn_to_online_page(), kdump, ...). Right now, the altmap always falls into an online section once the memory block is online, so pfn_to_online_page() and especially online_section() holds as soon as the altmap has reasonable content -- when the memory block is online and initialized the memmap. Maybe that can be worked around, just pointing it out.
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index 1ce8ad04a980..d282664f558e 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -89,7 +89,10 @@ static int set_memmap_mode(const char *val, const struct kernel_param *kp) else mode = MEMMAP_ON_MEMORY_DISABLE; } + /* Avoid changing memmap mode during hotplug. */ + get_online_mems(); *((int *)kp->arg) = mode; + put_online_mems(); if (mode == MEMMAP_ON_MEMORY_FORCE) { unsigned long memmap_pages = memory_block_memmap_on_memory_pages(); @@ -110,7 +113,7 @@ static const struct kernel_param_ops memmap_mode_ops = { .set = set_memmap_mode, .get = get_memmap_mode, }; -module_param_cb(memmap_on_memory, &memmap_mode_ops, &memmap_mode, 0444); +module_param_cb(memmap_on_memory, &memmap_mode_ops, &memmap_mode, 0644); MODULE_PARM_DESC(memmap_on_memory, "Enable memmap on memory for memory hotplug\n" "With value \"force\" it could result in memory wastage due " "to memmap size limitations (Y/N/force)"); @@ -2172,22 +2175,20 @@ static int __ref try_remove_memory(u64 start, u64 size) * We only support removing memory added with MHP_MEMMAP_ON_MEMORY in * the same granularity it was added - a single memory block. */ - if (mhp_memmap_on_memory()) { - ret = walk_memory_blocks(start, size, &mem, test_has_altmap_cb); - if (ret) { - if (size != memory_block_size_bytes()) { - pr_warn("Refuse to remove %#llx - %#llx," - "wrong granularity\n", - start, start + size); - return -EINVAL; - } - altmap = mem->altmap; - /* - * Mark altmap NULL so that we can add a debug - * check on memblock free. - */ - mem->altmap = NULL; + ret = walk_memory_blocks(start, size, &mem, test_has_altmap_cb); + if (ret) { + if (size != memory_block_size_bytes()) { + pr_warn("Refuse to remove %#llx - %#llx," + "wrong granularity\n", + start, start + size); + return -EINVAL; } + altmap = mem->altmap; + /* + * Mark altmap NULL so that we can add a debug + * check on memblock free. + */ + mem->altmap = NULL; } /* remove memmap entry */