| Message ID | 20220826092422.39591-4-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | A few cleanup patches for hugetlb | expand |
> 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
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:
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(-)