Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1728609 > unrolled thread

[PATCH 0/3] net: TCP/IP: A few minor cleanups

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] net: TCP/IP: A few minor cleanups Michael Witten <mfwitten@gmail.com> - 2017-09-08 08:00 +0200
    [PATCH 1/3] net: __sock_cmsg_send(): Remove unused parameter `msg' Michael Witten <mfwitten@gmail.com> - 2017-09-08 08:10 +0200
    [PATCH 3/3] net: skb_queue_purge(): lock/unlock the list only once Michael Witten <mfwitten@gmail.com> - 2017-09-08 08:10 +0200
      Re: [PATCH 3/3] net: skb_queue_purge(): lock/unlock the list only  once Eric Dumazet <eric.dumazet@gmail.com> - 2017-09-08 18:10 +0200
      Re: [PATCH 3/3] net: skb_queue_purge(): lock/unlock the list only  once Stephen Hemminger <stephen@networkplumber.org> - 2017-09-08 19:00 +0200
      [PATCH v1 3/3] net: skb_queue_purge(): lock/unlock the queue only once Michael Witten <mfwitten@gmail.com> - 2017-09-09 08:30 +0200
        Re: [PATCH v1 3/3] net: skb_queue_purge(): lock/unlock the queue  only once Eric Dumazet <eric.dumazet@gmail.com> - 2017-09-09 19:00 +0200
    [PATCH 2/3] net: inet_recvmsg(): Remove unnecessary bitwise operation Michael Witten <mfwitten@gmail.com> - 2017-09-08 08:10 +0200

#1728609 — [PATCH 0/3] net: TCP/IP: A few minor cleanups

The following patch series is an ad hoc "cleanup" that I made
while perusing the code (I'm not well versed in this code, so I
would not be surprised if there were objections to the changes):

  [1] net: __sock_cmsg_send(): Remove unused parameter `msg'
  [2] net: inet_recvmsg(): Remove unnecessary bitwise operation.
  [3] net: skb_queue_purge(): lock/unlock the list only once

Each patch will be sent as an individiual email; the total diff
is appended below for your convenience.

You may also fetch these patches from GitHub:

  git checkout -b test 5969d1bb3082b41eba8fd2c826559abe38ccb6df
  git pull https://github.com/mfwitten/linux.git net/tcp-ip/01-cleanup/00

Overall:

  include/net/sock.h     | 2 +-
  net/core/skbuff.c      | 6 +++++-
  net/core/sock.c        | 4 ++--
  net/ipv4/af_inet.c     | 2 +-
  net/ipv4/ip_sockglue.c | 2 +-
  net/ipv6/datagram.c    | 2 +-
  6 files changed, 11 insertions(+), 7 deletions(-)

Sincerly,
Michael Witten

diff --git a/include/net/sock.h b/include/net/sock.h
index 03a362568357..83373d7148a9 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1562,7 +1562,7 @@ struct sockcm_cookie {
 	u16 tsflags;
 };
 
-int __sock_cmsg_send(struct sock *sk, struct msghdr *msg, struct cmsghdr *cmsg,
+int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
 		     struct sockcm_cookie *sockc);
 int sock_cmsg_send(struct sock *sk, struct msghdr *msg,
 		   struct sockcm_cookie *sockc);
diff --git a/net/core/sock.c b/net/core/sock.c
index 9b7b6bbb2a23..425e03fe1c56 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2091,7 +2091,7 @@ struct sk_buff *sock_alloc_send_skb(struct sock *sk, unsigned long size,
 }
 EXPORT_SYMBOL(sock_alloc_send_skb);
 
-int __sock_cmsg_send(struct sock *sk, struct msghdr *msg, struct cmsghdr *cmsg,
+int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
 		     struct sockcm_cookie *sockc)
 {
 	u32 tsflags;
@@ -2137,7 +2137,7 @@ int sock_cmsg_send(struct sock *sk, struct msghdr *msg,
 			return -EINVAL;
 		if (cmsg->cmsg_level != SOL_SOCKET)
 			continue;
-		ret = __sock_cmsg_send(sk, msg, cmsg, sockc);
+		ret = __sock_cmsg_send(sk, cmsg, sockc);
 		if (ret)
 			return ret;
 	}
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index e558e4f9597b..c79b7822b0b9 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -263,7 +263,7 @@ int ip_cmsg_send(struct sock *sk, struct msghdr *msg, struct ipcm_cookie *ipc,
 		}
 #endif
 		if (cmsg->cmsg_level == SOL_SOCKET) {
-			err = __sock_cmsg_send(sk, msg, cmsg, &ipc->sockc);
+			err = __sock_cmsg_send(sk, cmsg, &ipc->sockc);
 			if (err)
 				return err;
 			continue;
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index a1f918713006..1d1926a4cbe2 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -756,7 +756,7 @@ int ip6_datagram_send_ctl(struct net *net, struct sock *sk,
 		}
 
 		if (cmsg->cmsg_level == SOL_SOCKET) {
-			err = __sock_cmsg_send(sk, msg, cmsg, sockc);
+			err = __sock_cmsg_send(sk, cmsg, sockc);
 			if (err)
 				return err;
 			continue;
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index e31108e5ef79..2dbed042a412 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -791,7 +791,7 @@ int inet_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
 	sock_rps_record_flow(sk);
 
 	err = sk->sk_prot->recvmsg(sk, msg, size, flags & MSG_DONTWAIT,
-				   flags & ~MSG_DONTWAIT, &addr_len);
+				   flags, &addr_len);
 	if (err >= 0)
 		msg->msg_namelen = addr_len;
 	return err;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 68065d7d383f..66c0731a2a5f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2834,9 +2834,13 @@ EXPORT_SYMBOL(skb_dequeue_tail);
  */
 void skb_queue_purge(struct sk_buff_head *list)
 {
+	unsigned long flags;
 	struct sk_buff *skb;
-	while ((skb = skb_dequeue(list)) != NULL)
+
+	spin_lock_irqsave(&list->lock, flags);
+	while ((skb = __skb_dequeue(list)) != NULL)
 		kfree_skb(skb);
+	spin_unlock_irqrestore(&list->lock, flags);
 }
 EXPORT_SYMBOL(skb_queue_purge);
 
-- 
2.14.1

[toc] | [next] | [standalone]

#1728611 — [PATCH 1/3] net: __sock_cmsg_send(): Remove unused parameter `msg'

Date: Thu, 7 Sep 2017 03:21:38 +0000
Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 include/net/sock.h     | 2 +-
 net/core/sock.c        | 4 ++--
 net/ipv4/ip_sockglue.c | 2 +-
 net/ipv6/datagram.c    | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 03a362568357..83373d7148a9 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1562,7 +1562,7 @@ struct sockcm_cookie {
 	u16 tsflags;
 };
 
-int __sock_cmsg_send(struct sock *sk, struct msghdr *msg, struct cmsghdr *cmsg,
+int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
 		     struct sockcm_cookie *sockc);
 int sock_cmsg_send(struct sock *sk, struct msghdr *msg,
 		   struct sockcm_cookie *sockc);
diff --git a/net/core/sock.c b/net/core/sock.c
index 9b7b6bbb2a23..425e03fe1c56 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2091,7 +2091,7 @@ struct sk_buff *sock_alloc_send_skb(struct sock *sk, unsigned long size,
 }
 EXPORT_SYMBOL(sock_alloc_send_skb);
 
-int __sock_cmsg_send(struct sock *sk, struct msghdr *msg, struct cmsghdr *cmsg,
+int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
 		     struct sockcm_cookie *sockc)
 {
 	u32 tsflags;
@@ -2137,7 +2137,7 @@ int sock_cmsg_send(struct sock *sk, struct msghdr *msg,
 			return -EINVAL;
 		if (cmsg->cmsg_level != SOL_SOCKET)
 			continue;
-		ret = __sock_cmsg_send(sk, msg, cmsg, sockc);
+		ret = __sock_cmsg_send(sk, cmsg, sockc);
 		if (ret)
 			return ret;
 	}
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index e558e4f9597b..c79b7822b0b9 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -263,7 +263,7 @@ int ip_cmsg_send(struct sock *sk, struct msghdr *msg, struct ipcm_cookie *ipc,
 		}
 #endif
 		if (cmsg->cmsg_level == SOL_SOCKET) {
-			err = __sock_cmsg_send(sk, msg, cmsg, &ipc->sockc);
+			err = __sock_cmsg_send(sk, cmsg, &ipc->sockc);
 			if (err)
 				return err;
 			continue;
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index a1f918713006..1d1926a4cbe2 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -756,7 +756,7 @@ int ip6_datagram_send_ctl(struct net *net, struct sock *sk,
 		}
 
 		if (cmsg->cmsg_level == SOL_SOCKET) {
-			err = __sock_cmsg_send(sk, msg, cmsg, sockc);
+			err = __sock_cmsg_send(sk, cmsg, sockc);
 			if (err)
 				return err;
 			continue;
-- 
2.14.1

[toc] | [prev] | [next] | [standalone]

#1728612 — [PATCH 3/3] net: skb_queue_purge(): lock/unlock the list only once

Date: Thu, 7 Sep 2017 20:07:40 +0000
With this commit, the list's lock is locked/unlocked only once
for the duration of `skb_queue_purge()'.

Hitherto, the list's lock has been locked/unlocked every time
an item is dequeued; this seems not only inefficient, but also
incorrect, as the whole point of `skb_queue_purge()' is to clear
the list, presumably without giving anything else a chance to
manipulate the list in the interim.

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 net/core/skbuff.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 68065d7d383f..66c0731a2a5f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2834,9 +2834,13 @@ EXPORT_SYMBOL(skb_dequeue_tail);
  */
 void skb_queue_purge(struct sk_buff_head *list)
 {
+	unsigned long flags;
 	struct sk_buff *skb;
-	while ((skb = skb_dequeue(list)) != NULL)
+
+	spin_lock_irqsave(&list->lock, flags);
+	while ((skb = __skb_dequeue(list)) != NULL)
 		kfree_skb(skb);
+	spin_unlock_irqrestore(&list->lock, flags);
 }
 EXPORT_SYMBOL(skb_queue_purge);
 
-- 
2.14.1

[toc] | [prev] | [next] | [standalone]

#1729055 — Re: [PATCH 3/3] net: skb_queue_purge(): lock/unlock the list only once

On Fri, 2017-09-08 at 05:06 +0000, Michael Witten wrote:
> Date: Thu, 7 Sep 2017 20:07:40 +0000
> With this commit, the list's lock is locked/unlocked only once
> for the duration of `skb_queue_purge()'.
> 
> Hitherto, the list's lock has been locked/unlocked every time
> an item is dequeued; this seems not only inefficient, but also
> incorrect, as the whole point of `skb_queue_purge()' is to clear
> the list, presumably without giving anything else a chance to
> manipulate the list in the interim.
> 
> Signed-off-by: Michael Witten <mfwitten@gmail.com>
> ---
>  net/core/skbuff.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 68065d7d383f..66c0731a2a5f 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2834,9 +2834,13 @@ EXPORT_SYMBOL(skb_dequeue_tail);
>   */
>  void skb_queue_purge(struct sk_buff_head *list)
>  {
> +	unsigned long flags;
>  	struct sk_buff *skb;
> -	while ((skb = skb_dequeue(list)) != NULL)
> +
> +	spin_lock_irqsave(&list->lock, flags);
> +	while ((skb = __skb_dequeue(list)) != NULL)
>  		kfree_skb(skb);
> +	spin_unlock_irqrestore(&list->lock, flags);
>  }
>  EXPORT_SYMBOL(skb_queue_purge);
>  


No, this is very wrong :

Holding hard IRQ for a potential very long time is going to break
horribly. Some lists can have 10,000+ skbs in them.

Note that net-next tree is currently closed, please read 
Documentation/networking/netdev-FAQ.txt

[toc] | [prev] | [next] | [standalone]

#1729090 — Re: [PATCH 3/3] net: skb_queue_purge(): lock/unlock the list only once

On Fri, 08 Sep 2017 05:06:30 -0000
Michael Witten <mfwitten@gmail.com> wrote:

> Date: Thu, 7 Sep 2017 20:07:40 +0000
> With this commit, the list's lock is locked/unlocked only once
> for the duration of `skb_queue_purge()'.
> 
> Hitherto, the list's lock has been locked/unlocked every time
> an item is dequeued; this seems not only inefficient, but also
> incorrect, as the whole point of `skb_queue_purge()' is to clear
> the list, presumably without giving anything else a chance to
> manipulate the list in the interim.
> 
> Signed-off-by: Michael Witten <mfwitten@gmail.com>
> ---
>  net/core/skbuff.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 68065d7d383f..66c0731a2a5f 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2834,9 +2834,13 @@ EXPORT_SYMBOL(skb_dequeue_tail);
>   */
>  void skb_queue_purge(struct sk_buff_head *list)
>  {
> +	unsigned long flags;
>  	struct sk_buff *skb;
> -	while ((skb = skb_dequeue(list)) != NULL)
> +
> +	spin_lock_irqsave(&list->lock, flags);
> +	while ((skb = __skb_dequeue(list)) != NULL)
>  		kfree_skb(skb);
> +	spin_unlock_irqrestore(&list->lock, flags);
>  }
>  EXPORT_SYMBOL(skb_queue_purge);
>  

As Eric said, this won't work.

Instead why not introduce something list splice which moves next/prev
of list head to a local list on  the stack.

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 68065d7d383f..4988b6efdcc8 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2824,6 +2824,44 @@ struct sk_buff *skb_dequeue_tail(struct sk_buff_head *list)
 }
 EXPORT_SYMBOL(skb_dequeue_tail);
 
+static void __skb_splice(const struct sk_buff_head *list,
+			 struct sk_buff *prev,
+			 struct sk_buff *next)
+{
+	struct sk_buff *first = list->next;
+	struct sk_buff *last = list->prev;
+
+	list->qlen = 0;
+
+	first->prev = prev;
+	prev->next = first;
+
+	list->next = next;
+	next->prev = last;
+}
+
+/**
+ *	skb_splice - join two lists, and initialize the emptied list
+ *	@list: the new list to add
+ *	@head: the pace to add it in the first list
+ *
+ *	Take the first list (@list) and merge it onto the
+ *	head of existing list (@head).
+ */
+static void skb_splice_init(const struct sk_buff_head *list,
+			    struct sk_buff_head *head)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&list->lock, flags);
+	if (list->qlen > 0) {
+		head->qlen += list->qlen;
+		__skb_splice(list, head, head->next);
+		__skb_queue_head_init(list);
+	}
+	spin_unlock_irqrestore(&list->lock, flags);
+}
+
 /**
  *	skb_queue_purge - empty a list
  *	@list: list to empty
@@ -2835,7 +2873,12 @@ EXPORT_SYMBOL(skb_dequeue_tail);
 void skb_queue_purge(struct sk_buff_head *list)
 {
 	struct sk_buff *skb;
-	while ((skb = skb_dequeue(list)) != NULL)
+	struct skb_buff_head tmp;
+
+	__skb_queue_head_init(&tmp);
+	skb_splice_init(list, &tmp);
+
+	while ((skb = __skb_dequeue(list)) != NULL)
 		kfree_skb(skb);
 }
 EXPORT_SYMBOL(skb_queue_purge);

[toc] | [prev] | [next] | [standalone]

#1729391 — [PATCH v1 3/3] net: skb_queue_purge(): lock/unlock the queue only once

Thanks for your input, Eric Dumazet and Stephen Hemminger; based on
your observations, this version of the patch implements a very
lightweight purging of the queue.

To apply this patch, save this email to:

  /path/to/email

and then run:

  git am --scissors /path/to/email

You may also fetch this patch from GitHub:

  git checkout -b test 5969d1bb3082b41eba8fd2c826559abe38ccb6df
  git pull https://github.com/mfwitten/linux.git net/tcp-ip/01-cleanup/02

Sincerely,
Michael Witten

8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----

Hitherto, the queue's lock has been locked/unlocked every time
an item is dequeued; this seems not only inefficient, but also
incorrect, as the whole point of `skb_queue_purge()' is to clear
the queue, presumably without giving any other thread a chance to
manipulate the queue in the interim.

With this commit, the queue's lock is locked/unlocked only once
when `skb_queue_purge()' is called, and in a way that disables
the IRQs for only a minimal amount of time.

This is achieved by atomically re-initializing the queue (thereby
clearing it), and then freeing each of the items as though it were
enqueued in a private queue that doesn't require locking.

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 net/core/skbuff.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 68065d7d383f..bd26b0bde784 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2825,18 +2825,28 @@ struct sk_buff *skb_dequeue_tail(struct sk_buff_head *list)
 EXPORT_SYMBOL(skb_dequeue_tail);
 
 /**
- *	skb_queue_purge - empty a list
- *	@list: list to empty
+ *	skb_queue_purge - empty a queue
+ *	@q: the queue to empty
  *
- *	Delete all buffers on an &sk_buff list. Each buffer is removed from
- *	the list and one reference dropped. This function takes the list
- *	lock and is atomic with respect to other list locking functions.
+ *	Dequeue and free each socket buffer that is in @q.
+ *
+ *	This function is atomic with respect to other queue-locking functions.
  */
-void skb_queue_purge(struct sk_buff_head *list)
+void skb_queue_purge(struct sk_buff_head *q)
 {
-	struct sk_buff *skb;
-	while ((skb = skb_dequeue(list)) != NULL)
+	unsigned long flags;
+	struct sk_buff *skb, *next, *head = (struct sk_buff *)q;
+
+	spin_lock_irqsave(&q->lock, flags);
+	skb = q->next;
+	__skb_queue_head_init(q);
+	spin_unlock_irqrestore(&q->lock, flags);
+
+	while (skb != head) {
+		next = skb->next;
 		kfree_skb(skb);
+		skb = next;
+	}
 }
 EXPORT_SYMBOL(skb_queue_purge);
 
-- 
2.14.1

[toc] | [prev] | [next] | [standalone]

#1729488 — Re: [PATCH v1 3/3] net: skb_queue_purge(): lock/unlock the queue only once

On Sat, 2017-09-09 at 05:50 +0000, Michael Witten wrote:
> Thanks for your input, Eric Dumazet and Stephen Hemminger; based on
> your observations, this version of the patch implements a very
> lightweight purging of the queue.

net-next is closed.

Documentation/networking/netdev-FAQ.txt

Meaning we are chasing bugs at this moment, not adding new ones.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1728619 — [PATCH 2/3] net: inet_recvmsg(): Remove unnecessary bitwise operation

Date: Fri, 8 Sep 2017 00:47:49 +0000
The flag `MSG_DONTWAIT' is handled by passing an argument through
the dedicated parameter `nonblock' of the function `tcp_recvmsg()'.

Presumably because `MSG_DONTWAIT' is handled so explicitly, it is
unset in the collection of flags that are passed to `tcp_recvmsg()';
yet, this unsetting appears to be unnecessary, and so this commit
removes the bitwise operation that performs the unsetting.

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 net/ipv4/af_inet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index e31108e5ef79..2dbed042a412 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -791,7 +791,7 @@ int inet_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
 	sock_rps_record_flow(sk);
 
 	err = sk->sk_prot->recvmsg(sk, msg, size, flags & MSG_DONTWAIT,
-				   flags & ~MSG_DONTWAIT, &addr_len);
+				   flags, &addr_len);
 	if (err >= 0)
 		msg->msg_namelen = addr_len;
 	return err;
-- 
2.14.1

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web