On Tue, Sep 27, 2022 at 10:44:20AM +0300, Alexander Atanasov wrote: > Hello, > > On 27.09.22 3:49, Hyeonggon Yoo wrote: > > On Fri, Sep 23, 2022 at 10:34:28AM +0300, Alexander Atanasov wrote: > > > Hello, > > > > > > On 21.09.22 14:30, Hyeonggon Yoo wrote: > > > > On Tue, Sep 20, 2022 at 03:11:11PM +0300, Alexander Atanasov wrote: > > > > > In (060807f841ac mm, slub: make remaining slub_debug related attributes > > > > > read-only) failslab was made read-only. > > > > > I think it became a collateral victim to the two other options for which > > > > > the reasons are perfectly valid. > > > > > Here is why: > > > > > - sanity_checks and trace are slab internal debug options, > > > > > failslab is used for fault injection. > > > > > - for fault injections, which by presumption are random, it > > > > > does not matter if it is not set atomically. And you need to > > > > > set atleast one more option to trigger fault injection. > > > > > - in a testing scenario you may need to change it at runtime > > > > > example: module loading - you test all allocations limited > > > > > by the space option. Then you move to test only your module's > > > > > own slabs. > > > > > - when set by command line flags it effectively disables all > > > > > cache merges. > > > > > > > > Maybe we can make failslab= boot parameter to consider cache filtering? > > > > > > > > With that, just pass something like this: > > > > failslab=X,X,X,X,cache_filter slub_debug=A,<cache-name>> > > > > > > > Users should pass slub_debug=A,<cache-name> anyway to prevent cache merging. > > > > > > It will be good to have this in case you want to test cache that is used > > > early. But why push something to command line option only when it can be > > > changed at runtime? > > > > Hmm okay. I'm not against changing it writable. (it looks okay to me.) > > Okay. Good to know that. > > > Just wanted to understand your use case! > > Can you please elaborate why booting with slub_debug=A,<your cache name> > > and enabling cache_filter after boot does not work? > > I didn't say it does not work - it does work but requires reboot. You may > want to test variations of caches for example. Cache A, Cache B ... C and so > on one by one. Reboots might be fast these days with VMs but you may not be > able to test everything in a VM. And ... reboots used to be the signature > move of one Other OS. Thank you for elaboration! Makes sense. > > > Or is it trying to changnig these steps, > > > > FROM > > 1. booting with slub_debug=A,<cache name> > > 2. write to cache_filter to enable cache filtering > > 3. setup probability, interval, times, size > > > > TO > > > > 1. write to failslab attribute of <cache name> (may fail it has alias) > > 2. write to cache_filter to enable cache filtering > > 3. setup probability, interval, times, size > > ? > > > > as you may know, SLAB_FAILSLAB does nothing whens > > cache_filter is disabled, and you should pass slub_debug=A,<cache name> anyway > > Okay , i think there awaits another problem: > bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) > { > ... > > if (failslab.cache_filter && !(s->flags & SLAB_FAILSLAB)) > return false; > ... > return should_fail(&failslab.attr, s->object_size); > } > > So if you do not have cache_filter set ... you go to should_fail for all > slabs. Yes. > I've been hit by that and spend a lot of time trying to understand why i got > crashes at random places. And the reason was that i read an old > documentation that said cache_filter is writable and i blindly wrote 1 to > it. > > If the intent is to only work with cache filter set - then i will update > the patch to do so. You mean to set cache_filter to true when writing to 'failslab', or when setting SLAB_FAILSLAB slab flag? I'm not so confident for that because it's implicitly changing. Maybe more documentation would be proper? what do you think, Vlastimil? > This is the only place where SLAB_FAILSLAB is explicitly > tested, other places check it as part of SLAB_NEVER_MERGE. > > But even for all caches it is kind of possible to test with size(space) > which is in turn useful because you need to figure out how you handle > failures from external caches - external to your code under test and you > don't want to keep track for all of them (same goes for too much options in > command line). Yeah, we should be able to inject fault in all caches, or a specific cache(s). > > to prevent doing cache merging with <cache name>. > > Or you can pass SLAB_FAILSLAB from your module when creating the cache to > prevent merge when under test. Right. I missed that. > > > -- > Regards, > Alexander Atanasov > -- Thanks, Hyeonggon