From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752721AbdFTJ5s (ORCPT ); Tue, 20 Jun 2017 05:57:48 -0400 Received: from LGEAMRELO11.lge.com ([156.147.23.51]:46980 "EHLO lgeamrelo11.lge.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752072AbdFTJ5n (ORCPT ); Tue, 20 Jun 2017 05:57:43 -0400 X-Original-SENDERIP: 156.147.1.121 X-Original-MAILFROM: minchan@kernel.org X-Original-SENDERIP: 10.177.220.163 X-Original-MAILFROM: minchan@kernel.org Date: Tue, 20 Jun 2017 18:57:40 +0900 From: Minchan Kim To: "Kirill A. Shutemov" Cc: Andrea Arcangeli , "Kirill A. Shutemov" , Andrew Morton , Vlastimil Babka , Vineet Gupta , Russell King , Will Deacon , Catalin Marinas , Ralf Baechle , "David S. Miller" , "Aneesh Kumar K . V" , Martin Schwidefsky , Heiko Carstens , linux-arch@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to track dirty/accessed bits Message-ID: <20170620095740.GC27702@bbox> References: <20170615145224.66200-1-kirill.shutemov@linux.intel.com> <20170615145224.66200-4-kirill.shutemov@linux.intel.com> <20170616030250.GA27637@bbox> <20170616131908.3rxtm2w73gdfex4a@node.shutemov.name> <20170616135209.GA29542@bbox> <20170616142720.GH11676@redhat.com> <20170616145333.GA29802@bbox> <20170619140322.iszk7sbhxblusygo@node.shutemov.name> <20170620025208.GB27702@bbox> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170620025208.GB27702@bbox> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jun 20, 2017 at 11:52:08AM +0900, Minchan Kim wrote: > Hello Kirill, > > On Mon, Jun 19, 2017 at 05:03:23PM +0300, Kirill A. Shutemov wrote: > > On Fri, Jun 16, 2017 at 11:53:33PM +0900, Minchan Kim wrote: > > > Hi Andrea, > > > > > > On Fri, Jun 16, 2017 at 04:27:20PM +0200, Andrea Arcangeli wrote: > > > > Hello Minchan, > > > > > > > > On Fri, Jun 16, 2017 at 10:52:09PM +0900, Minchan Kim wrote: > > > > > > > > @@ -1995,8 +1984,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd, > > > > > > > > if (soft_dirty) > > > > > > > > entry = pte_mksoft_dirty(entry); > > > > > > > > } > > > > > > > > - if (dirty) > > > > > > > > - SetPageDirty(page + i); > > > > > > > > pte = pte_offset_map(&_pmd, addr); > > > > [..] > > > > > > > > > > split_huge_page set PG_dirty to all subpages unconditionally? > > > > > If it's true, yes, it doesn't break MADV_FREE. However, I didn't spot > > > > > that piece of code. What I found one is just __split_huge_page_tail > > > > > which set PG_dirty to subpage if head page is dirty. IOW, if the head > > > > > page is not dirty, tail page will be clean, too. > > > > > Could you point out what routine set PG_dirty to all subpages unconditionally? > > > > When I wrote this code, I considered that we may want to track dirty > > status on per-4k basis for file-backed THPs. > > > > > > On a side note the snippet deleted above was useless, as long as > > > > there's one left hugepmd to split, the physical page has to be still > > > > compound and huge and as long as that's the case the tail pages > > > > PG_dirty bit is meaningless (even if set, it's going to be clobbered > > > > during the physical split). > > > > > > I got it during reviewing this patch. That's why I didn't argue > > > this patch would break MADV_FREE by deleting routine which propagate > > > dirty to pte of subpages. However, although it's useless, I prefer > > > not removing the transfer of dirty bit. Because it would help MADV_FREE > > > users who want to use smaps to know how many of pages are not freeable > > > (i.e, dirtied) since MADV_FREE although it is not 100% correct. > > > > > > > > > > > In short PG_dirty is only meaningful in the head as long as it's > > > > compound. The physical split in __split_huge_page_tail transfer the > > > > head value to the tails like you mentioned, that's all as far as I can > > > > tell. > > > > > > Thanks for the comment. Then, this patch is to fix MADV_FREE's bug > > > which has lost dirty bit by transferring dirty bit too early. > > > > Erghh. I've misread splitting code. Yes, it's not unconditional. So we fix > > actual bug. > > > > But I'm not sure it's subject for -stable. I haven't seen any bug reports > > that can be attributed to the bug. > > Okay, I'm not against but please rewrite changelog to indicate it fixes > the problem. One more thing, as I mentioned, I don't want to remove > pmd dirty bit -> PG_dirty propagate to subpage part because it would be > helpful for MADV_FREE users. Oops, I misread smap accouting code so no problem to remove useless propagation part I added for MADV_FREE. Thanks. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Minchan Kim Subject: Re: [PATCHv2 3/3] mm: Use updated pmdp_invalidate() inteface to track dirty/accessed bits Date: Tue, 20 Jun 2017 18:57:40 +0900 Message-ID: <20170620095740.GC27702@bbox> References: <20170615145224.66200-1-kirill.shutemov@linux.intel.com> <20170615145224.66200-4-kirill.shutemov@linux.intel.com> <20170616030250.GA27637@bbox> <20170616131908.3rxtm2w73gdfex4a@node.shutemov.name> <20170616135209.GA29542@bbox> <20170616142720.GH11676@redhat.com> <20170616145333.GA29802@bbox> <20170619140322.iszk7sbhxblusygo@node.shutemov.name> <20170620025208.GB27702@bbox> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20170620025208.GB27702@bbox> Sender: owner-linux-mm@kvack.org To: "Kirill A. Shutemov" Cc: Andrea Arcangeli , "Kirill A. Shutemov" , Andrew Morton , Vlastimil Babka , Vineet Gupta , Russell King , Will Deacon , Catalin Marinas , Ralf Baechle , "David S. Miller" , "Aneesh Kumar K . V" , Martin Schwidefsky , Heiko Carstens , linux-arch@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org List-Id: linux-arch.vger.kernel.org On Tue, Jun 20, 2017 at 11:52:08AM +0900, Minchan Kim wrote: > Hello Kirill, > > On Mon, Jun 19, 2017 at 05:03:23PM +0300, Kirill A. Shutemov wrote: > > On Fri, Jun 16, 2017 at 11:53:33PM +0900, Minchan Kim wrote: > > > Hi Andrea, > > > > > > On Fri, Jun 16, 2017 at 04:27:20PM +0200, Andrea Arcangeli wrote: > > > > Hello Minchan, > > > > > > > > On Fri, Jun 16, 2017 at 10:52:09PM +0900, Minchan Kim wrote: > > > > > > > > @@ -1995,8 +1984,6 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd, > > > > > > > > if (soft_dirty) > > > > > > > > entry = pte_mksoft_dirty(entry); > > > > > > > > } > > > > > > > > - if (dirty) > > > > > > > > - SetPageDirty(page + i); > > > > > > > > pte = pte_offset_map(&_pmd, addr); > > > > [..] > > > > > > > > > > split_huge_page set PG_dirty to all subpages unconditionally? > > > > > If it's true, yes, it doesn't break MADV_FREE. However, I didn't spot > > > > > that piece of code. What I found one is just __split_huge_page_tail > > > > > which set PG_dirty to subpage if head page is dirty. IOW, if the head > > > > > page is not dirty, tail page will be clean, too. > > > > > Could you point out what routine set PG_dirty to all subpages unconditionally? > > > > When I wrote this code, I considered that we may want to track dirty > > status on per-4k basis for file-backed THPs. > > > > > > On a side note the snippet deleted above was useless, as long as > > > > there's one left hugepmd to split, the physical page has to be still > > > > compound and huge and as long as that's the case the tail pages > > > > PG_dirty bit is meaningless (even if set, it's going to be clobbered > > > > during the physical split). > > > > > > I got it during reviewing this patch. That's why I didn't argue > > > this patch would break MADV_FREE by deleting routine which propagate > > > dirty to pte of subpages. However, although it's useless, I prefer > > > not removing the transfer of dirty bit. Because it would help MADV_FREE > > > users who want to use smaps to know how many of pages are not freeable > > > (i.e, dirtied) since MADV_FREE although it is not 100% correct. > > > > > > > > > > > In short PG_dirty is only meaningful in the head as long as it's > > > > compound. The physical split in __split_huge_page_tail transfer the > > > > head value to the tails like you mentioned, that's all as far as I can > > > > tell. > > > > > > Thanks for the comment. Then, this patch is to fix MADV_FREE's bug > > > which has lost dirty bit by transferring dirty bit too early. > > > > Erghh. I've misread splitting code. Yes, it's not unconditional. So we fix > > actual bug. > > > > But I'm not sure it's subject for -stable. I haven't seen any bug reports > > that can be attributed to the bug. > > Okay, I'm not against but please rewrite changelog to indicate it fixes > the problem. One more thing, as I mentioned, I don't want to remove > pmd dirty bit -> PG_dirty propagate to subpage part because it would be > helpful for MADV_FREE users. Oops, I misread smap accouting code so no problem to remove useless propagation part I added for MADV_FREE. Thanks. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org