Re: [PATCH v2] mm: Make failslab writable again

[Vlastimil Babka]

On 9/28/22 17:21, Hyeonggon Yoo wrote:
> 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.

I don't understand. It is writable for root, and you can enable it that way, no?

>> 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?

I also don't think we should change cache_filter when writing to a cache's
failslab attribute.

>> 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
>> 
>