* [PATCH net-next] net: add annotation for sock_{lock,unlock}_fast
@ 2020-11-16 10:36 Paolo Abeni
2020-11-16 22:27 ` Luc Van Oostenryck
0 siblings, 1 reply; 5+ messages in thread
From: Paolo Abeni @ 2020-11-16 10:36 UTC (permalink / raw)
To: netdev; +Cc: Jakub Kicinski, Eric Dumazet, linux-sparse
The static checker is fooled by the non-static locking scheme
implemented by the mentioned helpers.
Let's make its life easier adding some unconditional annotation
so that the helpers are now interpreted as a plain spinlock from
sparse.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
include/net/sock.h | 9 ++++++---
net/core/sock.c | 3 ++-
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index 1d29aeae74fd..60d321c6b5a5 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1595,7 +1595,8 @@ void release_sock(struct sock *sk);
SINGLE_DEPTH_NESTING)
#define bh_unlock_sock(__sk) spin_unlock(&((__sk)->sk_lock.slock))
-bool lock_sock_fast(struct sock *sk);
+bool lock_sock_fast(struct sock *sk) __acquires(&sk->sk_lock.slock);
+
/**
* unlock_sock_fast - complement of lock_sock_fast
* @sk: socket
@@ -1606,10 +1607,12 @@ bool lock_sock_fast(struct sock *sk);
*/
static inline void unlock_sock_fast(struct sock *sk, bool slow)
{
- if (slow)
+ if (slow) {
release_sock(sk);
- else
+ __release(&sk->sk_lock.slock);
+ } else {
spin_unlock_bh(&sk->sk_lock.slock);
+ }
}
/* Used by processes to "lock" a socket state, so that
diff --git a/net/core/sock.c b/net/core/sock.c
index 727ea1cc633c..9badbe7bb4e4 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -3078,7 +3078,7 @@ EXPORT_SYMBOL(release_sock);
*
* sk_lock.slock unlocked, owned = 1, BH enabled
*/
-bool lock_sock_fast(struct sock *sk)
+bool lock_sock_fast(struct sock *sk) __acquires(&sk->sk_lock.slock)
{
might_sleep();
spin_lock_bh(&sk->sk_lock.slock);
@@ -3096,6 +3096,7 @@ bool lock_sock_fast(struct sock *sk)
* The sk_lock has mutex_lock() semantics here:
*/
mutex_acquire(&sk->sk_lock.dep_map, 0, 0, _RET_IP_);
+ __acquire(&sk->sk_lock.slock);
local_bh_enable();
return true;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net: add annotation for sock_{lock,unlock}_fast
2020-11-16 10:36 [PATCH net-next] net: add annotation for sock_{lock,unlock}_fast Paolo Abeni
@ 2020-11-16 22:27 ` Luc Van Oostenryck
2020-11-17 8:38 ` Paolo Abeni
0 siblings, 1 reply; 5+ messages in thread
From: Luc Van Oostenryck @ 2020-11-16 22:27 UTC (permalink / raw)
To: Paolo Abeni; +Cc: netdev, Jakub Kicinski, Eric Dumazet, linux-sparse
On Mon, Nov 16, 2020 at 11:36:39AM +0100, Paolo Abeni wrote:
> The static checker is fooled by the non-static locking scheme
> implemented by the mentioned helpers.
> Let's make its life easier adding some unconditional annotation
> so that the helpers are now interpreted as a plain spinlock from
> sparse.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> include/net/sock.h | 9 ++++++---
> net/core/sock.c | 3 ++-
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 1d29aeae74fd..60d321c6b5a5 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1595,7 +1595,8 @@ void release_sock(struct sock *sk);
> SINGLE_DEPTH_NESTING)
> #define bh_unlock_sock(__sk) spin_unlock(&((__sk)->sk_lock.slock))
>
> -bool lock_sock_fast(struct sock *sk);
> +bool lock_sock_fast(struct sock *sk) __acquires(&sk->sk_lock.slock);
> +
Good.
> /**
> * unlock_sock_fast - complement of lock_sock_fast
> * @sk: socket
> @@ -1606,10 +1607,12 @@ bool lock_sock_fast(struct sock *sk);
> */
> static inline void unlock_sock_fast(struct sock *sk, bool slow)
> {
> - if (slow)
> + if (slow) {
> release_sock(sk);
> - else
> + __release(&sk->sk_lock.slock);
The correct solution would be to annotate the declaration of
release_sock() with '__releases(&sk->sk_lock.slock)'.
> /* Used by processes to "lock" a socket state, so that
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 727ea1cc633c..9badbe7bb4e4 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -3078,7 +3078,7 @@ EXPORT_SYMBOL(release_sock);
> *
> * sk_lock.slock unlocked, owned = 1, BH enabled
> */
> -bool lock_sock_fast(struct sock *sk)
> +bool lock_sock_fast(struct sock *sk) __acquires(&sk->sk_lock.slock)
> {
> might_sleep();
> spin_lock_bh(&sk->sk_lock.slock);
> @@ -3096,6 +3096,7 @@ bool lock_sock_fast(struct sock *sk)
> * The sk_lock has mutex_lock() semantics here:
> */
> mutex_acquire(&sk->sk_lock.dep_map, 0, 0, _RET_IP_);
> + __acquire(&sk->sk_lock.slock);
OK, given that the mutexes are not annotated.
-- Luc
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net: add annotation for sock_{lock,unlock}_fast
2020-11-16 22:27 ` Luc Van Oostenryck
@ 2020-11-17 8:38 ` Paolo Abeni
2020-11-17 16:58 ` Luc Van Oostenryck
0 siblings, 1 reply; 5+ messages in thread
From: Paolo Abeni @ 2020-11-17 8:38 UTC (permalink / raw)
To: Luc Van Oostenryck; +Cc: netdev, Jakub Kicinski, Eric Dumazet, linux-sparse
Hello,
Thank you for the feedback!
On Mon, 2020-11-16 at 23:27 +0100, Luc Van Oostenryck wrote:
> > @@ -1606,10 +1607,12 @@ bool lock_sock_fast(struct sock *sk);
> > */
> > static inline void unlock_sock_fast(struct sock *sk, bool slow)
> > {
> > - if (slow)
> > + if (slow) {
> > release_sock(sk);
> > - else
> > + __release(&sk->sk_lock.slock);
>
> The correct solution would be to annotate the declaration of
> release_sock() with '__releases(&sk->sk_lock.slock)'.
If I add such annotation to release_sock(), I'll get several sparse
warnings for context imbalance (on each lock_sock()/release_sock()
pair), unless I also add an '__acquires()' annotation to lock_sock().
The above does not look correct to me ?!? When release_sock() completes
the socket spin lock is not held. The annotation added above is
somewhat an artifact to let unlock_sock_fast() matches lock_sock_fast()
from sparse perspective. I intentionally avoided changing
the release_sock() annotation to avoid introducing more artifacts.
The proposed schema is not 100% accurate, as it will also allow e.g. a
really-not-fitting bh_lock_sock()/unlock_sock_fast() pair, but I could
not come-up with anything better.
Can we go with the schema I proposed?
Thanks,
Paolo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net: add annotation for sock_{lock,unlock}_fast
2020-11-17 8:38 ` Paolo Abeni
@ 2020-11-17 16:58 ` Luc Van Oostenryck
2020-11-17 17:36 ` Paolo Abeni
0 siblings, 1 reply; 5+ messages in thread
From: Luc Van Oostenryck @ 2020-11-17 16:58 UTC (permalink / raw)
To: Paolo Abeni; +Cc: netdev, Jakub Kicinski, Eric Dumazet, linux-sparse
On Tue, Nov 17, 2020 at 09:38:45AM +0100, Paolo Abeni wrote:
> Hello,
>
> Thank you for the feedback!
>
> On Mon, 2020-11-16 at 23:27 +0100, Luc Van Oostenryck wrote:
> > > @@ -1606,10 +1607,12 @@ bool lock_sock_fast(struct sock *sk);
> > > */
> > > static inline void unlock_sock_fast(struct sock *sk, bool slow)
> > > {
> > > - if (slow)
> > > + if (slow) {
> > > release_sock(sk);
> > > - else
> > > + __release(&sk->sk_lock.slock);
> >
> > The correct solution would be to annotate the declaration of
> > release_sock() with '__releases(&sk->sk_lock.slock)'.
>
> If I add such annotation to release_sock(), I'll get several sparse
> warnings for context imbalance (on each lock_sock()/release_sock()
> pair), unless I also add an '__acquires()' annotation to lock_sock().
>
> The above does not look correct to me ?!? When release_sock() completes
> the socket spin lock is not held.
Yes, that's fine, but I suppose it somehow releases the mutex that
is taken in lock_sock_fast() when returning true, right?
> The annotation added above is
> somewhat an artifact to let unlock_sock_fast() matches lock_sock_fast()
> from sparse perspective. I intentionally avoided changing
> the release_sock() annotation to avoid introducing more artifacts.
>
> The proposed schema is not 100% accurate, as it will also allow e.g. a
> really-not-fitting bh_lock_sock()/unlock_sock_fast() pair, but I could
> not come-up with anything better.
>
> Can we go with the schema I proposed?
Well, I suppose it's a first step.
But can you then add a '__releases(...)' to unlock_sock_fast()?
It's not needed by sparse because it's an inline function and sparse
can then deduce it but it will help to see the pairing with
lock_sock_fast() is OK.
-- Luc
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net: add annotation for sock_{lock,unlock}_fast
2020-11-17 16:58 ` Luc Van Oostenryck
@ 2020-11-17 17:36 ` Paolo Abeni
0 siblings, 0 replies; 5+ messages in thread
From: Paolo Abeni @ 2020-11-17 17:36 UTC (permalink / raw)
To: Luc Van Oostenryck; +Cc: netdev, Jakub Kicinski, Eric Dumazet, linux-sparse
Hello,
On Tue, 2020-11-17 at 17:58 +0100, Luc Van Oostenryck wrote:
> On Tue, Nov 17, 2020 at 09:38:45AM +0100, Paolo Abeni wrote:
> > Hello,
> >
> > Thank you for the feedback!
> >
> > On Mon, 2020-11-16 at 23:27 +0100, Luc Van Oostenryck wrote:
> > > > @@ -1606,10 +1607,12 @@ bool lock_sock_fast(struct sock *sk);
> > > > */
> > > > static inline void unlock_sock_fast(struct sock *sk, bool slow)
> > > > {
> > > > - if (slow)
> > > > + if (slow) {
> > > > release_sock(sk);
> > > > - else
> > > > + __release(&sk->sk_lock.slock);
> > >
> > > The correct solution would be to annotate the declaration of
> > > release_sock() with '__releases(&sk->sk_lock.slock)'.
> >
> > If I add such annotation to release_sock(), I'll get several sparse
> > warnings for context imbalance (on each lock_sock()/release_sock()
> > pair), unless I also add an '__acquires()' annotation to lock_sock().
> >
> > The above does not look correct to me ?!? When release_sock() completes
> > the socket spin lock is not held.
>
> Yes, that's fine, but I suppose it somehow releases the mutex that
> is taken in lock_sock_fast() when returning true, right?
Well, it has mutex semantics, but does not really acquire any mutex.
> > The annotation added above is
> > somewhat an artifact to let unlock_sock_fast() matches lock_sock_fast()
> > from sparse perspective. I intentionally avoided changing
> > the release_sock() annotation to avoid introducing more artifacts.
> >
> > The proposed schema is not 100% accurate, as it will also allow e.g. a
> > really-not-fitting bh_lock_sock()/unlock_sock_fast() pair, but I could
> > not come-up with anything better.
> >
> > Can we go with the schema I proposed?
>
> Well, I suppose it's a first step.
> But can you then add a '__releases(...)' to unlock_sock_fast()?
> It's not needed by sparse because it's an inline function and sparse
> can then deduce it but it will help to see the pairing with
> lock_sock_fast() is OK.
Ok, I'll send a v2 with such annotation.
Thanks!
Paolo
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-11-17 17:36 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-16 10:36 [PATCH net-next] net: add annotation for sock_{lock,unlock}_fast Paolo Abeni
2020-11-16 22:27 ` Luc Van Oostenryck
2020-11-17 8:38 ` Paolo Abeni
2020-11-17 16:58 ` Luc Van Oostenryck
2020-11-17 17:36 ` Paolo Abeni
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.