From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY, USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A4A18C433E0 for ; Wed, 12 Aug 2020 09:27:26 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 6FE04206C3 for ; Wed, 12 Aug 2020 09:27:26 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="d/xpxRpj"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=mediatek.com header.i=@mediatek.com header.b="FKy0fCxb" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6FE04206C3 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=mediatek.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Date:To:From: Subject:Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=YxSM3WXcDZvuguyoiTzsRb8tZjU0F1vHseO5MVl9GgU=; b=d/xpxRpjdL5L3kpt1czNx5g/q FZwoWaO8e5AE0ovgdejyofwYFTctowokhj/qVCAZvbQUREKYF5bSjzGxCs/ckISPySBsdu+xgOYHs aK4QoAm+wkdN/N9fjD0x0oNtuoSMnhs3y8AP2UPEFlHeYMEA2Iqg/i+ARU1fCrlDnBi8aWLltZtVo 3M2cWbHmwJT+6ibZpW/fjSb+JZ3Z75lpxwPf2K7RVNkC3xDdBjQb5FLWQV2G+xv7uesNXqDhPXLVs v4kvPcTcgO8cmZ6ELLQM1BWRzWRW2M7Z3NstrT3J01AtoB33FcvKBtmPzK1bZCIEPTGzzJ88rGdVC LJrTfpAMw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1k5n1S-0003q8-9Q; Wed, 12 Aug 2020 09:26:14 +0000 Received: from mailgw02.mediatek.com ([216.200.240.185]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1k5n1P-0003oS-Ik; Wed, 12 Aug 2020 09:26:12 +0000 X-UUID: bc12796cab914a4ca68cc4a6f0177629-20200812 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=mediatek.com; s=dk; h=Content-Transfer-Encoding:MIME-Version:Content-Type:References:In-Reply-To:Date:CC:To:From:Subject:Message-ID; bh=ubj2ydOGUWBxq24rYpXstL71ZWF/3bPYbt3glBglWZg=; b=FKy0fCxbi/CMJ+6HIO8CwF5mNN0ozbsXxyo4cRBgzCYeA5DolJwlwhRmfJMY0DJzF4tWVbdnfNM8vcOKSKTsa13V7XNLbi0E6ZBbI/hxaxiGWlLstEk5qDwbw5bBgFsc6o0aV7sa7zzjw4lPjrchcXGZf23PipL1rM4+WcICSKk=; X-UUID: bc12796cab914a4ca68cc4a6f0177629-20200812 Received: from mtkcas66.mediatek.inc [(172.29.193.44)] by mailgw02.mediatek.com (envelope-from ) (musrelay.mediatek.com ESMTP with TLS) with ESMTP id 1441867781; Wed, 12 Aug 2020 01:26:05 -0800 Received: from MTKMBS01N1.mediatek.inc (172.21.101.68) by MTKMBS62N2.mediatek.inc (172.29.193.42) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Wed, 12 Aug 2020 02:26:03 -0700 Received: from MTKCAS06.mediatek.inc (172.21.101.30) by mtkmbs01n1.mediatek.inc (172.21.101.68) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Wed, 12 Aug 2020 17:26:01 +0800 Received: from [172.21.77.33] (172.21.77.33) by MTKCAS06.mediatek.inc (172.21.101.73) with Microsoft SMTP Server id 15.0.1497.2 via Frontend Transport; Wed, 12 Aug 2020 17:26:01 +0800 Message-ID: <1597224363.32469.12.camel@mtkswgap22> Subject: Re: [PATCH 2/2] mm: proc: smaps_rollup: do not stall write attempts on mmap_lock From: Chinwen Chang To: Steven Price Date: Wed, 12 Aug 2020 17:26:03 +0800 In-Reply-To: References: <1597120955-16495-1-git-send-email-chinwen.chang@mediatek.com> <1597120955-16495-3-git-send-email-chinwen.chang@mediatek.com> X-Mailer: Evolution 3.2.3-0ubuntu6 MIME-Version: 1.0 X-MTK: N X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200812_052611_820763_5C594C16 X-CRM114-Status: GOOD ( 32.76 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-arm-kernel@lists.infradead.org, Song Liu , wsd_upstream@mediatek.com, Davidlohr Bueso , linux-kernel@vger.kernel.org, "Matthew Wilcox \(Oracle\)" , Daniel Jordan , Jason Gunthorpe , linux-mediatek@lists.infradead.org, Jimmy Assarsson , Huang Ying , Matthias Brugger , linux-fsdevel@vger.kernel.org, Andrew Morton , Michel Lespinasse , Alexey Dobriyan , Vlastimil Babka Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, 2020-08-12 at 09:39 +0100, Steven Price wrote: > On 11/08/2020 05:42, Chinwen Chang wrote: > > smaps_rollup will try to grab mmap_lock and go through the whole vma > > list until it finishes the iterating. When encountering large processes, > > the mmap_lock will be held for a longer time, which may block other > > write requests like mmap and munmap from progressing smoothly. > > > > There are upcoming mmap_lock optimizations like range-based locks, but > > the lock applied to smaps_rollup would be the coarse type, which doesn't > > avoid the occurrence of unpleasant contention. > > > > To solve aforementioned issue, we add a check which detects whether > > anyone wants to grab mmap_lock for write attempts. > > > > Signed-off-by: Chinwen Chang > > --- > > fs/proc/task_mmu.c | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c > > index dbda449..4b51f25 100644 > > --- a/fs/proc/task_mmu.c > > +++ b/fs/proc/task_mmu.c > > @@ -856,6 +856,27 @@ static int show_smaps_rollup(struct seq_file *m, void *v) > > for (vma = priv->mm->mmap; vma; vma = vma->vm_next) { > > smap_gather_stats(vma, &mss); > > last_vma_end = vma->vm_end; > > + > > + /* > > + * Release mmap_lock temporarily if someone wants to > > + * access it for write request. > > + */ > > + if (mmap_lock_is_contended(mm)) { > > + mmap_read_unlock(mm); > > + ret = mmap_read_lock_killable(mm); > > + if (ret) { > > + release_task_mempolicy(priv); > > + goto out_put_mm; > > + } > > + > > + /* Check whether current vma is available */ > > + vma = find_vma(mm, last_vma_end - 1); > > + if (vma && vma->vm_start < last_vma_end) > > I may be wrong, but this looks like it could return incorrect results. > For example if we start reading with the following VMAs: > > +------+------+-----------+ > | VMA1 | VMA2 | VMA3 | > +------+------+-----------+ > | | | | > 4k 8k 16k 400k > > Then after reading VMA2 we drop the lock due to contention. So: > > last_vma_end = 16k > > Then if VMA2 is freed while the lock is dropped, so we have: > > +------+ +-----------+ > | VMA1 | | VMA3 | > +------+ +-----------+ > | | | | > 4k 8k 16k 400k > > find_vma(mm, 16k-1) will then return VMA3 and the condition vm_start < > last_vma_end will be false. > Hi Steve, Thank you for reviewing this patch. You are correct. If the contention is detected and the current vma(here is VMA2) is freed while the lock is dropped, it will report an incomplete result. > > + continue; > > + > > + /* Current vma is not available, just break */ > > + break; > > Which means we break out here and report an incomplete output (the > numbers will be much smaller than reality). > > Would it be better to have a loop like: > > for (vma = priv->mm->mmap; vma;) { > smap_gather_stats(vma, &mss); > last_vma_end = vma->vm_end; > > if (contended) { > /* drop/acquire lock */ > > vma = find_vma(mm, last_vma_end - 1); > if (!vma) > break; > if (vma->vm_start >= last_vma_end) > continue; > } > vma = vma->vm_next; > } > > that way if the VMA is removed while the lock is dropped the loop can > just continue from the next VMA. > Thanks a lot for your great suggestion. > Or perhaps I missed something obvious? I haven't actually tested > anything above. > > Steve I will prepare new patch series for further reviews. Thank you. Chinwen > > > + } > > } > > > > show_vma_header_prefix(m, priv->mm->mmap->vm_start, > > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel