Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 27 Sep 2022 09:49:52 +0900
From:   Hyeonggon Yoo <42.hyeyoo@gmail.com>
To:     Alexander Atanasov <alexander.atanasov@virtuozzo.com>
Cc:     Jonathan Corbet <corbet@lwn.net>, Christoph Lameter <cl@linux.com>,
        Pekka Enberg <penberg@kernel.org>,
        David Rientjes <rientjes@google.com>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Vlastimil Babka <vbabka@suse.cz>,
        Roman Gushchin <roman.gushchin@linux.dev>, kernel@openvz.org,
        Kees Cook <keescook@chromium.org>,
        Roman Gushchin <guro@fb.com>, Jann Horn <jannh@google.com>,
        Vijayanand Jitta <vjitta@codeaurora.org>,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
Subject: Re: [PATCH v2] mm: Make failslab writable again
Message-ID: <YzJIsFZQoCEYntvR@hyeyoo>
References: <20220920121111.1792905-1-alexander.atanasov@virtuozzo.com>
 <Yyr1xONdw8dBgsKr@hyeyoo>
 <30063d97-69f0-bea2-9d59-108140995bfc@virtuozzo.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <30063d97-69f0-bea2-9d59-108140995bfc@virtuozzo.com>
Precedence: bulk

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