On Mon, Jul 10, 2023 at 12:19PM +0200, Marco Elver wrote: > On Mon, 10 Jul 2023 at 05:27, 'Peng Zhang' via kasan-dev > 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 > > 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. Forgot to attach .config -- attached config. All I see is: [ 0.303465] rcu: RCU calculated value of scheduler-enlistment delay is 100 jiffies. [ 0.304783] rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=8 [ 0.316800] NR_IRQS: 4352, nr_irqs: 488, preallocated irqs: 16 [ 0.318140] rcu: srcu_init: Setting srcu_struct sizes based on contention. [ 0.320001] kfence: kfence_init failed [ 0.326880] Console: colour VGA+ 80x25 [ 0.327585] printk: console [ttyS0] enabled [ 0.327585] printk: console [ttyS0] enabled around KFENCE initialization. > > --- > > 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? > > > + 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? > > > + 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); > > +#endif > > You moved this from kfence_init_pool_late - that did "__kfence_pool = > NULL" which is missing now.