| Message ID | 20220307130708.58771-2-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | add hugetlb_free_vmemmap sysctl | expand |
On Mon, Mar 07, 2022 at 09:07:05PM +0800, Muchun Song wrote: > If the size of "struct page" is not the power of two and this > feature is enabled, then the vmemmap pages of HugeTLB will be > corrupted after remapping (panic is about to happen in theory). Huh what? If a panic is possible best we prevent this in kconfig all together. I'd instead just put some work into this instead of adding all this run time hacks. Can you try to add kconfig magic to detect if a PAGE_SIZE is PO2? Luis
On Tue, Mar 8, 2022 at 12:35 AM Luis Chamberlain <mcgrof@kernel.org> wrote: > > On Mon, Mar 07, 2022 at 09:07:05PM +0800, Muchun Song wrote: > > If the size of "struct page" is not the power of two and this > > feature is enabled, then the vmemmap pages of HugeTLB will be > > corrupted after remapping (panic is about to happen in theory). > > Huh what? If a panic is possible best we prevent this in kconfig > all together. I'd instead just put some work into this instead of > adding all this run time hacks. If the size of `struct page` is not power of 2, then those lines added by this patch will be optimized away by the compiler, therefore there is going to be no extra overhead to detect this. > > Can you try to add kconfig magic to detect if a PAGE_SIZE is PO2? > I agree with you that it is better if we can move this check into Kconfig. I tried this a few months ago. It is not easy to do this. How to check if a `struct page size` is PO2 in Kconfig? If you have any thoughts please let me know. Thanks.
On Tue, Mar 8, 2022 at 1:03 AM Muchun Song <songmuchun@bytedance.com> wrote: > > On Tue, Mar 8, 2022 at 12:35 AM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > On Mon, Mar 07, 2022 at 09:07:05PM +0800, Muchun Song wrote: > > > If the size of "struct page" is not the power of two and this > > > feature is enabled, then the vmemmap pages of HugeTLB will be > > > corrupted after remapping (panic is about to happen in theory). > > > > Huh what? If a panic is possible best we prevent this in kconfig > > all together. I'd instead just put some work into this instead of > > adding all this run time hacks. > > If the size of `struct page` is not power of 2, then those lines added > by this patch will be optimized away by the compiler, therefore there > is going to be no extra overhead to detect this. > > > > > Can you try to add kconfig magic to detect if a PAGE_SIZE is PO2? > > > > I agree with you that it is better if we can move this check > into Kconfig. I tried this a few months ago. It is not easy to > do this. How to check if a `struct page size` is PO2 in > Kconfig? If you have any thoughts please let me know. > > Thanks. Here is a discussion [1] from a few months ago. [1] https://lore.kernel.org/all/CAMZfGtWfz8DcwKBLdf3j0x9Dt6ZvOd+MvjX6yXrAoKDeXxW95w@mail.gmail.com/
On Tue, Mar 08, 2022 at 01:03:08AM +0800, Muchun Song wrote: > On Tue, Mar 8, 2022 at 12:35 AM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > On Mon, Mar 07, 2022 at 09:07:05PM +0800, Muchun Song wrote: > > > If the size of "struct page" is not the power of two and this > > > feature is enabled, then the vmemmap pages of HugeTLB will be > > > corrupted after remapping (panic is about to happen in theory). > > > > Huh what? If a panic is possible best we prevent this in kconfig > > all together. I'd instead just put some work into this instead of > > adding all this run time hacks. > > If the size of `struct page` is not power of 2, then those lines added > by this patch will be optimized away by the compiler, therefore there > is going to be no extra overhead to detect this. > > > > > Can you try to add kconfig magic to detect if a PAGE_SIZE is PO2? > > > > I agree with you that it is better if we can move this check > into Kconfig. I tried this a few months ago. It is not easy to > do this. How to check if a `struct page size` is PO2 in > Kconfig? If you have any thoughts please let me know. Can you query this with a script? config HAS_PAGE_SIZE_PO2 bool default $(shell, scripts/check_po2_page_size.sh arguments_are_allowed) Luis
On Fri, Mar 11, 2022 at 5:31 AM Luis Chamberlain <mcgrof@kernel.org> wrote: > > On Tue, Mar 08, 2022 at 01:03:08AM +0800, Muchun Song wrote: > > On Tue, Mar 8, 2022 at 12:35 AM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > > > On Mon, Mar 07, 2022 at 09:07:05PM +0800, Muchun Song wrote: > > > > If the size of "struct page" is not the power of two and this > > > > feature is enabled, then the vmemmap pages of HugeTLB will be > > > > corrupted after remapping (panic is about to happen in theory). > > > > > > Huh what? If a panic is possible best we prevent this in kconfig > > > all together. I'd instead just put some work into this instead of > > > adding all this run time hacks. > > > > If the size of `struct page` is not power of 2, then those lines added > > by this patch will be optimized away by the compiler, therefore there > > is going to be no extra overhead to detect this. > > > > > > > > Can you try to add kconfig magic to detect if a PAGE_SIZE is PO2? > > > > > > > I agree with you that it is better if we can move this check > > into Kconfig. I tried this a few months ago. It is not easy to > > do this. How to check if a `struct page size` is PO2 in > > Kconfig? If you have any thoughts please let me know. > > Can you query this with a script? > > config HAS_PAGE_SIZE_PO2 > bool > default $(shell, scripts/check_po2_page_size.sh arguments_are_allowed) > Excellent. I'll try this approach. Thanks very much.
diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c index b3118dba0518..49bc7f845438 100644 --- a/mm/hugetlb_vmemmap.c +++ b/mm/hugetlb_vmemmap.c @@ -121,6 +121,18 @@ void __init hugetlb_vmemmap_init(struct hstate *h) if (!hugetlb_free_vmemmap_enabled()) return; + if (IS_ENABLED(CONFIG_HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON) && + !is_power_of_2(sizeof(struct page))) { + /* + * The hugetlb_free_vmemmap_enabled_key can be enabled when + * CONFIG_HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON. It should + * be disabled if "struct page" crosses page boundaries. + */ + pr_warn_once("cannot free vmemmap pages because \"struct page\" crosses page boundaries\n"); + static_branch_disable(&hugetlb_free_vmemmap_enabled_key); + return; + } + vmemmap_pages = (nr_pages * sizeof(struct page)) >> PAGE_SHIFT; /* * The head page is not to be freed to buddy allocator, the other tail
If the size of "struct page" is not the power of two and this feature is enabled, then the vmemmap pages of HugeTLB will be corrupted after remapping (panic is about to happen in theory). But this only exists when !CONFIG_MEMCG && !CONFIG_SLUB on x86_64. However, it is not a conventional configuration nowadays. So it is not a real word issue, just the result of a code review. But we cannot prevent anyone from configuring that combined configure. This feature should be disable in this case to fix this issue. Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- mm/hugetlb_vmemmap.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)