mm: memcontrol: clarify swapaccount=0 deprecation warning

[PATCH] mm: memcontrol: clarify swapaccount=0 deprecation warning

Posted by Johannes Weiner 1 year, 10 months ago

The swapaccount deprecation warning is throwing false positives. Since
we deprecated the knob and defaulted to enabling, the only reports
we've been getting are from folks that set swapaccount=1. While this
is a nice affirmation that always-enabling was the right choice, we
certainly don't want to warn when users request the supported mode.

Only warn when disabling is requested, and clarify the warning.

Fixes: b25806dcd3d5 ("mm: memcontrol: deprecate swapaccounting=0 mode")
Cc: stable@vger.kernel.org
Reported-by: "Jonas Schäfer" <jonas@wielicki.name>
Reported-by: Narcis Garcia <debianlists@actiu.net>
Suggested-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/memcontrol.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 1ed40f9d3a27..107ec5d36819 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -7971,9 +7971,13 @@ bool mem_cgroup_swap_full(struct folio *folio)
 
 static int __init setup_swap_account(char *s)
 {
-	pr_warn_once("The swapaccount= commandline option is deprecated. "
-		     "Please report your usecase to linux-mm@kvack.org if you "
-		     "depend on this functionality.\n");
+	bool res;
+
+	if (!kstrtobool(s, &res) && !res)
+		pr_warn_once("The swapaccount=0 commdandline option is deprecated "
+			     "in favor of configuring swap control via cgroupfs. "
+			     "Please report your usecase to linux-mm@kvack.org if you "
+			     "depend on this functionality.\n");
 	return 1;
 }
 __setup("swapaccount=", setup_swap_account);
-- 
2.43.0

Re: [PATCH] mm: memcontrol: clarify swapaccount=0 deprecation warning

Posted by Michal Koutný 1 year, 10 months ago

On Tue, Feb 13, 2024 at 03:16:34AM -0500, Johannes Weiner <hannes@cmpxchg.org> wrote:
> The swapaccount deprecation warning is throwing false positives. Since
> we deprecated the knob and defaulted to enabling, the only reports
> we've been getting are from folks that set swapaccount=1. While this
> is a nice affirmation that always-enabling was the right choice, we
> certainly don't want to warn when users request the supported mode.

But shouldn't such users be still warned about effectively unused option?
I think `return 0;` from the param handler should ensure that.


> +	if (!kstrtobool(s, &res) && !res)
> +		pr_warn_once("The swapaccount=0 commdandline option is deprecated "
                                                commandline

Regards,
Michal

Re: [PATCH] mm: memcontrol: clarify swapaccount=0 deprecation warning

Posted by Shakeel Butt 1 year, 10 months ago

On Tue, Feb 13, 2024 at 12:16 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> The swapaccount deprecation warning is throwing false positives. Since
> we deprecated the knob and defaulted to enabling, the only reports
> we've been getting are from folks that set swapaccount=1. While this
> is a nice affirmation that always-enabling was the right choice, we
> certainly don't want to warn when users request the supported mode.
>
> Only warn when disabling is requested, and clarify the warning.
>
> Fixes: b25806dcd3d5 ("mm: memcontrol: deprecate swapaccounting=0 mode")
> Cc: stable@vger.kernel.org
> Reported-by: "Jonas Schäfer" <jonas@wielicki.name>
> Reported-by: Narcis Garcia <debianlists@actiu.net>
> Suggested-by: Yosry Ahmed <yosryahmed@google.com>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

Acked-by: Shakeel Butt <shakeelb@google.com>

Re: [PATCH] mm: memcontrol: clarify swapaccount=0 deprecation warning

Posted by Michal Hocko 1 year, 10 months ago

On Tue 13-02-24 03:16:34, Johannes Weiner wrote:
> The swapaccount deprecation warning is throwing false positives. Since
> we deprecated the knob and defaulted to enabling, the only reports
> we've been getting are from folks that set swapaccount=1. While this
> is a nice affirmation that always-enabling was the right choice, we
> certainly don't want to warn when users request the supported mode.
> 
> Only warn when disabling is requested, and clarify the warning.
> 
> Fixes: b25806dcd3d5 ("mm: memcontrol: deprecate swapaccounting=0 mode")
> Cc: stable@vger.kernel.org
> Reported-by: "Jonas Schäfer" <jonas@wielicki.name>
> Reported-by: Narcis Garcia <debianlists@actiu.net>
> Suggested-by: Yosry Ahmed <yosryahmed@google.com>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

> ---
>  mm/memcontrol.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 1ed40f9d3a27..107ec5d36819 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -7971,9 +7971,13 @@ bool mem_cgroup_swap_full(struct folio *folio)
>  
>  static int __init setup_swap_account(char *s)
>  {
> -	pr_warn_once("The swapaccount= commandline option is deprecated. "
> -		     "Please report your usecase to linux-mm@kvack.org if you "
> -		     "depend on this functionality.\n");
> +	bool res;
> +
> +	if (!kstrtobool(s, &res) && !res)
> +		pr_warn_once("The swapaccount=0 commdandline option is deprecated "
> +			     "in favor of configuring swap control via cgroupfs. "
> +			     "Please report your usecase to linux-mm@kvack.org if you "
> +			     "depend on this functionality.\n");
>  	return 1;
>  }
>  __setup("swapaccount=", setup_swap_account);
> -- 
> 2.43.0

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: memcontrol: clarify swapaccount=0 deprecation warning

Posted by Yosry Ahmed 1 year, 10 months ago

On Tue, Feb 13, 2024 at 12:16 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> The swapaccount deprecation warning is throwing false positives. Since
> we deprecated the knob and defaulted to enabling, the only reports
> we've been getting are from folks that set swapaccount=1. While this
> is a nice affirmation that always-enabling was the right choice, we
> certainly don't want to warn when users request the supported mode.
>
> Only warn when disabling is requested, and clarify the warning.
>
> Fixes: b25806dcd3d5 ("mm: memcontrol: deprecate swapaccounting=0 mode")
> Cc: stable@vger.kernel.org
> Reported-by: "Jonas Schäfer" <jonas@wielicki.name>
> Reported-by: Narcis Garcia <debianlists@actiu.net>
> Suggested-by: Yosry Ahmed <yosryahmed@google.com>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

Reviewed-by: Yosry Ahmed <yosryahmed@google.com>

> ---
>  mm/memcontrol.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 1ed40f9d3a27..107ec5d36819 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -7971,9 +7971,13 @@ bool mem_cgroup_swap_full(struct folio *folio)
>
>  static int __init setup_swap_account(char *s)
>  {
> -       pr_warn_once("The swapaccount= commandline option is deprecated. "
> -                    "Please report your usecase to linux-mm@kvack.org if you "
> -                    "depend on this functionality.\n");
> +       bool res;
> +
> +       if (!kstrtobool(s, &res) && !res)
> +               pr_warn_once("The swapaccount=0 commdandline option is deprecated "
> +                            "in favor of configuring swap control via cgroupfs. "
> +                            "Please report your usecase to linux-mm@kvack.org if you "
> +                            "depend on this functionality.\n");

This line is surely getting long, but I see other similar instances so
I guess that's okay.

>         return 1;
>  }
>  __setup("swapaccount=", setup_swap_account);
> --
> 2.43.0
>