From: Michael Witten <hidden> Date: 2017-09-08 05:54:35
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
From: Michael Witten <hidden> Date: 2017-09-08 06:02:23
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 <redacted>
---
net/ipv4/af_inet.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Michael Witten <hidden> Date: 2017-09-08 06:02:59
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 <redacted>
---
net/core/skbuff.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
From: Eric Dumazet <hidden> Date: 2017-09-08 16:01:25
On Fri, 2017-09-08 at 05:06 +0000, Michael Witten wrote:
quoted hunk
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 <redacted>
---
net/core/skbuff.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
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
From: Stephen Hemminger <stephen@networkplumber.org> Date: 2017-09-08 16:51:34
On Fri, 08 Sep 2017 05:06:30 -0000
Michael Witten [off-list ref] wrote:
quoted hunk
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 <redacted>
---
net/core/skbuff.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
From: Michael Witten <hidden> Date: 2017-09-09 06:28:39
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 <redacted>
---
net/core/skbuff.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
From: Eric Dumazet <hidden> Date: 2017-09-09 16:52:54
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.
From: Michael Witten <hidden> Date: 2017-10-01 22:31:42
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 queue only once
Each patch will be sent as an individual reply to this email;
the total diff is appended below for your convenience.
You may also fetch these patches from GitHub:
git checkout --detach 5969d1bb3082b41eba8fd2c826559abe38ccb6df
git pull https://github.com/mfwitten/linux.git net/tcp-ip/01-cleanup/02
Overall:
include/net/sock.h | 2 +-
net/core/skbuff.c | 26 ++++++++++++++++++--------
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, 24 insertions(+), 14 deletions(-)
Sincerly,
Michael Witten
From: Michael Witten <hidden> Date: 2017-10-01 22:43:19
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 <redacted>
---
net/ipv4/af_inet.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Michael Witten <hidden> Date: 2017-10-01 22:43:34
Date: Sat, 9 Sep 2017 05:50:23 +0000
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 <redacted>
---
net/core/skbuff.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
Other code manipulating lists uses splice operation and
a sk_buff_head temporary on the stack. That would be easier
to understand.
struct sk_buf_head head;
__skb_queue_head_init(&head);
spin_lock_irqsave(&q->lock, flags);
skb_queue_splice_init(q, &head);
spin_unlock_irqrestore(&q->lock, flags);
+ while (skb != head) {
+ next = skb->next;
kfree_skb(skb);
+ skb = next;
+ }
It would be cleaner if you could use
skb_queue_walk_safe rather than open coding the loop.
skb_queue_walk_safe(&head, skb, tmp)
kfree_skb(skb);
Other code manipulating lists uses splice operation and
a sk_buff_head temporary on the stack. That would be easier
to understand.
struct sk_buf_head head;
__skb_queue_head_init(&head);
spin_lock_irqsave(&q->lock, flags);
skb_queue_splice_init(q, &head);
spin_unlock_irqrestore(&q->lock, flags);
quoted
+ while (skb != head) {
+ next = skb->next;
kfree_skb(skb);
+ skb = next;
+ }
It would be cleaner if you could use
skb_queue_walk_safe rather than open coding the loop.
skb_queue_walk_safe(&head, skb, tmp)
kfree_skb(skb);
I appreciate abstraction as much as anybody, but I do not believe
that such abstractions would actually be an improvement here.
* Splice-initing seems more like an idiom than an abstraction;
at first blush, it wouldn't be clear to me what the intention
is.
* Such abstractions are fairly unnecessary.
* The function as written is already so short as to be
easily digested.
* More to the point, this function is not some generic,
higher-level algorithm that just happens to employ the
socket buffer interface; rather, it is a function that
implements part of that very interface, and may thus
twiddle the intimate bits of these data structures
without being accused of abusing a leaky abstraction.
* Such abstractions add overhead, if only conceptually. In this
case, a temporary socket buffer queue allocates *3* unnecessary
struct members, including a whole `spinlock_t' member:
prev
qlen
lock
It's possible that the compiler will be smart enough to leave
those out, but I have my suspicions that it won't, not only
given that the interface contract requires that the temporary
socket buffer queue be properly initialized before use, but
also because splicing into the temporary will manipulate its
`qlen'. Yet, why worry whether optimization happens? The whole
issue can simply be avoided by exploiting the intimate details
that are already philosophically available to us.
Similarly, the function `skb_queue_walk_safe' is nice, but it
loses value both because a temporary queue loses value (as just
described), and because it ignores the fact that legitimate
access to the internals of these data structures allows for
setting up the requested loop in advance; that is to say, the
two parts of the function that we are now debating can be woven
together more tightly than `skb_queue_walk_safe' allows.
For these reasons, I stand by the way that the patch currently
implements this function; it does exactly what is desired, no more
or less.
Sincerely,
Michael Witten
Other code manipulating lists uses splice operation and
a sk_buff_head temporary on the stack. That would be easier
to understand.
struct sk_buf_head head;
__skb_queue_head_init(&head);
spin_lock_irqsave(&q->lock, flags);
skb_queue_splice_init(q, &head);
spin_unlock_irqrestore(&q->lock, flags);
quoted
+ while (skb != head) {
+ next = skb->next;
kfree_skb(skb);
+ skb = next;
+ }
It would be cleaner if you could use
skb_queue_walk_safe rather than open coding the loop.
skb_queue_walk_safe(&head, skb, tmp)
kfree_skb(skb);
I appreciate abstraction as much as anybody, but I do not believe
that such abstractions would actually be an improvement here.
* Splice-initing seems more like an idiom than an abstraction;
at first blush, it wouldn't be clear to me what the intention
is.
* Such abstractions are fairly unnecessary.
* The function as written is already so short as to be
easily digested.
* More to the point, this function is not some generic,
higher-level algorithm that just happens to employ the
socket buffer interface; rather, it is a function that
implements part of that very interface, and may thus
twiddle the intimate bits of these data structures
without being accused of abusing a leaky abstraction.
* Such abstractions add overhead, if only conceptually. In this
case, a temporary socket buffer queue allocates *3* unnecessary
struct members, including a whole `spinlock_t' member:
prev
qlen
lock
It's possible that the compiler will be smart enough to leave
those out, but I have my suspicions that it won't, not only
given that the interface contract requires that the temporary
socket buffer queue be properly initialized before use, but
also because splicing into the temporary will manipulate its
`qlen'. Yet, why worry whether optimization happens? The whole
issue can simply be avoided by exploiting the intimate details
that are already philosophically available to us.
Similarly, the function `skb_queue_walk_safe' is nice, but it
loses value both because a temporary queue loses value (as just
described), and because it ignores the fact that legitimate
access to the internals of these data structures allows for
setting up the requested loop in advance; that is to say, the
two parts of the function that we are now debating can be woven
together more tightly than `skb_queue_walk_safe' allows.
For these reasons, I stand by the way that the patch currently
implements this function; it does exactly what is desired, no more
or less.
Sincerely,
Michael Witten
The point is that there was discussion in the past of replacing
the next/prev as used in skb with more generic code from list.h.
If the abstraction was used, then this code would just work.
The temporary skb_buff_head is on the stack, and any
access to updating those fields like qlen are in CPU cache
and therefore have very little impact on any peformance.
From: Michael Witten <hidden> Date: 2018-02-06 01:08:08
Strictly speaking, these patches streamline the code at both
compile-time and run-time, and seem to face no technical
objection of note.
The strongest objection was a dubious *potential* refactoring of
similar code, a refactoring which is clearly vaporware, and which
I myself would have tried to complete if only this initial foray
had been applied.
If this is considered "new" code (it isn't) and if this email is
received outside of an appropriate merge window, then save this
email for later consideration---this isn't a real-time conversation;
this is email, so it doesn't matter when you receive it.
Sincerely,
Michael Witten
On Sun, 01 Oct 2017 22:19:02 -0000, Michael Witten wrote:
quoted hunk
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 queue only once
Each patch will be sent as an individual reply to this email;
the total diff is appended below for your convenience.
You may also fetch these patches from GitHub:
git checkout --detach 5969d1bb3082b41eba8fd2c826559abe38ccb6df
git pull https://github.com/mfwitten/linux.git net/tcp-ip/01-cleanup/02
Overall:
include/net/sock.h | 2 +-
net/core/skbuff.c | 26 ++++++++++++++++++--------
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, 24 insertions(+), 14 deletions(-)
Sincerly,
Michael Witten
From: David Miller <davem@davemloft.net> Date: 2018-02-06 01:12:25
From: Michael Witten <redacted>
Date: Tue, 06 Feb 2018 00:54:35 -0000
If this is considered "new" code (it isn't) and if this email is
received outside of an appropriate merge window, then save this
email for later consideration---this isn't a real-time conversation;
this is email, so it doesn't matter when you receive it.
Sorry, things don't work that way.
You must submit your changes at the appropriate time.
Please learn how the community works, and how to interact with
developers and maintainers in that community appropriately.
Thank you.
From: Michael Witten <hidden> Date: 2018-02-06 01:33:09
On Mon, 05 Feb 2018 20:12:11 -0500 (EST), David Miller wrote:
quoted
If this is considered "new" code (it isn't) and if this email is
received outside of an appropriate merge window, then save this
email for later consideration---this isn't a real-time conversation;
this is email, so it doesn't matter when you receive it.
Sorry, things don't work that way.
You must submit your changes at the appropriate time.
Please learn how the community works, and how to interact with
developers and maintainers in that community appropriately.
I already tried that.
If you're unwilling to be an effective maintainer, then please hand
off the responsibiilty to someone else.
Sincerely,
Michael Witten
From: Andrew Lunn <andrew@lunn.ch> Date: 2018-02-06 01:42:37
quoted
Please learn how the community works, and how to interact with
developers and maintainers in that community appropriately.
I already tried that.
If you're unwilling to be an effective maintainer, then please hand
off the responsibiilty to someone else.
Could i suggest you read:
https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt
And in particular, the bit about netdev being closed.
I suggest you wait a week for netdev to open, and then submit the
patches again. Actual patches, which cleanly apply to net-next.
Andrew
From: Michael Witten <hidden> Date: 2018-02-06 02:22:12
On Tue, 6 Feb 2018 02:42:17 +0100, Andrew Lunn wrote:
quoted
quoted
Please learn how the community works, and how to interact with
developers and maintainers in that community appropriately.
I already tried that.
If you're unwilling to be an effective maintainer, then please hand
off the responsibiilty to someone else.
Could i suggest you read:
https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt
And in particular, the bit about netdev being closed.
I suggest you wait a week for netdev to open, and then submit the
patches again. Actual patches, which cleanly apply to net-next.
Thank you kindly for the suggestion.
However, I'm fully versed on the scripture.
I'm glad to report that the patches apply cleanly to `net-next'; the
actual patches are, of course, still available in my previous emails.
Also, as already described, they can be easily fetched from GitHub.
Sincerely,
Michael Witten
From: Andrew Lunn <andrew@lunn.ch> Date: 2018-02-06 12:59:24
However, I'm fully versed on the scripture.
I'm glad to report that the patches apply cleanly to `net-next'; the
actual patches are, of course, still available in my previous emails.
Also, as already described, they can be easily fetched from GitHub.
Sincerely,
Michael Witten
Hi Michael
If you were fully versed in the scriptures, you would know none of
these methods for submitting patches are applicable for netdev. Please
follow the process, repost the patches in a weeks time.
Andrew