* [PATCH v2] mm: Make failslab writable again @ 2022-09-20 12:11 Alexander Atanasov 2022-09-21 11:30 ` Hyeonggon Yoo 2022-10-14 8:51 ` Vlastimil Babka 0 siblings, 2 replies; 8+ messages in thread From: Alexander Atanasov @ 2022-09-20 12:11 UTC (permalink / raw) To: Jonathan Corbet, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton, Vlastimil Babka, Roman Gushchin, Hyeonggon Yoo Cc: kernel, Alexander Atanasov, Kees Cook, Roman Gushchin, Jann Horn, Vijayanand Jitta, linux-doc, linux-kernel, linux-mm 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. Cc: Vlastimil Babka <vbabka@suse.cz> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Kees Cook <keescook@chromium.org> Cc: Roman Gushchin <guro@fb.com> Cc: Christoph Lameter <cl@linux.com> Cc: Jann Horn <jannh@google.com> Cc: Vijayanand Jitta <vjitta@codeaurora.org> Cc: David Rientjes <rientjes@google.com> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com> Cc: Pekka Enberg <penberg@kernel.org> Link: http://lkml.kernel.org/r/20200610163135.17364-5-vbabka@suse.cz Signed-off-by: Alexander Atanasov <alexander.atanasov@virtuozzo.com> --- Documentation/mm/slub.rst | 2 ++ mm/slub.c | 16 +++++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) V1->V2: Fixed commit message. Flags are set using WRITE_ONCE. diff --git a/Documentation/mm/slub.rst b/Documentation/mm/slub.rst index 43063ade737a..86837073a39e 100644 --- a/Documentation/mm/slub.rst +++ b/Documentation/mm/slub.rst @@ -116,6 +116,8 @@ options from the ``slub_debug`` parameter translate to the following files:: T trace A failslab +failslab file is writable, so writing 1 or 0 will enable or disable +the option at runtime. Write returns -EINVAL if cache is an alias. Careful with tracing: It may spew out lots of information and never stop if used on the wrong slab. diff --git a/mm/slub.c b/mm/slub.c index 862dbd9af4f5..57cf18936526 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -5617,7 +5617,21 @@ static ssize_t failslab_show(struct kmem_cache *s, char *buf) { return sysfs_emit(buf, "%d\n", !!(s->flags & SLAB_FAILSLAB)); } -SLAB_ATTR_RO(failslab); + +static ssize_t failslab_store(struct kmem_cache *s, const char *buf, + size_t length) +{ + if (s->refcount > 1) + return -EINVAL; + + if (buf[0] == '1') + WRITE_ONCE(s->flags, s->flags | SLAB_FAILSLAB); + else + WRITE_ONCE(s->flags, s->flags & ~SLAB_FAILSLAB); + + return length; +} +SLAB_ATTR(failslab); #endif static ssize_t shrink_show(struct kmem_cache *s, char *buf) base-commit: 80e78fcce86de0288793a0ef0f6acf37656ee4cf -- 2.31.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] mm: Make failslab writable again 2022-09-20 12:11 [PATCH v2] mm: Make failslab writable again Alexander Atanasov @ 2022-09-21 11:30 ` Hyeonggon Yoo 2022-09-23 7:34 ` Alexander Atanasov 2022-10-14 8:51 ` Vlastimil Babka 1 sibling, 1 reply; 8+ messages in thread From: Hyeonggon Yoo @ 2022-09-21 11:30 UTC (permalink / raw) To: Alexander Atanasov Cc: Jonathan Corbet, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton, Vlastimil Babka, Roman Gushchin, kernel, Kees Cook, Roman Gushchin, Jann Horn, Vijayanand Jitta, linux-doc, linux-kernel, linux-mm 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. > > Cc: Vlastimil Babka <vbabka@suse.cz> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Kees Cook <keescook@chromium.org> > Cc: Roman Gushchin <guro@fb.com> > Cc: Christoph Lameter <cl@linux.com> > Cc: Jann Horn <jannh@google.com> > Cc: Vijayanand Jitta <vjitta@codeaurora.org> > Cc: David Rientjes <rientjes@google.com> > Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com> > Cc: Pekka Enberg <penberg@kernel.org> > Link: http://lkml.kernel.org/r/20200610163135.17364-5-vbabka@suse.cz > > Signed-off-by: Alexander Atanasov <alexander.atanasov@virtuozzo.com> > --- > Documentation/mm/slub.rst | 2 ++ > mm/slub.c | 16 +++++++++++++++- > 2 files changed, 17 insertions(+), 1 deletion(-) > > V1->V2: Fixed commit message. Flags are set using WRITE_ONCE. > > diff --git a/Documentation/mm/slub.rst b/Documentation/mm/slub.rst > index 43063ade737a..86837073a39e 100644 > --- a/Documentation/mm/slub.rst > +++ b/Documentation/mm/slub.rst > @@ -116,6 +116,8 @@ options from the ``slub_debug`` parameter translate to the following files:: > T trace > A failslab > > +failslab file is writable, so writing 1 or 0 will enable or disable > +the option at runtime. Write returns -EINVAL if cache is an alias. > Careful with tracing: It may spew out lots of information and never stop if > used on the wrong slab. > > diff --git a/mm/slub.c b/mm/slub.c > index 862dbd9af4f5..57cf18936526 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -5617,7 +5617,21 @@ static ssize_t failslab_show(struct kmem_cache *s, char *buf) > { > return sysfs_emit(buf, "%d\n", !!(s->flags & SLAB_FAILSLAB)); > } > -SLAB_ATTR_RO(failslab); > + > +static ssize_t failslab_store(struct kmem_cache *s, const char *buf, > + size_t length) > +{ > + if (s->refcount > 1) > + return -EINVAL; > + > + if (buf[0] == '1') > + WRITE_ONCE(s->flags, s->flags | SLAB_FAILSLAB); > + else > + WRITE_ONCE(s->flags, s->flags & ~SLAB_FAILSLAB); > + > + return length; > +} > +SLAB_ATTR(failslab); > #endif > > static ssize_t shrink_show(struct kmem_cache *s, char *buf) > > base-commit: 80e78fcce86de0288793a0ef0f6acf37656ee4cf > -- > 2.31.1 > -- Thanks, Hyeonggon ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] mm: Make failslab writable again 2022-09-21 11:30 ` Hyeonggon Yoo @ 2022-09-23 7:34 ` Alexander Atanasov 2022-09-27 0:49 ` Hyeonggon Yoo 0 siblings, 1 reply; 8+ messages in thread From: Alexander Atanasov @ 2022-09-23 7:34 UTC (permalink / raw) To: Hyeonggon Yoo Cc: Jonathan Corbet, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton, Vlastimil Babka, Roman Gushchin, kernel, Kees Cook, Roman Gushchin, Jann Horn, Vijayanand Jitta, linux-doc, linux-kernel, linux-mm 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? -- Regards, Alexander Atanasov ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] mm: Make failslab writable again 2022-09-23 7:34 ` Alexander Atanasov @ 2022-09-27 0:49 ` Hyeonggon Yoo 2022-09-27 7:44 ` Alexander Atanasov 0 siblings, 1 reply; 8+ messages in thread From: Hyeonggon Yoo @ 2022-09-27 0:49 UTC (permalink / raw) To: Alexander Atanasov Cc: Jonathan Corbet, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton, Vlastimil Babka, Roman Gushchin, kernel, Kees Cook, Roman Gushchin, Jann Horn, Vijayanand Jitta, linux-doc, linux-kernel, linux-mm 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.) 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? 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 when cache_filter is disabled, and you should pass slub_debug=A,<cache name> anyway to prevent doing cache merging with <cache name>. -- Thanks, Hyeonggon ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] mm: Make failslab writable again 2022-09-27 0:49 ` Hyeonggon Yoo @ 2022-09-27 7:44 ` Alexander Atanasov 2022-09-28 15:21 ` Hyeonggon Yoo 0 siblings, 1 reply; 8+ messages in thread From: Alexander Atanasov @ 2022-09-27 7:44 UTC (permalink / raw) To: Hyeonggon Yoo Cc: Jonathan Corbet, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton, Vlastimil Babka, Roman Gushchin, kernel, Kees Cook, Roman Gushchin, Jann Horn, Vijayanand Jitta, linux-doc, linux-kernel, linux-mm 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. > 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. 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. 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). > 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. -- Regards, Alexander Atanasov ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] mm: Make failslab writable again 2022-09-27 7:44 ` Alexander Atanasov @ 2022-09-28 15:21 ` Hyeonggon Yoo 2022-10-14 8:48 ` Vlastimil Babka 0 siblings, 1 reply; 8+ messages in thread From: Hyeonggon Yoo @ 2022-09-28 15:21 UTC (permalink / raw) To: Alexander Atanasov Cc: Jonathan Corbet, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton, Vlastimil Babka, Roman Gushchin, kernel, Kees Cook, Roman Gushchin, Jann Horn, Vijayanand Jitta, linux-doc, linux-kernel, linux-mm 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] mm: Make failslab writable again 2022-09-28 15:21 ` Hyeonggon Yoo @ 2022-10-14 8:48 ` Vlastimil Babka 0 siblings, 0 replies; 8+ messages in thread From: Vlastimil Babka @ 2022-10-14 8:48 UTC (permalink / raw) To: Hyeonggon Yoo, Alexander Atanasov Cc: Jonathan Corbet, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton, Roman Gushchin, kernel, Kees Cook, Roman Gushchin, Jann Horn, Vijayanand Jitta, linux-doc, linux-kernel, linux-mm 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 >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] mm: Make failslab writable again 2022-09-20 12:11 [PATCH v2] mm: Make failslab writable again Alexander Atanasov 2022-09-21 11:30 ` Hyeonggon Yoo @ 2022-10-14 8:51 ` Vlastimil Babka 1 sibling, 0 replies; 8+ messages in thread From: Vlastimil Babka @ 2022-10-14 8:51 UTC (permalink / raw) To: Alexander Atanasov, Jonathan Corbet, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton, Roman Gushchin, Hyeonggon Yoo Cc: kernel, Kees Cook, Roman Gushchin, Jann Horn, Vijayanand Jitta, linux-doc, linux-kernel, linux-mm On 9/20/22 14:11, 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. > > Cc: Vlastimil Babka <vbabka@suse.cz> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Kees Cook <keescook@chromium.org> > Cc: Roman Gushchin <guro@fb.com> > Cc: Christoph Lameter <cl@linux.com> > Cc: Jann Horn <jannh@google.com> > Cc: Vijayanand Jitta <vjitta@codeaurora.org> > Cc: David Rientjes <rientjes@google.com> > Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com> > Cc: Pekka Enberg <penberg@kernel.org> > Link: http://lkml.kernel.org/r/20200610163135.17364-5-vbabka@suse.cz > > Signed-off-by: Alexander Atanasov <alexander.atanasov@virtuozzo.com> Thanks, added to slab.git for-6.2/slub-sysfs > --- > Documentation/mm/slub.rst | 2 ++ > mm/slub.c | 16 +++++++++++++++- > 2 files changed, 17 insertions(+), 1 deletion(-) > > V1->V2: Fixed commit message. Flags are set using WRITE_ONCE. > > diff --git a/Documentation/mm/slub.rst b/Documentation/mm/slub.rst > index 43063ade737a..86837073a39e 100644 > --- a/Documentation/mm/slub.rst > +++ b/Documentation/mm/slub.rst > @@ -116,6 +116,8 @@ options from the ``slub_debug`` parameter translate to the following files:: > T trace > A failslab > > +failslab file is writable, so writing 1 or 0 will enable or disable > +the option at runtime. Write returns -EINVAL if cache is an alias. > Careful with tracing: It may spew out lots of information and never stop if > used on the wrong slab. > > diff --git a/mm/slub.c b/mm/slub.c > index 862dbd9af4f5..57cf18936526 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -5617,7 +5617,21 @@ static ssize_t failslab_show(struct kmem_cache *s, char *buf) > { > return sysfs_emit(buf, "%d\n", !!(s->flags & SLAB_FAILSLAB)); > } > -SLAB_ATTR_RO(failslab); > + > +static ssize_t failslab_store(struct kmem_cache *s, const char *buf, > + size_t length) > +{ > + if (s->refcount > 1) > + return -EINVAL; > + > + if (buf[0] == '1') > + WRITE_ONCE(s->flags, s->flags | SLAB_FAILSLAB); > + else > + WRITE_ONCE(s->flags, s->flags & ~SLAB_FAILSLAB); > + > + return length; > +} > +SLAB_ATTR(failslab); > #endif > > static ssize_t shrink_show(struct kmem_cache *s, char *buf) > > base-commit: 80e78fcce86de0288793a0ef0f6acf37656ee4cf ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-10-14 8:52 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-20 12:11 [PATCH v2] mm: Make failslab writable again Alexander Atanasov 2022-09-21 11:30 ` Hyeonggon Yoo 2022-09-23 7:34 ` Alexander Atanasov 2022-09-27 0:49 ` Hyeonggon Yoo 2022-09-27 7:44 ` Alexander Atanasov 2022-09-28 15:21 ` Hyeonggon Yoo 2022-10-14 8:48 ` Vlastimil Babka 2022-10-14 8:51 ` Vlastimil Babka
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).