| Message ID | 4F28E22A.703@cn.fujitsu.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Le mercredi 01 février 2012 à 14:56 +0800, Li Zefan a écrit : > We've already used rcu_read_lock/unlock inside task_classid(), > so don't use the lock/unlock pair twice in this hot path. > > Signed-off-by: Li Zefan <lizf@cn.fujitsu.com> > --- > net/core/sock.c | 2 -- > 1 files changed, 0 insertions(+), 2 deletions(-) > > diff --git a/net/core/sock.c b/net/core/sock.c > index 213c856..c0bab23 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -1160,9 +1160,7 @@ void sock_update_classid(struct sock *sk) > { > u32 classid; > > - rcu_read_lock(); /* doing current task, which cannot vanish. */ > classid = task_cls_classid(current); > - rcu_read_unlock(); > if (classid && classid != sk->sk_classid) > sk->sk_classid = classid; Yes, this seems fine. Then, I wonder why we do the "if (classid && classid != sk->sk_classid)" before the : sk->sk_classid = classid; This seems unnecessary checks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Wed, 01 Feb 2012 08:07:19 +0100 > Then, I wonder why we do the "if (classid && classid != sk->sk_classid)" > > before the : > > sk->sk_classid = classid; > > This seems unnecessary checks. > Avoiding dirtying the sk->sk_classid cache line unnecessarily? I actually have no idea actually how often this routine can get invoked in real world scenerios. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Cc: Herbert Xu <herbert@gondor.apana.org.au> >> diff --git a/net/core/sock.c b/net/core/sock.c >> index 213c856..c0bab23 100644 >> --- a/net/core/sock.c >> +++ b/net/core/sock.c >> @@ -1160,9 +1160,7 @@ void sock_update_classid(struct sock *sk) >> { >> u32 classid; >> >> - rcu_read_lock(); /* doing current task, which cannot vanish. */ >> classid = task_cls_classid(current); >> - rcu_read_unlock(); >> if (classid && classid != sk->sk_classid) >> sk->sk_classid = classid; > > Yes, this seems fine. > > Then, I wonder why we do the "if (classid && classid != sk->sk_classid)" > > before the : > > sk->sk_classid = classid; > > This seems unnecessary checks. > I was wondering about this too. He who added this may provide us with an answer. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 01, 2012 at 03:20:00PM +0800, Li Zefan wrote: > Cc: Herbert Xu <herbert@gondor.apana.org.au> > > >> diff --git a/net/core/sock.c b/net/core/sock.c > >> index 213c856..c0bab23 100644 > >> --- a/net/core/sock.c > >> +++ b/net/core/sock.c > >> @@ -1160,9 +1160,7 @@ void sock_update_classid(struct sock *sk) > >> { > >> u32 classid; > >> > >> - rcu_read_lock(); /* doing current task, which cannot vanish. */ > >> classid = task_cls_classid(current); > >> - rcu_read_unlock(); > >> if (classid && classid != sk->sk_classid) > >> sk->sk_classid = classid; > > > > Yes, this seems fine. > > > > Then, I wonder why we do the "if (classid && classid != sk->sk_classid)" > > > > before the : > > > > sk->sk_classid = classid; > > > > This seems unnecessary checks. > > > > I was wondering about this too. He who added this may provide us with an > answer. Well writing a cache-line unnecessarily is bad. Cheers,
diff --git a/net/core/sock.c b/net/core/sock.c index 213c856..c0bab23 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -1160,9 +1160,7 @@ void sock_update_classid(struct sock *sk) { u32 classid; - rcu_read_lock(); /* doing current task, which cannot vanish. */ classid = task_cls_classid(current); - rcu_read_unlock(); if (classid && classid != sk->sk_classid) sk->sk_classid = classid; }
We've already used rcu_read_lock/unlock inside task_classid(), so don't use the lock/unlock pair twice in this hot path. Signed-off-by: Li Zefan <lizf@cn.fujitsu.com> --- net/core/sock.c | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-)