Re: [PATCH v4 3/3] net: unix: optimize wakeups in unix_dgram_recvmsg()

[Jason Baron]

On 10/09/2015 12:29 AM, kbuild test robot wrote:
> Hi Jason,
> 
> [auto build test ERROR on v4.3-rc3 -- if it's inappropriate base, please 
> ignore]
> 
> config: x86_64-randconfig-i0-201540 (attached as .config)
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64 
> 
> All errors (new ones prefixed by >>):
> 
>net/unix/af_unix.c: In function 'unix_dgram_writable':
>>> net/unix/af_unix.c:2465:3: error: 'other_full' undeclared (first use in 
>>> this function)
>  *other_full = false;
>   ^
>net/unix/af_unix.c:2465:3: note: each undeclared identifier is reported 
> only once for each function it appears in
> 


Forgot to refresh this patch before sending. The one that I tested with
is below.

Thanks,

-Jason




Now that connect() permanently registers a callback routine, we can induce
extra overhead in unix_dgram_recvmsg(), which unconditionally wakes up
its peer_wait queue on every receive. This patch makes the wakeup there
conditional on there being waiters.

Tested using: http://www.spinics.net/lists/netdev/msg145533.html

Signed-off-by: Jason Baron 
---
 include/net/af_unix.h |  1 +
 net/unix/af_unix.c| 92 +--
 2 files changed, 69 insertions(+), 24 deletions(-)

diff --git a/include/net/af_unix.h b/include/net/af_unix.h
index 6a4a345..cf21ffd 100644
--- a/include/net/af_unix.h
+++ b/include/net/af_unix.h
@@ -61,6 +61,7 @@ struct unix_sock {
unsigned long   flags;
 #define UNIX_GC_CANDIDATE  0
 #define UNIX_GC_MAYBE_CYCLE1
+#define UNIX_NOSPACE   2
struct socket_wqpeer_wq;
wait_queue_twait;
 };
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index f789423..ac9bcd8 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -326,7 +326,7 @@ found:
return s;
 }
 
-static inline int unix_writable(struct sock *sk)
+static inline bool unix_writable(struct sock *sk)
 {
return (atomic_read(>sk_wmem_alloc) << 2) <= sk->sk_sndbuf;
 }
@@ -1079,6 +1079,12 @@ static long unix_wait_for_peer(struct sock *other, long 
timeo)
 
prepare_to_wait_exclusive(>peer_wait, , TASK_INTERRUPTIBLE);
 
+   set_bit(UNIX_NOSPACE, >flags);
+   /* Ensure that we either see space in the peer sk_receive_queue via the
+* unix_recvq_full() check below, or we receive a wakeup when it
+* empties. Pairs with the mb in unix_dgram_recvmsg().
+*/
+   smp_mb__after_atomic();
sched = !sock_flag(other, SOCK_DEAD) &&
!(other->sk_shutdown & RCV_SHUTDOWN) &&
unix_recvq_full(other);
@@ -1623,17 +1629,27 @@ restart:
 
if (unix_peer(other) != sk && unix_recvq_full(other)) {
if (!timeo) {
-   err = -EAGAIN;
-   goto out_unlock;
-   }
-
-   timeo = unix_wait_for_peer(other, timeo);
+   set_bit(UNIX_NOSPACE, _sk(other)->flags);
+   /* Ensure that we either see space in the peer
+* sk_receive_queue via the unix_recvq_full() check
+* below, or we receive a wakeup when it empties. This
+* makes sure that epoll ET triggers correctly. Pairs
+* with the mb in unix_dgram_recvmsg().
+*/
+   smp_mb__after_atomic();
+   if (unix_recvq_full(other)) {
+   err = -EAGAIN;
+   goto out_unlock;
+   }
+   } else {
+   timeo = unix_wait_for_peer(other, timeo);
 
-   err = sock_intr_errno(timeo);
-   if (signal_pending(current))
-   goto out_free;
+   err = sock_intr_errno(timeo);
+   if (signal_pending(current))
+   goto out_free;
 
-   goto restart;
+   goto restart;
+   }
}
 
if (sock_flag(other, SOCK_RCVTSTAMP))
@@ -1939,8 +1955,19 @@ static int unix_dgram_recvmsg(struct socket *sock, 
struct msghdr *msg,
goto out_unlock;
}
 
-   wake_up_interruptible_sync_poll(>peer_wait,
-   POLLOUT | POLLWRNORM | POLLWRBAND);
+   /* Ensure that waiters on our sk->sk_receive_queue draining that check
+* via unix_recvq_full() either see space in the queue or get a wakeup
+* below. sk->sk_receive_queue is reduece by the __skb_recv_datagram()
+* call above. Pairs with the mb in unix_dgram_sendmsg(),
+*unix_dgram_poll(), and unix_wait_for_peer().
+*/
+   smp_mb();
+   if (test_bit(UNIX_NOSPACE, >flags)) {
+   clear_bit(UNIX_NOSPACE, >flags);
+   wake_up_interruptible_sync_poll(>peer_wait,
+  

Re: [PATCH v4 3/3] net: unix: optimize wakeups in unix_dgram_recvmsg()

[Jason Baron]

On 10/09/2015 12:29 AM, kbuild test robot wrote:
> Hi Jason,
> 
> [auto build test ERROR on v4.3-rc3 -- if it's inappropriate base, please 
> ignore]
> 
> config: x86_64-randconfig-i0-201540 (attached as .config)
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64 
> 
> All errors (new ones prefixed by >>):
> 
>net/unix/af_unix.c: In function 'unix_dgram_writable':
>>> net/unix/af_unix.c:2465:3: error: 'other_full' undeclared (first use in 
>>> this function)
>  *other_full = false;
>   ^
>net/unix/af_unix.c:2465:3: note: each undeclared identifier is reported 
> only once for each function it appears in
> 


Forgot to refresh this patch before sending. The one that I tested with
is below.

Thanks,

-Jason




Now that connect() permanently registers a callback routine, we can induce
extra overhead in unix_dgram_recvmsg(), which unconditionally wakes up
its peer_wait queue on every receive. This patch makes the wakeup there
conditional on there being waiters.

Tested using: http://www.spinics.net/lists/netdev/msg145533.html

Signed-off-by: Jason Baron 
---
 include/net/af_unix.h |  1 +
 net/unix/af_unix.c| 92 +--
 2 files changed, 69 insertions(+), 24 deletions(-)

diff --git a/include/net/af_unix.h b/include/net/af_unix.h
index 6a4a345..cf21ffd 100644
--- a/include/net/af_unix.h
+++ b/include/net/af_unix.h
@@ -61,6 +61,7 @@ struct unix_sock {
unsigned long   flags;
 #define UNIX_GC_CANDIDATE  0
 #define UNIX_GC_MAYBE_CYCLE1
+#define UNIX_NOSPACE   2
struct socket_wqpeer_wq;
wait_queue_twait;
 };
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index f789423..ac9bcd8 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -326,7 +326,7 @@ found:
return s;
 }
 
-static inline int unix_writable(struct sock *sk)
+static inline bool unix_writable(struct sock *sk)
 {
return (atomic_read(>sk_wmem_alloc) << 2) <= sk->sk_sndbuf;
 }
@@ -1079,6 +1079,12 @@ static long unix_wait_for_peer(struct sock *other, long 
timeo)
 
prepare_to_wait_exclusive(>peer_wait, , TASK_INTERRUPTIBLE);
 
+   set_bit(UNIX_NOSPACE, >flags);
+   /* Ensure that we either see space in the peer sk_receive_queue via the
+* unix_recvq_full() check below, or we receive a wakeup when it
+* empties. Pairs with the mb in unix_dgram_recvmsg().
+*/
+   smp_mb__after_atomic();
sched = !sock_flag(other, SOCK_DEAD) &&
!(other->sk_shutdown & RCV_SHUTDOWN) &&
unix_recvq_full(other);
@@ -1623,17 +1629,27 @@ restart:
 
if (unix_peer(other) != sk && unix_recvq_full(other)) {
if (!timeo) {
-   err = -EAGAIN;
-   goto out_unlock;
-   }
-
-   timeo = unix_wait_for_peer(other, timeo);
+   set_bit(UNIX_NOSPACE, _sk(other)->flags);
+   /* Ensure that we either see space in the peer
+* sk_receive_queue via the unix_recvq_full() check
+* below, or we receive a wakeup when it empties. This
+* makes sure that epoll ET triggers correctly. Pairs
+* with the mb in unix_dgram_recvmsg().
+*/
+   smp_mb__after_atomic();
+   if (unix_recvq_full(other)) {
+   err = -EAGAIN;
+   goto out_unlock;
+   }
+   } else {
+   timeo = unix_wait_for_peer(other, timeo);
 
-   err = sock_intr_errno(timeo);
-   if (signal_pending(current))
-   goto out_free;
+   err = sock_intr_errno(timeo);
+   if (signal_pending(current))
+   goto out_free;
 
-   goto restart;
+   goto restart;
+   }
}
 
if (sock_flag(other, SOCK_RCVTSTAMP))
@@ -1939,8 +1955,19 @@ static int unix_dgram_recvmsg(struct socket *sock, 
struct msghdr *msg,
goto out_unlock;
}
 
-   wake_up_interruptible_sync_poll(>peer_wait,
-   POLLOUT | POLLWRNORM | POLLWRBAND);
+   /* Ensure that waiters on our sk->sk_receive_queue draining that check
+* via unix_recvq_full() either see space in the queue or get a wakeup
+* below. sk->sk_receive_queue is reduece by the __skb_recv_datagram()
+* call above. Pairs with the mb in unix_dgram_sendmsg(),
+*unix_dgram_poll(), and unix_wait_for_peer().
+*/
+   smp_mb();
+   if (test_bit(UNIX_NOSPACE, >flags)) {
+   clear_bit(UNIX_NOSPACE, >flags);
+   wake_up_interruptible_sync_poll(>peer_wait,
+   

Re: [PATCH v4 3/3] net: unix: optimize wakeups in unix_dgram_recvmsg()

[kbuild test robot]

Hi Jason,

[auto build test ERROR on v4.3-rc3 -- if it's inappropriate base, please ignore]

config: x86_64-randconfig-i0-201540 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

   net/unix/af_unix.c: In function 'unix_dgram_writable':
>> net/unix/af_unix.c:2465:3: error: 'other_full' undeclared (first use in this 
>> function)
 *other_full = false;
  ^
   net/unix/af_unix.c:2465:3: note: each undeclared identifier is reported only 
once for each function it appears in

vim +/other_full +2465 net/unix/af_unix.c

  2459  return mask;
  2460  }
  2461  
  2462  static bool unix_dgram_writable(struct sock *sk, struct sock *other,
  2463  bool *other_nospace)
  2464  {
> 2465  *other_full = false;
  2466  
  2467  if (other && unix_peer(other) != sk && unix_recvq_full(other)) {
  2468  *other_full = true;

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH v4 3/3] net: unix: optimize wakeups in unix_dgram_recvmsg()

[kbuild test robot]

Hi Jason,

[auto build test ERROR on v4.3-rc3 -- if it's inappropriate base, please ignore]

config: x86_64-randconfig-i0-201540 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

   net/unix/af_unix.c: In function 'unix_dgram_writable':
>> net/unix/af_unix.c:2465:3: error: 'other_full' undeclared (first use in this 
>> function)
 *other_full = false;
  ^
   net/unix/af_unix.c:2465:3: note: each undeclared identifier is reported only 
once for each function it appears in

vim +/other_full +2465 net/unix/af_unix.c

  2459  return mask;
  2460  }
  2461  
  2462  static bool unix_dgram_writable(struct sock *sk, struct sock *other,
  2463  bool *other_nospace)
  2464  {
> 2465  *other_full = false;
  2466  
  2467  if (other && unix_peer(other) != sk && unix_recvq_full(other)) {
  2468  *other_full = true;

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data