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
Date: Thu, 7 Sep 2017 03:21:38 +0000
Signed-off-by: Michael Witten <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
On Fri, 08 Sep 2017 05:06:30 -0000
Michael Witten <[email protected]> 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 <[email protected]>
> ---
> 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);
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 <[email protected]>
---
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
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.