[PATCH] mm/zswap: change params from hidden to ro

[PATCH] mm/zswap: change params from hidden to ro

[Dan Streetman]

The "compressor" and "enabled" params are currently hidden,
this changes them to read-only, so userspace can tell if
zswap is enabled or not and see what compressor is in use.

Signed-off-by: Dan Streetman <ddstreet@ieee.org>
---
 mm/zswap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index d93510c..36b268b 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -77,12 +77,12 @@ static u64 zswap_duplicate_entry;
 **********************************/
 /* Enable/disable zswap (disabled by default, fixed at boot for now) */
 static bool zswap_enabled __read_mostly;
-module_param_named(enabled, zswap_enabled, bool, 0);
+module_param_named(enabled, zswap_enabled, bool, 0444);
 
 /* Compressor to be used by zswap (fixed at boot for now) */
 #define ZSWAP_COMPRESSOR_DEFAULT "lzo"
 static char *zswap_compressor = ZSWAP_COMPRESSOR_DEFAULT;
-module_param_named(compressor, zswap_compressor, charp, 0);
+module_param_named(compressor, zswap_compressor, charp, 0444);
 
 /* The maximum percentage of memory that the compressed pool can occupy */
 static unsigned int zswap_max_pool_percent = 20;
-- 
1.8.3.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zswap: change params from hidden to ro

[Vladimir Murzin]

Hi Dan!

On Wed, Nov 20, 2013 at 11:38:42AM -0500, Dan Streetman wrote:
> The "compressor" and "enabled" params are currently hidden,
> this changes them to read-only, so userspace can tell if
> zswap is enabled or not and see what compressor is in use.

Could you elaborate more why this pice of information is necessary for
userspace?

Vladimir

> 
> Signed-off-by: Dan Streetman <ddstreet@ieee.org>
> ---
>  mm/zswap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/zswap.c b/mm/zswap.c
> index d93510c..36b268b 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -77,12 +77,12 @@ static u64 zswap_duplicate_entry;
>  **********************************/
>  /* Enable/disable zswap (disabled by default, fixed at boot for now) */
>  static bool zswap_enabled __read_mostly;
> -module_param_named(enabled, zswap_enabled, bool, 0);
> +module_param_named(enabled, zswap_enabled, bool, 0444);
>  
>  /* Compressor to be used by zswap (fixed at boot for now) */
>  #define ZSWAP_COMPRESSOR_DEFAULT "lzo"
>  static char *zswap_compressor = ZSWAP_COMPRESSOR_DEFAULT;
> -module_param_named(compressor, zswap_compressor, charp, 0);
> +module_param_named(compressor, zswap_compressor, charp, 0444);
>  
>  /* The maximum percentage of memory that the compressed pool can occupy */
>  static unsigned int zswap_max_pool_percent = 20;
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zswap: change params from hidden to ro

[Dan Streetman]

On Wed, Nov 20, 2013 at 12:33 PM, Vladimir Murzin <murzin.v@gmail.com> wrote:
> Hi Dan!
>
> On Wed, Nov 20, 2013 at 11:38:42AM -0500, Dan Streetman wrote:
>> The "compressor" and "enabled" params are currently hidden,
>> this changes them to read-only, so userspace can tell if
>> zswap is enabled or not and see what compressor is in use.
>
> Could you elaborate more why this pice of information is necessary for
> userspace?

For anyone interested in zswap, it's handy to be able to tell if it's
enabled or not ;-)  Technically people can check to see if the zswap
debug files are in /sys/kernel/debug/zswap, but I think the actual
"enabled" param is more obvious.  And the compressor param is really
the only way anyone from userspace can see what compressor's being
used; that's helpful to know for anyone that might want to be using a
non-default compressor.

And of course, eventually we'll want to make the params writable, so
the compressor can be changed dynamically, and zswap can be enabled or
disabled dynamically (or at least enabled after boot).

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zswap: change params from hidden to ro

[Weijie Yang]

On Thu, Nov 21, 2013 at 1:52 AM, Dan Streetman <ddstreet@ieee.org> wrote:
> On Wed, Nov 20, 2013 at 12:33 PM, Vladimir Murzin <murzin.v@gmail.com> wrote:
>> Hi Dan!
>>
>> On Wed, Nov 20, 2013 at 11:38:42AM -0500, Dan Streetman wrote:
>>> The "compressor" and "enabled" params are currently hidden,
>>> this changes them to read-only, so userspace can tell if
>>> zswap is enabled or not and see what compressor is in use.
>>
>> Could you elaborate more why this pice of information is necessary for
>> userspace?
>
> For anyone interested in zswap, it's handy to be able to tell if it's
> enabled or not ;-)  Technically people can check to see if the zswap
> debug files are in /sys/kernel/debug/zswap, but I think the actual
> "enabled" param is more obvious.  And the compressor param is really
> the only way anyone from userspace can see what compressor's being
> used; that's helpful to know for anyone that might want to be using a
> non-default compressor.
>
> And of course, eventually we'll want to make the params writable, so
> the compressor can be changed dynamically, and zswap can be enabled or
> disabled dynamically (or at least enabled after boot).

Please do not make them writable.
There is no requirement to do that, and it will make zswap more complex.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zswap: change params from hidden to ro

[Dan Streetman]

On Wed, Nov 20, 2013 at 10:12 PM, Weijie Yang <weijie.yang.kh@gmail.com> wrote:
> On Thu, Nov 21, 2013 at 1:52 AM, Dan Streetman <ddstreet@ieee.org> wrote:
>> On Wed, Nov 20, 2013 at 12:33 PM, Vladimir Murzin <murzin.v@gmail.com> wrote:
>>> Hi Dan!
>>>
>>> On Wed, Nov 20, 2013 at 11:38:42AM -0500, Dan Streetman wrote:
>>>> The "compressor" and "enabled" params are currently hidden,
>>>> this changes them to read-only, so userspace can tell if
>>>> zswap is enabled or not and see what compressor is in use.
>>>
>>> Could you elaborate more why this pice of information is necessary for
>>> userspace?
>>
>> For anyone interested in zswap, it's handy to be able to tell if it's
>> enabled or not ;-)  Technically people can check to see if the zswap
>> debug files are in /sys/kernel/debug/zswap, but I think the actual
>> "enabled" param is more obvious.  And the compressor param is really
>> the only way anyone from userspace can see what compressor's being
>> used; that's helpful to know for anyone that might want to be using a
>> non-default compressor.
>>
>> And of course, eventually we'll want to make the params writable, so
>> the compressor can be changed dynamically, and zswap can be enabled or
>> disabled dynamically (or at least enabled after boot).
>
> Please do not make them writable.

This patch doesn't make them writable.

> There is no requirement to do that, and it will make zswap more complex.

I can't speak for others, but I personally think being able to at
least enable zswap after boot will be useful, as well as being able to
change the compressor after boot.  Obviously it will make zswap more
complex, but that's the nature of improving software ;-)

>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zswap: change params from hidden to ro

[Vladimir Murzin]

On Wed, Nov 20, 2013 at 12:52:47PM -0500, Dan Streetman wrote: > On Wed, Nov
20, 2013 at 12:33 PM, Vladimir Murzin <murzin.v@gmail.com> wrote: > > Hi Dan!
> >
> > On Wed, Nov 20, 2013 at 11:38:42AM -0500, Dan Streetman wrote:
> >> The "compressor" and "enabled" params are currently hidden,
> >> this changes them to read-only, so userspace can tell if
> >> zswap is enabled or not and see what compressor is in use.
> >
> > Could you elaborate more why this pice of information is necessary for
> > userspace?
> 
> For anyone interested in zswap, it's handy to be able to tell if it's
> enabled or not ;-)  Technically people can check to see if the zswap
> debug files are in /sys/kernel/debug/zswap, but I think the actual
> "enabled" param is more obvious.  And the compressor param is really
> the only way anyone from userspace can see what compressor's being
> used; that's helpful to know for anyone that might want to be using a
> non-default compressor.

So, it is needed for user not userspace? I tend to think that users are smart
enough to check cmdline for that. 

AFAICS module_param exist here to provide simplest ability to handle the
setting via cmdline. zsawap is not able to be loaded as a module for now. If
it could, than there was reason to check which params were used while module
loading and guess how they affected zswap state. 

> 
> And of course, eventually we'll want to make the params writable, so
> the compressor can be changed dynamically, and zswap can be enabled or
> disabled dynamically (or at least enabled after boot).

module_params is not the best place to handle this, because they do not
provide any hooks for handling write unless I missed something. However,
making zswap state dynamically adjustable require not only setting the
numbers, but handling the correctness switching from one state to another.

Vladimir

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zswap: change params from hidden to ro

[Dan Streetman]

On Fri, Nov 22, 2013 at 2:38 AM, Vladimir Murzin <murzin.v@gmail.com> wrote:
> On Wed, Nov 20, 2013 at 12:52:47PM -0500, Dan Streetman wrote: > On Wed, Nov
> 20, 2013 at 12:33 PM, Vladimir Murzin <murzin.v@gmail.com> wrote: > > Hi Dan!
>> >
>> > On Wed, Nov 20, 2013 at 11:38:42AM -0500, Dan Streetman wrote:
>> >> The "compressor" and "enabled" params are currently hidden,
>> >> this changes them to read-only, so userspace can tell if
>> >> zswap is enabled or not and see what compressor is in use.
>> >
>> > Could you elaborate more why this pice of information is necessary for
>> > userspace?
>>
>> For anyone interested in zswap, it's handy to be able to tell if it's
>> enabled or not ;-)  Technically people can check to see if the zswap
>> debug files are in /sys/kernel/debug/zswap, but I think the actual
>> "enabled" param is more obvious.  And the compressor param is really
>> the only way anyone from userspace can see what compressor's being
>> used; that's helpful to know for anyone that might want to be using a
>> non-default compressor.
>
> So, it is needed for user not userspace? I tend to think that users are smart
> enough to check cmdline for that.

Let's try a different way.  Can you explain what the problem is with
making these params user-readable?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zswap: change params from hidden to ro

[Seth Jennings]

On Wed, Nov 20, 2013 at 11:38:42AM -0500, Dan Streetman wrote:
> The "compressor" and "enabled" params are currently hidden,
> this changes them to read-only, so userspace can tell if
> zswap is enabled or not and see what compressor is in use.

Reasonable to me.

Acked-by: Seth Jennings <sjennings@variantweb.net>

> 
> Signed-off-by: Dan Streetman <ddstreet@ieee.org>
> ---
>  mm/zswap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/zswap.c b/mm/zswap.c
> index d93510c..36b268b 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -77,12 +77,12 @@ static u64 zswap_duplicate_entry;
>  **********************************/
>  /* Enable/disable zswap (disabled by default, fixed at boot for now) */
>  static bool zswap_enabled __read_mostly;
> -module_param_named(enabled, zswap_enabled, bool, 0);
> +module_param_named(enabled, zswap_enabled, bool, 0444);
>  
>  /* Compressor to be used by zswap (fixed at boot for now) */
>  #define ZSWAP_COMPRESSOR_DEFAULT "lzo"
>  static char *zswap_compressor = ZSWAP_COMPRESSOR_DEFAULT;
> -module_param_named(compressor, zswap_compressor, charp, 0);
> +module_param_named(compressor, zswap_compressor, charp, 0444);
>  
>  /* The maximum percentage of memory that the compressed pool can occupy */
>  static unsigned int zswap_max_pool_percent = 20;
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zswap: change params from hidden to ro

[Vladimir Murzin]

On Fri, Nov 22, 2013 at 12:18:01PM -0500, Dan Streetman wrote:
> On Fri, Nov 22, 2013 at 2:38 AM, Vladimir Murzin <murzin.v@gmail.com> wrote:
> > On Wed, Nov 20, 2013 at 12:52:47PM -0500, Dan Streetman wrote: > On Wed, Nov
> > 20, 2013 at 12:33 PM, Vladimir Murzin <murzin.v@gmail.com> wrote: > > Hi Dan!
> >> >
> >> > On Wed, Nov 20, 2013 at 11:38:42AM -0500, Dan Streetman wrote:
> >> >> The "compressor" and "enabled" params are currently hidden,
> >> >> this changes them to read-only, so userspace can tell if
> >> >> zswap is enabled or not and see what compressor is in use.
> >> >
> >> > Could you elaborate more why this pice of information is necessary for
> >> > userspace?
> >>
> >> For anyone interested in zswap, it's handy to be able to tell if it's
> >> enabled or not ;-)  Technically people can check to see if the zswap
> >> debug files are in /sys/kernel/debug/zswap, but I think the actual
> >> "enabled" param is more obvious.  And the compressor param is really
> >> the only way anyone from userspace can see what compressor's being
> >> used; that's helpful to know for anyone that might want to be using a
> >> non-default compressor.
> >
> > So, it is needed for user not userspace? I tend to think that users are smart
> > enough to check cmdline for that.
> 
> Let's try a different way.  Can you explain what the problem is with
> making these params user-readable?

This patch is absolutely neutral for me - nothing bad and nothing good. I've just
been curious what argument for this patch you have except "let it be".

Thanks
Vadimir

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zswap: change params from hidden to ro

[Dan Streetman]

On Fri, Nov 22, 2013 at 12:21 PM, Seth Jennings
<sjennings@variantweb.net> wrote:
> On Wed, Nov 20, 2013 at 11:38:42AM -0500, Dan Streetman wrote:
>> The "compressor" and "enabled" params are currently hidden,
>> this changes them to read-only, so userspace can tell if
>> zswap is enabled or not and see what compressor is in use.
>
> Reasonable to me.
>
> Acked-by: Seth Jennings <sjennings@variantweb.net>

Ping to see if this patch could get picked up.

>
>>
>> Signed-off-by: Dan Streetman <ddstreet@ieee.org>
>> ---
>>  mm/zswap.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/zswap.c b/mm/zswap.c
>> index d93510c..36b268b 100644
>> --- a/mm/zswap.c
>> +++ b/mm/zswap.c
>> @@ -77,12 +77,12 @@ static u64 zswap_duplicate_entry;
>>  **********************************/
>>  /* Enable/disable zswap (disabled by default, fixed at boot for now) */
>>  static bool zswap_enabled __read_mostly;
>> -module_param_named(enabled, zswap_enabled, bool, 0);
>> +module_param_named(enabled, zswap_enabled, bool, 0444);
>>
>>  /* Compressor to be used by zswap (fixed at boot for now) */
>>  #define ZSWAP_COMPRESSOR_DEFAULT "lzo"
>>  static char *zswap_compressor = ZSWAP_COMPRESSOR_DEFAULT;
>> -module_param_named(compressor, zswap_compressor, charp, 0);
>> +module_param_named(compressor, zswap_compressor, charp, 0444);
>>
>>  /* The maximum percentage of memory that the compressed pool can occupy */
>>  static unsigned int zswap_max_pool_percent = 20;
>> --
>> 1.8.3.1
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>