[PATCH net] Fix u32's systematic failure to free IDR entries for hnodes.

[PATCH net] Fix u32's systematic failure to free IDR entries for hnodes.

[Alexandre Ferrieux]

To generate hnode handles (in gen_new_htid()), u32 uses IDR and
encodes the returned small integer into a strucured 32-bit
word. Unfortunately, at disposal time, the needed decoding
is not done. As a result, idr_remove() fails, and the IDR
fills up. Since its size is 2048, the following script ends up
with "Filter already exists":

  tc filter add dev myve $FILTER1
  tc filter add dev myve $FILTER2
  for i in {1..2048}
  do
    echo $i
    tc filter del dev myve $FILTER2
    tc filter add dev myve $FILTER2
  done

This patch adds the missing decoding logic for handles that
deserve it.

Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@orange.com>
---
 net/sched/cls_u32.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 9412d88a99bc..54b5fca623da 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -41,6 +41,16 @@
 #include <linux/idr.h>
 #include <net/tc_wrapper.h>
 
+static inline unsigned int handle2id(unsigned int h)
+{
+	return ((h & 0x80000000) ? ((h >> 20) & 0x7FF) : h);
+}
+
+static inline unsigned int id2handle(unsigned int id)
+{
+	return (id | 0x800U) << 20;
+}
+
 struct tc_u_knode {
 	struct tc_u_knode __rcu	*next;
 	u32			handle;
@@ -310,7 +320,7 @@ static u32 gen_new_htid(struct tc_u_common *tp_c, struct tc_u_hnode *ptr)
 	int id = idr_alloc_cyclic(&tp_c->handle_idr, ptr, 1, 0x7FF, GFP_KERNEL);
 	if (id < 0)
 		return 0;
-	return (id | 0x800U) << 20;
+	return id2handle(id);
 }
 
 static struct hlist_head *tc_u_common_hash;
@@ -360,7 +370,7 @@ static int u32_init(struct tcf_proto *tp)
 		return -ENOBUFS;
 
 	refcount_set(&root_ht->refcnt, 1);
-	root_ht->handle = tp_c ? gen_new_htid(tp_c, root_ht) : 0x80000000;
+	root_ht->handle = tp_c ? gen_new_htid(tp_c, root_ht) : id2handle(0);
 	root_ht->prio = tp->prio;
 	root_ht->is_root = true;
 	idr_init(&root_ht->handle_idr);
@@ -612,7 +622,7 @@ static int u32_destroy_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht,
 		if (phn == ht) {
 			u32_clear_hw_hnode(tp, ht, extack);
 			idr_destroy(&ht->handle_idr);
-			idr_remove(&tp_c->handle_idr, ht->handle);
+			idr_remove(&tp_c->handle_idr, handle2id(ht->handle));
 			RCU_INIT_POINTER(*hn, ht->next);
 			kfree_rcu(ht, rcu);
 			return 0;
@@ -989,7 +999,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 
 		err = u32_replace_hw_hnode(tp, ht, userflags, extack);
 		if (err) {
-			idr_remove(&tp_c->handle_idr, handle);
+			idr_remove(&tp_c->handle_idr, handle2id(handle));
 			kfree(ht);
 			return err;
 		}
-- 
2.30.2

Re: [PATCH net] Fix u32's systematic failure to free IDR entries for hnodes.

[Eric Dumazet]

On Fri, Nov 1, 2024 at 7:43 PM Alexandre Ferrieux
<alexandre.ferrieux@gmail.com> wrote:
>
> To generate hnode handles (in gen_new_htid()), u32 uses IDR and
> encodes the returned small integer into a strucured 32-bit
> word. Unfortunately, at disposal time, the needed decoding
> is not done. As a result, idr_remove() fails, and the IDR
> fills up. Since its size is 2048, the following script ends up
> with "Filter already exists":
>
>   tc filter add dev myve $FILTER1
>   tc filter add dev myve $FILTER2
>   for i in {1..2048}
>   do
>     echo $i
>     tc filter del dev myve $FILTER2
>     tc filter add dev myve $FILTER2
>   done
>
> This patch adds the missing decoding logic for handles that
> deserve it.
>
> Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@orange.com>
> ---


$ scripts/get_maintainer.pl net/sched/cls_u32.c

->

Jamal Hadi Salim <jhs@mojatatu.com> (maintainer:TC subsystem)
Cong Wang <xiyou.wangcong@gmail.com> (maintainer:TC subsystem)
Jiri Pirko <jiri@resnulli.us> (maintainer:TC subsystem)

Please resend adding them to get their feedback ?

Thank you.

>  net/sched/cls_u32.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index 9412d88a99bc..54b5fca623da 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -41,6 +41,16 @@
>  #include <linux/idr.h>
>  #include <net/tc_wrapper.h>
>
> +static inline unsigned int handle2id(unsigned int h)
> +{
> +       return ((h & 0x80000000) ? ((h >> 20) & 0x7FF) : h);
> +}
> +
> +static inline unsigned int id2handle(unsigned int id)
> +{
> +       return (id | 0x800U) << 20;
> +}
> +
>  struct tc_u_knode {
>         struct tc_u_knode __rcu *next;
>         u32                     handle;
> @@ -310,7 +320,7 @@ static u32 gen_new_htid(struct tc_u_common *tp_c, struct tc_u_hnode *ptr)
>         int id = idr_alloc_cyclic(&tp_c->handle_idr, ptr, 1, 0x7FF, GFP_KERNEL);
>         if (id < 0)
>                 return 0;
> -       return (id | 0x800U) << 20;
> +       return id2handle(id);
>  }
>
>  static struct hlist_head *tc_u_common_hash;
> @@ -360,7 +370,7 @@ static int u32_init(struct tcf_proto *tp)
>                 return -ENOBUFS;
>
>         refcount_set(&root_ht->refcnt, 1);
> -       root_ht->handle = tp_c ? gen_new_htid(tp_c, root_ht) : 0x80000000;
> +       root_ht->handle = tp_c ? gen_new_htid(tp_c, root_ht) : id2handle(0);
>         root_ht->prio = tp->prio;
>         root_ht->is_root = true;
>         idr_init(&root_ht->handle_idr);
> @@ -612,7 +622,7 @@ static int u32_destroy_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht,
>                 if (phn == ht) {
>                         u32_clear_hw_hnode(tp, ht, extack);
>                         idr_destroy(&ht->handle_idr);
> -                       idr_remove(&tp_c->handle_idr, ht->handle);
> +                       idr_remove(&tp_c->handle_idr, handle2id(ht->handle));
>                         RCU_INIT_POINTER(*hn, ht->next);
>                         kfree_rcu(ht, rcu);
>                         return 0;
> @@ -989,7 +999,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
>
>                 err = u32_replace_hw_hnode(tp, ht, userflags, extack);
>                 if (err) {
> -                       idr_remove(&tp_c->handle_idr, handle);
> +                       idr_remove(&tp_c->handle_idr, handle2id(handle));
>                         kfree(ht);
>                         return err;
>                 }
> --
> 2.30.2
>

[PATCH net] Fix u32's systematic failure to free IDR entries for hnodes.

[Alexandre Ferrieux]

To generate hnode handles (in gen_new_htid()), u32 uses IDR and
encodes the returned small integer into a structured 32-bit
word. Unfortunately, at disposal time, the needed decoding
is not done. As a result, idr_remove() fails, and the IDR
fills up. Since its size is 2048, the following script ends up
with "Filter already exists":

  tc filter add dev myve $FILTER1
  tc filter add dev myve $FILTER2
  for i in {1..2048}
  do
    echo $i
    tc filter del dev myve $FILTER2
    tc filter add dev myve $FILTER2
  done

This patch adds the missing decoding logic for handles that
deserve it.

Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@orange.com>
---
 net/sched/cls_u32.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 9412d88a99bc..54b5fca623da 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -41,6 +41,16 @@
 #include <linux/idr.h>
 #include <net/tc_wrapper.h>
 
+static inline unsigned int handle2id(unsigned int h)
+{
+	return ((h & 0x80000000) ? ((h >> 20) & 0x7FF) : h);
+}
+
+static inline unsigned int id2handle(unsigned int id)
+{
+	return (id | 0x800U) << 20;
+}
+
 struct tc_u_knode {
 	struct tc_u_knode __rcu	*next;
 	u32			handle;
@@ -310,7 +320,7 @@ static u32 gen_new_htid(struct tc_u_common *tp_c, struct tc_u_hnode *ptr)
 	int id = idr_alloc_cyclic(&tp_c->handle_idr, ptr, 1, 0x7FF, GFP_KERNEL);
 	if (id < 0)
 		return 0;
-	return (id | 0x800U) << 20;
+	return id2handle(id);
 }
 
 static struct hlist_head *tc_u_common_hash;
@@ -360,7 +370,7 @@ static int u32_init(struct tcf_proto *tp)
 		return -ENOBUFS;
 
 	refcount_set(&root_ht->refcnt, 1);
-	root_ht->handle = tp_c ? gen_new_htid(tp_c, root_ht) : 0x80000000;
+	root_ht->handle = tp_c ? gen_new_htid(tp_c, root_ht) : id2handle(0);
 	root_ht->prio = tp->prio;
 	root_ht->is_root = true;
 	idr_init(&root_ht->handle_idr);
@@ -612,7 +622,7 @@ static int u32_destroy_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht,
 		if (phn == ht) {
 			u32_clear_hw_hnode(tp, ht, extack);
 			idr_destroy(&ht->handle_idr);
-			idr_remove(&tp_c->handle_idr, ht->handle);
+			idr_remove(&tp_c->handle_idr, handle2id(ht->handle));
 			RCU_INIT_POINTER(*hn, ht->next);
 			kfree_rcu(ht, rcu);
 			return 0;
@@ -989,7 +999,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 
 		err = u32_replace_hw_hnode(tp, ht, userflags, extack);
 		if (err) {
-			idr_remove(&tp_c->handle_idr, handle);
+			idr_remove(&tp_c->handle_idr, handle2id(handle));
 			kfree(ht);
 			return err;
 		}
-- 
2.30.2

Re: [PATCH net] Fix u32's systematic failure to free IDR entries for hnodes.

[Pedro Tammela]

On 04/11/2024 07:26, Alexandre Ferrieux wrote:
> To generate hnode handles (in gen_new_htid()), u32 uses IDR and
> encodes the returned small integer into a structured 32-bit
> word. Unfortunately, at disposal time, the needed decoding
> is not done. As a result, idr_remove() fails, and the IDR
> fills up. Since its size is 2048, the following script ends up
> with "Filter already exists":
> 
>    tc filter add dev myve $FILTER1
>    tc filter add dev myve $FILTER2
>    for i in {1..2048}
>    do
>      echo $i
>      tc filter del dev myve $FILTER2
>      tc filter add dev myve $FILTER2
>    done
> 
> This patch adds the missing decoding logic for handles that
> deserve it.
> 
> Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@orange.com>

SoB does not match sender, probably missing 'From:' tag
Also, this seems to deserve a 'Fixes:' tag as well

> ---
>   net/sched/cls_u32.c | 18 ++++++++++++++----
>   1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index 9412d88a99bc..54b5fca623da 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -41,6 +41,16 @@
>   #include <linux/idr.h>
>   #include <net/tc_wrapper.h>
>   
> +static inline unsigned int handle2id(unsigned int h)
> +{
> +	return ((h & 0x80000000) ? ((h >> 20) & 0x7FF) : h);
> +}
> +
> +static inline unsigned int id2handle(unsigned int id)
> +{
> +	return (id | 0x800U) << 20;
> +}
> +

'static inline' is discouraged in .c files

>   struct tc_u_knode {
>   	struct tc_u_knode __rcu	*next;
>   	u32			handle;
> @@ -310,7 +320,7 @@ static u32 gen_new_htid(struct tc_u_common *tp_c, struct tc_u_hnode *ptr)
>   	int id = idr_alloc_cyclic(&tp_c->handle_idr, ptr, 1, 0x7FF, GFP_KERNEL);
>   	if (id < 0)
>   		return 0;
> -	return (id | 0x800U) << 20;
> +	return id2handle(id);
>   }
>   
>   static struct hlist_head *tc_u_common_hash;
> @@ -360,7 +370,7 @@ static int u32_init(struct tcf_proto *tp)
>   		return -ENOBUFS;
>   
>   	refcount_set(&root_ht->refcnt, 1);
> -	root_ht->handle = tp_c ? gen_new_htid(tp_c, root_ht) : 0x80000000;
> +	root_ht->handle = tp_c ? gen_new_htid(tp_c, root_ht) : id2handle(0);
>   	root_ht->prio = tp->prio;
>   	root_ht->is_root = true;
>   	idr_init(&root_ht->handle_idr);
> @@ -612,7 +622,7 @@ static int u32_destroy_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht,
>   		if (phn == ht) {
>   			u32_clear_hw_hnode(tp, ht, extack);
>   			idr_destroy(&ht->handle_idr);
> -			idr_remove(&tp_c->handle_idr, ht->handle);
> +			idr_remove(&tp_c->handle_idr, handle2id(ht->handle));
>   			RCU_INIT_POINTER(*hn, ht->next);
>   			kfree_rcu(ht, rcu);
>   			return 0;
> @@ -989,7 +999,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
>   
>   		err = u32_replace_hw_hnode(tp, ht, userflags, extack);
>   		if (err) {
> -			idr_remove(&tp_c->handle_idr, handle);
> +			idr_remove(&tp_c->handle_idr, handle2id(handle));
>   			kfree(ht);
>   			return err;
>   		}

Re: [PATCH net] Fix u32's systematic failure to free IDR entries for hnodes.

[Alexandre Ferrieux]

On 04/11/2024 18:00, Pedro Tammela wrote:
>> 
>> Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@orange.com>
> 
> SoB does not match sender, probably missing 'From:' tag

Due to dumb administrativia at my organization, I am compelled to post from my
personal gmail accout in order for my posts to be acceptable on this mailing
list; while I'd like to keep my official address in commit logs. Is it possible ?

> Also, this seems to deserve a 'Fixes:' tag as well

This would be the initial commit:

 ^1da177e4c3f4 (Linus Torvalds           2005-04-16 15:20:36 -0700   19)

Is that what you mean ?

> 'static inline' is discouraged in .c files

Why ?

It could have been a local macro, but an inline has (a bit) better type
checking. And I didn't want to add it to a .h that is included by many other
unrelated components, as it makes no sense to them. So, what is the recommendation ?

Re: [PATCH net] Fix u32's systematic failure to free IDR entries for hnodes.

[Vadim Fedorenko]

On 04/11/2024 20:26, Alexandre Ferrieux wrote:
> On 04/11/2024 18:00, Pedro Tammela wrote:
>>>
>>> Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@orange.com>
>>
>> SoB does not match sender, probably missing 'From:' tag
> 
> Due to dumb administrativia at my organization, I am compelled to post from my
> personal gmail accout in order for my posts to be acceptable on this mailing
> list; while I'd like to keep my official address in commit logs. Is it possible ?

Yes, it's possible, the author of commit in your local git should use
email account of company, then git format-patch will generate proper header.

>> Also, this seems to deserve a 'Fixes:' tag as well
> 
> This would be the initial commit:
> 
>   ^1da177e4c3f4 (Linus Torvalds           2005-04-16 15:20:36 -0700   19)
> 
> Is that what you mean ?
> 

you can add
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

>> 'static inline' is discouraged in .c files
> 
> Why ?
> 
> It could have been a local macro, but an inline has (a bit) better type
> checking. And I didn't want to add it to a .h that is included by many other
> unrelated components, as it makes no sense to them. So, what is the recommendation ?

Either move it to some local header file, or use 'static u32 
handle2id(u32 h)'
and let compiler decide whether to include it or not. But in either
cases use u32 as types to be consistent with other types in the
functions you modify.

Re: [PATCH net] Fix u32's systematic failure to free IDR entries for hnodes.

[Alexandre Ferrieux]

On 04/11/2024 22:33, Vadim Fedorenko wrote:
> On 04/11/2024 20:26, Alexandre Ferrieux wrote:
>> On 04/11/2024 18:00, Pedro Tammela wrote:
>>>>
>>>> Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@orange.com>
>>>
>>> SoB does not match sender, probably missing 'From:' tag
>> 
>> Due to dumb administrativia at my organization, I am compelled to post from my
>> personal gmail accout in order for my posts to be acceptable on this mailing
>> list; while I'd like to keep my official address in commit logs. Is it possible ?
> 
> Yes, it's possible, the author of commit in your local git should use
> email account of company, then git format-patch will generate proper header.

That's exactly what I did, and the file generated by format-patch does have the
proper From:, but it gets overridden by Gmail when sending. That's why, as a
last resort, I tried Signed-off-by... Any hope ?

> you can add
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

Ok.

>>> 'static inline' is discouraged in .c files
>> 
>> Why ?
>> 
>> It could have been a local macro, but an inline has (a bit) better type
>> checking. And I didn't want to add it to a .h that is included by many other
>> unrelated components, as it makes no sense to them. So, what is the recommendation ?
> 
> Either move it to some local header file, or use 'static u32 
> handle2id(u32 h)'
> and let compiler decide whether to include it or not.

I believe you mean "let the compiler decide whether to _inline_ it or not".
Sure, with a sufficiently modern Gcc this will do. However, what about more
exotic environments ? Wouldn't it risk a perf regression for style reasons ?

And speaking of style, what about the dozens of instances of "static inline" in
net/sched/*.c alone ? Why is it a concern suddenly ?

> But in either
> cases use u32 as types to be consistent with other types in the
> functions you modify.

Ok.

Re: [PATCH net] Fix u32's systematic failure to free IDR entries for hnodes.

[Florian Fainelli]

On 11/4/24 13:51, Alexandre Ferrieux wrote:
> On 04/11/2024 22:33, Vadim Fedorenko wrote:
>> On 04/11/2024 20:26, Alexandre Ferrieux wrote:
>>> On 04/11/2024 18:00, Pedro Tammela wrote:
>>>>>
>>>>> Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@orange.com>
>>>>
>>>> SoB does not match sender, probably missing 'From:' tag
>>>
>>> Due to dumb administrativia at my organization, I am compelled to post from my
>>> personal gmail accout in order for my posts to be acceptable on this mailing
>>> list; while I'd like to keep my official address in commit logs. Is it possible ?
>>
>> Yes, it's possible, the author of commit in your local git should use
>> email account of company, then git format-patch will generate proper header.
> 
> That's exactly what I did, and the file generated by format-patch does have the
> proper From:, but it gets overridden by Gmail when sending. That's why, as a
> last resort, I tried Signed-off-by... Any hope ?

You might try b4 send and see if that helps: 
https://b4.docs.kernel.org/en/latest/contributor/send.html
-- 
Florian

Re: [PATCH net] Fix u32's systematic failure to free IDR entries for hnodes.

[Alexandre Ferrieux]

> On 04/11/2024 22:33, Vadim Fedorenko wrote:
>>> On 04/11/2024 18:00, Pedro Tammela wrote:
>>>> 'static inline' is discouraged in .c files
>>> 
>>> Why ?
>>> 
>>> It could have been a local macro, but an inline has (a bit) better type
>>> checking. And I didn't want to add it to a .h that is included by many other
>>> unrelated components, as it makes no sense to them. So, what is the recommendation ?
>> 
>> Either move it to some local header file, or use 'static u32 
>> handle2id(u32 h)'
>> and let compiler decide whether to include it or not.
> 
> I believe you mean "let the compiler decide whether to _inline_ it or not".
> Sure, with a sufficiently modern Gcc this will do. However, what about more
> exotic environments ? Wouldn't it risk a perf regression for style reasons ?
> 
> And speaking of style, what about the dozens of instances of "static inline" in
> net/sched/*.c alone ? Why is it a concern suddenly ?

Can you please explain *why* in the first place you're saying "'static inline'
is discouraged in .c files" ? I see no trace if this in coding-style.rst, and
the kernel contains hundreds of counter-examples.

Re: [PATCH net] Fix u32's systematic failure to free IDR entries for hnodes.

[Vadim Fedorenko]

On 05/11/2024 22:14, Alexandre Ferrieux wrote:
>> On 04/11/2024 22:33, Vadim Fedorenko wrote:
>>>> On 04/11/2024 18:00, Pedro Tammela wrote:
>>>>> 'static inline' is discouraged in .c files
>>>>
>>>> Why ?
>>>>
>>>> It could have been a local macro, but an inline has (a bit) better type
>>>> checking. And I didn't want to add it to a .h that is included by many other
>>>> unrelated components, as it makes no sense to them. So, what is the recommendation ?
>>>
>>> Either move it to some local header file, or use 'static u32
>>> handle2id(u32 h)'
>>> and let compiler decide whether to include it or not.
>>
>> I believe you mean "let the compiler decide whether to _inline_ it or not".
>> Sure, with a sufficiently modern Gcc this will do. However, what about more
>> exotic environments ? Wouldn't it risk a perf regression for style reasons ?
>>
>> And speaking of style, what about the dozens of instances of "static inline" in
>> net/sched/*.c alone ? Why is it a concern suddenly ?
> 
> Can you please explain *why* in the first place you're saying "'static inline'
> is discouraged in .c files" ? I see no trace if this in coding-style.rst, and
> the kernel contains hundreds of counter-examples.

The biggest reason is because it will mask unused function warnings when
"static inline" function will not be used because of some future
patches. There is no big reason to refactor old code that's why there
are counter-examples in the kernel, but the new code shouldn't have it.

I don't really understand what kind of exotic environment you are
thinking about, but modern kernel has proper gcc/clang version
dependency, both of these compilers have good heuristics to identify
which function to inline.

Re: [PATCH net] Fix u32's systematic failure to free IDR entries for hnodes.

[Alexandre Ferrieux]

On 06/11/2024 00:42, Vadim Fedorenko wrote:
> On 05/11/2024 22:14, Alexandre Ferrieux wrote:
>> 
>> Can you please explain *why* in the first place you're saying "'static inline'
>> is discouraged in .c files" ? I see no trace if this in coding-style.rst, and
>> the kernel contains hundreds of counter-examples.
> 
> The biggest reason is because it will mask unused function warnings when
> "static inline" function will not be used because of some future
> patches. There is no big reason to refactor old code that's why there
> are counter-examples in the kernel, but the new code shouldn't have it.

A macro doesn't elicit unused function warnings either, so this looks like a
very weak motivation. While coding-style.rst explicitly encourages to use static
inline instead of macros, as they have better type checking and syntaxic isolation.

Regarding old vs new code, below are the last two month's fresh commits of
"static inline" in *.c. So it looks like the motivation is not shared by other
maintainers. Do we expect to see "local styles" emerge ?


$ git log --pretty='%h %as %ae'   -p | gawk
'/^[0-9a-f]{12}/{c=$0;next}/^diff/{f=$NF;next}/^[+].*static.inline/{if
(f~/[.]c$/){print c "\t"gensub(/.*\//,"","1",f)}}'

baa802d2aa5c 2024-10-21 daniel@iogearbox.net    verifier_const.c
baa802d2aa5c 2024-10-21 daniel@iogearbox.net    verifier_const.c
d1744a4c975b 2024-10-21 bp@alien8.de    amd.c
d1744a4c975b 2024-10-21 bp@alien8.de    amd.c
a6e0ceb7bf48 2024-10-11 sidhartha.kumar@oracle.com      maple.c
78f636e82b22 2024-09-25 freude@linux.ibm.com    ap_queue.c
19773ec99720 2024-10-07 kent.overstreet@linux.dev       disk_accounting.c
9b23fdbd5d29 2024-09-29 kent.overstreet@linux.dev       inode.c
9b23fdbd5d29 2024-09-29 kent.overstreet@linux.dev       inode.c
3d5854d75e31 2024-09-30 agordeev@linux.ibm.com  kcore.c
3d5854d75e31 2024-09-30 agordeev@linux.ibm.com  kcore.c
38864eccf78b 2024-09-30 kent.overstreet@linux.dev       fsck.c
d278a9de5e18 2024-10-02 perex@perex.cz  init.c
f811b83879fb 2024-10-02 mpatocka@redhat.com     dm-verity-target.c
4c411cca33cf 2024-09-13 artem.bityutskiy@linux.intel.com        intel_idle.c
42268ad0eb41 2024-09-24 tj@kernel.org   ext.c
56bcd0f07fdb 2024-09-05 snitzer@kernel.org      localio.c
1b11c4d36548 2024-09-01 kent.overstreet@linux.dev       ec.c
7a51608d0125 2024-09-04 kent.overstreet@linux.dev       btree_cache.c
7a51608d0125 2024-09-04 kent.overstreet@linux.dev       btree_cache.c
691f2cba2291 2024-09-05 kent.overstreet@linux.dev       btree_cache.c

Re: [PATCH net] Fix u32's systematic failure to free IDR entries for hnodes.

[Vadim Fedorenko]

On 06/11/2024 10:15, Alexandre Ferrieux wrote:
> On 06/11/2024 00:42, Vadim Fedorenko wrote:
>> On 05/11/2024 22:14, Alexandre Ferrieux wrote:
>>>
>>> Can you please explain *why* in the first place you're saying "'static inline'
>>> is discouraged in .c files" ? I see no trace if this in coding-style.rst, and
>>> the kernel contains hundreds of counter-examples.
>>
>> The biggest reason is because it will mask unused function warnings when
>> "static inline" function will not be used because of some future
>> patches. There is no big reason to refactor old code that's why there
>> are counter-examples in the kernel, but the new code shouldn't have it.
> 
> A macro doesn't elicit unused function warnings either, so this looks like a
> very weak motivation. While coding-style.rst explicitly encourages to use static
> inline instead of macros, as they have better type checking and syntaxic isolation.

Unused macro will not generate any code and will not make build time
longer. But don't get me wrong, I'm not encouraging you to use macro in
this case.

> Regarding old vs new code, below are the last two month's fresh commits of
> "static inline" in *.c. So it looks like the motivation is not shared by other
> maintainers. Do we expect to see "local styles" emerge ?

There are some "local styles" differences in different subsystems. If
you filter out netdev related diffs from awk-magic below, you will find
that is was mostly refactoring of already existing code.

Anyways, you can ignore this suggestion, it's always up to submitter
how to use review feedback given by others.

> 
> $ git log --pretty='%h %as %ae'   -p | gawk
> '/^[0-9a-f]{12}/{c=$0;next}/^diff/{f=$NF;next}/^[+].*static.inline/{if
> (f~/[.]c$/){print c "\t"gensub(/.*\//,"","1",f)}}'
> 
> baa802d2aa5c 2024-10-21 daniel@iogearbox.net    verifier_const.c
> baa802d2aa5c 2024-10-21 daniel@iogearbox.net    verifier_const.c
> d1744a4c975b 2024-10-21 bp@alien8.de    amd.c
> d1744a4c975b 2024-10-21 bp@alien8.de    amd.c
> a6e0ceb7bf48 2024-10-11 sidhartha.kumar@oracle.com      maple.c
> 78f636e82b22 2024-09-25 freude@linux.ibm.com    ap_queue.c
> 19773ec99720 2024-10-07 kent.overstreet@linux.dev       disk_accounting.c
> 9b23fdbd5d29 2024-09-29 kent.overstreet@linux.dev       inode.c
> 9b23fdbd5d29 2024-09-29 kent.overstreet@linux.dev       inode.c
> 3d5854d75e31 2024-09-30 agordeev@linux.ibm.com  kcore.c
> 3d5854d75e31 2024-09-30 agordeev@linux.ibm.com  kcore.c
> 38864eccf78b 2024-09-30 kent.overstreet@linux.dev       fsck.c
> d278a9de5e18 2024-10-02 perex@perex.cz  init.c
> f811b83879fb 2024-10-02 mpatocka@redhat.com     dm-verity-target.c
> 4c411cca33cf 2024-09-13 artem.bityutskiy@linux.intel.com        intel_idle.c
> 42268ad0eb41 2024-09-24 tj@kernel.org   ext.c
> 56bcd0f07fdb 2024-09-05 snitzer@kernel.org      localio.c
> 1b11c4d36548 2024-09-01 kent.overstreet@linux.dev       ec.c
> 7a51608d0125 2024-09-04 kent.overstreet@linux.dev       btree_cache.c
> 7a51608d0125 2024-09-04 kent.overstreet@linux.dev       btree_cache.c
> 691f2cba2291 2024-09-05 kent.overstreet@linux.dev       btree_cache.c
> 

Re: [PATCH net] Fix u32's systematic failure to free IDR entries for hnodes.

[Simon Horman]

On Mon, Nov 04, 2024 at 10:51:01PM +0100, Alexandre Ferrieux wrote:
> On 04/11/2024 22:33, Vadim Fedorenko wrote:
> > On 04/11/2024 20:26, Alexandre Ferrieux wrote:
> >> On 04/11/2024 18:00, Pedro Tammela wrote:
> >>>>
> >>>> Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@orange.com>
> >>>
> >>> SoB does not match sender, probably missing 'From:' tag
> >> 
> >> Due to dumb administrativia at my organization, I am compelled to post from my
> >> personal gmail accout in order for my posts to be acceptable on this mailing
> >> list; while I'd like to keep my official address in commit logs. Is it possible ?
> > 
> > Yes, it's possible, the author of commit in your local git should use
> > email account of company, then git format-patch will generate proper header.
> 
> That's exactly what I did, and the file generated by format-patch does have the
> proper From:, but it gets overridden by Gmail when sending. That's why, as a
> last resort, I tried Signed-off-by... Any hope ?
> 
> > you can add
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> 
> Ok.
> 
> >>> 'static inline' is discouraged in .c files
> >> 
> >> Why ?
> >> 
> >> It could have been a local macro, but an inline has (a bit) better type
> >> checking. And I didn't want to add it to a .h that is included by many other
> >> unrelated components, as it makes no sense to them. So, what is the recommendation ?
> > 
> > Either move it to some local header file, or use 'static u32 
> > handle2id(u32 h)'
> > and let compiler decide whether to include it or not.
> 
> I believe you mean "let the compiler decide whether to _inline_ it or not".
> Sure, with a sufficiently modern Gcc this will do. However, what about more
> exotic environments ? Wouldn't it risk a perf regression for style reasons ?
> 
> And speaking of style, what about the dozens of instances of "static inline" in
> net/sched/*.c alone ? Why is it a concern suddenly ?

Hi Alexandre,

It's not suddenly a concern. It is a long standing style guideline for
Networking code, even if not always followed. Possibly some of the code
you have found in net/sched/*.c is even longer standing than the
guideline.

Please don't add new instances of inline to .c files unless there is a
demonstrable - usually performance - reason to do so.

Thanks!

Re: [PATCH net] Fix u32's systematic failure to free IDR entries for hnodes.

[Alexandre Ferrieux]

On 10/11/2024 15:00, Simon Horman wrote:
> On Mon, Nov 04, 2024 at 10:51:01PM +0100, Alexandre Ferrieux wrote:
>> 
>> I believe you mean "let the compiler decide whether to _inline_ it or not".
>> Sure, with a sufficiently modern Gcc this will do. However, what about more
>> exotic environments ? Wouldn't it risk a perf regression for style reasons ?
>> 
>> And speaking of style, what about the dozens of instances of "static inline" in
>> net/sched/*.c alone ? Why is it a concern suddenly ?
> 
> Hi Alexandre,
> 
> It's not suddenly a concern. It is a long standing style guideline for
> Networking code, even if not always followed. Possibly some of the code
> you have found in net/sched/*.c is even longer standing than the
> guideline.
> 
> Please don't add new instances of inline to .c files unless there is a
> demonstrable - usually performance - reason to do so.

Sure, I will abide in the next version :)

That said, please note it is hard to understand why such a rule would be
enforced both locally and tacitly. Things would be entirely different if it were
listed in coding-style.rst, advertising both consensus and wide applicability.

Re: [PATCH net] Fix u32's systematic failure to free IDR entries for hnodes.

[Simon Horman]

On Sun, Nov 10, 2024 at 04:40:26PM +0100, Alexandre Ferrieux wrote:
> On 10/11/2024 15:00, Simon Horman wrote:
> > On Mon, Nov 04, 2024 at 10:51:01PM +0100, Alexandre Ferrieux wrote:
> >> 
> >> I believe you mean "let the compiler decide whether to _inline_ it or not".
> >> Sure, with a sufficiently modern Gcc this will do. However, what about more
> >> exotic environments ? Wouldn't it risk a perf regression for style reasons ?
> >> 
> >> And speaking of style, what about the dozens of instances of "static inline" in
> >> net/sched/*.c alone ? Why is it a concern suddenly ?
> > 
> > Hi Alexandre,
> > 
> > It's not suddenly a concern. It is a long standing style guideline for
> > Networking code, even if not always followed. Possibly some of the code
> > you have found in net/sched/*.c is even longer standing than the
> > guideline.
> > 
> > Please don't add new instances of inline to .c files unless there is a
> > demonstrable - usually performance - reason to do so.
> 
> Sure, I will abide in the next version :)
> 
> That said, please note it is hard to understand why such a rule would be
> enforced both locally and tacitly. Things would be entirely different if it were
> listed in coding-style.rst, advertising both consensus and wide applicability.

Thanks,

I completely agree that it would be better if it was documented somewhere.
I will add that to my TODO list.