diff mbox series

hugetlbfs: make hugepage size conversion more readable

Message ID 20210120092348.13811-1-linmiaohe@huawei.com (mailing list archive)
State New
Headers show
Series hugetlbfs: make hugepage size conversion more readable | expand

Commit Message

Miaohe Lin Jan. 20, 2021, 9:23 a.m. UTC
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(-)

Comments

Mike Kravetz Jan. 21, 2021, 7 p.m. UTC | #1
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.
Miaohe Lin Jan. 22, 2021, 1:42 a.m. UTC | #2
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.
Mike Kravetz Jan. 22, 2021, 5:02 a.m. UTC | #3
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.
Miaohe Lin Jan. 22, 2021, 6:12 a.m. UTC | #4
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 mbox series

Patch

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;
 }