On Mon, Dec 19, 2022 at 12:31 PM Marco Elver <elver@xxxxxxxxxx> wrote: > > On a whole: > > Reviewed-by: Marco Elver <elver@xxxxxxxxxx> > > This looks much better, given it'll automatically do the right thing > without marking costly allocation sites. Agreed, thank you for the suggestion! > > +- ``kasan.page_alloc.sample.order=<minimum page order>`` specifies the minimum > > + order of allocations that are affected by sampling (default: ``3``). > > + Only applies when ``kasan.page_alloc.sample`` is set to a non-default value. > > "set to a value greater than 1"? The additional indirection through > "non-default" seems unnecessary. Will fix in v4. > > + This parameter is intended to allow sampling only large page_alloc > > + allocations, which is the biggest source of the performace overhead. > > s/performace/performance/ Will fix in v4. > > --- a/mm/kasan/hw_tags.c > > +++ b/mm/kasan/hw_tags.c > > @@ -59,6 +59,24 @@ EXPORT_SYMBOL_GPL(kasan_mode); > > /* Whether to enable vmalloc tagging. */ > > DEFINE_STATIC_KEY_TRUE(kasan_flag_vmalloc); > > > > +#define PAGE_ALLOC_SAMPLE_DEFAULT 1 > > +#define PAGE_ALLOC_SAMPLE_ORDER_DEFAULT 3 > > Why not just set it to PAGE_ALLOC_COSTLY_ORDER? I've been thinking about this, but technically PAGE_ALLOC_COSTLY_ORDER is related to allocations that are costly to service due to fragmentation/reclaim-related issues. We also don't rely on PAGE_ALLOC_COSTLY_ORDER only, but also on SKB_FRAG_PAGE_ORDER. (I guess some clean-up is possible wrt these constants: I suspect both have the same value for the same reason. But I don't want to attempt it with this patch. ) We could add a BUILD_BUG_ON that makes sure that all 3 constants are the same. But then the only thing to do if one of them is changed is to remove the BUG_ON, which doesn't seem very useful. I'll leave the current implementation in v4. Thank you, Marco!
