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;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.117.160 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.117.160; helo=mail.nvidia.com; pr=C
Message-ID: <a3e05607-f165-04d1-29e0-f25bbaa1667e@nvidia.com>
Date:   Thu, 8 Dec 2022 13:50:17 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.1
Subject: Re: [PATCH v2 09/10] mm/hugetlb: Introduce hugetlb_walk()
Content-Language: en-US
To:     Peter Xu <peterx@redhat.com>
CC:     <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>,
        Jann Horn <jannh@google.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        James Houghton <jthoughton@google.com>,
        Rik van Riel <riel@surriel.com>,
        Miaohe Lin <linmiaohe@huawei.com>,
        Nadav Amit <nadav.amit@gmail.com>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        David Hildenbrand <david@redhat.com>,
        "Andrew Morton" <akpm@linux-foundation.org>,
        Muchun Song <songmuchun@bytedance.com>
References: <20221207203034.650899-1-peterx@redhat.com>
 <20221207203156.651077-1-peterx@redhat.com>
 <3b5579e8-8e51-a2e2-4c93-6709f8833723@nvidia.com> <Y5JQvElUNz6yGsrT@x1n>
From:   John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <Y5JQvElUNz6yGsrT@x1n>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Dec 2022 21:50:29.8970
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 0d9e549f-31fe-419a-95d3-08dad9663977
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.117.160];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: DM6NAM11FT040.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR12MB6561
X-Spam-Status: No, score=-1.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_NONE autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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