From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751162AbcEHGDz (ORCPT ); Sun, 8 May 2016 02:03:55 -0400 Received: from szxga01-in.huawei.com ([58.251.152.64]:24575 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751002AbcEHGDy (ORCPT ); Sun, 8 May 2016 02:03:54 -0400 Message-ID: <572ED69C.2000800@huawei.com> Date: Sun, 8 May 2016 14:03:08 +0800 From: zhouchengming User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20120428 Thunderbird/12.0.1 MIME-Version: 1.0 To: Andrea Arcangeli CC: , , , , , , , , , , , , Subject: Re: [PATCH v2] ksm: fix conflict between mmput and scan_get_next_rmap_item References: <1462505256-37301-1-git-send-email-zhouchengming1@huawei.com> <20160506142431.GA4855@redhat.com> In-Reply-To: <20160506142431.GA4855@redhat.com> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.177.236.183] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A090202.572ED6AB.007A,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: c9c943b8bc0e78bb725b889346affcba Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2016/5/6 22:24, Andrea Arcangeli wrote: > On Fri, May 06, 2016 at 11:27:36AM +0800, Zhou Chengming wrote: >> @@ -1650,16 +1647,22 @@ next_mm: >> */ >> hash_del(&slot->link); >> list_del(&slot->mm_list); >> - spin_unlock(&ksm_mmlist_lock); >> >> free_mm_slot(slot); >> clear_bit(MMF_VM_MERGEABLE,&mm->flags); >> up_read(&mm->mmap_sem); >> mmdrop(mm); >> } else { >> - spin_unlock(&ksm_mmlist_lock); >> up_read(&mm->mmap_sem); >> } >> + /* >> + * up_read(&mm->mmap_sem) first because after >> + * spin_unlock(&ksm_mmlist_lock) run, the "mm" may >> + * already have been freed under us by __ksm_exit() >> + * because the "mm_slot" is still hashed and >> + * ksm_scan.mm_slot doesn't point to it anymore. >> + */ >> + spin_unlock(&ksm_mmlist_lock); >> >> /* Repeat until we've completed scanning the whole list */ >> slot = ksm_scan.mm_slot; > > Reviewed-by: Andrea Arcangeli > > While the above patch is correct, I would however prefer if you could > update it to keep releasing the ksm_mmlist_lock as before (I'm talking > only about the quoted part, not the other one not quoted), because > it's "strictier" and it better documents that it's only needed up > until: > > hash_del(&slot->link); > list_del(&slot->mm_list); > > It should be also a bit more scalable but to me this is just about > keeping implicit documentation on the locking by keeping it strict. > > The fact up_read happens exactly after clear_bit also avoided me to > overlook that it was really needed, same thing with the > ksm_mmlist_lock after list_del, I'd like to keep it there and just > invert the order of spin_unlock; up_read in the else branch. Thanks a lot for your review and comment. It's my fault to misunderstand your last reply. Yes it's better and more scalable to just invert the order of spin_unlock/up_read in the else branch. And it's also enough. Thanks! > > That should be enough because after hash_del get_mm_slot will return > NULL so the mmdrop will not happen anymore in __ksm_exit, this is > further explicit by the code doing mmdrop itself just after > up_read. > > The SMP race condition is fixed by just the two liner that reverse the > order of spin_unlock; up_read without increasing the size of the > spinlock critical section for the ksm_scan.address == 0 case. This is > also why it wasn't reproducible because it's about 1 instruction window. > > Thanks! > Andrea > > . >