From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from canpmsgout06.his.huawei.com (canpmsgout06.his.huawei.com [113.46.200.221]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 386442B9BA for ; Wed, 5 Nov 2025 01:04:54 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=113.46.200.221 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1762304698; cv=none; b=ar136rYaxdZaFICPjQKkRr3YdR8j4+aQHiCumcU7NebUH3k9fm9r9EHM4vYb7LI1sVZtmPm1ziN6OvBeCmA8K4CRm+pzM9zjNKe19MaKzsb7+giBlobeAz/jKBTZ394GlVrDOexEj73hcNxME3CYW2ylW5QTvF1mjpnt9Rz3Hvw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1762304698; c=relaxed/simple; bh=UXn4iY2R/X0kBs5ZUFjV3YRJpoVod5+1VOjzkKrq2dM=; h=Message-ID:Date:MIME-Version:Subject:To:CC:References:From: In-Reply-To:Content-Type; b=B/8jvV9ZF2Fr6Dvv1+HQHQYMuzPQb2S29MASQh7uQIVm0lmcQUkct+XyF1U/SpNrNVPDtMFtM9u3jmPrvKaKhtdLk8LTmsF2vFVXNIdDp+rrQb6yjbswCISvn0+/uUUbrjrtyg7p9d18U6wLqfha37+tCJYI3nWZ2/Pgm9PbUVw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=huawei.com; spf=pass smtp.mailfrom=huawei.com; dkim=pass (1024-bit key) header.d=huawei.com header.i=@huawei.com header.b=YyPBOKz8; arc=none smtp.client-ip=113.46.200.221 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=huawei.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=huawei.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=huawei.com header.i=@huawei.com header.b="YyPBOKz8" dkim-signature: v=1; a=rsa-sha256; d=huawei.com; s=dkim; c=relaxed/relaxed; q=dns/txt; h=From; bh=CwhJO6D1YRqJyWqNTP219TL5jK04SEJ2/5eraLPP68E=; b=YyPBOKz8KlSTPlZmtrXSfd1exx5/7AM8+l+NIdMItrrelTblzNAr/vAe6rMUT2v10CWlSjnPd NSg5NGaqmsITjCQhW0kn3dQHSzSKg+hObriQq4alXHUBlo5BaSaLrfJTjaw2L06a4mvCxeUaLoS hTy5terBnIr/JV3RPhkoql0= Received: from mail.maildlp.com (unknown [172.19.163.48]) by canpmsgout06.his.huawei.com (SkyGuard) with ESMTPS id 4d1Rrw2HVYzRhxs; Wed, 5 Nov 2025 09:03:12 +0800 (CST) Received: from dggpemf100008.china.huawei.com (unknown [7.185.36.138]) by mail.maildlp.com (Postfix) with ESMTPS id CE120180080; Wed, 5 Nov 2025 09:04:46 +0800 (CST) Received: from [10.174.177.243] (10.174.177.243) by dggpemf100008.china.huawei.com (7.185.36.138) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1544.11; Wed, 5 Nov 2025 09:04:45 +0800 Message-ID: <545c6e40-db22-4964-868a-74893d6ad03f@huawei.com> Date: Wed, 5 Nov 2025 09:04:44 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4] mm: use per_vma lock for MADV_DONTNEED To: Lorenzo Stoakes CC: Barry Song <21cnbao@gmail.com>, , , Suren Baghdasaryan , , Barry Song , "Liam R. Howlett" , David Hildenbrand , Vlastimil Babka , Jann Horn , Lokesh Gidra , Tangquan Zheng , Qi Zheng References: <20250607220150.2980-1-21cnbao@gmail.com> <564941f2-b538-462a-ac55-f38d3e8a6f2e@lucifer.local> <7998c1f1-fd53-45e9-b748-55043522f1c7@lucifer.local> Content-Language: en-US From: Kefeng Wang In-Reply-To: <7998c1f1-fd53-45e9-b748-55043522f1c7@lucifer.local> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-ClientProxiedBy: kwepems200002.china.huawei.com (7.221.188.68) To dggpemf100008.china.huawei.com (7.185.36.138) On 2025/11/4 23:21, Lorenzo Stoakes wrote: > On Tue, Nov 04, 2025 at 08:09:58PM +0800, Kefeng Wang wrote: >> >> >> On 2025/11/4 17:01, Lorenzo Stoakes wrote: >>> On Tue, Nov 04, 2025 at 04:34:35PM +0800, Kefeng Wang wrote: >>>>> +static enum madvise_lock_mode get_lock_mode(struct madvise_behavior *madv_behavior) >>>>> { >>>>> + int behavior = madv_behavior->behavior; >>>>> + >>>>> if (is_memory_failure(behavior)) >>>>> - return 0; >>>>> + return MADVISE_NO_LOCK; >>>>> - if (madvise_need_mmap_write(behavior)) { >>>>> + switch (behavior) { >>>>> + case MADV_REMOVE: >>>>> + case MADV_WILLNEED: >>>>> + case MADV_COLD: >>>>> + case MADV_PAGEOUT: >>>>> + case MADV_FREE: >>>>> + case MADV_POPULATE_READ: >>>>> + case MADV_POPULATE_WRITE: >>>>> + case MADV_COLLAPSE: >>>>> + case MADV_GUARD_INSTALL: >>>>> + case MADV_GUARD_REMOVE: >>>>> + return MADVISE_MMAP_READ_LOCK; >>>>> + case MADV_DONTNEED: >>>>> + case MADV_DONTNEED_LOCKED: >>>>> + return MADVISE_VMA_READ_LOCK; >>>> >>>> I have a question, we will try per-vma lock for dontneed, >>>> but there is a mmap_assert_locked() during madvise_dontneed_free(), >>> >>> Hmm, this is only in the THP PUD huge case, and MADV_FREE is only valid for >>> anonymous memory, and I think only DAX can have some weird THP PUD case. >>> >>> So I don't think we can hit this. >> >> Yes, we don't support pud THP for anonymous pages. > > Right, so we can't hit this. > >> >>> >>> In any event, I think this mmap_assert_locked() is mistaken, as we should >>> only need a VMA lock here. >>> >>> So we could replace with a: >>> >>> if (!rwsem_is_locked(&tlb->mm->mmap_lock)) >>> vma_assert_locked(vma); >>> >>> ? >>> >> >> The pmd dax/anon split don't have assert, for PUD dax, we maybe remove this >> assert? > > Well, we probably do want to assert that we hold a lock. OK, let's convert to vma_assert_locked. > >> >> >> >> >>>> >>>> madvise_dontneed_free >>>> madvise_dontneed_single_vma >>>> zap_page_range_single_batched >>>> unmap_single_vma >>>> unmap_page_range >>>> zap_pud_range >>>> mmap_assert_locked >>>> >>>> We could fix it by passing the lock_mode into zap_detial and then check >>>> the right lock here, but I'm not sure whether it is safe to zap page >>>> only with vma lock? >>> >>> It's fine to zap with the VMA lock. You need only hold the VMA stable which >>> a VMA lock achieves. >>> >>> See https://docs.kernel.org/mm/process_addrs.html >> >> Thanks, I will learn it. > > Hopefully useful, I made it to remind myself of these things as they're very > fiddly + otherwise I find myself constantly forgetting these details :) That should be definitely useful :) > >> >>> >>>> >>>> And another about 4f8ba33bbdfc （"mm: madvise: use per_vma lock >>>> for MADV_FREE"）, it called walk_page_range_vma() in >>>> madvise_free_single_vma(), but from link[1] and 5631da56c9a8 >>>> ("fs/proc/task_mmu: read proc/pid/maps under per-vma lock"), it saids >>>> >>>> "Note that similar approach would not work for /proc/pid/smaps >>>> reading as it also walks the page table and that's not RCU-safe" >>>> >>>> We could use walk_page_range_vma() instead of walk_page_range() in >>>> smap_gather_stats(), and same question, why 4f8ba33bbdfc(for MADV_FREEE) >>>> is safe but not for show_numa_map()/show_smap()? >>> >>> We only use walk_page_range() there in case 4 listed in show_smaps_rollup() >>> where the mmap lock is dropped on contention. >> >> Sorry, I mean the walk_page_range() in smap_gather_stats() called by >> show_smap() from /proc/pid/smaps, not the walk_page_range() in >> show_smaps_rollup() from /proc/pid/smaps_rollup. > > show_smaps() > -> smap_gather_stats(..., start = 0) > -> walk_page_vma() > > Because: > > if (!start) > walk_page_vma(vma, ops, mss); > > The only case where start is non-zero is show_smaps_rollup() case 4. So we are > already using walk_page_vma() here right? > > I may be missing something here :) You are right, I don't check start value :( > >> >> >>> >>>> >>>> Thanks. >>>> >>>> [1] https://lkml.kernel.org/r/20250719182854.3166724-1-surenb@google.com >>> >>> AFAICT That's referring to a previous approach that tried to walk >>> /proc/$pid/swaps under RCU _alone_ without VMA locks. This is not safe as >>> page tables can be yanked from under you not under RCU. >> >> But for now it tries per-vma lock or fallback to mmap lock, not lockless, so >> do you mean we could try per-vma lock for /proc/pid/numa_maps or >> /proc/pid/smaps ? > > Probably we could, but I'm not sure if it'd be really worth it, since traversing > page tables is a very heavy operation and so optimising it against contention > like this seems probably not all that worth it? > > Suren maybe could comment on this. They only operate a single vma(walk_page_vma), I think it is always better if we could only hold the vma lock, but wait for Suren comment on it.