| Message ID | 20241001193352.151102-1-yyyynoom@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: add inline annotation to fix the build warning | expand |
On Wed, Oct 2, 2024 at 3:47 PM Moon Yeounsu <yyyynoom@gmail.com> wrote: > > Moon is stupid. He doesn't understand what's going on. It makes me upset. > > https://lore.kernel.org/netdev/20240919145609.GF1571683@kernel.org/ > > Simon did the best effort for him, but he didn't remember that. > > Please don't reply to this careless patch. > > Replies to me to remember all the maintainer's dedication and thoughtfulness and to take this to heart. > > Before I send the patch, I'll check it again and again. And fix the subject `net` to `net-next`. > > I'm very very disappointed to myself :( LOCKDEP is more powerful than sparse, I would not bother with this at all.
On Wed, Oct 2, 2024 at 11:41 PM Eric Dumazet <edumazet@google.com> wrote: > > On Wed, Oct 2, 2024 at 3:47 PM Moon Yeounsu <yyyynoom@gmail.com> wrote: > > > > Moon is stupid. He doesn't understand what's going on. It makes me upset. > > > > https://lore.kernel.org/netdev/20240919145609.GF1571683@kernel.org/ > > > > Simon did the best effort for him, but he didn't remember that. > > > > Please don't reply to this careless patch. > > > > Replies to me to remember all the maintainer's dedication and thoughtfulness and to take this to heart. > > > > Before I send the patch, I'll check it again and again. And fix the subject `net` to `net-next`. > > > > I'm very very disappointed to myself :( > > LOCKDEP is more powerful than sparse, I would not bother with this at all. Totally agree with that. `Sparse` has a lot of problems derived from its nature. And It is too annoying to silence the warning message. I know that this patch just fixes for a fix. (What a trivial?) But, even though `LOCKDEP` is more powerful than `Sparse`, that can't be the reason to ignore the warning message. It is only my opinion and this topic may be outside of the net subsystem. Please don't be offended by my words and ignorance. I don't want to make a problem, rather want to fix a problem. If there's no reason to use `Sparse`, then, how about just removing it from the kernel? If It can't, we have to make Sparse more useful at least make to have to care about this warning message. > LOCKDEP is more powerful than sparse, I would not bother with this at all. Leastways, This sentence is irrational in my view. Let me know, world!
On Thu, Oct 3, 2024 at 3:57 PM Moon Yeounsu <yyyynoom@gmail.com> wrote: > > On Wed, Oct 2, 2024 at 11:41 PM Eric Dumazet <edumazet@google.com> wrote: > > > > On Wed, Oct 2, 2024 at 3:47 PM Moon Yeounsu <yyyynoom@gmail.com> wrote: > > > > > > Moon is stupid. He doesn't understand what's going on. It makes me upset. > > > > > > https://lore.kernel.org/netdev/20240919145609.GF1571683@kernel.org/ > > > > > > Simon did the best effort for him, but he didn't remember that. > > > > > > Please don't reply to this careless patch. > > > > > > Replies to me to remember all the maintainer's dedication and thoughtfulness and to take this to heart. > > > > > > Before I send the patch, I'll check it again and again. And fix the subject `net` to `net-next`. > > > > > > I'm very very disappointed to myself :( > > > > LOCKDEP is more powerful than sparse, I would not bother with this at all. > > Totally agree with that. `Sparse` has a lot of problems derived from its nature. > And It is too annoying to silence the warning message. I know that > this patch just fixes for a fix. (What a trivial?) > But, even though `LOCKDEP` is more powerful than `Sparse`, that can't > be the reason to ignore the warning message. > > It is only my opinion and this topic may be outside of the net > subsystem. Please don't be offended by my words and ignorance. I don't > want to make a problem, rather want to fix a problem. > If there's no reason to use `Sparse`, then, how about just removing it > from the kernel? If It can't, we have to make Sparse more useful at > least make to have to care about this warning message. sparse is not in the kernel. Feel free to remove it from your hosts. Anyway, the __acquires(XXX) annotations means nothing, XXX is completely ignored. $ diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c index 09e31757e96c7472af2a9dfff7a731d4d076aa11..50fc48c6d0c99d91f5a8eb15c4e3dd0304a83e0b 100644 --- a/net/ipv4/fib_trie.c +++ b/net/ipv4/fib_trie.c @@ -2888,7 +2888,7 @@ static struct key_vector *fib_route_get_idx(struct fib_route_iter *iter, } static void *fib_route_seq_start(struct seq_file *seq, loff_t *pos) - __acquires(RCU) + __acquires(some_random_stuff) { struct fib_route_iter *iter = seq->private; struct fib_table *tb; $ make C=1 net/ipv4/fib_trie.o CALL scripts/checksyscalls.sh DESCEND objtool INSTALL libsubcmd_headers DESCEND bpf/resolve_btfids INSTALL libsubcmd_headers CC net/ipv4/fib_trie.o CHECK net/ipv4/fib_trie.c No error at all. It also does not know about conditional locking, it is quite useless.
> sparse is not in the kernel. Feel free to remove it from your hosts. Oh... I see. Yes, you are right. Sparse is just a program like other tools like gcc. > > $ diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c > index 09e31757e96c7472af2a9dfff7a731d4d076aa11..50fc48c6d0c99d91f5a8eb15c4e3dd0304a83e0b > 100644 > --- a/net/ipv4/fib_trie.c > +++ b/net/ipv4/fib_trie.c > @@ -2888,7 +2888,7 @@ static struct key_vector > *fib_route_get_idx(struct fib_route_iter *iter, > } > > static void *fib_route_seq_start(struct seq_file *seq, loff_t *pos) > - __acquires(RCU) > + __acquires(some_random_stuff) > { > struct fib_route_iter *iter = seq->private; > struct fib_table *tb; > > > $ make C=1 net/ipv4/fib_trie.o > CALL scripts/checksyscalls.sh > DESCEND objtool > INSTALL libsubcmd_headers > DESCEND bpf/resolve_btfids > INSTALL libsubcmd_headers > CC net/ipv4/fib_trie.o > CHECK net/ipv4/fib_trie.c > > No error at all. > It also does not know about conditional locking, it is quite useless. Yes, exactly. And It makes me crazy. `net/ipv6/icmp.c` was written to use the conditional lock as you mentioned. This is not a problem and can easily be verified intuitively, but Sparse can't sense it. To refactor the code to silent `Sparse` is putting the cart before the horse. NON-SENSE. So... What do you think about who wants to send the patch to silence the Sparse's warning message, nevertheless? I know him who was just about to write the next patch by correcting mistakes (Seems like he wrote the subject prefix to `net`, not a `net-next`, what a foolish one). Is he wasting his life and taking other people's invaluable time? What do you think about it?
On 03/10/2024 16:33, Moon Yeounsu wrote: > On 03/10/2024 15:19, Eric Dumazet wrote: >> It also does not know about conditional locking, it is quite useless. > So... What do you think about who wants to send the patch to silence > the Sparse's warning message, nevertheless? Fwiw, my experience is that if I can't explain the locking to sparse that's usually a sign that the code is too complex and needs to be rewritten. In general I'm in favour of patches to fix sparse warnings. In this case it looks like what's needed is __cond_acquires, but the patch to implement that in sparse[1] doesn't seem to have gotten anywhere near Luc's tree. (Yet it's present and occasionally used in the kernel...) CCing the sparse ML to find out why. -ed [1]: https://lore.kernel.org/all/CAHk-=wjZfO9hGqJ2_hGQG3U_XzSh9_XaXze=HgPdvJbgrvASfA@mail.gmail.com/#t
On Thu, 3 Oct 2024 17:11:26 +0100 Edward Cree <ecree.xilinx@gmail.com> wrote: > On 03/10/2024 16:33, Moon Yeounsu wrote: > > On 03/10/2024 15:19, Eric Dumazet wrote: > >> It also does not know about conditional locking, it is quite useless. > > So... What do you think about who wants to send the patch to silence > > the Sparse's warning message, nevertheless? In my experience, conditional locking is often a cause of bugs.
diff --git a/net/core/neighbour.c b/net/core/neighbour.c index 77b819cd995b..6b5ec9a44556 100644 --- a/net/core/neighbour.c +++ b/net/core/neighbour.c @@ -441,6 +441,7 @@ EXPORT_SYMBOL(neigh_changeaddr); static int __neigh_ifdown(struct neigh_table *tbl, struct net_device *dev, bool skip_perm) + __acquires(&tbl->lock) { write_lock_bh(&tbl->lock); neigh_flush_dev(tbl, dev, skip_perm); @@ -453,6 +454,7 @@ static int __neigh_ifdown(struct neigh_table *tbl, struct net_device *dev, } int neigh_carrier_down(struct neigh_table *tbl, struct net_device *dev) + __acquires(&tbl->lock) { __neigh_ifdown(tbl, dev, true); return 0; @@ -460,6 +462,7 @@ int neigh_carrier_down(struct neigh_table *tbl, struct net_device *dev) EXPORT_SYMBOL(neigh_carrier_down); int neigh_ifdown(struct neigh_table *tbl, struct net_device *dev) + __acquires(&tbl->lock) { __neigh_ifdown(tbl, dev, false); return 0; @@ -848,6 +851,7 @@ int pneigh_delete(struct neigh_table *tbl, struct net *net, const void *pkey, static int pneigh_ifdown_and_unlock(struct neigh_table *tbl, struct net_device *dev) + __releases(&tbl->lock) { struct pneigh_entry *n, **np, *freelist = NULL; u32 h;
This patch fixes two `sparse` warnings: net/core/neighbour.c:453:9: warning: context imbalance in '__neigh_ifdown' - wrong count at exit net/core/neighbour.c:871:9: warning: context imbalance in 'pneigh_ifdown_and_unlock' - unexpected unlock You can check it by running: `make C=1 net/core/neighbour.o` Signed-off-by: Moon Yeounsu <yyyynoom@gmail.com> --- net/core/neighbour.c | 4 ++++ 1 file changed, 4 insertions(+)