| Message ID | 20241104102615.257784-1-alexandre.ferrieux@orange.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] Fix u32's systematic failure to free IDR entries for hnodes. | expand |
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; > }
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 ?
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.
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.
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
> 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.
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.
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
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 >
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!
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.
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.
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; }
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(-)