| Message ID | 20201210035526.38938-13-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Free some vmemmap pages of HugeTLB page | expand |
On 2020-12-10 04:55, Muchun Song wrote: > We cannot optimize if a "struct page" crosses page boundaries. If > it is true, we can optimize the code with the help of a compiler. > When free_vmemmap_pages_per_hpage() returns zero, most functions are > optimized by the compiler. As I said earlier, I would squash this patch with patch#10 and remove the !is_power_of_2 check in hugetlb_vmemmap_init and leave only the check for the boot parameter. That should be enough. > static inline bool is_hugetlb_free_vmemmap_enabled(void) > { > - return hugetlb_free_vmemmap_enabled; > + return hugetlb_free_vmemmap_enabled && > + is_power_of_2(sizeof(struct page)); Why? hugetlb_free_vmemmap_enabled can only become true if the is_power_of_2 check succeeds in early_hugetlb_free_vmemmap_param. The "is_power_of_2" check here can go. > diff --git a/mm/hugetlb_vmemmap.h b/mm/hugetlb_vmemmap.h > index 0a1c0d33a316..5f5e90c81cd2 100644 > --- a/mm/hugetlb_vmemmap.h > +++ b/mm/hugetlb_vmemmap.h > @@ -21,7 +21,7 @@ void free_huge_page_vmemmap(struct hstate *h, struct > page *head); > */ > static inline unsigned int free_vmemmap_pages_per_hpage(struct hstate > *h) > { > - return h->nr_free_vmemmap_pages; > + return h->nr_free_vmemmap_pages && is_power_of_2(sizeof(struct > page)); If hugetlb_free_vmemmap_enabled is false, hugetlb_vmemmap_init() leaves h->nr_free_vmemmap_pages unset to 0, so no need for the is_power_of_2 check here.
On Thu, Dec 10, 2020 at 7:39 PM Oscar Salvador <osalvador@suse.de> wrote: > > On 2020-12-10 04:55, Muchun Song wrote: > > We cannot optimize if a "struct page" crosses page boundaries. If > > it is true, we can optimize the code with the help of a compiler. > > When free_vmemmap_pages_per_hpage() returns zero, most functions are > > optimized by the compiler. > > As I said earlier, I would squash this patch with patch#10 and > remove the !is_power_of_2 check in hugetlb_vmemmap_init and leave > only the check for the boot parameter. > That should be enough. Yeah, you are right. I just want the compiler to do optimization. > > > static inline bool is_hugetlb_free_vmemmap_enabled(void) > > { > > - return hugetlb_free_vmemmap_enabled; > > + return hugetlb_free_vmemmap_enabled && > > + is_power_of_2(sizeof(struct page)); > > Why? hugetlb_free_vmemmap_enabled can only become true > if the is_power_of_2 check succeeds in early_hugetlb_free_vmemmap_param. > The "is_power_of_2" check here can go. > > > diff --git a/mm/hugetlb_vmemmap.h b/mm/hugetlb_vmemmap.h > > index 0a1c0d33a316..5f5e90c81cd2 100644 > > --- a/mm/hugetlb_vmemmap.h > > +++ b/mm/hugetlb_vmemmap.h > > @@ -21,7 +21,7 @@ void free_huge_page_vmemmap(struct hstate *h, struct > > page *head); > > */ > > static inline unsigned int free_vmemmap_pages_per_hpage(struct hstate > > *h) > > { > > - return h->nr_free_vmemmap_pages; > > + return h->nr_free_vmemmap_pages && is_power_of_2(sizeof(struct > > page)); > > If hugetlb_free_vmemmap_enabled is false, hugetlb_vmemmap_init() leaves > h->nr_free_vmemmap_pages unset to 0, so no need for the is_power_of_2 > check here. Yeah, you are right. But if we do this check can make the code simple. For example, here is a code snippet. void func(void) { if (free_vmemmap_pages_per_hpage()) return; /* Do something */ } With this patch, the func will be optimized to null when is_power_of_2 returns false. void func(void) { } Without this patch, the compiler cannot do this optimization. Thanks. > > > -- > Oscar Salvador > SUSE L3
On Thu, Dec 10, 2020 at 08:14:18PM +0800, Muchun Song wrote: > Yeah, you are right. But if we do this check can make the code simple. > > For example, here is a code snippet. > > void func(void) > { > if (free_vmemmap_pages_per_hpage()) > return; > /* Do something */ > } > > With this patch, the func will be optimized to null when is_power_of_2 > returns false. > > void func(void) > { > } > > Without this patch, the compiler cannot do this optimization. Ok, I misread the changelog. So, then is_hugetlb_free_vmemmap_enabled, free_huge_page_vmemmap, free_vmemmap_pages_per_hpage and hugetlb_vmemmap_init are optimized out, right?
On Thu, Dec 10, 2020 at 9:16 PM Oscar Salvador <osalvador@suse.de> wrote: > > On Thu, Dec 10, 2020 at 08:14:18PM +0800, Muchun Song wrote: > > Yeah, you are right. But if we do this check can make the code simple. > > > > For example, here is a code snippet. > > > > void func(void) > > { > > if (free_vmemmap_pages_per_hpage()) > > return; > > /* Do something */ > > } > > > > With this patch, the func will be optimized to null when is_power_of_2 > > returns false. > > > > void func(void) > > { > > } > > > > Without this patch, the compiler cannot do this optimization. > > Ok, I misread the changelog. > > So, then is_hugetlb_free_vmemmap_enabled, free_huge_page_vmemmap, > free_vmemmap_pages_per_hpage and hugetlb_vmemmap_init are optimized > out, right? Yes, that's right. I have disassembled to make sure of this. Thanks. > > -- > Oscar Salvador > SUSE L3
On Thu, Dec 10, 2020 at 9:29 PM Muchun Song <songmuchun@bytedance.com> wrote: > > On Thu, Dec 10, 2020 at 9:16 PM Oscar Salvador <osalvador@suse.de> wrote: > > > > On Thu, Dec 10, 2020 at 08:14:18PM +0800, Muchun Song wrote: > > > Yeah, you are right. But if we do this check can make the code simple. > > > > > > For example, here is a code snippet. > > > > > > void func(void) > > > { > > > if (free_vmemmap_pages_per_hpage()) > > > return; > > > /* Do something */ > > > } > > > > > > With this patch, the func will be optimized to null when is_power_of_2 > > > returns false. > > > > > > void func(void) > > > { > > > } > > > > > > Without this patch, the compiler cannot do this optimization. > > > > Ok, I misread the changelog. > > > > So, then is_hugetlb_free_vmemmap_enabled, free_huge_page_vmemmap, > > free_vmemmap_pages_per_hpage and hugetlb_vmemmap_init are optimized > > out, right? > > Yes, that's right. I have disassembled to make sure of this. Thanks. Hi Oscar, Because this is an optimization for code, I leave it in this separate patch. Do you still suggest squash this with patch#10? Thanks. > > > > > -- > > Oscar Salvador > > SUSE L3 > > > > -- > Yours, > Muchun
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h index 7295f6b3d55e..adc17765e0e9 100644 --- a/include/linux/hugetlb.h +++ b/include/linux/hugetlb.h @@ -791,7 +791,8 @@ extern bool hugetlb_free_vmemmap_enabled; static inline bool is_hugetlb_free_vmemmap_enabled(void) { - return hugetlb_free_vmemmap_enabled; + return hugetlb_free_vmemmap_enabled && + is_power_of_2(sizeof(struct page)); } #else static inline bool is_hugetlb_free_vmemmap_enabled(void) diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c index 7f0b9e002be4..819ab9bb9298 100644 --- a/mm/hugetlb_vmemmap.c +++ b/mm/hugetlb_vmemmap.c @@ -208,6 +208,9 @@ bool hugetlb_free_vmemmap_enabled; static int __init early_hugetlb_free_vmemmap_param(char *buf) { + if (!is_power_of_2(sizeof(struct page))) + return 0; + if (!buf) return -EINVAL; diff --git a/mm/hugetlb_vmemmap.h b/mm/hugetlb_vmemmap.h index 0a1c0d33a316..5f5e90c81cd2 100644 --- a/mm/hugetlb_vmemmap.h +++ b/mm/hugetlb_vmemmap.h @@ -21,7 +21,7 @@ void free_huge_page_vmemmap(struct hstate *h, struct page *head); */ static inline unsigned int free_vmemmap_pages_per_hpage(struct hstate *h) { - return h->nr_free_vmemmap_pages; + return h->nr_free_vmemmap_pages && is_power_of_2(sizeof(struct page)); } #else static inline void hugetlb_vmemmap_init(struct hstate *h)
We cannot optimize if a "struct page" crosses page boundaries. If it is true, we can optimize the code with the help of a compiler. When free_vmemmap_pages_per_hpage() returns zero, most functions are optimized by the compiler. Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- include/linux/hugetlb.h | 3 ++- mm/hugetlb_vmemmap.c | 3 +++ mm/hugetlb_vmemmap.h | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-)