We can avoid unneeded WRITE_ONCE() overhead by using LIST_HEAD() to define
a list head.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/hugetlb.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 24f580ddf130..b3e6592247a3 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -459,14 +459,12 @@ static int allocate_file_region_entries(struct resv_map *resv,
 					int regions_needed)
 	__must_hold(&resv->lock)
 {
-	struct list_head allocated_regions;
+	LIST_HEAD(allocated_regions);
 	int to_allocate = 0, i = 0;
 	struct file_region *trg = NULL, *rg = NULL;
 
 	VM_BUG_ON(regions_needed < 0);
 
-	INIT_LIST_HEAD(&allocated_regions);
-
 	/*
 	 * Check for sufficient descriptors in the cache to accommodate
 	 * the number of in progress add operations plus regions_needed.
@@ -2352,7 +2350,7 @@ struct page *alloc_huge_page_vma(struct hstate *h, struct vm_area_struct *vma,
 static int gather_surplus_pages(struct hstate *h, long delta)
 	__must_hold(&hugetlb_lock)
 {
-	struct list_head surplus_list;
+	LIST_HEAD(surplus_list);
 	struct page *page, *tmp;
 	int ret;
 	long i;
@@ -2367,7 +2365,6 @@ static int gather_surplus_pages(struct hstate *h, long delta)
 	}
 
 	allocated = 0;
-	INIT_LIST_HEAD(&surplus_list);
 
 	ret = -ENOMEM;
 retry:
-- 
2.23.0

> On Aug 26, 2022, at 17:24, Miaohe Lin <linmiaohe@huawei.com> wrote:
> 
> We can avoid unneeded WRITE_ONCE() overhead by using LIST_HEAD() to define
> a list head.

IIUC, the overhead doesn’t change. Right?

I’m fine with your changes.

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks.

> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
> mm/hugetlb.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 24f580ddf130..b3e6592247a3 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -459,14 +459,12 @@ static int allocate_file_region_entries(struct resv_map *resv,
> 					int regions_needed)
> 	__must_hold(&resv->lock)
> {
> -	struct list_head allocated_regions;
> +	LIST_HEAD(allocated_regions);
> 	int to_allocate = 0, i = 0;
> 	struct file_region *trg = NULL, *rg = NULL;
> 
> 	VM_BUG_ON(regions_needed < 0);
> 
> -	INIT_LIST_HEAD(&allocated_regions);
> -
> 	/*
> 	 * Check for sufficient descriptors in the cache to accommodate
> 	 * the number of in progress add operations plus regions_needed.
> @@ -2352,7 +2350,7 @@ struct page *alloc_huge_page_vma(struct hstate *h, struct vm_area_struct *vma,
> static int gather_surplus_pages(struct hstate *h, long delta)
> 	__must_hold(&hugetlb_lock)
> {
> -	struct list_head surplus_list;
> +	LIST_HEAD(surplus_list);
> 	struct page *page, *tmp;
> 	int ret;
> 	long i;
> @@ -2367,7 +2365,6 @@ static int gather_surplus_pages(struct hstate *h, long delta)
> 	}
> 
> 	allocated = 0;
> -	INIT_LIST_HEAD(&surplus_list);
> 
> 	ret = -ENOMEM;
> retry:
> -- 
> 2.23.0
> 
> 

On 2022/8/27 9:47, Muchun Song wrote:
> 
> 
>> On Aug 26, 2022, at 17:24, Miaohe Lin <linmiaohe@huawei.com> wrote:
>>
>> We can avoid unneeded WRITE_ONCE() overhead by using LIST_HEAD() to define
>> a list head.
> 
> IIUC, the overhead doesn’t change. Right?

I think the overhead is changed. LIST_HEAD is initialized without using WRITE_ONCE():

#define LIST_HEAD_INIT(name) { &(name), &(name) }

#define LIST_HEAD(name) \
	struct list_head name = LIST_HEAD_INIT(name)

while INIT_LIST_HEAD has:

static inline void INIT_LIST_HEAD(struct list_head *list)
{
	WRITE_ONCE(list->next, list);
	WRITE_ONCE(list->prev, list);
}

Or am I miss something?

> 
> I’m fine with your changes.
> 
> Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Many thanks for your review and comment. :)

Thanks,
Miaohe Lin

> On Aug 27, 2022, at 10:27, Miaohe Lin <linmiaohe@huawei.com> wrote:
> 
> On 2022/8/27 9:47, Muchun Song wrote:
>> 
>> 
>>> On Aug 26, 2022, at 17:24, Miaohe Lin <linmiaohe@huawei.com> wrote:
>>> 
>>> We can avoid unneeded WRITE_ONCE() overhead by using LIST_HEAD() to define
>>> a list head.
>> 
>> IIUC, the overhead doesn’t change. Right?
> 
> I think the overhead is changed. LIST_HEAD is initialized without using WRITE_ONCE():

I think there is no special difference with "WRITE_ONCE(var, 0)" vs "var = 0” in
assembly code. Both code of line will be compiled to a mov or movq instruction.
I didn’t confirm if the assembly code is different (I tend to think it is similar).
Just some analysis from me.

> 
> #define LIST_HEAD_INIT(name) { &(name), &(name) }
> 
> #define LIST_HEAD(name) \
> 	struct list_head name = LIST_HEAD_INIT(name)
> 
> while INIT_LIST_HEAD has:
> 
> static inline void INIT_LIST_HEAD(struct list_head *list)
> {
> 	WRITE_ONCE(list->next, list);
> 	WRITE_ONCE(list->prev, list);
> }
> 
> Or am I miss something?
> 
>> 
>> I’m fine with your changes.
>> 
>> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
> 
> Many thanks for your review and comment. :)
> 
> Thanks,
> Miaohe Lin
> 

On 2022/8/27 10:48, Muchun Song wrote:
> 
> 
>> On Aug 27, 2022, at 10:27, Miaohe Lin <linmiaohe@huawei.com> wrote:
>>
>> On 2022/8/27 9:47, Muchun Song wrote:
>>>
>>>
>>>> On Aug 26, 2022, at 17:24, Miaohe Lin <linmiaohe@huawei.com> wrote:
>>>>
>>>> We can avoid unneeded WRITE_ONCE() overhead by using LIST_HEAD() to define
>>>> a list head.
>>>
>>> IIUC, the overhead doesn’t change. Right?
>>
>> I think the overhead is changed. LIST_HEAD is initialized without using WRITE_ONCE():
> 
> I think there is no special difference with "WRITE_ONCE(var, 0)" vs "var = 0” in

It's not write var to 0 indeed. But there seems are no special difference.

> assembly code. Both code of line will be compiled to a mov or movq instruction.
> I didn’t confirm if the assembly code is different (I tend to think it is similar).
> Just some analysis from me.

I checked the generated code in x86, they're almost same. And in aarch64, there's difference
between one "stp" instruction vs two "str" instruction. So I think you're right. Thanks for
pointing this out. I should tweak the commit log in next version.

Thanks a lot,
Miaohe Lin