Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 8 Sep 2022 13:38:31 -0400
From:   Peter Xu <peterx@redhat.com>
To:     James Houghton <jthoughton@google.com>
Cc:     Mike Kravetz <mike.kravetz@oracle.com>,
        Muchun Song <songmuchun@bytedance.com>,
        David Hildenbrand <david@redhat.com>,
        David Rientjes <rientjes@google.com>,
        Axel Rasmussen <axelrasmussen@google.com>,
        Mina Almasry <almasrymina@google.com>,
        Jue Wang <juew@google.com>,
        Manish Mishra <manish.mishra@nutanix.com>,
        "Dr . David Alan Gilbert" <dgilbert@redhat.com>,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 07/26] hugetlb: add hugetlb_pte to track HugeTLB page
 table entries
Message-ID: <Yxool80IgkXnqS5Y@xz-m1.local>
References: <20220624173656.2033256-1-jthoughton@google.com>
 <20220624173656.2033256-8-jthoughton@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20220624173656.2033256-8-jthoughton@google.com>
Precedence: bulk

James,

On Fri, Jun 24, 2022 at 05:36:37PM +0000, James Houghton wrote:
> +static inline
> +spinlock_t *hugetlb_pte_lockptr(struct mm_struct *mm, struct hugetlb_pte *hpte)
> +{
> +
> +	BUG_ON(!hpte->ptep);
> +	// Only use huge_pte_lockptr if we are at leaf-level. Otherwise use
> +	// the regular page table lock.
> +	if (hugetlb_pte_none(hpte) || hugetlb_pte_present_leaf(hpte))
> +		return huge_pte_lockptr(hugetlb_pte_shift(hpte),
> +				mm, hpte->ptep);
> +	return &mm->page_table_lock;
> +}

Today when I re-read part of this thread, I found that I'm not sure whether
this is safe.  IIUC taking different locks depending on the state of pte
may lead to issues.

For example, could below race happen where two threads can be taking
different locks even if stumbled over the same pmd entry?

         thread 1                          thread 2
         --------                          --------

    hugetlb_pte_lockptr (for pmd level)
      pte_none()==true,
        take pmd lock
    pmd_alloc()
                                hugetlb_pte_lockptr (for pmd level)
                                  pte is pgtable entry (so !none, !present_leaf)
                                    take page_table_lock
                                (can run concurrently with thread 1...)
    pte_alloc()
    ...

-- 
Peter Xu