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=-2.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no 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 01314C1975A for ; Thu, 12 Mar 2020 17:02:24 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id C1B9D206F1 for ; Thu, 12 Mar 2020 17:02:23 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C1B9D206F1 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 45C216B000C; Thu, 12 Mar 2020 13:02:23 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 40CCB6B000D; Thu, 12 Mar 2020 13:02:23 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3231D6B000E; Thu, 12 Mar 2020 13:02:23 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0181.hostedemail.com [216.40.44.181]) by kanga.kvack.org (Postfix) with ESMTP id 169156B000C for ; Thu, 12 Mar 2020 13:02:23 -0400 (EDT) Received: from smtpin15.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id B3DA9181AEF39 for ; Thu, 12 Mar 2020 17:02:22 +0000 (UTC) X-FDA: 76587328524.15.title95_4ee9159fa8561 X-HE-Tag: title95_4ee9159fa8561 X-Filterd-Recvd-Size: 4804 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf33.hostedemail.com (Postfix) with ESMTP for ; Thu, 12 Mar 2020 17:02:22 +0000 (UTC) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 4EBA730E; Thu, 12 Mar 2020 10:02:21 -0700 (PDT) Received: from [10.57.15.252] (unknown [10.57.15.252]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id A670B3F6CF; Thu, 12 Mar 2020 10:02:19 -0700 (PDT) Subject: Re: [PATCH] mm/hmm: Simplify hmm_vma_walk_pud slightly To: Jason Gunthorpe Cc: Jerome Glisse , Ralph Campbell , "Felix.Kuehling@amd.com" , Philip Yang , John Hubbard , "amd-gfx@lists.freedesktop.org" , "linux-mm@kvack.org" , "dri-devel@lists.freedesktop.org" , Christoph Hellwig References: <5bd778fa-51e5-3e0c-d9bb-b38539b03c8d@arm.com> <20200312102813.56699-1-steven.price@arm.com> <20200312142749.GM31668@ziepe.ca> <58e296a6-d32b-bb37-28ce-ade0f784454d@arm.com> <20200312151113.GO31668@ziepe.ca> <689d3c56-3d19-4655-21f5-f9aeab3089df@arm.com> <20200312163734.GR31668@ziepe.ca> From: Steven Price Message-ID: Date: Thu, 12 Mar 2020 17:02:18 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <20200312163734.GR31668@ziepe.ca> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 12/03/2020 16:37, Jason Gunthorpe wrote: > On Thu, Mar 12, 2020 at 04:16:33PM +0000, Steven Price wrote: >>> Actually, while you are looking at this, do you think we should be >>> adding at least READ_ONCE in the pagewalk.c walk_* functions? The >>> multiple references of pmd, pud, etc without locking seems sketchy to >>> me. >> >> I agree it seems worrying. I'm not entirely sure whether the holding of >> mmap_sem is sufficient, > > I looked at this question, and at least for PMD, mmap_sem is not > sufficient. I didn't easilly figure it out for the other ones > > I'm guessing if PMD is not safe then none of them are. > >> this isn't something that I changed so I've just >> been hoping that it's sufficient since it seems to have been working >> (whether that's by chance because the compiler didn't generate multiple >> reads I've no idea). For walking the kernel's page tables the lack of >> READ_ONCE is also not great, but at least for PTDUMP we don't care too much >> about accuracy and it should be crash proof because there's no RCU grace >> period. And again the code I was replacing didn't have any special >> protection. >> >> I can't see any harm in updating the code to include READ_ONCE and I'm happy >> to review a patch. > > The reason I ask is because hmm's walkers often have this pattern > where they get the pointer and then de-ref it (again) then > immediately have to recheck the 'again' conditions of the walker > itself because the re-read may have given a different value. > > Having the walker deref the pointer and pass the value it into the ops > for use rather than repeatedly de-refing an unlocked value seems like > a much safer design to me. Yeah that sounds like a good idea. > If this also implicitly relies on a RCU grace period then it is also > missing RCU locking... True - I'm not 100% sure in what situations a page table entry can be freed. Anshuman has added locking to deal with memory hotplug[1]. I believe this is sufficient. [1] bf2b59f60ee1 ("arm64/mm: Hold memory hotplug lock while walking for kernel page table dump") > I also didn't quite understand why walk_pte_range() skipped locking > the pte in the no_vma case - I don't get why vma would be related to > locking here. The no_vma case is for walking the kernel's page tables and they may have entries that are not backed by struct page, so there isn't (reliably) a PTE lock to take. > I also saw that hmm open coded the pte walk, presumably for > performance, so I was thinking of adding some kind of pte_range() > callback to avoid the expensive indirect function call per pte, but > hmm also can't have the pmd locked... Yeah the callback per PTE is a bit heavy because of the indirect function call. I'm not sure how to optimise it beyond open coding at the PMD level. One option would be to provide helper functions to make it a bit more generic. Do you have an idea of what pte_range() would look like? Steve