Re: [RFC v4 0/3] mm: zap pages with read mmap_sem in munmap for large mapping
On 7/12/18 1:04 AM, Michal Hocko wrote: On Wed 11-07-18 10:04:48, Yang Shi wrote: [...] One approach is to save all the vmas on a separate list, then zap_page_range does unmap with this list. Just detached unmapped vma chain from mm. You can keep the existing vm_next chain and reuse it. Yes. Other than this, we still need do: * Tell zap_page_range not update vm_flags as what I did in v4. Of course without VM_DEAD this time * Extract pagetable free code then do it after zap_page_range. I think I can just cal free_pgd_range() directly.
Re: [RFC v4 0/3] mm: zap pages with read mmap_sem in munmap for large mapping
On Wed 11-07-18 15:49:54, Andrew Morton wrote: > On Wed, 11 Jul 2018 12:33:12 +0200 Michal Hocko wrote: > > > > Approach: > > > Zapping pages is the most time consuming part, according to the > > > suggestion from > > > Michal Hocko [1], zapping pages can be done with holding read mmap_sem, > > > like > > > what MADV_DONTNEED does. Then re-acquire write mmap_sem to cleanup vmas. > > > > > > But, we can't call MADV_DONTNEED directly, since there are two major > > > drawbacks: > > > * The unexpected state from PF if it wins the race in the middle of > > > munmap. > > > It may return zero page, instead of the content or SIGSEGV. > > > * Can’t handle VM_LOCKED | VM_HUGETLB | VM_PFNMAP and uprobe mappings, > > > which > > > is a showstopper from akpm > > > > I do not really understand why this is a showstopper. This is a mere > > optimization. VM_LOCKED ranges are usually not that large. VM_HUGETLB > > can be quite large alright but this should be doable on top. Is there > > any reason to block any "cover most mappings first" patch? > > Somebody somewhere is going to want to unmap vast mlocked regions and > they're going to report softlockup warnings. So we shouldn't implement > something which can't address these cases. Maybe it doesn't do so in > the first version, but we should at least have a plan to handle all > cases. Absolutely. I was just responding to the "showstopper" part. This is improving some cases but it shouldn't make others worse so going incremental should be perfectly reasonable. -- Michal Hocko SUSE Labs
Re: [RFC v4 0/3] mm: zap pages with read mmap_sem in munmap for large mapping
On Wed 11-07-18 10:04:48, Yang Shi wrote: [...] > One approach is to save all the vmas on a separate list, then zap_page_range > does unmap with this list. Just detached unmapped vma chain from mm. You can keep the existing vm_next chain and reuse it. -- Michal Hocko SUSE Labs
Re: [RFC v4 0/3] mm: zap pages with read mmap_sem in munmap for large mapping
On Wed, 11 Jul 2018 12:33:12 +0200 Michal Hocko wrote: > > Approach: > > Zapping pages is the most time consuming part, according to the suggestion > > from > > Michal Hocko [1], zapping pages can be done with holding read mmap_sem, like > > what MADV_DONTNEED does. Then re-acquire write mmap_sem to cleanup vmas. > > > > But, we can't call MADV_DONTNEED directly, since there are two major > > drawbacks: > > * The unexpected state from PF if it wins the race in the middle of > > munmap. > > It may return zero page, instead of the content or SIGSEGV. > > * Can’t handle VM_LOCKED | VM_HUGETLB | VM_PFNMAP and uprobe mappings, > > which > > is a showstopper from akpm > > I do not really understand why this is a showstopper. This is a mere > optimization. VM_LOCKED ranges are usually not that large. VM_HUGETLB > can be quite large alright but this should be doable on top. Is there > any reason to block any "cover most mappings first" patch? Somebody somewhere is going to want to unmap vast mlocked regions and they're going to report softlockup warnings. So we shouldn't implement something which can't address these cases. Maybe it doesn't do so in the first version, but we should at least have a plan to handle all cases.
Re: [RFC v4 0/3] mm: zap pages with read mmap_sem in munmap for large mapping
On 7/11/18 4:53 AM, Michal Hocko wrote: On Wed 11-07-18 14:13:12, Kirill A. Shutemov wrote: On Wed, Jul 11, 2018 at 12:33:12PM +0200, Michal Hocko wrote: this is not a small change for something that could be achieved from the userspace trivially (just call madvise before munmap - library can hide this). Most workloads will even not care about races because they simply do not play tricks with mmaps and userspace MM. So why do we want to put the additional complexity into the kernel? As I said before, kernel latency issues have to be addressed in kernel. We cannot rely on userspace being kind here. Those who really care and create really large mappings will know how to do this properly. Most others just do not care enough. So I am not really sure this alone is a sufficient argument. I personally like the in kernel auto tuning but as I've said the changelog should be really clear why all the complications are justified. This would be a lot easier to argue about if it was a simple if (len > THARSHOLD) do_madvise(DONTNEED) munmap(). The main difference AFAICS, is it can't deal with the parallel faults and those special mappings. Someone may not care about it, but someone may. Yang approach. But if we really have to care about parallel faults and munmap consitency this will always be tricky
Re: [RFC v4 0/3] mm: zap pages with read mmap_sem in munmap for large mapping
On 7/11/18 4:10 AM, Kirill A. Shutemov wrote: On Wed, Jul 11, 2018 at 07:34:06AM +0800, Yang Shi wrote: Background: Recently, when we ran some vm scalability tests on machines with large memory, we ran into a couple of mmap_sem scalability issues when unmapping large memory space, please refer to https://lkml.org/lkml/2017/12/14/733 and https://lkml.org/lkml/2018/2/20/576. History: Then akpm suggested to unmap large mapping section by section and drop mmap_sem at a time to mitigate it (see https://lkml.org/lkml/2018/3/6/784). V1 patch series was submitted to the mailing list per Andrew's suggestion (see https://lkml.org/lkml/2018/3/20/786). Then I received a lot great feedback and suggestions. Then this topic was discussed on LSFMM summit 2018. In the summit, Michal Hocko suggested (also in the v1 patches review) to try "two phases" approach. Zapping pages with read mmap_sem, then doing via cleanup with write mmap_sem (for discussion detail, see https://lwn.net/Articles/753269/) Approach: Zapping pages is the most time consuming part, according to the suggestion from Michal Hocko [1], zapping pages can be done with holding read mmap_sem, like what MADV_DONTNEED does. Then re-acquire write mmap_sem to cleanup vmas. But, we can't call MADV_DONTNEED directly, since there are two major drawbacks: * The unexpected state from PF if it wins the race in the middle of munmap. It may return zero page, instead of the content or SIGSEGV. * Can’t handle VM_LOCKED | VM_HUGETLB | VM_PFNMAP and uprobe mappings, which is a showstopper from akpm And, some part may need write mmap_sem, for example, vma splitting. So, the design is as follows: acquire write mmap_sem lookup vmas (find and split vmas) set VM_DEAD flags deal with special mappings downgrade_write zap pages release mmap_sem retake mmap_sem exclusively cleanup vmas release mmap_sem Define large mapping size thresh as PUD size, just zap pages with read mmap_sem for mappings which are >= PUD_SIZE. So, unmapping less than PUD_SIZE area still goes with the regular path. All vmas which will be zapped soon will have VM_DEAD flag set. Since PF may race with munmap, may just return the right content or SIGSEGV before the optimization, but with the optimization, it may return a zero page. Here use this flag to mark PF to this area is unstable, will trigger SIGSEGV, in order to prevent from the unexpected 3rd state. If the vma has VM_LOCKED | VM_HUGETLB | VM_PFNMAP or uprobe, they are considered as special mappings. They will be dealt with before zapping pages with write mmap_sem held. Basically, just update vm_flags. The actual unmapping is still done with read mmap_sem. And, since they are also manipulated by unmap_single_vma() which is called by zap_page_range() with read mmap_sem held in this case, to prevent from updating vm_flags in read critical section and considering the complexity of coding, just check if VM_DEAD is set, then skip any VM_DEAD area since they should be handled before. When cleaning up vmas, just call do_munmap() without carrying vmas from the above to avoid race condition, since the address space might be already changed under our feet after retaking exclusive lock. For the time being, just do this in munmap syscall path. Other vm_munmap() or do_munmap() call sites (i.e mmap, mremap, etc) remain intact for stability reason. And, make this 64 bit only explicitly per akpm's suggestion. I still see VM_DEAD as unnecessary complication. We should be fine without it. But looks like I'm in the minority :/ It's okay. I have another suggestion that also doesn't require VM_DEAD trick too :) 1. Take mmap_sem for write; 2. Adjust VMA layout (split/remove). After the step all memory we try to unmap is outside any VMA. 3. Downgrade mmap_sem to read. 4. Zap the page range. 5. Drop mmap_sem. I believe it should be safe. Yes, it looks so. But, a further question is all the vmas have been removed, how zap_page_range could do its job? It depends on the vmas. One approach is to save all the vmas on a separate list, then zap_page_range does unmap with this list. Yang The pages in the range cannot be re-faulted after step 3 as find_vma() will not see the corresponding VMA and deliver SIGSEGV. New VMAs cannot be created in the range before step 5 since we hold the semaphore at least for read the whole time. Do you see problem in this approach?
Re: [RFC v4 0/3] mm: zap pages with read mmap_sem in munmap for large mapping
On 7/11/18 3:33 AM, Michal Hocko wrote: On Wed 11-07-18 07:34:06, Yang Shi wrote: Background: Recently, when we ran some vm scalability tests on machines with large memory, we ran into a couple of mmap_sem scalability issues when unmapping large memory space, please refer to https://lkml.org/lkml/2017/12/14/733 and https://lkml.org/lkml/2018/2/20/576. History: Then akpm suggested to unmap large mapping section by section and drop mmap_sem at a time to mitigate it (see https://lkml.org/lkml/2018/3/6/784). V1 patch series was submitted to the mailing list per Andrew's suggestion (see https://lkml.org/lkml/2018/3/20/786). Then I received a lot great feedback and suggestions. Then this topic was discussed on LSFMM summit 2018. In the summit, Michal Hocko suggested (also in the v1 patches review) to try "two phases" approach. Zapping pages with read mmap_sem, then doing via cleanup with write mmap_sem (for discussion detail, see https://lwn.net/Articles/753269/) Approach: Zapping pages is the most time consuming part, according to the suggestion from Michal Hocko [1], zapping pages can be done with holding read mmap_sem, like what MADV_DONTNEED does. Then re-acquire write mmap_sem to cleanup vmas. But, we can't call MADV_DONTNEED directly, since there are two major drawbacks: * The unexpected state from PF if it wins the race in the middle of munmap. It may return zero page, instead of the content or SIGSEGV. * Can’t handle VM_LOCKED | VM_HUGETLB | VM_PFNMAP and uprobe mappings, which is a showstopper from akpm I do not really understand why this is a showstopper. This is a mere optimization. VM_LOCKED ranges are usually not that large. VM_HUGETLB can be quite large alright but this should be doable on top. Is there any reason to block any "cover most mappings first" patch? And, some part may need write mmap_sem, for example, vma splitting. So, the design is as follows: acquire write mmap_sem lookup vmas (find and split vmas) set VM_DEAD flags deal with special mappings downgrade_write zap pages release mmap_sem retake mmap_sem exclusively cleanup vmas release mmap_sem Please explain why dropping the lock and then ratake it to cleanup vmas is OK. This is really important because parallel thread could have changed the underlying address space range. Yes, the address space could be changed after retaking the lock. Actually, here do_munmap() is called in the new patch to do the cleanup work as Kirill suggested, which will re-lookup vmas and deal with any address space change. If there is no address space change, actually it just clean up vmas. Moreover include/linux/mm.h | 8 +++ include/linux/oom.h | 20 --- mm/huge_memory.c| 4 +- mm/hugetlb.c| 5 ++ mm/memory.c | 57 --- mm/mmap.c | 221 +- mm/shmem.c | 9 ++- 7 files changed, 255 insertions(+), 69 deletions(-) this is not a small change for something that could be achieved from the userspace trivially (just call madvise before munmap - library can hide this). Most workloads will even not care about races because they simply do not play tricks with mmaps and userspace MM. So why do we want to put the additional complexity into the kernel? Note that I am _not_ saying this is a wrong idea, we just need some pretty sounds arguments to justify the additional complexity which is mostly based on our fear that somebody might be doing something (half)insane or dubious at best. I agree with Kirill that we can't rely on sane userspace to handle kernel latency issue. Moreover, we even don't know if they are sane enough or not at all. Yang
Re: [RFC v4 0/3] mm: zap pages with read mmap_sem in munmap for large mapping
On Wed 11-07-18 14:10:52, Kirill A. Shutemov wrote: [...] > It's okay. I have another suggestion that also doesn't require VM_DEAD > trick too :) > > 1. Take mmap_sem for write; > 2. Adjust VMA layout (split/remove). After the step all memory we try to >unmap is outside any VMA. > 3. Downgrade mmap_sem to read. > 4. Zap the page range. > 5. Drop mmap_sem. > > I believe it should be safe. > > The pages in the range cannot be re-faulted after step 3 as find_vma() > will not see the corresponding VMA and deliver SIGSEGV. > > New VMAs cannot be created in the range before step 5 since we hold the > semaphore at least for read the whole time. > > Do you see problem in this approach? Yes this seems to be safe. At least from the first glance. -- Michal Hocko SUSE Labs
Re: [RFC v4 0/3] mm: zap pages with read mmap_sem in munmap for large mapping
On Wed 11-07-18 14:13:12, Kirill A. Shutemov wrote: > On Wed, Jul 11, 2018 at 12:33:12PM +0200, Michal Hocko wrote: > > this is not a small change for something that could be achieved > > from the userspace trivially (just call madvise before munmap - library > > can hide this). Most workloads will even not care about races because > > they simply do not play tricks with mmaps and userspace MM. So why do we > > want to put the additional complexity into the kernel? > > As I said before, kernel latency issues have to be addressed in kernel. > We cannot rely on userspace being kind here. Those who really care and create really large mappings will know how to do this properly. Most others just do not care enough. So I am not really sure this alone is a sufficient argument. I personally like the in kernel auto tuning but as I've said the changelog should be really clear why all the complications are justified. This would be a lot easier to argue about if it was a simple if (len > THARSHOLD) do_madvise(DONTNEED) munmap(). approach. But if we really have to care about parallel faults and munmap consitency this will always be tricky -- Michal Hocko SUSE Labs
Re: [RFC v4 0/3] mm: zap pages with read mmap_sem in munmap for large mapping
On Wed, Jul 11, 2018 at 12:33:12PM +0200, Michal Hocko wrote: > this is not a small change for something that could be achieved > from the userspace trivially (just call madvise before munmap - library > can hide this). Most workloads will even not care about races because > they simply do not play tricks with mmaps and userspace MM. So why do we > want to put the additional complexity into the kernel? As I said before, kernel latency issues have to be addressed in kernel. We cannot rely on userspace being kind here. -- Kirill A. Shutemov
Re: [RFC v4 0/3] mm: zap pages with read mmap_sem in munmap for large mapping
On Wed, Jul 11, 2018 at 07:34:06AM +0800, Yang Shi wrote: > > Background: > Recently, when we ran some vm scalability tests on machines with large memory, > we ran into a couple of mmap_sem scalability issues when unmapping large > memory > space, please refer to https://lkml.org/lkml/2017/12/14/733 and > https://lkml.org/lkml/2018/2/20/576. > > > History: > Then akpm suggested to unmap large mapping section by section and drop > mmap_sem > at a time to mitigate it (see https://lkml.org/lkml/2018/3/6/784). > > V1 patch series was submitted to the mailing list per Andrew's suggestion > (see https://lkml.org/lkml/2018/3/20/786). Then I received a lot great > feedback > and suggestions. > > Then this topic was discussed on LSFMM summit 2018. In the summit, Michal > Hocko > suggested (also in the v1 patches review) to try "two phases" approach. > Zapping > pages with read mmap_sem, then doing via cleanup with write mmap_sem (for > discussion detail, see https://lwn.net/Articles/753269/) > > > Approach: > Zapping pages is the most time consuming part, according to the suggestion > from > Michal Hocko [1], zapping pages can be done with holding read mmap_sem, like > what MADV_DONTNEED does. Then re-acquire write mmap_sem to cleanup vmas. > > But, we can't call MADV_DONTNEED directly, since there are two major > drawbacks: > * The unexpected state from PF if it wins the race in the middle of munmap. > It may return zero page, instead of the content or SIGSEGV. > * Can’t handle VM_LOCKED | VM_HUGETLB | VM_PFNMAP and uprobe mappings, which > is a showstopper from akpm > > And, some part may need write mmap_sem, for example, vma splitting. So, the > design is as follows: > acquire write mmap_sem > lookup vmas (find and split vmas) > set VM_DEAD flags > deal with special mappings > downgrade_write > > zap pages > release mmap_sem > > retake mmap_sem exclusively > cleanup vmas > release mmap_sem > > Define large mapping size thresh as PUD size, just zap pages with read > mmap_sem > for mappings which are >= PUD_SIZE. So, unmapping less than PUD_SIZE area > still > goes with the regular path. > > All vmas which will be zapped soon will have VM_DEAD flag set. Since PF may > race > with munmap, may just return the right content or SIGSEGV before the > optimization, > but with the optimization, it may return a zero page. Here use this flag to > mark > PF to this area is unstable, will trigger SIGSEGV, in order to prevent from > the > unexpected 3rd state. > > If the vma has VM_LOCKED | VM_HUGETLB | VM_PFNMAP or uprobe, they are > considered > as special mappings. They will be dealt with before zapping pages with write > mmap_sem held. Basically, just update vm_flags. The actual unmapping is still > done with read mmap_sem. > > And, since they are also manipulated by unmap_single_vma() which is called by > zap_page_range() with read mmap_sem held in this case, to prevent from > updating > vm_flags in read critical section and considering the complexity of coding, > just > check if VM_DEAD is set, then skip any VM_DEAD area since they should be > handled > before. > > When cleaning up vmas, just call do_munmap() without carrying vmas from the > above > to avoid race condition, since the address space might be already changed > under > our feet after retaking exclusive lock. > > For the time being, just do this in munmap syscall path. Other vm_munmap() or > do_munmap() call sites (i.e mmap, mremap, etc) remain intact for stability > reason. > And, make this 64 bit only explicitly per akpm's suggestion. I still see VM_DEAD as unnecessary complication. We should be fine without it. But looks like I'm in the minority :/ It's okay. I have another suggestion that also doesn't require VM_DEAD trick too :) 1. Take mmap_sem for write; 2. Adjust VMA layout (split/remove). After the step all memory we try to unmap is outside any VMA. 3. Downgrade mmap_sem to read. 4. Zap the page range. 5. Drop mmap_sem. I believe it should be safe. The pages in the range cannot be re-faulted after step 3 as find_vma() will not see the corresponding VMA and deliver SIGSEGV. New VMAs cannot be created in the range before step 5 since we hold the semaphore at least for read the whole time. Do you see problem in this approach? -- Kirill A. Shutemov
Re: [RFC v4 0/3] mm: zap pages with read mmap_sem in munmap for large mapping
On Wed 11-07-18 07:34:06, Yang Shi wrote: > > Background: > Recently, when we ran some vm scalability tests on machines with large memory, > we ran into a couple of mmap_sem scalability issues when unmapping large > memory > space, please refer to https://lkml.org/lkml/2017/12/14/733 and > https://lkml.org/lkml/2018/2/20/576. > > > History: > Then akpm suggested to unmap large mapping section by section and drop > mmap_sem > at a time to mitigate it (see https://lkml.org/lkml/2018/3/6/784). > > V1 patch series was submitted to the mailing list per Andrew's suggestion > (see https://lkml.org/lkml/2018/3/20/786). Then I received a lot great > feedback > and suggestions. > > Then this topic was discussed on LSFMM summit 2018. In the summit, Michal > Hocko > suggested (also in the v1 patches review) to try "two phases" approach. > Zapping > pages with read mmap_sem, then doing via cleanup with write mmap_sem (for > discussion detail, see https://lwn.net/Articles/753269/) > > > Approach: > Zapping pages is the most time consuming part, according to the suggestion > from > Michal Hocko [1], zapping pages can be done with holding read mmap_sem, like > what MADV_DONTNEED does. Then re-acquire write mmap_sem to cleanup vmas. > > But, we can't call MADV_DONTNEED directly, since there are two major > drawbacks: > * The unexpected state from PF if it wins the race in the middle of munmap. > It may return zero page, instead of the content or SIGSEGV. > * Can’t handle VM_LOCKED | VM_HUGETLB | VM_PFNMAP and uprobe mappings, which > is a showstopper from akpm I do not really understand why this is a showstopper. This is a mere optimization. VM_LOCKED ranges are usually not that large. VM_HUGETLB can be quite large alright but this should be doable on top. Is there any reason to block any "cover most mappings first" patch? > And, some part may need write mmap_sem, for example, vma splitting. So, the > design is as follows: > acquire write mmap_sem > lookup vmas (find and split vmas) > set VM_DEAD flags > deal with special mappings > downgrade_write > > zap pages > release mmap_sem > > retake mmap_sem exclusively > cleanup vmas > release mmap_sem Please explain why dropping the lock and then ratake it to cleanup vmas is OK. This is really important because parallel thread could have changed the underlying address space range. Moreover > include/linux/mm.h | 8 +++ > include/linux/oom.h | 20 --- > mm/huge_memory.c| 4 +- > mm/hugetlb.c| 5 ++ > mm/memory.c | 57 --- > mm/mmap.c | 221 > +- > mm/shmem.c | 9 ++- > 7 files changed, 255 insertions(+), 69 deletions(-) this is not a small change for something that could be achieved from the userspace trivially (just call madvise before munmap - library can hide this). Most workloads will even not care about races because they simply do not play tricks with mmaps and userspace MM. So why do we want to put the additional complexity into the kernel? Note that I am _not_ saying this is a wrong idea, we just need some pretty sounds arguments to justify the additional complexity which is mostly based on our fear that somebody might be doing something (half)insane or dubious at best. -- Michal Hocko SUSE Labs
