| Message ID | 1540551662-26458-1-git-send-email-arunks@codeaurora.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | mm: convert totalram_pages, totalhigh_pages and managed pages to atomic | expand |
Any comments? Regards, Arun On 2018-10-26 16:30, Arun KS wrote: > This series convert totalram_pages, totalhigh_pages and > zone->managed_pages to atomic variables. > > The patch was comiple tested on x86(x86_64_defconfig & i386_defconfig) > on tip of linux-mmotm. And memory hotplug tested on arm64, but on an > older version of kernel. > > Arun KS (4): > mm: Fix multiple evaluvations of totalram_pages and managed_pages > mm: Convert zone->managed_pages to atomic variable > mm: convert totalram_pages and totalhigh_pages variables to atomic > mm: Remove managed_page_count spinlock > > arch/csky/mm/init.c | 4 +- > arch/powerpc/platforms/pseries/cmm.c | 10 ++-- > arch/s390/mm/init.c | 2 +- > arch/um/kernel/mem.c | 3 +- > arch/x86/kernel/cpu/microcode/core.c | 5 +- > drivers/char/agp/backend.c | 4 +- > drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 2 +- > drivers/gpu/drm/i915/i915_gem.c | 2 +- > drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 4 +- > drivers/hv/hv_balloon.c | 19 +++---- > drivers/md/dm-bufio.c | 2 +- > drivers/md/dm-crypt.c | 2 +- > drivers/md/dm-integrity.c | 2 +- > drivers/md/dm-stats.c | 2 +- > drivers/media/platform/mtk-vpu/mtk_vpu.c | 2 +- > drivers/misc/vmw_balloon.c | 2 +- > drivers/parisc/ccio-dma.c | 4 +- > drivers/parisc/sba_iommu.c | 4 +- > drivers/staging/android/ion/ion_system_heap.c | 2 +- > drivers/xen/xen-selfballoon.c | 6 +-- > fs/ceph/super.h | 2 +- > fs/file_table.c | 7 +-- > fs/fuse/inode.c | 2 +- > fs/nfs/write.c | 2 +- > fs/nfsd/nfscache.c | 2 +- > fs/ntfs/malloc.h | 2 +- > fs/proc/base.c | 2 +- > include/linux/highmem.h | 28 ++++++++++- > include/linux/mm.h | 27 +++++++++- > include/linux/mmzone.h | 15 +++--- > include/linux/swap.h | 1 - > kernel/fork.c | 5 +- > kernel/kexec_core.c | 5 +- > kernel/power/snapshot.c | 2 +- > lib/show_mem.c | 2 +- > mm/highmem.c | 4 +- > mm/huge_memory.c | 2 +- > mm/kasan/quarantine.c | 2 +- > mm/memblock.c | 6 +-- > mm/memory_hotplug.c | 4 +- > mm/mm_init.c | 2 +- > mm/oom_kill.c | 2 +- > mm/page_alloc.c | 71 > +++++++++++++-------------- > mm/shmem.c | 7 +-- > mm/slab.c | 2 +- > mm/swap.c | 2 +- > mm/util.c | 2 +- > mm/vmalloc.c | 4 +- > mm/vmstat.c | 4 +- > mm/workingset.c | 2 +- > mm/zswap.c | 4 +- > net/dccp/proto.c | 7 +-- > net/decnet/dn_route.c | 2 +- > net/ipv4/tcp_metrics.c | 2 +- > net/netfilter/nf_conntrack_core.c | 7 +-- > net/netfilter/xt_hashlimit.c | 5 +- > net/sctp/protocol.c | 7 +-- > security/integrity/ima/ima_kexec.c | 2 +- > 58 files changed, 195 insertions(+), 144 deletions(-)
On 06.11.2018 8:38, Arun KS wrote: > Any comments? Looks good. Except unclear motivation behind this change. This should be in comment of one of patch. Reviewed-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru> > > Regards, > Arun > > On 2018-10-26 16:30, Arun KS wrote: >> This series convert totalram_pages, totalhigh_pages and >> zone->managed_pages to atomic variables. >> >> The patch was comiple tested on x86(x86_64_defconfig & i386_defconfig) >> on tip of linux-mmotm. And memory hotplug tested on arm64, but on an >> older version of kernel. >> >> Arun KS (4): >> mm: Fix multiple evaluvations of totalram_pages and managed_pages >> mm: Convert zone->managed_pages to atomic variable >> mm: convert totalram_pages and totalhigh_pages variables to atomic >> mm: Remove managed_page_count spinlock >> >> arch/csky/mm/init.c | 4 +- >> arch/powerpc/platforms/pseries/cmm.c | 10 ++-- >> arch/s390/mm/init.c | 2 +- >> arch/um/kernel/mem.c | 3 +- >> arch/x86/kernel/cpu/microcode/core.c | 5 +- >> drivers/char/agp/backend.c | 4 +- >> drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 2 +- >> drivers/gpu/drm/i915/i915_gem.c | 2 +- >> drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 4 +- >> drivers/hv/hv_balloon.c | 19 +++---- >> drivers/md/dm-bufio.c | 2 +- >> drivers/md/dm-crypt.c | 2 +- >> drivers/md/dm-integrity.c | 2 +- >> drivers/md/dm-stats.c | 2 +- >> drivers/media/platform/mtk-vpu/mtk_vpu.c | 2 +- >> drivers/misc/vmw_balloon.c | 2 +- >> drivers/parisc/ccio-dma.c | 4 +- >> drivers/parisc/sba_iommu.c | 4 +- >> drivers/staging/android/ion/ion_system_heap.c | 2 +- >> drivers/xen/xen-selfballoon.c | 6 +-- >> fs/ceph/super.h | 2 +- >> fs/file_table.c | 7 +-- >> fs/fuse/inode.c | 2 +- >> fs/nfs/write.c | 2 +- >> fs/nfsd/nfscache.c | 2 +- >> fs/ntfs/malloc.h | 2 +- >> fs/proc/base.c | 2 +- >> include/linux/highmem.h | 28 ++++++++++- >> include/linux/mm.h | 27 +++++++++- >> include/linux/mmzone.h | 15 +++--- >> include/linux/swap.h | 1 - >> kernel/fork.c | 5 +- >> kernel/kexec_core.c | 5 +- >> kernel/power/snapshot.c | 2 +- >> lib/show_mem.c | 2 +- >> mm/highmem.c | 4 +- >> mm/huge_memory.c | 2 +- >> mm/kasan/quarantine.c | 2 +- >> mm/memblock.c | 6 +-- >> mm/memory_hotplug.c | 4 +- >> mm/mm_init.c | 2 +- >> mm/oom_kill.c | 2 +- >> mm/page_alloc.c | 71 +++++++++++++-------------- >> mm/shmem.c | 7 +-- >> mm/slab.c | 2 +- >> mm/swap.c | 2 +- >> mm/util.c | 2 +- >> mm/vmalloc.c | 4 +- >> mm/vmstat.c | 4 +- >> mm/workingset.c | 2 +- >> mm/zswap.c | 4 +- >> net/dccp/proto.c | 7 +-- >> net/decnet/dn_route.c | 2 +- >> net/ipv4/tcp_metrics.c | 2 +- >> net/netfilter/nf_conntrack_core.c | 7 +-- >> net/netfilter/xt_hashlimit.c | 5 +- >> net/sctp/protocol.c | 7 +-- >> security/integrity/ima/ima_kexec.c | 2 +- >> 58 files changed, 195 insertions(+), 144 deletions(-)
On 2018-11-06 13:47, Konstantin Khlebnikov wrote: > On 06.11.2018 8:38, Arun KS wrote: >> Any comments? > > Looks good. > Except unclear motivation behind this change. > This should be in comment of one of patch. totalram_pages, zone->managed_pages and totalhigh_pages are sometimes modified outside managed_page_count_lock. Hence convert these variable to atomic to avoid readers potentially seeing a store tear. Will update the comment. Regards, Arun > > Reviewed-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru> > >> >> Regards, >> Arun >> >> On 2018-10-26 16:30, Arun KS wrote: >>> This series convert totalram_pages, totalhigh_pages and >>> zone->managed_pages to atomic variables. >>> >>> The patch was comiple tested on x86(x86_64_defconfig & >>> i386_defconfig) >>> on tip of linux-mmotm. And memory hotplug tested on arm64, but on an >>> older version of kernel. >>> >>> Arun KS (4): >>> mm: Fix multiple evaluvations of totalram_pages and managed_pages >>> mm: Convert zone->managed_pages to atomic variable >>> mm: convert totalram_pages and totalhigh_pages variables to atomic >>> mm: Remove managed_page_count spinlock >>> >>> arch/csky/mm/init.c | 4 +- >>> arch/powerpc/platforms/pseries/cmm.c | 10 ++-- >>> arch/s390/mm/init.c | 2 +- >>> arch/um/kernel/mem.c | 3 +- >>> arch/x86/kernel/cpu/microcode/core.c | 5 +- >>> drivers/char/agp/backend.c | 4 +- >>> drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 2 +- >>> drivers/gpu/drm/i915/i915_gem.c | 2 +- >>> drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 4 +- >>> drivers/hv/hv_balloon.c | 19 +++---- >>> drivers/md/dm-bufio.c | 2 +- >>> drivers/md/dm-crypt.c | 2 +- >>> drivers/md/dm-integrity.c | 2 +- >>> drivers/md/dm-stats.c | 2 +- >>> drivers/media/platform/mtk-vpu/mtk_vpu.c | 2 +- >>> drivers/misc/vmw_balloon.c | 2 +- >>> drivers/parisc/ccio-dma.c | 4 +- >>> drivers/parisc/sba_iommu.c | 4 +- >>> drivers/staging/android/ion/ion_system_heap.c | 2 +- >>> drivers/xen/xen-selfballoon.c | 6 +-- >>> fs/ceph/super.h | 2 +- >>> fs/file_table.c | 7 +-- >>> fs/fuse/inode.c | 2 +- >>> fs/nfs/write.c | 2 +- >>> fs/nfsd/nfscache.c | 2 +- >>> fs/ntfs/malloc.h | 2 +- >>> fs/proc/base.c | 2 +- >>> include/linux/highmem.h | 28 ++++++++++- >>> include/linux/mm.h | 27 +++++++++- >>> include/linux/mmzone.h | 15 +++--- >>> include/linux/swap.h | 1 - >>> kernel/fork.c | 5 +- >>> kernel/kexec_core.c | 5 +- >>> kernel/power/snapshot.c | 2 +- >>> lib/show_mem.c | 2 +- >>> mm/highmem.c | 4 +- >>> mm/huge_memory.c | 2 +- >>> mm/kasan/quarantine.c | 2 +- >>> mm/memblock.c | 6 +-- >>> mm/memory_hotplug.c | 4 +- >>> mm/mm_init.c | 2 +- >>> mm/oom_kill.c | 2 +- >>> mm/page_alloc.c | 71 >>> +++++++++++++-------------- >>> mm/shmem.c | 7 +-- >>> mm/slab.c | 2 +- >>> mm/swap.c | 2 +- >>> mm/util.c | 2 +- >>> mm/vmalloc.c | 4 +- >>> mm/vmstat.c | 4 +- >>> mm/workingset.c | 2 +- >>> mm/zswap.c | 4 +- >>> net/dccp/proto.c | 7 +-- >>> net/decnet/dn_route.c | 2 +- >>> net/ipv4/tcp_metrics.c | 2 +- >>> net/netfilter/nf_conntrack_core.c | 7 +-- >>> net/netfilter/xt_hashlimit.c | 5 +- >>> net/sctp/protocol.c | 7 +-- >>> security/integrity/ima/ima_kexec.c | 2 +- >>> 58 files changed, 195 insertions(+), 144 deletions(-)
On 06.11.2018 11:30, Arun KS wrote: > On 2018-11-06 13:47, Konstantin Khlebnikov wrote: >> On 06.11.2018 8:38, Arun KS wrote: >>> Any comments? >> >> Looks good. >> Except unclear motivation behind this change. >> This should be in comment of one of patch. > > totalram_pages, zone->managed_pages and totalhigh_pages are sometimes modified outside managed_page_count_lock. Hence convert these variable > to atomic to avoid readers potentially seeing a store tear. So, this is just theoretical issue or splat from sanitizer. After boot memory online\offline are strictly serialized by rw-semaphore. > > Will update the comment. > > Regards, > Arun > >> >> Reviewed-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru> >> >>> >>> Regards, >>> Arun >>> >>> On 2018-10-26 16:30, Arun KS wrote: >>>> This series convert totalram_pages, totalhigh_pages and >>>> zone->managed_pages to atomic variables. >>>> >>>> The patch was comiple tested on x86(x86_64_defconfig & i386_defconfig) >>>> on tip of linux-mmotm. And memory hotplug tested on arm64, but on an >>>> older version of kernel. >>>> >>>> Arun KS (4): >>>> mm: Fix multiple evaluvations of totalram_pages and managed_pages >>>> mm: Convert zone->managed_pages to atomic variable >>>> mm: convert totalram_pages and totalhigh_pages variables to atomic >>>> mm: Remove managed_page_count spinlock >>>> >>>> arch/csky/mm/init.c | 4 +- >>>> arch/powerpc/platforms/pseries/cmm.c | 10 ++-- >>>> arch/s390/mm/init.c | 2 +- >>>> arch/um/kernel/mem.c | 3 +- >>>> arch/x86/kernel/cpu/microcode/core.c | 5 +- >>>> drivers/char/agp/backend.c | 4 +- >>>> drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 2 +- >>>> drivers/gpu/drm/i915/i915_gem.c | 2 +- >>>> drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 4 +- >>>> drivers/hv/hv_balloon.c | 19 +++---- >>>> drivers/md/dm-bufio.c | 2 +- >>>> drivers/md/dm-crypt.c | 2 +- >>>> drivers/md/dm-integrity.c | 2 +- >>>> drivers/md/dm-stats.c | 2 +- >>>> drivers/media/platform/mtk-vpu/mtk_vpu.c | 2 +- >>>> drivers/misc/vmw_balloon.c | 2 +- >>>> drivers/parisc/ccio-dma.c | 4 +- >>>> drivers/parisc/sba_iommu.c | 4 +- >>>> drivers/staging/android/ion/ion_system_heap.c | 2 +- >>>> drivers/xen/xen-selfballoon.c | 6 +-- >>>> fs/ceph/super.h | 2 +- >>>> fs/file_table.c | 7 +-- >>>> fs/fuse/inode.c | 2 +- >>>> fs/nfs/write.c | 2 +- >>>> fs/nfsd/nfscache.c | 2 +- >>>> fs/ntfs/malloc.h | 2 +- >>>> fs/proc/base.c | 2 +- >>>> include/linux/highmem.h | 28 ++++++++++- >>>> include/linux/mm.h | 27 +++++++++- >>>> include/linux/mmzone.h | 15 +++--- >>>> include/linux/swap.h | 1 - >>>> kernel/fork.c | 5 +- >>>> kernel/kexec_core.c | 5 +- >>>> kernel/power/snapshot.c | 2 +- >>>> lib/show_mem.c | 2 +- >>>> mm/highmem.c | 4 +- >>>> mm/huge_memory.c | 2 +- >>>> mm/kasan/quarantine.c | 2 +- >>>> mm/memblock.c | 6 +-- >>>> mm/memory_hotplug.c | 4 +- >>>> mm/mm_init.c | 2 +- >>>> mm/oom_kill.c | 2 +- >>>> mm/page_alloc.c | 71 +++++++++++++-------------- >>>> mm/shmem.c | 7 +-- >>>> mm/slab.c | 2 +- >>>> mm/swap.c | 2 +- >>>> mm/util.c | 2 +- >>>> mm/vmalloc.c | 4 +- >>>> mm/vmstat.c | 4 +- >>>> mm/workingset.c | 2 +- >>>> mm/zswap.c | 4 +- >>>> net/dccp/proto.c | 7 +-- >>>> net/decnet/dn_route.c | 2 +- >>>> net/ipv4/tcp_metrics.c | 2 +- >>>> net/netfilter/nf_conntrack_core.c | 7 +-- >>>> net/netfilter/xt_hashlimit.c | 5 +- >>>> net/sctp/protocol.c | 7 +-- >>>> security/integrity/ima/ima_kexec.c | 2 +- >>>> 58 files changed, 195 insertions(+), 144 deletions(-)
On 2018-11-06 14:07, Konstantin Khlebnikov wrote: > On 06.11.2018 11:30, Arun KS wrote: >> On 2018-11-06 13:47, Konstantin Khlebnikov wrote: >>> On 06.11.2018 8:38, Arun KS wrote: >>>> Any comments? >>> >>> Looks good. >>> Except unclear motivation behind this change. >>> This should be in comment of one of patch. >> >> totalram_pages, zone->managed_pages and totalhigh_pages are sometimes >> modified outside managed_page_count_lock. Hence convert these variable >> to atomic to avoid readers potentially seeing a store tear. > > So, this is just theoretical issue or splat from sanitizer. > After boot memory online\offline are strictly serialized by > rw-semaphore. Few instances which can race with hot add. Please see below, https://patchwork.kernel.org/patch/10627521/ Regards, Arun > >> >> Will update the comment. >> >> Regards, >> Arun >> >>> >>> Reviewed-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru> >>> >>>> >>>> Regards, >>>> Arun >>>> >>>> On 2018-10-26 16:30, Arun KS wrote: >>>>> This series convert totalram_pages, totalhigh_pages and >>>>> zone->managed_pages to atomic variables. >>>>> >>>>> The patch was comiple tested on x86(x86_64_defconfig & >>>>> i386_defconfig) >>>>> on tip of linux-mmotm. And memory hotplug tested on arm64, but on >>>>> an >>>>> older version of kernel. >>>>> >>>>> Arun KS (4): >>>>> mm: Fix multiple evaluvations of totalram_pages and managed_pages >>>>> mm: Convert zone->managed_pages to atomic variable >>>>> mm: convert totalram_pages and totalhigh_pages variables to >>>>> atomic >>>>> mm: Remove managed_page_count spinlock >>>>> >>>>> arch/csky/mm/init.c | 4 +- >>>>> arch/powerpc/platforms/pseries/cmm.c | 10 ++-- >>>>> arch/s390/mm/init.c | 2 +- >>>>> arch/um/kernel/mem.c | 3 +- >>>>> arch/x86/kernel/cpu/microcode/core.c | 5 +- >>>>> drivers/char/agp/backend.c | 4 +- >>>>> drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 2 +- >>>>> drivers/gpu/drm/i915/i915_gem.c | 2 +- >>>>> drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 4 +- >>>>> drivers/hv/hv_balloon.c | 19 +++---- >>>>> drivers/md/dm-bufio.c | 2 +- >>>>> drivers/md/dm-crypt.c | 2 +- >>>>> drivers/md/dm-integrity.c | 2 +- >>>>> drivers/md/dm-stats.c | 2 +- >>>>> drivers/media/platform/mtk-vpu/mtk_vpu.c | 2 +- >>>>> drivers/misc/vmw_balloon.c | 2 +- >>>>> drivers/parisc/ccio-dma.c | 4 +- >>>>> drivers/parisc/sba_iommu.c | 4 +- >>>>> drivers/staging/android/ion/ion_system_heap.c | 2 +- >>>>> drivers/xen/xen-selfballoon.c | 6 +-- >>>>> fs/ceph/super.h | 2 +- >>>>> fs/file_table.c | 7 +-- >>>>> fs/fuse/inode.c | 2 +- >>>>> fs/nfs/write.c | 2 +- >>>>> fs/nfsd/nfscache.c | 2 +- >>>>> fs/ntfs/malloc.h | 2 +- >>>>> fs/proc/base.c | 2 +- >>>>> include/linux/highmem.h | 28 ++++++++++- >>>>> include/linux/mm.h | 27 +++++++++- >>>>> include/linux/mmzone.h | 15 +++--- >>>>> include/linux/swap.h | 1 - >>>>> kernel/fork.c | 5 +- >>>>> kernel/kexec_core.c | 5 +- >>>>> kernel/power/snapshot.c | 2 +- >>>>> lib/show_mem.c | 2 +- >>>>> mm/highmem.c | 4 +- >>>>> mm/huge_memory.c | 2 +- >>>>> mm/kasan/quarantine.c | 2 +- >>>>> mm/memblock.c | 6 +-- >>>>> mm/memory_hotplug.c | 4 +- >>>>> mm/mm_init.c | 2 +- >>>>> mm/oom_kill.c | 2 +- >>>>> mm/page_alloc.c | 71 >>>>> +++++++++++++-------------- >>>>> mm/shmem.c | 7 +-- >>>>> mm/slab.c | 2 +- >>>>> mm/swap.c | 2 +- >>>>> mm/util.c | 2 +- >>>>> mm/vmalloc.c | 4 +- >>>>> mm/vmstat.c | 4 +- >>>>> mm/workingset.c | 2 +- >>>>> mm/zswap.c | 4 +- >>>>> net/dccp/proto.c | 7 +-- >>>>> net/decnet/dn_route.c | 2 +- >>>>> net/ipv4/tcp_metrics.c | 2 +- >>>>> net/netfilter/nf_conntrack_core.c | 7 +-- >>>>> net/netfilter/xt_hashlimit.c | 5 +- >>>>> net/sctp/protocol.c | 7 +-- >>>>> security/integrity/ima/ima_kexec.c | 2 +- >>>>> 58 files changed, 195 insertions(+), 144 deletions(-)
On Fri, 26 Oct 2018 16:30:58 +0530 Arun KS <arunks@codeaurora.org> wrote: > This series convert totalram_pages, totalhigh_pages and > zone->managed_pages to atomic variables. The whole point appears to be removal of managed_page_count_lock, yes? Why? What is the value of this patchset? If "performance" then are any measurements available?
On 2018-11-07 05:52, Andrew Morton wrote: > On Fri, 26 Oct 2018 16:30:58 +0530 Arun KS <arunks@codeaurora.org> > wrote: > >> This series convert totalram_pages, totalhigh_pages and >> zone->managed_pages to atomic variables. > > The whole point appears to be removal of managed_page_count_lock, yes? > > Why? What is the value of this patchset? If "performance" then are > any > measurements available? Hello Andrew, https://patchwork.kernel.org/patch/10670787/ In version 2, I have added motivation behind this conversion. Pasting same here, totalram_pages, zone->managed_pages and totalhigh_pages updates are protected by managed_page_count_lock, but readers never care about it. Convert these variables to atomic to avoid readers potentially seeing a store tear. I don't think we have a performance improvement here. Regards, Arun
On 06.11.2018 11:43, Arun KS wrote: > On 2018-11-06 14:07, Konstantin Khlebnikov wrote: >> On 06.11.2018 11:30, Arun KS wrote: >>> On 2018-11-06 13:47, Konstantin Khlebnikov wrote: >>>> On 06.11.2018 8:38, Arun KS wrote: >>>>> Any comments? >>>> >>>> Looks good. >>>> Except unclear motivation behind this change. >>>> This should be in comment of one of patch. >>> >>> totalram_pages, zone->managed_pages and totalhigh_pages are sometimes modified outside managed_page_count_lock. Hence convert these >>> variable to atomic to avoid readers potentially seeing a store tear. >> >> So, this is just theoretical issue or splat from sanitizer. >> After boot memory online\offline are strictly serialized by rw-semaphore. > > Few instances which can race with hot add. Please see below, > https://patchwork.kernel.org/patch/10627521/ Could you point what exactly are you fixing with this set? from v2: > totalram_pages, zone->managed_pages and totalhigh_pages updates > are protected by managed_page_count_lock, but readers never care > about it. Convert these variables to atomic to avoid readers > potentially seeing a store tear. This? Aligned unsigned long almost always stored at once. To make it completely correct you could replace a += b; with WRITE_ONCE(a, a + b); > > Regards, > Arun > >> >>> >>> Will update the comment. >>> >>> Regards, >>> Arun >>> >>>> >>>> Reviewed-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru> >>>> >>>>> >>>>> Regards, >>>>> Arun >>>>> >>>>> On 2018-10-26 16:30, Arun KS wrote: >>>>>> This series convert totalram_pages, totalhigh_pages and >>>>>> zone->managed_pages to atomic variables. >>>>>> >>>>>> The patch was comiple tested on x86(x86_64_defconfig & i386_defconfig) >>>>>> on tip of linux-mmotm. And memory hotplug tested on arm64, but on an >>>>>> older version of kernel. >>>>>> >>>>>> Arun KS (4): >>>>>> mm: Fix multiple evaluvations of totalram_pages and managed_pages >>>>>> mm: Convert zone->managed_pages to atomic variable >>>>>> mm: convert totalram_pages and totalhigh_pages variables to atomic >>>>>> mm: Remove managed_page_count spinlock >>>>>> >>>>>> arch/csky/mm/init.c | 4 +- >>>>>> arch/powerpc/platforms/pseries/cmm.c | 10 ++-- >>>>>> arch/s390/mm/init.c | 2 +- >>>>>> arch/um/kernel/mem.c | 3 +- >>>>>> arch/x86/kernel/cpu/microcode/core.c | 5 +- >>>>>> drivers/char/agp/backend.c | 4 +- >>>>>> drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 2 +- >>>>>> drivers/gpu/drm/i915/i915_gem.c | 2 +- >>>>>> drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 4 +- >>>>>> drivers/hv/hv_balloon.c | 19 +++---- >>>>>> drivers/md/dm-bufio.c | 2 +- >>>>>> drivers/md/dm-crypt.c | 2 +- >>>>>> drivers/md/dm-integrity.c | 2 +- >>>>>> drivers/md/dm-stats.c | 2 +- >>>>>> drivers/media/platform/mtk-vpu/mtk_vpu.c | 2 +- >>>>>> drivers/misc/vmw_balloon.c | 2 +- >>>>>> drivers/parisc/ccio-dma.c | 4 +- >>>>>> drivers/parisc/sba_iommu.c | 4 +- >>>>>> drivers/staging/android/ion/ion_system_heap.c | 2 +- >>>>>> drivers/xen/xen-selfballoon.c | 6 +-- >>>>>> fs/ceph/super.h | 2 +- >>>>>> fs/file_table.c | 7 +-- >>>>>> fs/fuse/inode.c | 2 +- >>>>>> fs/nfs/write.c | 2 +- >>>>>> fs/nfsd/nfscache.c | 2 +- >>>>>> fs/ntfs/malloc.h | 2 +- >>>>>> fs/proc/base.c | 2 +- >>>>>> include/linux/highmem.h | 28 ++++++++++- >>>>>> include/linux/mm.h | 27 +++++++++- >>>>>> include/linux/mmzone.h | 15 +++--- >>>>>> include/linux/swap.h | 1 - >>>>>> kernel/fork.c | 5 +- >>>>>> kernel/kexec_core.c | 5 +- >>>>>> kernel/power/snapshot.c | 2 +- >>>>>> lib/show_mem.c | 2 +- >>>>>> mm/highmem.c | 4 +- >>>>>> mm/huge_memory.c | 2 +- >>>>>> mm/kasan/quarantine.c | 2 +- >>>>>> mm/memblock.c | 6 +-- >>>>>> mm/memory_hotplug.c | 4 +- >>>>>> mm/mm_init.c | 2 +- >>>>>> mm/oom_kill.c | 2 +- >>>>>> mm/page_alloc.c | 71 +++++++++++++-------------- >>>>>> mm/shmem.c | 7 +-- >>>>>> mm/slab.c | 2 +- >>>>>> mm/swap.c | 2 +- >>>>>> mm/util.c | 2 +- >>>>>> mm/vmalloc.c | 4 +- >>>>>> mm/vmstat.c | 4 +- >>>>>> mm/workingset.c | 2 +- >>>>>> mm/zswap.c | 4 +- >>>>>> net/dccp/proto.c | 7 +-- >>>>>> net/decnet/dn_route.c | 2 +- >>>>>> net/ipv4/tcp_metrics.c | 2 +- >>>>>> net/netfilter/nf_conntrack_core.c | 7 +-- >>>>>> net/netfilter/xt_hashlimit.c | 5 +- >>>>>> net/sctp/protocol.c | 7 +-- >>>>>> security/integrity/ima/ima_kexec.c | 2 +- >>>>>> 58 files changed, 195 insertions(+), 144 deletions(-)
On 11/7/18 8:02 AM, Konstantin Khlebnikov wrote: > On 06.11.2018 11:43, Arun KS wrote: >> On 2018-11-06 14:07, Konstantin Khlebnikov wrote: >>> On 06.11.2018 11:30, Arun KS wrote: >>>> On 2018-11-06 13:47, Konstantin Khlebnikov wrote: >>>>> On 06.11.2018 8:38, Arun KS wrote: >>>>>> Any comments? >>>>> >>>>> Looks good. >>>>> Except unclear motivation behind this change. >>>>> This should be in comment of one of patch. >>>> >>>> totalram_pages, zone->managed_pages and totalhigh_pages are sometimes modified outside managed_page_count_lock. Hence convert these >>>> variable to atomic to avoid readers potentially seeing a store tear. >>> >>> So, this is just theoretical issue or splat from sanitizer. >>> After boot memory online\offline are strictly serialized by rw-semaphore. >> >> Few instances which can race with hot add. Please see below, >> https://patchwork.kernel.org/patch/10627521/ > Could you point what exactly are you fixing with this set? > > from v2: > > > totalram_pages, zone->managed_pages and totalhigh_pages updates > > are protected by managed_page_count_lock, but readers never care > > about it. Convert these variables to atomic to avoid readers > > potentially seeing a store tear. > > This? > > > Aligned unsigned long almost always stored at once. The point is "almost always", so better not rely on it :) But the main motivation was that managed_page_count_lock handling was complicating Arun's "memory_hotplug: Free pages as higher order" patch and it seemed a better idea to just remove and convert this to atomics, with preventing potential store-to-read tearing as a bonus. It would be nice to mention it in the changelogs though. > To make it completely correct you could replace > > a += b; > > with > > WRITE_ONCE(a, a + b); Wouldn't be enough to get rid of the locks.
On Wed 07-11-18 09:50:10, Vlastimil Babka wrote: > On 11/7/18 8:02 AM, Konstantin Khlebnikov wrote: [...] > > Could you point what exactly are you fixing with this set? > > > > from v2: > > > > > totalram_pages, zone->managed_pages and totalhigh_pages updates > > > are protected by managed_page_count_lock, but readers never care > > > about it. Convert these variables to atomic to avoid readers > > > potentially seeing a store tear. > > > > This? > > > > > > Aligned unsigned long almost always stored at once. > > The point is "almost always", so better not rely on it :) But the main > motivation was that managed_page_count_lock handling was complicating > Arun's "memory_hotplug: Free pages as higher order" patch and it seemed > a better idea to just remove and convert this to atomics, with > preventing potential store-to-read tearing as a bonus. And more importantly the lock itself seems bogus as mentioned here http://lkml.kernel.org/r/20181106141732.GR27423@dhcp22.suse.cz > It would be nice to mention it in the changelogs though. agreed
On Wed 07-11-18 11:28:37, Michal Hocko wrote: > On Wed 07-11-18 09:50:10, Vlastimil Babka wrote: > > On 11/7/18 8:02 AM, Konstantin Khlebnikov wrote: > [...] > > > Could you point what exactly are you fixing with this set? > > > > > > from v2: > > > > > > > totalram_pages, zone->managed_pages and totalhigh_pages updates > > > > are protected by managed_page_count_lock, but readers never care > > > > about it. Convert these variables to atomic to avoid readers > > > > potentially seeing a store tear. > > > > > > This? > > > > > > > > > Aligned unsigned long almost always stored at once. > > > > The point is "almost always", so better not rely on it :) But the main > > motivation was that managed_page_count_lock handling was complicating > > Arun's "memory_hotplug: Free pages as higher order" patch and it seemed > > a better idea to just remove and convert this to atomics, with > > preventing potential store-to-read tearing as a bonus. > > And more importantly the lock itself seems bogus as mentioned here > http://lkml.kernel.org/r/20181106141732.GR27423@dhcp22.suse.cz Should be http://lkml.kernel.org/r/20181107103630.GF2453@dhcp22.suse.cz