在 2023/7/12 16:28, Peng Zhang 写道:
在 2023/7/10 18:19, Marco Elver 写道:On Mon, 10 Jul 2023 at 05:27, 'Peng Zhang' via kasan-dev <kasan-dev@xxxxxxxxxxxxxxxx> wrote:kfence_metadata is currently a static array. For the purpose of allocating scalable __kfence_pool, we first change it to runtime allocation of metadata. Since the size of an object of kfence_metadata is 1160 bytes, we can save at least 72 pages (with default 256 objects) without enabling kfence. Below is the numbers obtained in qemu (with default 256 objects). before: Memory: 8134692K/8388080K available (3668K bss) after: Memory: 8136740K/8388080K available (1620K bss) More than expected, it saves 2MB memory. Signed-off-by: Peng Zhang <zhangpeng.00@xxxxxxxxxxxxx>Seems like a reasonable optimization, but see comments below. Also with this patch applied on top of v6.5-rc1, KFENCE just doesn't init at all anymore (early init). Please fix.I'm very sorry because I made a slight modification before sending the patch but it has not been tested, which caused it to not work properly. I fixed some of the issues you mentioned in v2[1].[1] https://lore.kernel.org/lkml/20230712081616.45177-1-zhangpeng.00@xxxxxxxxxxxxx/--- mm/kfence/core.c | 102 ++++++++++++++++++++++++++++++++------------- mm/kfence/kfence.h | 5 ++- 2 files changed, 78 insertions(+), 29 deletions(-) diff --git a/mm/kfence/core.c b/mm/kfence/core.c index dad3c0eb70a0..b9fec1c46e3d 100644 --- a/mm/kfence/core.c +++ b/mm/kfence/core.c@@ -116,7 +116,7 @@ EXPORT_SYMBOL(__kfence_pool); /* Export for test modules. */* backing pages (in __kfence_pool). */ static_assert(CONFIG_KFENCE_NUM_OBJECTS > 0); -struct kfence_metadata kfence_metadata[CONFIG_KFENCE_NUM_OBJECTS]; +struct kfence_metadata *kfence_metadata; /* Freelist with available objects. */static struct list_head kfence_freelist = LIST_HEAD_INIT(kfence_freelist);@@ -643,13 +643,56 @@ static unsigned long kfence_init_pool(void) return addr; } +static int kfence_alloc_metadata(void) +{ + unsigned long nr_pages = KFENCE_METADATA_SIZE / PAGE_SIZE; + +#ifdef CONFIG_CONTIG_ALLOC + struct page *pages; ++ pages = alloc_contig_pages(nr_pages, GFP_KERNEL, first_online_node,+ NULL); + if (pages) + kfence_metadata = page_to_virt(pages); +#else + if (nr_pages > MAX_ORDER_NR_PAGES) {+ pr_warn("KFENCE_NUM_OBJECTS too large for buddy allocator\n");Does this mean that KFENCE won't work at all if we can't allocate the metadata? I.e. it won't work either in early nor late init modes?I know we already have this limitation for _late init_ of the KFENCE pool.So I have one major question: when doing _early init_, what is the maximum size of the KFENCE pool (#objects) with this change?It will be limited to 2^10/sizeof(struct kfence_metadata) by buddy
Sorry, 2^10*PAGE_SIZE/sizeof(struct kfence_metadata)
system, so I used memblock to allocate kfence_metadata in v2.+ return -EINVAL; + } + kfence_metadata = alloc_pages_exact(KFENCE_METADATA_SIZE, + GFP_KERNEL); +#endif + + if (!kfence_metadata) + return -ENOMEM; + + memset(kfence_metadata, 0, KFENCE_METADATA_SIZE);memzero_explicit, or pass __GFP_ZERO to alloc_pages?Unfortunately, __GFP_ZERO does not work successfully in alloc_contig_pages(), so I used memzero_explicit() in v2. Even though I don't know if memzero_explicit() is necessary (it just uses the barrier).+ return 0; +} + +static void kfence_free_metadata(void) +{ + if (WARN_ON(!kfence_metadata)) + return; +#ifdef CONFIG_CONTIG_ALLOC+ free_contig_range(page_to_pfn(virt_to_page((void *)kfence_metadata)),+ KFENCE_METADATA_SIZE / PAGE_SIZE); +#else + free_pages_exact((void *)kfence_metadata, KFENCE_METADATA_SIZE); +#endif + kfence_metadata = NULL; +} + static bool __init kfence_init_pool_early(void) { - unsigned long addr; + unsigned long addr = (unsigned long)__kfence_pool; if (!__kfence_pool) return false; + if (!kfence_alloc_metadata()) + goto free_pool; + addr = kfence_init_pool(); if (!addr) { @@ -663,6 +706,7 @@ static bool __init kfence_init_pool_early(void) return true; } + kfence_free_metadata(); /** Only release unprotected pages, and do not try to go back and change * page attributes due to risk of failing to do so as well. If changing@@ -670,31 +714,12 @@ static bool __init kfence_init_pool_early(void)* fails for the first page, and therefore expect addr==__kfence_pool in* most failure cases. */ +free_pool:memblock_free_late(__pa(addr), KFENCE_POOL_SIZE - (addr - (unsigned long)__kfence_pool));__kfence_pool = NULL; return false; } -static bool kfence_init_pool_late(void) -{ - unsigned long addr, free_size; - - addr = kfence_init_pool(); - - if (!addr) - return true; - - /* Same as above. */- free_size = KFENCE_POOL_SIZE - (addr - (unsigned long)__kfence_pool);-#ifdef CONFIG_CONTIG_ALLOC- free_contig_range(page_to_pfn(virt_to_page((void *)addr)), free_size / PAGE_SIZE);-#else - free_pages_exact((void *)addr, free_size); -#endif - __kfence_pool = NULL; - return false; -} -/* === DebugFS Interface ==================================================== */static int stats_show(struct seq_file *seq, void *v) @@ -896,6 +921,10 @@ void __init kfence_init(void) static int kfence_init_late(void) { const unsigned long nr_pages = KFENCE_POOL_SIZE / PAGE_SIZE; + unsigned long addr = (unsigned long)__kfence_pool; + unsigned long free_size = KFENCE_POOL_SIZE; + int ret; + #ifdef CONFIG_CONTIG_ALLOC struct page *pages; @@ -913,15 +942,29 @@ static int kfence_init_late(void) return -ENOMEM; #endif - if (!kfence_init_pool_late()) { - pr_err("%s failed\n", __func__); - return -EBUSY; + ret = kfence_alloc_metadata(); + if (!ret) + goto free_pool; + + addr = kfence_init_pool(); + if (!addr) { + kfence_init_enable(); + kfence_debugfs_init(); + return 0; } - kfence_init_enable(); - kfence_debugfs_init(); + pr_err("%s failed\n", __func__); + kfence_free_metadata();+ free_size = KFENCE_POOL_SIZE - (addr - (unsigned long)__kfence_pool);+ ret = -EBUSY; - return 0; +free_pool: +#ifdef CONFIG_CONTIG_ALLOC+ free_contig_range(page_to_pfn(virt_to_page((void *)addr)), free_size / PAGE_SIZE);+#else + free_pages_exact((void *)addr, free_size); +#endifYou moved this from kfence_init_pool_late - that did "__kfence_pool = NULL" which is missing now.Thanks for spotting this, I added it in v2.
