[PATCH -next 0/4] Introduce several opposite string choice helpers

[PATCH -next 0/4] Introduce several opposite string choice helpers

[Hongbo Li]

Similar to the exists helper: str_enable_disable/
str_enabled_disabled/str_on_off/str_yes_no helpers, we can
add the opposite helpers. That's str_disable_enable,
str_disabled_enabled, str_off_on and str_no_yes.

There are more than 10 cases currently (expect
str_disable_enable now has 3 use cases) exist in the code
can be replaced with these helper.

Patch 1: Introduce the string choice helpers
Patch 2~4: Give the relative use cases to use these helpers.

Hongbo Li (4):
  lib/string_choices: Introduce several opposite string choice helpers
  tun: Make use of str_disabled_enabled helper
  mm: page_alloc: Make use of str_off_on helper
  net: sock: Make use of str_no_yes() helper

 drivers/net/tun.c              | 2 +-
 include/linux/string_choices.h | 4 ++++
 mm/page_alloc.c                | 3 +--
 net/core/sock.c                | 2 +-
 4 files changed, 7 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH -next 1/4] lib/string_choices: Introduce several opposite string choice helpers

[Hongbo Li]

Similar to the exists helper: str_enable_disable/
str_enabled_disabled/str_on_off/str_yes_no helpers, we can
add the opposite helpers. That's str_disable_enable,
str_disabled_enabled, str_off_on and str_no_yes.

There are more than 10 cases currently (expect
str_disable_enable now has 3 use cases) exist in the code
can be replaced with these helper.

Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 include/linux/string_choices.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/string_choices.h b/include/linux/string_choices.h
index f3670dbd1169..c2134eeda1fd 100644
--- a/include/linux/string_choices.h
+++ b/include/linux/string_choices.h
@@ -8,11 +8,13 @@ static inline const char *str_enable_disable(bool v)
 {
 	return v ? "enable" : "disable";
 }
+#define str_disable_enable(v)		str_enable_disable(!(v))
 
 static inline const char *str_enabled_disabled(bool v)
 {
 	return v ? "enabled" : "disabled";
 }
+#define str_disabled_enabled(v)		str_enabled_disabled(!(v))
 
 static inline const char *str_hi_lo(bool v)
 {
@@ -36,11 +38,13 @@ static inline const char *str_on_off(bool v)
 {
 	return v ? "on" : "off";
 }
+#define str_off_on(v)		str_on_off(!(v))
 
 static inline const char *str_yes_no(bool v)
 {
 	return v ? "yes" : "no";
 }
+#define str_no_yes(v)		str_yes_no(!(v))
 
 static inline const char *str_true_false(bool v)
 {
-- 
2.34.1

[PATCH -next 2/4] tun: Make use of str_disabled_enabled helper

[Hongbo Li]

Use str_disabled_enabled() helper instead of open
coding the same.

Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 drivers/net/tun.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 6fe5e8f7017c..29647704bda8 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -3178,7 +3178,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
 
 		/* [unimplemented] */
 		netif_info(tun, drv, tun->dev, "ignored: set checksum %s\n",
-			   arg ? "disabled" : "enabled");
+			   str_disabled_enabled(arg));
 		break;
 
 	case TUNSETPERSIST:
-- 
2.34.1

Re: [PATCH -next 2/4] tun: Make use of str_disabled_enabled helper

[Jakub Kicinski]

On Sat, 31 Aug 2024 17:58:38 +0800 Hongbo Li wrote:
> Use str_disabled_enabled() helper instead of open
> coding the same.

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 6fe5e8f7017c..29647704bda8 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -3178,7 +3178,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
>  
>  		/* [unimplemented] */
>  		netif_info(tun, drv, tun->dev, "ignored: set checksum %s\n",
> -			   arg ? "disabled" : "enabled");
> +			   str_disabled_enabled(arg));

You don't explain the 'why'. How is this an improvement?
nack on this and 2 similar networking changes you sent

Re: [PATCH -next 2/4] tun: Make use of str_disabled_enabled helper

[Hongbo Li]

On 2024/9/1 4:07, Jakub Kicinski wrote:
> On Sat, 31 Aug 2024 17:58:38 +0800 Hongbo Li wrote:
>> Use str_disabled_enabled() helper instead of open
>> coding the same.
> 
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index 6fe5e8f7017c..29647704bda8 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
>> @@ -3178,7 +3178,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
>>   
>>   		/* [unimplemented] */
>>   		netif_info(tun, drv, tun->dev, "ignored: set checksum %s\n",
>> -			   arg ? "disabled" : "enabled");
>> +			   str_disabled_enabled(arg));
> 
> You don't explain the 'why'. How is this an improvement?
> nack on this and 2 similar networking changes you sent
> 
This just give an example of using lib/string_choices' helper which 
prevents the function from being removed due to detection of non-use. 
These helpers are convenient. It's functionally equivalent, this avoids 
the dispersion of such writing styles( ? XXX : noXXX) everywhere.

Thanks,
Hongbo

Re: [PATCH -next 2/4] tun: Make use of str_disabled_enabled helper

[Gal Pressman]

On 31/08/2024 23:07, Jakub Kicinski wrote:
> On Sat, 31 Aug 2024 17:58:38 +0800 Hongbo Li wrote:
>> Use str_disabled_enabled() helper instead of open
>> coding the same.
> 
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index 6fe5e8f7017c..29647704bda8 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
>> @@ -3178,7 +3178,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
>>  
>>  		/* [unimplemented] */
>>  		netif_info(tun, drv, tun->dev, "ignored: set checksum %s\n",
>> -			   arg ? "disabled" : "enabled");
>> +			   str_disabled_enabled(arg));
> 
> You don't explain the 'why'. How is this an improvement?
> nack on this and 2 similar networking changes you sent
> 

Are you against the concept of string_choices in general, or this
specific change?

Re: [PATCH -next 2/4] tun: Make use of str_disabled_enabled helper

[Jakub Kicinski]

On Mon, 2 Sep 2024 09:10:33 +0300 Gal Pressman wrote:
> > You don't explain the 'why'. How is this an improvement?
> > nack on this and 2 similar networking changes you sent
> 
> Are you against the concept of string_choices in general, or this
> specific change?

Willem verbalized my opinion better than I can.

If a (driver|subsystem) maintainer likes string_choices that's
perfectly fine, but IMHO they are not worth converting to, and
not something we should suggest during review.

Re: [PATCH -next 2/4] tun: Make use of str_disabled_enabled helper

[Andy Shevchenko]

On Sat, Aug 31, 2024 at 01:07:41PM -0700, Jakub Kicinski wrote:
> On Sat, 31 Aug 2024 17:58:38 +0800 Hongbo Li wrote:
> > Use str_disabled_enabled() helper instead of open
> > coding the same.

...

> >  		netif_info(tun, drv, tun->dev, "ignored: set checksum %s\n",
> > -			   arg ? "disabled" : "enabled");
> > +			   str_disabled_enabled(arg));
> 
> You don't explain the 'why'. How is this an improvement?
> nack on this and 2 similar networking changes you sent

Side opinion: This makes the messages more unified and not prone to typos
and/or grammatical mistakes. Unification allows to shrink binary due to
linker efforts on string literals deduplication.

That said, I see an improvement here, however it might be not recognised
as a Big Win.

And yes, I agree on the commit message poor explanations.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH -next 2/4] tun: Make use of str_disabled_enabled helper

[Willem de Bruijn]

Andy Shevchenko wrote:
> On Sat, Aug 31, 2024 at 01:07:41PM -0700, Jakub Kicinski wrote:
> > On Sat, 31 Aug 2024 17:58:38 +0800 Hongbo Li wrote:
> > > Use str_disabled_enabled() helper instead of open
> > > coding the same.
> 
> ...
> 
> > >  		netif_info(tun, drv, tun->dev, "ignored: set checksum %s\n",
> > > -			   arg ? "disabled" : "enabled");
> > > +			   str_disabled_enabled(arg));
> > 
> > You don't explain the 'why'. How is this an improvement?
> > nack on this and 2 similar networking changes you sent
> 
> Side opinion: This makes the messages more unified and not prone to typos
> and/or grammatical mistakes. Unification allows to shrink binary due to
> linker efforts on string literals deduplication.

This adds a layer of indirection.

The original code is immediately obvious. When I see the new code I
have to take a detour through cscope to figure out what it does.

To me, in this case, the benefit is too marginal to justify that.

Re: [PATCH -next 2/4] tun: Make use of str_disabled_enabled helper

[Hongbo Li]

On 2024/9/2 22:30, Willem de Bruijn wrote:
> Andy Shevchenko wrote:
>> On Sat, Aug 31, 2024 at 01:07:41PM -0700, Jakub Kicinski wrote:
>>> On Sat, 31 Aug 2024 17:58:38 +0800 Hongbo Li wrote:
>>>> Use str_disabled_enabled() helper instead of open
>>>> coding the same.
>>
>> ...
>>
>>>>   		netif_info(tun, drv, tun->dev, "ignored: set checksum %s\n",
>>>> -			   arg ? "disabled" : "enabled");
>>>> +			   str_disabled_enabled(arg));
>>>
>>> You don't explain the 'why'. How is this an improvement?
>>> nack on this and 2 similar networking changes you sent
>>
>> Side opinion: This makes the messages more unified and not prone to typos
>> and/or grammatical mistakes. Unification allows to shrink binary due to
>> linker efforts on string literals deduplication.
> 
> This adds a layer of indirection.
> 
> The original code is immediately obvious. When I see the new code I
> have to take a detour through cscope to figure out what it does.
If they have used it once, there is no need for more jumps, because it's 
relatively simple.

Using a dedicated function seems very elegant and unified, especially 
for some string printing situations, such as disable/enable. Even in 
today's kernel tree, there are several different formats that appear: 
'enable/disable', 'enabled/disabled', 'en/dis'.

Thanks,
Hongbo

> 
> To me, in this case, the benefit is too marginal to justify that.

Re: [PATCH -next 2/4] tun: Make use of str_disabled_enabled helper

[Andy Shevchenko]

On Tue, Sep 03, 2024 at 02:25:53PM +0800, Hongbo Li wrote:
> On 2024/9/2 22:30, Willem de Bruijn wrote:
> > Andy Shevchenko wrote:
> > > On Sat, Aug 31, 2024 at 01:07:41PM -0700, Jakub Kicinski wrote:
> > > > On Sat, 31 Aug 2024 17:58:38 +0800 Hongbo Li wrote:

...

> > > > >   		netif_info(tun, drv, tun->dev, "ignored: set checksum %s\n",
> > > > > -			   arg ? "disabled" : "enabled");
> > > > > +			   str_disabled_enabled(arg));
> > > > 
> > > > You don't explain the 'why'. How is this an improvement?
> > > > nack on this and 2 similar networking changes you sent
> > > 
> > > Side opinion: This makes the messages more unified and not prone to typos
> > > and/or grammatical mistakes. Unification allows to shrink binary due to
> > > linker efforts on string literals deduplication.
> > 
> > This adds a layer of indirection.
> > 
> > The original code is immediately obvious. When I see the new code I
> > have to take a detour through cscope to figure out what it does.
> If they have used it once, there is no need for more jumps, because it's
> relatively simple.
> 
> Using a dedicated function seems very elegant and unified, especially for
> some string printing situations, such as disable/enable. Even in today's
> kernel tree, there are several different formats that appear:
> 'enable/disable', 'enabled/disabled', 'en/dis'.

Not to mention that the longer word is the more error prone the spelling.

> > To me, in this case, the benefit is too marginal to justify that.

Hongbo, perhaps you need to add a top comment to the string_choices.h to
explain the following:
1) the convention to use is str_$TRUE_$FALSE(), where $TRUE and $FALSE the
respective words printed;
2) the pros of having unified output,
3) including but not limited to the linker deduplication facilities, making
the binary smaller.

With that you may always point people to the ad-hoc documentation.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH -next 2/4] tun: Make use of str_disabled_enabled helper

[Hongbo Li]

On 2024/9/3 23:09, Andy Shevchenko wrote:
> On Tue, Sep 03, 2024 at 02:25:53PM +0800, Hongbo Li wrote:
>> On 2024/9/2 22:30, Willem de Bruijn wrote:
>>> Andy Shevchenko wrote:
>>>> On Sat, Aug 31, 2024 at 01:07:41PM -0700, Jakub Kicinski wrote:
>>>>> On Sat, 31 Aug 2024 17:58:38 +0800 Hongbo Li wrote:
> 
> ...
> 
>>>>>>    		netif_info(tun, drv, tun->dev, "ignored: set checksum %s\n",
>>>>>> -			   arg ? "disabled" : "enabled");
>>>>>> +			   str_disabled_enabled(arg));
>>>>>
>>>>> You don't explain the 'why'. How is this an improvement?
>>>>> nack on this and 2 similar networking changes you sent
>>>>
>>>> Side opinion: This makes the messages more unified and not prone to typos
>>>> and/or grammatical mistakes. Unification allows to shrink binary due to
>>>> linker efforts on string literals deduplication.
>>>
>>> This adds a layer of indirection.
>>>
>>> The original code is immediately obvious. When I see the new code I
>>> have to take a detour through cscope to figure out what it does.
>> If they have used it once, there is no need for more jumps, because it's
>> relatively simple.
>>
>> Using a dedicated function seems very elegant and unified, especially for
>> some string printing situations, such as disable/enable. Even in today's
>> kernel tree, there are several different formats that appear:
>> 'enable/disable', 'enabled/disabled', 'en/dis'.
> 
> Not to mention that the longer word is the more error prone the spelling.
> 
>>> To me, in this case, the benefit is too marginal to justify that.
> 
> Hongbo, perhaps you need to add a top comment to the string_choices.h to
> explain the following:
> 1) the convention to use is str_$TRUE_$FALSE(), where $TRUE and $FALSE the
> respective words printed;
> 2) the pros of having unified output,
> 3) including but not limited to the linker deduplication facilities, making
> the binary smaller.
> 
> With that you may always point people to the ad-hoc documentation.
> 

ok, thank you, and I can try it.

However, with these modifications, I'm not sure whether Willem and Jakub 
agree with the changes. If they don't agree, then I'll have to remove 
this example in the next version. In the future, we can guide other 
developers to use these helpers directly instead of rewriting it themselves.

Thanks,
Hongbo

Re: [PATCH -next 2/4] tun: Make use of str_disabled_enabled helper

[Jakub Kicinski]

On Wed, 4 Sep 2024 10:27:18 +0800 Hongbo Li wrote:
> However, with these modifications, I'm not sure whether Willem and Jakub 
> agree with the changes. If they don't agree, then I'll have to remove 
> this example in the next version.

This and, to be clear, patch 4 as well.

> In the future, we can guide other 
> developers to use these helpers directly instead of rewriting it themselves.

[PATCH -next 3/4] mm: page_alloc: Make use of str_off_on helper

[Hongbo Li]

The helper str_off_on is introduced to reback "off/on"
string literal. We can use str_off_on() helper instead
of open coding the same.

Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 mm/page_alloc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 04a90dfbce09..8adda0b67253 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5517,10 +5517,9 @@ void __ref build_all_zonelists(pg_data_t *pgdat)
 		page_group_by_mobility_disabled = 1;
 	else
 		page_group_by_mobility_disabled = 0;
-
 	pr_info("Built %u zonelists, mobility grouping %s.  Total pages: %ld\n",
 		nr_online_nodes,
-		page_group_by_mobility_disabled ? "off" : "on",
+		str_off_on(page_group_by_mobility_disabled),
 		vm_total_pages);
 #ifdef CONFIG_NUMA
 	pr_info("Policy zone: %s\n", zone_names[policy_zone]);
-- 
2.34.1

[PATCH net-next 4/4] net: sock: Make use of str_no_yes() helper

[Hongbo Li]

The helper str_no_yes is introduced to reback "no/yes"
string literal. We can use str_no_yes() helper instead
of open coding the same.

Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 net/core/sock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index 542bc5462cb5..f976bb4a402f 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -4086,7 +4086,7 @@ static void proto_seq_printf(struct seq_file *seq, struct proto *proto)
 		   sock_prot_memory_allocated(proto),
 		   sock_prot_memory_pressure(proto),
 		   proto->max_header,
-		   proto->slab == NULL ? "no" : "yes",
+		   str_no_yes(proto->slab == NULL),
 		   module_name(proto->owner),
 		   proto_method_implemented(proto->close),
 		   proto_method_implemented(proto->connect),
-- 
2.34.1