diff mbox series

[RFC,v5,16/16] dcache: Add CONFIG_DCACHE_SMO

Message ID 20190520054017.32299-17-tobin@kernel.org (mailing list archive)
State New, archived
Headers show
Series Slab Movable Objects (SMO) | expand

Commit Message

Tobin C. Harding May 20, 2019, 5:40 a.m. UTC
In an attempt to make the SMO patchset as non-invasive as possible add a
config option CONFIG_DCACHE_SMO (under "Memory Management options") for
enabling SMO for the DCACHE.  Whithout this option dcache constructor is
used but no other code is built in, with this option enabled slab
mobility is enabled and the isolate/migrate functions are built in.

Add CONFIG_DCACHE_SMO to guard the partial shrinking of the dcache via
Slab Movable Objects infrastructure.

Signed-off-by: Tobin C. Harding <tobin@kernel.org>
---
 fs/dcache.c | 4 ++++
 mm/Kconfig  | 7 +++++++
 2 files changed, 11 insertions(+)

Comments

Roman Gushchin May 21, 2019, 12:57 a.m. UTC | #1
On Mon, May 20, 2019 at 03:40:17PM +1000, Tobin C. Harding wrote:
> In an attempt to make the SMO patchset as non-invasive as possible add a
> config option CONFIG_DCACHE_SMO (under "Memory Management options") for
> enabling SMO for the DCACHE.  Whithout this option dcache constructor is
> used but no other code is built in, with this option enabled slab
> mobility is enabled and the isolate/migrate functions are built in.
> 
> Add CONFIG_DCACHE_SMO to guard the partial shrinking of the dcache via
> Slab Movable Objects infrastructure.

Hm, isn't it better to make it a static branch? Or basically anything
that allows switching on the fly?

It seems that the cost of just building it in shouldn't be that high.
And the question if the defragmentation worth the trouble is so much
easier to answer if it's possible to turn it on and off without rebooting.

Thanks!

> 
> Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> ---
>  fs/dcache.c | 4 ++++
>  mm/Kconfig  | 7 +++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 0dfe580c2d42..96063e872366 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -3072,6 +3072,7 @@ void d_tmpfile(struct dentry *dentry, struct inode *inode)
>  }
>  EXPORT_SYMBOL(d_tmpfile);
>  
> +#ifdef CONFIG_DCACHE_SMO
>  /*
>   * d_isolate() - Dentry isolation callback function.
>   * @s: The dentry cache.
> @@ -3144,6 +3145,7 @@ static void d_partial_shrink(struct kmem_cache *s, void **_unused, int __unused,
>  
>  	kfree(private);
>  }
> +#endif	/* CONFIG_DCACHE_SMO */
>  
>  static __initdata unsigned long dhash_entries;
>  static int __init set_dhash_entries(char *str)
> @@ -3190,7 +3192,9 @@ static void __init dcache_init(void)
>  					   sizeof_field(struct dentry, d_iname),
>  					   dcache_ctor);
>  
> +#ifdef CONFIG_DCACHE_SMO
>  	kmem_cache_setup_mobility(dentry_cache, d_isolate, d_partial_shrink);
> +#endif
>  
>  	/* Hash may have been set up in dcache_init_early */
>  	if (!hashdist)
> diff --git a/mm/Kconfig b/mm/Kconfig
> index aa8d60e69a01..7dcea76e5ecc 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -265,6 +265,13 @@ config SMO_NODE
>         help
>           On NUMA systems enable moving objects to and from a specified node.
>  
> +config DCACHE_SMO
> +       bool "Enable Slab Movable Objects for the dcache"
> +       depends on SLUB
> +       help
> +         Under memory pressure we can try to free dentry slab cache objects from
> +         the partial slab list if this is enabled.
> +
>  config PHYS_ADDR_T_64BIT
>  	def_bool 64BIT
>  
> -- 
> 2.21.0
>
Tobin Harding May 21, 2019, 1:31 a.m. UTC | #2
On Tue, May 21, 2019 at 12:57:47AM +0000, Roman Gushchin wrote:
> On Mon, May 20, 2019 at 03:40:17PM +1000, Tobin C. Harding wrote:
> > In an attempt to make the SMO patchset as non-invasive as possible add a
> > config option CONFIG_DCACHE_SMO (under "Memory Management options") for
> > enabling SMO for the DCACHE.  Whithout this option dcache constructor is
> > used but no other code is built in, with this option enabled slab
> > mobility is enabled and the isolate/migrate functions are built in.
> > 
> > Add CONFIG_DCACHE_SMO to guard the partial shrinking of the dcache via
> > Slab Movable Objects infrastructure.
> 
> Hm, isn't it better to make it a static branch? Or basically anything
> that allows switching on the fly?

If that is wanted, turning SMO on and off per cache, we can probably do
this in the SMO code in SLUB.

> It seems that the cost of just building it in shouldn't be that high.
> And the question if the defragmentation worth the trouble is so much
> easier to answer if it's possible to turn it on and off without rebooting.

If the question is 'is defragmentation worth the trouble for the
dcache', I'm not sure having SMO turned off helps answer that question.
If one doesn't shrink the dentry cache there should be very little
overhead in having SMO enabled.  So if one wants to explore this
question then they can turn on the config option.  Please correct me if
I'm wrong.

The ifdef guard is there so memory management is not having any negative
effects on the dcache/VFS (no matter how small).  It also means that the
VFS guys don't have to keep an eye on what SMO is doing, they can
just configure SMO out.  The dcache is already fairly complex, I'm not
sure adding complexity to it without good reason is sound practice.  At
best SMO is only going to by mildly useful to the dcache so I feel we
should err on the side of caution.

Open to ideas.

Thanks,
Tobin.
Roman Gushchin May 21, 2019, 2:05 a.m. UTC | #3
On Tue, May 21, 2019 at 11:31:18AM +1000, Tobin C. Harding wrote:
> On Tue, May 21, 2019 at 12:57:47AM +0000, Roman Gushchin wrote:
> > On Mon, May 20, 2019 at 03:40:17PM +1000, Tobin C. Harding wrote:
> > > In an attempt to make the SMO patchset as non-invasive as possible add a
> > > config option CONFIG_DCACHE_SMO (under "Memory Management options") for
> > > enabling SMO for the DCACHE.  Whithout this option dcache constructor is
> > > used but no other code is built in, with this option enabled slab
> > > mobility is enabled and the isolate/migrate functions are built in.
> > > 
> > > Add CONFIG_DCACHE_SMO to guard the partial shrinking of the dcache via
> > > Slab Movable Objects infrastructure.
> > 
> > Hm, isn't it better to make it a static branch? Or basically anything
> > that allows switching on the fly?
> 
> If that is wanted, turning SMO on and off per cache, we can probably do
> this in the SMO code in SLUB.

Not necessarily per cache, but without recompiling the kernel.
> 
> > It seems that the cost of just building it in shouldn't be that high.
> > And the question if the defragmentation worth the trouble is so much
> > easier to answer if it's possible to turn it on and off without rebooting.
> 
> If the question is 'is defragmentation worth the trouble for the
> dcache', I'm not sure having SMO turned off helps answer that question.
> If one doesn't shrink the dentry cache there should be very little
> overhead in having SMO enabled.  So if one wants to explore this
> question then they can turn on the config option.  Please correct me if
> I'm wrong.

The problem with a config option is that it's hard to switch over.

So just to test your changes in production a new kernel should be built,
tested and rolled out to a representative set of machines (which can be
measured in thousands of machines). Then if results are questionable,
it should be rolled back.

What you're actually guarding is the kmem_cache_setup_mobility() call,
which can be perfectly avoided using a boot option, for example. Turning
it on and off completely dynamic isn't that hard too.

Of course, it's up to you, it's just probably easier to find new users
of a new feature, when it's easy to test it.

Thanks!
Tobin Harding May 21, 2019, 3:15 a.m. UTC | #4
On Tue, May 21, 2019 at 02:05:38AM +0000, Roman Gushchin wrote:
> On Tue, May 21, 2019 at 11:31:18AM +1000, Tobin C. Harding wrote:
> > On Tue, May 21, 2019 at 12:57:47AM +0000, Roman Gushchin wrote:
> > > On Mon, May 20, 2019 at 03:40:17PM +1000, Tobin C. Harding wrote:
> > > > In an attempt to make the SMO patchset as non-invasive as possible add a
> > > > config option CONFIG_DCACHE_SMO (under "Memory Management options") for
> > > > enabling SMO for the DCACHE.  Whithout this option dcache constructor is
> > > > used but no other code is built in, with this option enabled slab
> > > > mobility is enabled and the isolate/migrate functions are built in.
> > > > 
> > > > Add CONFIG_DCACHE_SMO to guard the partial shrinking of the dcache via
> > > > Slab Movable Objects infrastructure.
> > > 
> > > Hm, isn't it better to make it a static branch? Or basically anything
> > > that allows switching on the fly?
> > 
> > If that is wanted, turning SMO on and off per cache, we can probably do
> > this in the SMO code in SLUB.
> 
> Not necessarily per cache, but without recompiling the kernel.
> > 
> > > It seems that the cost of just building it in shouldn't be that high.
> > > And the question if the defragmentation worth the trouble is so much
> > > easier to answer if it's possible to turn it on and off without rebooting.
> > 
> > If the question is 'is defragmentation worth the trouble for the
> > dcache', I'm not sure having SMO turned off helps answer that question.
> > If one doesn't shrink the dentry cache there should be very little
> > overhead in having SMO enabled.  So if one wants to explore this
> > question then they can turn on the config option.  Please correct me if
> > I'm wrong.
> 
> The problem with a config option is that it's hard to switch over.
> 
> So just to test your changes in production a new kernel should be built,
> tested and rolled out to a representative set of machines (which can be
> measured in thousands of machines). Then if results are questionable,
> it should be rolled back.
> 
> What you're actually guarding is the kmem_cache_setup_mobility() call,
> which can be perfectly avoided using a boot option, for example. Turning
> it on and off completely dynamic isn't that hard too.
> 
> Of course, it's up to you, it's just probably easier to find new users
> of a new feature, when it's easy to test it.

Ok, cool - I like it.  Will add for next version.

thanks,
Tobin.
Tobin Harding May 29, 2019, 3:54 a.m. UTC | #5
On Tue, May 21, 2019 at 02:05:38AM +0000, Roman Gushchin wrote:
> On Tue, May 21, 2019 at 11:31:18AM +1000, Tobin C. Harding wrote:
> > On Tue, May 21, 2019 at 12:57:47AM +0000, Roman Gushchin wrote:
> > > On Mon, May 20, 2019 at 03:40:17PM +1000, Tobin C. Harding wrote:
> > > > In an attempt to make the SMO patchset as non-invasive as possible add a
> > > > config option CONFIG_DCACHE_SMO (under "Memory Management options") for
> > > > enabling SMO for the DCACHE.  Whithout this option dcache constructor is
> > > > used but no other code is built in, with this option enabled slab
> > > > mobility is enabled and the isolate/migrate functions are built in.
> > > > 
> > > > Add CONFIG_DCACHE_SMO to guard the partial shrinking of the dcache via
> > > > Slab Movable Objects infrastructure.
> > > 
> > > Hm, isn't it better to make it a static branch? Or basically anything
> > > that allows switching on the fly?
> > 
> > If that is wanted, turning SMO on and off per cache, we can probably do
> > this in the SMO code in SLUB.
> 
> Not necessarily per cache, but without recompiling the kernel.
> > 
> > > It seems that the cost of just building it in shouldn't be that high.
> > > And the question if the defragmentation worth the trouble is so much
> > > easier to answer if it's possible to turn it on and off without rebooting.
> > 
> > If the question is 'is defragmentation worth the trouble for the
> > dcache', I'm not sure having SMO turned off helps answer that question.
> > If one doesn't shrink the dentry cache there should be very little
> > overhead in having SMO enabled.  So if one wants to explore this
> > question then they can turn on the config option.  Please correct me if
> > I'm wrong.
> 
> The problem with a config option is that it's hard to switch over.
> 
> So just to test your changes in production a new kernel should be built,
> tested and rolled out to a representative set of machines (which can be
> measured in thousands of machines). Then if results are questionable,
> it should be rolled back.
> 
> What you're actually guarding is the kmem_cache_setup_mobility() call,
> which can be perfectly avoided using a boot option, for example. Turning
> it on and off completely dynamic isn't that hard too.

Hi Roman,

I've added a boot parameter to SLUB so that admins can enable/disable
SMO at boot time system wide.  Then for each object that implements SMO
(currently XArray and dcache) I've also added a boot parameter to
enable/disable SMO for that cache specifically (these depend on SMO
being enabled system wide).

All three boot parameters default to 'off', I've added a config option
to default each to 'on'.

I've got a little more testing to do on another part of the set then the
PATCH version is coming at you :)

This is more a courtesy email than a request for comment, but please
feel free to shout if you don't like the method outlined above.

Fully dynamic config is not currently possible because currently the SMO
implementation does not support disabling mobility for a cache once it
is turned on, a bit of extra logic would need to be added and some state
stored - I'm not sure it warrants it ATM but that can be easily added
later if wanted.  Maybe Christoph will give his opinion on this.

thanks,
Tobin.
Roman Gushchin May 29, 2019, 4:16 p.m. UTC | #6
On Wed, May 29, 2019 at 01:54:06PM +1000, Tobin C. Harding wrote:
> On Tue, May 21, 2019 at 02:05:38AM +0000, Roman Gushchin wrote:
> > On Tue, May 21, 2019 at 11:31:18AM +1000, Tobin C. Harding wrote:
> > > On Tue, May 21, 2019 at 12:57:47AM +0000, Roman Gushchin wrote:
> > > > On Mon, May 20, 2019 at 03:40:17PM +1000, Tobin C. Harding wrote:
> > > > > In an attempt to make the SMO patchset as non-invasive as possible add a
> > > > > config option CONFIG_DCACHE_SMO (under "Memory Management options") for
> > > > > enabling SMO for the DCACHE.  Whithout this option dcache constructor is
> > > > > used but no other code is built in, with this option enabled slab
> > > > > mobility is enabled and the isolate/migrate functions are built in.
> > > > > 
> > > > > Add CONFIG_DCACHE_SMO to guard the partial shrinking of the dcache via
> > > > > Slab Movable Objects infrastructure.
> > > > 
> > > > Hm, isn't it better to make it a static branch? Or basically anything
> > > > that allows switching on the fly?
> > > 
> > > If that is wanted, turning SMO on and off per cache, we can probably do
> > > this in the SMO code in SLUB.
> > 
> > Not necessarily per cache, but without recompiling the kernel.
> > > 
> > > > It seems that the cost of just building it in shouldn't be that high.
> > > > And the question if the defragmentation worth the trouble is so much
> > > > easier to answer if it's possible to turn it on and off without rebooting.
> > > 
> > > If the question is 'is defragmentation worth the trouble for the
> > > dcache', I'm not sure having SMO turned off helps answer that question.
> > > If one doesn't shrink the dentry cache there should be very little
> > > overhead in having SMO enabled.  So if one wants to explore this
> > > question then they can turn on the config option.  Please correct me if
> > > I'm wrong.
> > 
> > The problem with a config option is that it's hard to switch over.
> > 
> > So just to test your changes in production a new kernel should be built,
> > tested and rolled out to a representative set of machines (which can be
> > measured in thousands of machines). Then if results are questionable,
> > it should be rolled back.
> > 
> > What you're actually guarding is the kmem_cache_setup_mobility() call,
> > which can be perfectly avoided using a boot option, for example. Turning
> > it on and off completely dynamic isn't that hard too.
> 
> Hi Roman,
> 
> I've added a boot parameter to SLUB so that admins can enable/disable
> SMO at boot time system wide.  Then for each object that implements SMO
> (currently XArray and dcache) I've also added a boot parameter to
> enable/disable SMO for that cache specifically (these depend on SMO
> being enabled system wide).
> 
> All three boot parameters default to 'off', I've added a config option
> to default each to 'on'.
> 
> I've got a little more testing to do on another part of the set then the
> PATCH version is coming at you :)
> 
> This is more a courtesy email than a request for comment, but please
> feel free to shout if you don't like the method outlined above.
> 
> Fully dynamic config is not currently possible because currently the SMO
> implementation does not support disabling mobility for a cache once it
> is turned on, a bit of extra logic would need to be added and some state
> stored - I'm not sure it warrants it ATM but that can be easily added
> later if wanted.  Maybe Christoph will give his opinion on this.

Perfect!

Thanks.
Tobin Harding June 3, 2019, 4:26 a.m. UTC | #7
On Wed, May 29, 2019 at 04:16:51PM +0000, Roman Gushchin wrote:
> On Wed, May 29, 2019 at 01:54:06PM +1000, Tobin C. Harding wrote:
> > On Tue, May 21, 2019 at 02:05:38AM +0000, Roman Gushchin wrote:
> > > On Tue, May 21, 2019 at 11:31:18AM +1000, Tobin C. Harding wrote:
> > > > On Tue, May 21, 2019 at 12:57:47AM +0000, Roman Gushchin wrote:
> > > > > On Mon, May 20, 2019 at 03:40:17PM +1000, Tobin C. Harding wrote:
> > > > > > In an attempt to make the SMO patchset as non-invasive as possible add a
> > > > > > config option CONFIG_DCACHE_SMO (under "Memory Management options") for
> > > > > > enabling SMO for the DCACHE.  Whithout this option dcache constructor is
> > > > > > used but no other code is built in, with this option enabled slab
> > > > > > mobility is enabled and the isolate/migrate functions are built in.
> > > > > > 
> > > > > > Add CONFIG_DCACHE_SMO to guard the partial shrinking of the dcache via
> > > > > > Slab Movable Objects infrastructure.
> > > > > 
> > > > > Hm, isn't it better to make it a static branch? Or basically anything
> > > > > that allows switching on the fly?
> > > > 
> > > > If that is wanted, turning SMO on and off per cache, we can probably do
> > > > this in the SMO code in SLUB.
> > > 
> > > Not necessarily per cache, but without recompiling the kernel.
> > > > 
> > > > > It seems that the cost of just building it in shouldn't be that high.
> > > > > And the question if the defragmentation worth the trouble is so much
> > > > > easier to answer if it's possible to turn it on and off without rebooting.
> > > > 
> > > > If the question is 'is defragmentation worth the trouble for the
> > > > dcache', I'm not sure having SMO turned off helps answer that question.
> > > > If one doesn't shrink the dentry cache there should be very little
> > > > overhead in having SMO enabled.  So if one wants to explore this
> > > > question then they can turn on the config option.  Please correct me if
> > > > I'm wrong.
> > > 
> > > The problem with a config option is that it's hard to switch over.
> > > 
> > > So just to test your changes in production a new kernel should be built,
> > > tested and rolled out to a representative set of machines (which can be
> > > measured in thousands of machines). Then if results are questionable,
> > > it should be rolled back.
> > > 
> > > What you're actually guarding is the kmem_cache_setup_mobility() call,
> > > which can be perfectly avoided using a boot option, for example. Turning
> > > it on and off completely dynamic isn't that hard too.
> > 
> > Hi Roman,
> > 
> > I've added a boot parameter to SLUB so that admins can enable/disable
> > SMO at boot time system wide.  Then for each object that implements SMO
> > (currently XArray and dcache) I've also added a boot parameter to
> > enable/disable SMO for that cache specifically (these depend on SMO
> > being enabled system wide).
> > 
> > All three boot parameters default to 'off', I've added a config option
> > to default each to 'on'.
> > 
> > I've got a little more testing to do on another part of the set then the
> > PATCH version is coming at you :)
> > 
> > This is more a courtesy email than a request for comment, but please
> > feel free to shout if you don't like the method outlined above.
> > 
> > Fully dynamic config is not currently possible because currently the SMO
> > implementation does not support disabling mobility for a cache once it
> > is turned on, a bit of extra logic would need to be added and some state
> > stored - I'm not sure it warrants it ATM but that can be easily added
> > later if wanted.  Maybe Christoph will give his opinion on this.
> 
> Perfect!

Hi Roman,

I'm about to post PATCH series.  I have removed all the boot time config
options in contrast to what I stated in this thread.  I feel it requires
some comment so as not to seem rude to you.  Please feel free to
re-raise these issues on the series if you feel it is a better place to
do it than on this thread.

I still hear you re making testing easier if there are boot parameters.
I don't have extensive experience testing on a large number of machines
so I have no basis to contradict what you said.

It was suggested to me that having switches to turn SMO off implies the
series is not ready.  I am claiming that SMO _is_ ready and also that it
has no negative effects (especially on the dcache).  I therefore think
this comment is pertinent.

So ... I re-did the boot parameters defaulting to 'on'.  However I could
then see no reason (outside of testing) to turn them off.  It seems ugly
to have code that is only required during testing and never after.
Please correct me if I'm wrong.

Finally I decided that since adding a boot parameter is trivial that
hackers could easily add one to test if they wanted to test a specific
cache.  Otherwise we just test 'patched kernel' vs 'unpatched kernel'.
Again, please correct me if I'm wrong.

So, that said, please feel free to voice your opinion as strongly as you
wish.  I am super appreciative of the time you have already taken to
look at these patches.  I hope I have made the best technical decision,
and I am totally open to being told I'm wrong :)

thanks,
Tobin.
Roman Gushchin June 3, 2019, 8:34 p.m. UTC | #8
On Mon, Jun 03, 2019 at 02:26:20PM +1000, Tobin C. Harding wrote:
> On Wed, May 29, 2019 at 04:16:51PM +0000, Roman Gushchin wrote:
> > On Wed, May 29, 2019 at 01:54:06PM +1000, Tobin C. Harding wrote:
> > > On Tue, May 21, 2019 at 02:05:38AM +0000, Roman Gushchin wrote:
> > > > On Tue, May 21, 2019 at 11:31:18AM +1000, Tobin C. Harding wrote:
> > > > > On Tue, May 21, 2019 at 12:57:47AM +0000, Roman Gushchin wrote:
> > > > > > On Mon, May 20, 2019 at 03:40:17PM +1000, Tobin C. Harding wrote:
> > > > > > > In an attempt to make the SMO patchset as non-invasive as possible add a
> > > > > > > config option CONFIG_DCACHE_SMO (under "Memory Management options") for
> > > > > > > enabling SMO for the DCACHE.  Whithout this option dcache constructor is
> > > > > > > used but no other code is built in, with this option enabled slab
> > > > > > > mobility is enabled and the isolate/migrate functions are built in.
> > > > > > > 
> > > > > > > Add CONFIG_DCACHE_SMO to guard the partial shrinking of the dcache via
> > > > > > > Slab Movable Objects infrastructure.
> > > > > > 
> > > > > > Hm, isn't it better to make it a static branch? Or basically anything
> > > > > > that allows switching on the fly?
> > > > > 
> > > > > If that is wanted, turning SMO on and off per cache, we can probably do
> > > > > this in the SMO code in SLUB.
> > > > 
> > > > Not necessarily per cache, but without recompiling the kernel.
> > > > > 
> > > > > > It seems that the cost of just building it in shouldn't be that high.
> > > > > > And the question if the defragmentation worth the trouble is so much
> > > > > > easier to answer if it's possible to turn it on and off without rebooting.
> > > > > 
> > > > > If the question is 'is defragmentation worth the trouble for the
> > > > > dcache', I'm not sure having SMO turned off helps answer that question.
> > > > > If one doesn't shrink the dentry cache there should be very little
> > > > > overhead in having SMO enabled.  So if one wants to explore this
> > > > > question then they can turn on the config option.  Please correct me if
> > > > > I'm wrong.
> > > > 
> > > > The problem with a config option is that it's hard to switch over.
> > > > 
> > > > So just to test your changes in production a new kernel should be built,
> > > > tested and rolled out to a representative set of machines (which can be
> > > > measured in thousands of machines). Then if results are questionable,
> > > > it should be rolled back.
> > > > 
> > > > What you're actually guarding is the kmem_cache_setup_mobility() call,
> > > > which can be perfectly avoided using a boot option, for example. Turning
> > > > it on and off completely dynamic isn't that hard too.
> > > 
> > > Hi Roman,
> > > 
> > > I've added a boot parameter to SLUB so that admins can enable/disable
> > > SMO at boot time system wide.  Then for each object that implements SMO
> > > (currently XArray and dcache) I've also added a boot parameter to
> > > enable/disable SMO for that cache specifically (these depend on SMO
> > > being enabled system wide).
> > > 
> > > All three boot parameters default to 'off', I've added a config option
> > > to default each to 'on'.
> > > 
> > > I've got a little more testing to do on another part of the set then the
> > > PATCH version is coming at you :)
> > > 
> > > This is more a courtesy email than a request for comment, but please
> > > feel free to shout if you don't like the method outlined above.
> > > 
> > > Fully dynamic config is not currently possible because currently the SMO
> > > implementation does not support disabling mobility for a cache once it
> > > is turned on, a bit of extra logic would need to be added and some state
> > > stored - I'm not sure it warrants it ATM but that can be easily added
> > > later if wanted.  Maybe Christoph will give his opinion on this.
> > 
> > Perfect!
> 
> Hi Roman,
> 
> I'm about to post PATCH series.  I have removed all the boot time config
> options in contrast to what I stated in this thread.  I feel it requires
> some comment so as not to seem rude to you.  Please feel free to
> re-raise these issues on the series if you feel it is a better place to
> do it than on this thread.
> 
> I still hear you re making testing easier if there are boot parameters.
> I don't have extensive experience testing on a large number of machines
> so I have no basis to contradict what you said.
> 
> It was suggested to me that having switches to turn SMO off implies the
> series is not ready.  I am claiming that SMO _is_ ready and also that it
> has no negative effects (especially on the dcache).  I therefore think
> this comment is pertinent.
> 
> So ... I re-did the boot parameters defaulting to 'on'.  However I could
> then see no reason (outside of testing) to turn them off.  It seems ugly
> to have code that is only required during testing and never after.
> Please correct me if I'm wrong.
> 
> Finally I decided that since adding a boot parameter is trivial that
> hackers could easily add one to test if they wanted to test a specific
> cache.  Otherwise we just test 'patched kernel' vs 'unpatched kernel'.
> Again, please correct me if I'm wrong.
> 
> So, that said, please feel free to voice your opinion as strongly as you
> wish.  I am super appreciative of the time you have already taken to
> look at these patches.  I hope I have made the best technical decision,
> and I am totally open to being told I'm wrong :)

Hi Tobin!

No boot options looks totally fine to me. I just don't like new config
options. No options at all is always the best.

Btw, thank you for this clarification!

I'll definitely try to look into the patchset on this week.

Thanks!
diff mbox series

Patch

diff --git a/fs/dcache.c b/fs/dcache.c
index 0dfe580c2d42..96063e872366 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -3072,6 +3072,7 @@  void d_tmpfile(struct dentry *dentry, struct inode *inode)
 }
 EXPORT_SYMBOL(d_tmpfile);
 
+#ifdef CONFIG_DCACHE_SMO
 /*
  * d_isolate() - Dentry isolation callback function.
  * @s: The dentry cache.
@@ -3144,6 +3145,7 @@  static void d_partial_shrink(struct kmem_cache *s, void **_unused, int __unused,
 
 	kfree(private);
 }
+#endif	/* CONFIG_DCACHE_SMO */
 
 static __initdata unsigned long dhash_entries;
 static int __init set_dhash_entries(char *str)
@@ -3190,7 +3192,9 @@  static void __init dcache_init(void)
 					   sizeof_field(struct dentry, d_iname),
 					   dcache_ctor);
 
+#ifdef CONFIG_DCACHE_SMO
 	kmem_cache_setup_mobility(dentry_cache, d_isolate, d_partial_shrink);
+#endif
 
 	/* Hash may have been set up in dcache_init_early */
 	if (!hashdist)
diff --git a/mm/Kconfig b/mm/Kconfig
index aa8d60e69a01..7dcea76e5ecc 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -265,6 +265,13 @@  config SMO_NODE
        help
          On NUMA systems enable moving objects to and from a specified node.
 
+config DCACHE_SMO
+       bool "Enable Slab Movable Objects for the dcache"
+       depends on SLUB
+       help
+         Under memory pressure we can try to free dentry slab cache objects from
+         the partial slab list if this is enabled.
+
 config PHYS_ADDR_T_64BIT
 	def_bool 64BIT