* [PATCH] hugetlbfs: make hugepage size conversion more readable
@ 2021-01-20 9:23 Miaohe Lin
2021-01-21 19:00 ` Mike Kravetz
0 siblings, 1 reply; 5+ messages in thread
From: Miaohe Lin @ 2021-01-20 9:23 UTC (permalink / raw)
To: akpm, mike.kravetz; +Cc: linux-mm, linux-kernel, linmiaohe
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);
return mnt;
}
--
2.19.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] hugetlbfs: make hugepage size conversion more readable
2021-01-20 9:23 [PATCH] hugetlbfs: make hugepage size conversion more readable Miaohe Lin
@ 2021-01-21 19:00 ` Mike Kravetz
2021-01-22 1:42 ` Miaohe Lin
0 siblings, 1 reply; 5+ messages in thread
From: Mike Kravetz @ 2021-01-21 19:00 UTC (permalink / raw)
To: Miaohe Lin, akpm; +Cc: linux-mm, linux-kernel
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.
--
Mike Kravetz
> return mnt;
> }
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] hugetlbfs: make hugepage size conversion more readable
2021-01-21 19:00 ` Mike Kravetz
@ 2021-01-22 1:42 ` Miaohe Lin
2021-01-22 5:02 ` Mike Kravetz
0 siblings, 1 reply; 5+ messages in thread
From: Miaohe Lin @ 2021-01-22 1:42 UTC (permalink / raw)
To: Mike Kravetz; +Cc: linux-mm, linux-kernel, Andrew Morton
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.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] hugetlbfs: make hugepage size conversion more readable
2021-01-22 1:42 ` Miaohe Lin
@ 2021-01-22 5:02 ` Mike Kravetz
2021-01-22 6:12 ` Miaohe Lin
0 siblings, 1 reply; 5+ messages in thread
From: Mike Kravetz @ 2021-01-22 5:02 UTC (permalink / raw)
To: Miaohe Lin; +Cc: linux-mm, linux-kernel, Andrew Morton
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.
--
Mike Kravetz
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] hugetlbfs: make hugepage size conversion more readable
2021-01-22 5:02 ` Mike Kravetz
@ 2021-01-22 6:12 ` Miaohe Lin
0 siblings, 0 replies; 5+ messages in thread
From: Miaohe Lin @ 2021-01-22 6:12 UTC (permalink / raw)
To: Mike Kravetz; +Cc: linux-mm, linux-kernel, Andrew Morton
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-01-22 6:12 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-20 9:23 [PATCH] hugetlbfs: make hugepage size conversion more readable Miaohe Lin
2021-01-21 19:00 ` Mike Kravetz
2021-01-22 1:42 ` Miaohe Lin
2021-01-22 5:02 ` Mike Kravetz
2021-01-22 6:12 ` Miaohe Lin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).