| Message ID | 20210116092644.42697-1-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hugetlbfs: remove meaningless variable avoid_reserve | expand |
On Sat, Jan 16, 2021 at 2:57 PM Miaohe Lin <linmiaohe@huawei.com> wrote: > > The variable avoid_reserve is meaningless because we never changed its > value and just passed it to alloc_huge_page(). So remove it to make code > more clear that in hugetlbfs_fallocate, we never avoid reserve when alloc > hugepage yet. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> Acked-by: Souptick Joarder <jrdr.linux@gmail.com> > --- > fs/hugetlbfs/inode.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c > index 88751e35e69d..23ad6ed8b75f 100644 > --- a/fs/hugetlbfs/inode.c > +++ b/fs/hugetlbfs/inode.c > @@ -680,7 +680,6 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset, > */ > struct page *page; > unsigned long addr; > - int avoid_reserve = 0; > > cond_resched(); > > @@ -717,7 +716,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset, > } > > /* Allocate page and add to page cache */ > - page = alloc_huge_page(&pseudo_vma, addr, avoid_reserve); > + page = alloc_huge_page(&pseudo_vma, addr, 0); > hugetlb_drop_vma_policy(&pseudo_vma); > if (IS_ERR(page)) { > mutex_unlock(&hugetlb_fault_mutex_table[hash]); > -- > 2.19.1 > >
On 16.01.21 10:26, Miaohe Lin wrote: > The variable avoid_reserve is meaningless because we never changed its > value and just passed it to alloc_huge_page(). So remove it to make code > more clear that in hugetlbfs_fallocate, we never avoid reserve when alloc > hugepage yet. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > fs/hugetlbfs/inode.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c > index 88751e35e69d..23ad6ed8b75f 100644 > --- a/fs/hugetlbfs/inode.c > +++ b/fs/hugetlbfs/inode.c > @@ -680,7 +680,6 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset, > */ > struct page *page; > unsigned long addr; > - int avoid_reserve = 0; > > cond_resched(); > > @@ -717,7 +716,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset, > } > > /* Allocate page and add to page cache */ > - page = alloc_huge_page(&pseudo_vma, addr, avoid_reserve); > + page = alloc_huge_page(&pseudo_vma, addr, 0); > hugetlb_drop_vma_policy(&pseudo_vma); > if (IS_ERR(page)) { > mutex_unlock(&hugetlb_fault_mutex_table[hash]); > Reviewed-by: David Hildenbrand <david@redhat.com>
Please CC Andrew on hugetlb patches as they need to go through his tree. On 1/16/21 1:26 AM, Miaohe Lin wrote: > The variable avoid_reserve is meaningless because we never changed its > value and just passed it to alloc_huge_page(). So remove it to make code > more clear that in hugetlbfs_fallocate, we never avoid reserve when alloc > hugepage yet. One might argue that using a named variable makes the call to alloc_huge_page more clear. I do not disagree with the change, However, there are some subtle reasons why alloc_huge_page is called with 'avoid_reserve = 0' from fallocate. Therefore, I would prefer that a comment be added above the call in addition to this change. See below. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > fs/hugetlbfs/inode.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c > index 88751e35e69d..23ad6ed8b75f 100644 > --- a/fs/hugetlbfs/inode.c > +++ b/fs/hugetlbfs/inode.c > @@ -680,7 +680,6 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset, > */ > struct page *page; > unsigned long addr; > - int avoid_reserve = 0; > > cond_resched(); > > @@ -717,7 +716,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset, > } > > /* Allocate page and add to page cache */ Perhaps, change comment to read: /* * Allocate page without setting the avoid_reserve argument. * There certainly are no reserves associated with the * pseudo_vma. However, there could be shared mappings with * reserves for the file at the inode level. If we fallocate * pages in these areas, we need to consume the reserves * to keep reservation accounting consistent. */
Hi: On 2021/1/20 2:41, Mike Kravetz wrote: > Please CC Andrew on hugetlb patches as they need to go through his tree. > > On 1/16/21 1:26 AM, Miaohe Lin wrote: >> The variable avoid_reserve is meaningless because we never changed its >> value and just passed it to alloc_huge_page(). So remove it to make code >> more clear that in hugetlbfs_fallocate, we never avoid reserve when alloc >> hugepage yet. > > One might argue that using a named variable makes the call to alloc_huge_page > more clear. I do not disagree with the change, However, there are some > subtle reasons why alloc_huge_page is called with 'avoid_reserve = 0' from > fallocate. Therefore, I would prefer that a comment be added above the call > in addition to this change. See below. > >> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> --- >> fs/hugetlbfs/inode.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c >> index 88751e35e69d..23ad6ed8b75f 100644 >> --- a/fs/hugetlbfs/inode.c >> +++ b/fs/hugetlbfs/inode.c >> @@ -680,7 +680,6 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset, >> */ >> struct page *page; >> unsigned long addr; >> - int avoid_reserve = 0; >> >> cond_resched(); >> >> @@ -717,7 +716,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset, >> } >> >> /* Allocate page and add to page cache */ > > Perhaps, change comment to read: > > /* > * Allocate page without setting the avoid_reserve argument. > * There certainly are no reserves associated with the > * pseudo_vma. However, there could be shared mappings with > * reserves for the file at the inode level. If we fallocate > * pages in these areas, we need to consume the reserves > * to keep reservation accounting consistent. > */ > Many thanks for detailed and excellent comment. Will do it in v2. Thanks again.
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c index 88751e35e69d..23ad6ed8b75f 100644 --- a/fs/hugetlbfs/inode.c +++ b/fs/hugetlbfs/inode.c @@ -680,7 +680,6 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset, */ struct page *page; unsigned long addr; - int avoid_reserve = 0; cond_resched(); @@ -717,7 +716,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset, } /* Allocate page and add to page cache */ - page = alloc_huge_page(&pseudo_vma, addr, avoid_reserve); + page = alloc_huge_page(&pseudo_vma, addr, 0); hugetlb_drop_vma_policy(&pseudo_vma); if (IS_ERR(page)) { mutex_unlock(&hugetlb_fault_mutex_table[hash]);
The variable avoid_reserve is meaningless because we never changed its value and just passed it to alloc_huge_page(). So remove it to make code more clear that in hugetlbfs_fallocate, we never avoid reserve when alloc hugepage yet. Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- fs/hugetlbfs/inode.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)