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