linux-kernel - Re: [PATCH v2 09/10] mm/hugetlb: Introduce hugetlb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a3e05607-f165-04d1-29e0-f25bbaa1667e@nvidia.com>
Date:   Thu, 8 Dec 2022 13:50:17 -0800
From:   John Hubbard <jhubbard@...dia.com>
To:     Peter Xu <peterx@...hat.com>
CC:     <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
        Jann Horn <jannh@...gle.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        James Houghton <jthoughton@...gle.com>,
        Rik van Riel <riel@...riel.com>,
        Miaohe Lin <linmiaohe@...wei.com>,
        Nadav Amit <nadav.amit@...il.com>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        David Hildenbrand <david@...hat.com>,
        "Andrew Morton" <akpm@...ux-foundation.org>,
        Muchun Song <songmuchun@...edance.com>
Subject: Re: [PATCH v2 09/10] mm/hugetlb: Introduce hugetlb_walk()

On 12/8/22 13:01, Peter Xu wrote:
>> At this point, it is very clear that huge_pte_offset() should be renamed.
>> I'd suggest something like one of these:
>>
>>      __hugetlb_walk()
>>      hugetlb_walk_raw()
> 
> We can.
> 
> Not only because that's an arch api for years (didn't want to touch more
> arch code unless necessary), but also since we have hugetlb_walk() that'll
> be the future interface not huge_pte_offset().
> 
> Actually it's good when that's the only thing people can find from its name
> when they want to have a huge pgtable walk. :)
> 
> So totally makes sense to do so, but I don't strongly feel like doing it in
> this patchset if you're okay with it.
> 

Sounds good.

>>
>>> +static inline pte_t *
>>> +hugetlb_walk(struct vm_area_struct *vma, unsigned long addr, unsigned long sz)
>>> +{
>>> +#if defined(CONFIG_HUGETLB_PAGE) && \
>>> +	defined(CONFIG_ARCH_WANT_HUGE_PMD_SHARE) && defined(CONFIG_LOCKDEP)
>>> +	struct hugetlb_vma_lock *vma_lock = vma->vm_private_data;
>>> +
>>> +	/*
>>> +	 * If pmd sharing possible, locking needed to safely walk the
>>> +	 * hugetlb pgtables.  More information can be found at the comment
>>> +	 * above huge_pte_offset() in the same file.
>>> +	 *
>>> +	 * NOTE: lockdep_is_held() is only defined with CONFIG_LOCKDEP.
>>> +	 */
>>> +	if (__vma_shareable_flags_pmd(vma))
>>> +		WARN_ON_ONCE(!lockdep_is_held(&vma_lock->rw_sema) &&
>>> +			     !lockdep_is_held(
>>> +				 &vma->vm_file->f_mapping->i_mmap_rwsem));
>>> +#endif
>>> +	return huge_pte_offset(vma->vm_mm, addr, sz);
>>> +}
>>
>> Let's please not slice up C functions with ifdefs. Instead, stick to the
>> standard approach of
>>
>> #ifdef X
>> functionC()
>> {
>> 	...implementation
>> }
>> #else
>> functionC()
>> {
>> 	...simpler or shorter or stub implementation
>> }
> 
> Personally I like the slicing because it clearly tells what's the
> difference with/without the macros defined.
> 

Ha, I think you have a higher tolerance for complexity on the screen.
The fact that you can see more of that complexity at once, is what
slows down human readers.

So when someone is working through the code, if they can look at one
config at a time, that's shorter and cleaner. This is why folks (I'm
very much not the only one) have this common pattern.

However, of course I won't insist here, as there are clearly preferences
in both directions. And the code is still small in either form in this
case so really a non-issue.

thanks,
-- 
John Hubbard
NVIDIA