| Message ID | 20210120092348.13811-1-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hugetlbfs: make hugepage size conversion more readable | expand |
On 1/20/21 1:23 AM, Miaohe Lin wrote: > The calculation 1U << (h->order + PAGE_SHIFT - 10) is actually equal to > (PAGE_SHIFT << (h->order)) >> 10. So we can make it more readable by > replace it with huge_page_size(h) / SZ_1K. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > fs/hugetlbfs/inode.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c > index 25c1857ff45d..f94b8f6553fa 100644 > --- a/fs/hugetlbfs/inode.c > +++ b/fs/hugetlbfs/inode.c > @@ -1519,8 +1519,8 @@ static struct vfsmount *__init mount_one_hugetlbfs(struct hstate *h) > put_fs_context(fc); > } > if (IS_ERR(mnt)) > - pr_err("Cannot mount internal hugetlbfs for page size %uK", > - 1U << (h->order + PAGE_SHIFT - 10)); > + pr_err("Cannot mount internal hugetlbfs for page size %luK", > + huge_page_size(h) / SZ_1K); I appreciate the effort to make the code more readable. The existing calculation does take a minute to understand. However, it is correct and anyone modifying the code should be able to understand. With my compiler, your proposed change adds an additional instruction to the routine mount_one_hugetlbfs. I know this is not significant, but still it does increase the kernel size for a change that is of questionable value. In the kernel, size in KB is often calculated as (size << (PAGE_SHIFT - 10)). If you change the calculation in the hugetlb code to be: huge_page_size(h) << (PAGE_SHIFT - 10) my compiler will actually reduce the size of the routine by one instruction.
Hi: On 2021/1/22 3:00, Mike Kravetz wrote: > On 1/20/21 1:23 AM, Miaohe Lin wrote: >> The calculation 1U << (h->order + PAGE_SHIFT - 10) is actually equal to >> (PAGE_SHIFT << (h->order)) >> 10. So we can make it more readable by >> replace it with huge_page_size(h) / SZ_1K. >> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> --- >> fs/hugetlbfs/inode.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c >> index 25c1857ff45d..f94b8f6553fa 100644 >> --- a/fs/hugetlbfs/inode.c >> +++ b/fs/hugetlbfs/inode.c >> @@ -1519,8 +1519,8 @@ static struct vfsmount *__init mount_one_hugetlbfs(struct hstate *h) >> put_fs_context(fc); >> } >> if (IS_ERR(mnt)) >> - pr_err("Cannot mount internal hugetlbfs for page size %uK", >> - 1U << (h->order + PAGE_SHIFT - 10)); >> + pr_err("Cannot mount internal hugetlbfs for page size %luK", >> + huge_page_size(h) / SZ_1K); > > I appreciate the effort to make the code more readable. The existing > calculation does take a minute to understand. However, it is correct and > anyone modifying the code should be able to understand. > > With my compiler, your proposed change adds an additional instruction to > the routine mount_one_hugetlbfs. I know this is not significant, but still I thought compiler would generate the same code... > it does increase the kernel size for a change that is of questionable value. > > In the kernel, size in KB is often calculated as (size << (PAGE_SHIFT - 10)). > If you change the calculation in the hugetlb code to be: > > huge_page_size(h) << (PAGE_SHIFT - 10) I'am sorry but this looks not really correct. I think the calculation shoud be huge_page_size(h) >> 10. What do you think? > > my compiler will actually reduce the size of the routine by one instruction. > Many thanks.
On 1/21/21 5:42 PM, Miaohe Lin wrote: > Hi: > On 2021/1/22 3:00, Mike Kravetz wrote: >> On 1/20/21 1:23 AM, Miaohe Lin wrote: >>> The calculation 1U << (h->order + PAGE_SHIFT - 10) is actually equal to >>> (PAGE_SHIFT << (h->order)) >> 10. So we can make it more readable by >>> replace it with huge_page_size(h) / SZ_1K. >>> >>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >>> --- >>> fs/hugetlbfs/inode.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c >>> index 25c1857ff45d..f94b8f6553fa 100644 >>> --- a/fs/hugetlbfs/inode.c >>> +++ b/fs/hugetlbfs/inode.c >>> @@ -1519,8 +1519,8 @@ static struct vfsmount *__init mount_one_hugetlbfs(struct hstate *h) >>> put_fs_context(fc); >>> } >>> if (IS_ERR(mnt)) >>> - pr_err("Cannot mount internal hugetlbfs for page size %uK", >>> - 1U << (h->order + PAGE_SHIFT - 10)); >>> + pr_err("Cannot mount internal hugetlbfs for page size %luK", >>> + huge_page_size(h) / SZ_1K); >> >> I appreciate the effort to make the code more readable. The existing >> calculation does take a minute to understand. However, it is correct and >> anyone modifying the code should be able to understand. >> >> With my compiler, your proposed change adds an additional instruction to >> the routine mount_one_hugetlbfs. I know this is not significant, but still > > I thought compiler would generate the same code... > >> it does increase the kernel size for a change that is of questionable value. >> >> In the kernel, size in KB is often calculated as (size << (PAGE_SHIFT - 10)). >> If you change the calculation in the hugetlb code to be: >>> huge_page_size(h) << (PAGE_SHIFT - 10) > > I'am sorry but this looks not really correct. I think the calculation shoud be > huge_page_size(h) >> 10. What do you think? My bad! I was looking at code that converts page counts to KB. Sorry. Yes, huge_page_size(h) >> 10 is correct.
Hi: On 2021/1/22 13:02, Mike Kravetz wrote: > On 1/21/21 5:42 PM, Miaohe Lin wrote: >> Hi: >> On 2021/1/22 3:00, Mike Kravetz wrote: >>> On 1/20/21 1:23 AM, Miaohe Lin wrote: >>>> The calculation 1U << (h->order + PAGE_SHIFT - 10) is actually equal to >>>> (PAGE_SHIFT << (h->order)) >> 10. So we can make it more readable by >>>> replace it with huge_page_size(h) / SZ_1K. >>>> >>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >>>> --- >>>> fs/hugetlbfs/inode.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c >>>> index 25c1857ff45d..f94b8f6553fa 100644 >>>> --- a/fs/hugetlbfs/inode.c >>>> +++ b/fs/hugetlbfs/inode.c >>>> @@ -1519,8 +1519,8 @@ static struct vfsmount *__init mount_one_hugetlbfs(struct hstate *h) >>>> put_fs_context(fc); >>>> } >>>> if (IS_ERR(mnt)) >>>> - pr_err("Cannot mount internal hugetlbfs for page size %uK", >>>> - 1U << (h->order + PAGE_SHIFT - 10)); >>>> + pr_err("Cannot mount internal hugetlbfs for page size %luK", >>>> + huge_page_size(h) / SZ_1K); >>> >>> I appreciate the effort to make the code more readable. The existing >>> calculation does take a minute to understand. However, it is correct and >>> anyone modifying the code should be able to understand. >>> >>> With my compiler, your proposed change adds an additional instruction to >>> the routine mount_one_hugetlbfs. I know this is not significant, but still >> >> I thought compiler would generate the same code... >> >>> it does increase the kernel size for a change that is of questionable value. >>> >>> In the kernel, size in KB is often calculated as (size << (PAGE_SHIFT - 10)). >>> If you change the calculation in the hugetlb code to be: >>>> huge_page_size(h) << (PAGE_SHIFT - 10) >> >> I'am sorry but this looks not really correct. I think the calculation shoud be >> huge_page_size(h) >> 10. What do you think? > > My bad! I was looking at code that converts page counts to KB. Sorry. > > Yes, huge_page_size(h) >> 10 is correct. > So I will send v2 with this change. Many thanks.
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c index 25c1857ff45d..f94b8f6553fa 100644 --- a/fs/hugetlbfs/inode.c +++ b/fs/hugetlbfs/inode.c @@ -1519,8 +1519,8 @@ static struct vfsmount *__init mount_one_hugetlbfs(struct hstate *h) put_fs_context(fc); } if (IS_ERR(mnt)) - pr_err("Cannot mount internal hugetlbfs for page size %uK", - 1U << (h->order + PAGE_SHIFT - 10)); + pr_err("Cannot mount internal hugetlbfs for page size %luK", + huge_page_size(h) / SZ_1K); return mnt; }
The calculation 1U << (h->order + PAGE_SHIFT - 10) is actually equal to (PAGE_SHIFT << (h->order)) >> 10. So we can make it more readable by replace it with huge_page_size(h) / SZ_1K. Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- fs/hugetlbfs/inode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)