[PATCH v3 net-next 0/2] clean up SOCK_DEBUG()

[PATCH v3 net-next 0/2] clean up SOCK_DEBUG()

[Yafang Shao]

Per discussion with Daniel[1] and Eric[2], these SOCK_DEBUG() calles in
TCP are not needed now.
We'd better clean up it.

Plus undefine SOCK_DEBUGGING by default.

[1] https://patchwork.ozlabs.org/patch/1035573/
[2] https://patchwork.ozlabs.org/patch/1040533/

Yafang Shao (2):
  tcp: clean up SOCK_DEBUG()
  net: sock: undefine SOCK_DEBUGGING

 include/net/sock.h   | 13 ++++++++-----
 net/core/sock.c      |  3 +++
 net/ipv4/tcp_input.c | 19 +------------------
 net/ipv6/tcp_ipv6.c  |  2 --
 4 files changed, 12 insertions(+), 25 deletions(-)

-- 
1.8.3.1

[PATCH v3 net-next 1/2] tcp: clean up SOCK_DEBUG()

[Yafang Shao]

Per discussion with Daniel[1] and Eric[2], these SOCK_DEBUG() calles in
TCP are not needed now.
We'd better clean up it.

[1] https://patchwork.ozlabs.org/patch/1035573/
[2] https://patchwork.ozlabs.org/patch/1040533/

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 net/ipv4/tcp_input.c | 19 +------------------
 net/ipv6/tcp_ipv6.c  |  2 --
 2 files changed, 1 insertion(+), 20 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 7a027dec..6d2750e 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3595,7 +3595,7 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
 	 * this segment (RFC793 Section 3.9).
 	 */
 	if (after(ack, tp->snd_nxt))
-		goto invalid_ack;
+		return -1;
 
 	if (after(ack, prior_snd_una)) {
 		flag |= FLAG_SND_UNA_ADVANCED;
@@ -3714,10 +3714,6 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
 		tcp_process_tlp_ack(sk, ack, flag);
 	return 1;
 
-invalid_ack:
-	SOCK_DEBUG(sk, "Ack %u after %u:%u\n", ack, tp->snd_una, tp->snd_nxt);
-	return -1;
-
 old_ack:
 	/* If data was SACKed, tag it and see if we should send more data.
 	 * If data was DSACKed, see if we can undo a cwnd reduction.
@@ -3731,7 +3727,6 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
 		tcp_xmit_recovery(sk, rexmit);
 	}
 
-	SOCK_DEBUG(sk, "Ack %u before %u:%u\n", ack, tp->snd_una, tp->snd_nxt);
 	return 0;
 }
 
@@ -4432,13 +4427,9 @@ static void tcp_ofo_queue(struct sock *sk)
 		rb_erase(&skb->rbnode, &tp->out_of_order_queue);
 
 		if (unlikely(!after(TCP_SKB_CB(skb)->end_seq, tp->rcv_nxt))) {
-			SOCK_DEBUG(sk, "ofo packet was already received\n");
 			tcp_drop(sk, skb);
 			continue;
 		}
-		SOCK_DEBUG(sk, "ofo requeuing : rcv_next %X seq %X - %X\n",
-			   tp->rcv_nxt, TCP_SKB_CB(skb)->seq,
-			   TCP_SKB_CB(skb)->end_seq);
 
 		tail = skb_peek_tail(&sk->sk_receive_queue);
 		eaten = tail && tcp_try_coalesce(sk, tail, skb, &fragstolen);
@@ -4502,8 +4493,6 @@ static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)
 	NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPOFOQUEUE);
 	seq = TCP_SKB_CB(skb)->seq;
 	end_seq = TCP_SKB_CB(skb)->end_seq;
-	SOCK_DEBUG(sk, "out of order segment: rcv_next %X seq %X - %X\n",
-		   tp->rcv_nxt, seq, end_seq);
 
 	p = &tp->out_of_order_queue.rb_node;
 	if (RB_EMPTY_ROOT(&tp->out_of_order_queue)) {
@@ -4779,10 +4768,6 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb)
 
 	if (before(TCP_SKB_CB(skb)->seq, tp->rcv_nxt)) {
 		/* Partial packet, seq < rcv_next < end_seq */
-		SOCK_DEBUG(sk, "partial packet: rcv_next %X seq %X - %X\n",
-			   tp->rcv_nxt, TCP_SKB_CB(skb)->seq,
-			   TCP_SKB_CB(skb)->end_seq);
-
 		tcp_dsack_set(sk, TCP_SKB_CB(skb)->seq, tp->rcv_nxt);
 
 		/* If window is closed, drop tail of packet. But after
@@ -5061,8 +5046,6 @@ static int tcp_prune_queue(struct sock *sk)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
 
-	SOCK_DEBUG(sk, "prune_queue: c=%x\n", tp->copied_seq);
-
 	NET_INC_STATS(sock_net(sk), LINUX_MIB_PRUNECALLED);
 
 	if (atomic_read(&sk->sk_rmem_alloc) >= sk->sk_rcvbuf)
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index e51cda7..57ef69a1 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -220,8 +220,6 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr *uaddr,
 		u32 exthdrlen = icsk->icsk_ext_hdr_len;
 		struct sockaddr_in sin;
 
-		SOCK_DEBUG(sk, "connect: ipv4 mapped\n");
-
 		if (__ipv6_only_sock(sk))
 			return -ENETUNREACH;
 
-- 
1.8.3.1

[PATCH v3 net-next 2/2] net: sock: undefine SOCK_DEBUGGING

[Yafang Shao]

SOCK_DEBUG() is a old facility for debugging.
If the user want to use it for debugging, the user must modify the
application first, that doesn't seem like a good way.
Now we have more powerful facilities, i.e. bpf or tracepoint, for this kind
of debugging purpose.
So we'd better disable it by default.
The reason why I don't remove it comepletely is that someone may still
would like to use it for debugging.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Suggested-by: Joe Perches <joe@perches.com>
---
 include/net/sock.h | 13 ++++++++-----
 net/core/sock.c    |  3 +++
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 6679f3c..444e92f 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -81,14 +81,17 @@
  */
 
 /* Define this to get the SOCK_DBG debugging facility. */
-#define SOCK_DEBUGGING
+/* #define SOCK_DEBUGGING */
 #ifdef SOCK_DEBUGGING
-#define SOCK_DEBUG(sk, msg...) do { if ((sk) && sock_flag((sk), SOCK_DBG)) \
-					printk(KERN_DEBUG msg); } while (0)
+#define SOCK_DEBUG(sk, fmt, ...)			\
+do {							\
+	if ((sk) && sock_flag((sk), SOCK_DBG))		\
+		printk(KERN_DEBUG fmt, ##__VA_ARGS__);	\
+} while (0)
 #else
 /* Validate arguments and do nothing */
-static inline __printf(2, 3)
-void SOCK_DEBUG(const struct sock *sk, const char *msg, ...)
+__printf(2, 3)
+static inline void SOCK_DEBUG(const struct sock *sk, const char *fmt, ...)
 {
 }
 #endif
diff --git a/net/core/sock.c b/net/core/sock.c
index 71ded4d..7c15835 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -753,6 +753,9 @@ int sock_setsockopt(struct socket *sock, int level, int optname,
 
 	switch (optname) {
 	case SO_DEBUG:
+		/* This option takes effect only when SOCK_DEBUGGING
+		 * is defined.
+		 */
 		if (val && !capable(CAP_NET_ADMIN))
 			ret = -EACCES;
 		else
-- 
1.8.3.1

Re: [PATCH v3 net-next 2/2] net: sock: undefine SOCK_DEBUGGING

[David Miller]

From: Yafang Shao <laoar.shao@gmail.com>
Date: Sun, 17 Feb 2019 22:26:32 +0800

> SOCK_DEBUG() is a old facility for debugging.
> If the user want to use it for debugging, the user must modify the
> application first, that doesn't seem like a good way.
> Now we have more powerful facilities, i.e. bpf or tracepoint, for this kind
> of debugging purpose.
> So we'd better disable it by default.
> The reason why I don't remove it comepletely is that someone may still
> would like to use it for debugging.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Suggested-by: Joe Perches <joe@perches.com>

Sorry, I'm not applying this.

You are forcing everyone who wants to use this to add a curstom local
source code change into their build.

I'm OK with patch #1, and Eric suggested it, so if you want to submit
that separately that's fine.

Re: [PATCH v3 net-next 2/2] net: sock: undefine SOCK_DEBUGGING

[Joe Perches]

On Sat, 2019-02-23 at 13:21 -0800, David Miller wrote:
> From: Yafang Shao <laoar.shao@gmail.com>
> Date: Sun, 17 Feb 2019 22:26:32 +0800
> 
> > SOCK_DEBUG() is a old facility for debugging.
> > If the user want to use it for debugging, the user must modify the
> > application first, that doesn't seem like a good way.
> > Now we have more powerful facilities, i.e. bpf or tracepoint, for this kind
> > of debugging purpose.
> > So we'd better disable it by default.
> > The reason why I don't remove it comepletely is that someone may still
> > would like to use it for debugging.
> > 
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > Suggested-by: Joe Perches <joe@perches.com>
> 
> Sorry, I'm not applying this.
> 
> You are forcing everyone who wants to use this to add a curstom local
> source code change into their build.

That may not actually be a bad thing.
It could become a CONFIG bit too.

I'm not sure how often it's actually used.

Outside of the likely to be removed tcp uses, it's
currently used only in appletalk/ddp, x25, and the 1 use
in dccp.

All of the existing uses could likely be removed without
much notice by anyone.

Re: [PATCH v3 net-next 2/2] net: sock: undefine SOCK_DEBUGGING

[Cong Wang]

On Sat, Feb 23, 2019 at 1:21 PM David Miller <davem@davemloft.net> wrote:
>
> You are forcing everyone who wants to use this to add a curstom local
> source code change into their build.

What's wrong with this? People carry custom changes in anyway,
do we really need to care about all the downstream changes?

If yes, can I just add some debugging prink's that no one is going
to use except me? My argument would be you can't force me
to add a custom local source code change.

Can we just play this fairly instead of just for any special people?

Thanks!

Re: [PATCH v3 net-next 2/2] net: sock: undefine SOCK_DEBUGGING

[Cong Wang]

On Sat, Feb 23, 2019 at 1:21 PM David Miller <davem@davemloft.net> wrote:
>
> From: Yafang Shao <laoar.shao@gmail.com>
> Date: Sun, 17 Feb 2019 22:26:32 +0800
> > The reason why I don't remove it comepletely is that someone may still
> > would like to use it for debugging.

Please remove it completely.

The rule here is we upstream only care about in-tree users, any out-of-tree
user is beyond what we care. They either need to push their code that uses
this to upstream, or have to carry a revert downstream.

People carry downstream changes all the time, this one isn't an exception.

Let's respect rules.

Thanks.

Re: [PATCH v3 net-next 2/2] net: sock: undefine SOCK_DEBUGGING

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Sat, 23 Feb 2019 17:11:08 -0800

> On Sat, Feb 23, 2019 at 1:21 PM David Miller <davem@davemloft.net> wrote:
>>
>> You are forcing everyone who wants to use this to add a curstom local
>> source code change into their build.
> 
> What's wrong with this? People carry custom changes in anyway,
> do we really need to care about all the downstream changes?

No way am I allowing this, sorry.

Either it's something in the tree that people can use or it's
a crap hack.

When we see stuff like this in a driver we ask the driver author to
remove it.

Re: [PATCH v3 net-next 2/2] net: sock: undefine SOCK_DEBUGGING

[Cong Wang]

On Sat, Feb 23, 2019 at 6:31 PM David Miller <davem@davemloft.net> wrote:
>
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Sat, 23 Feb 2019 17:11:08 -0800
>
> > On Sat, Feb 23, 2019 at 1:21 PM David Miller <davem@davemloft.net> wrote:
> >>
> >> You are forcing everyone who wants to use this to add a curstom local
> >> source code change into their build.
> >
> > What's wrong with this? People carry custom changes in anyway,
> > do we really need to care about all the downstream changes?
>
> No way am I allowing this, sorry.
>
> Either it's something in the tree that people can use or it's
> a crap hack.
>
> When we see stuff like this in a driver we ask the driver author to
> remove it.

Just to clarify, I have been suggesting to completely remove
this unused macro, never suggest to just undefine it in-tree.

There is no reason to keep it in-tree, whether defined or undefined,
just for downstream users.

Thanks.

Re: [PATCH v3 net-next 2/2] net: sock: undefine SOCK_DEBUGGING

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 25 Feb 2019 14:00:09 -0800

> Just to clarify, I have been suggesting to completely remove
> this unused macro, never suggest to just undefine it in-tree.
> 
> There is no reason to keep it in-tree, whether defined or undefined,
> just for downstream users.

And this is where you and I fundamentally disagree.

Re: [PATCH v3 net-next 2/2] net: sock: undefine SOCK_DEBUGGING

[Cong Wang]

On Mon, Feb 25, 2019 at 2:29 PM David Miller <davem@davemloft.net> wrote:
>
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Mon, 25 Feb 2019 14:00:09 -0800
>
> > Just to clarify, I have been suggesting to completely remove
> > this unused macro, never suggest to just undefine it in-tree.
> >
> > There is no reason to keep it in-tree, whether defined or undefined,
> > just for downstream users.
>
> And this is where you and I fundamentally disagree.

So you agree that I can add debugging printk's only for my own use?
I can claim that I only use them downstream and you can't force me
to carry local changes?

If not, what is your criteria for accepting debugging printk's? Whose
can be accepted and whose can't?

Please be specific, and ideally make it a formal doc in netdev-FAQ.txt.

Thanks!

Re: [PATCH v3 net-next 2/2] net: sock: undefine SOCK_DEBUGGING

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 25 Feb 2019 14:58:36 -0800

> So you agree that I can add debugging printk's only for my own use?

Let's put it this way.

If I ignore the fact that some of the data centers with the furtest
reach on the entire planet use something that is in the tree already,
then I am being unreasonable and insensitive.

This means no adding, but keeping things we already have.

Don't try to weasel word what I'm trying to say here Cong.

Have a nice day.

Re: [PATCH v3 net-next 2/2] net: sock: undefine SOCK_DEBUGGING

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 25 Feb 2019 14:58:36 -0800

> Please be specific, and ideally make it a formal doc in netdev-FAQ.txt.

And this isn't happening either Cong, I reserve the right to apply my
judgment as networking maintainer on a case by case basis as I so see
fit.

Re: [PATCH v3 net-next 2/2] net: sock: undefine SOCK_DEBUGGING

[Cong Wang]

(Cc'ing LKML)

On Mon, Feb 25, 2019 at 3:11 PM David Miller <davem@davemloft.net> wrote:
>
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Mon, 25 Feb 2019 14:58:36 -0800
>
> > Please be specific, and ideally make it a formal doc in netdev-FAQ.txt.
>
> And this isn't happening either Cong, I reserve the right to apply my
> judgment as networking maintainer on a case by case basis as I so see
> fit.

Thanks for rephrasing non-transparency in such a wonderful way.

Re: [PATCH v3 net-next 2/2] net: sock: undefine SOCK_DEBUGGING

[Yafang Shao]

On Tue, Feb 26, 2019 at 6:58 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Mon, Feb 25, 2019 at 2:29 PM David Miller <davem@davemloft.net> wrote:
> >
> > From: Cong Wang <xiyou.wangcong@gmail.com>
> > Date: Mon, 25 Feb 2019 14:00:09 -0800
> >
> > > Just to clarify, I have been suggesting to completely remove
> > > this unused macro, never suggest to just undefine it in-tree.
> > >
> > > There is no reason to keep it in-tree, whether defined or undefined,
> > > just for downstream users.
> >
> > And this is where you and I fundamentally disagree.
>
> So you agree that I can add debugging printk's only for my own use?
> I can claim that I only use them downstream and you can't force me
> to carry local changes?
>
> If not, what is your criteria for accepting debugging printk's? Whose
> can be accepted and whose can't?
>
> Please be specific, and ideally make it a formal doc in netdev-FAQ.txt.
>

Per my personal view, I agree with you that we should remove it completely.

Clean up such kind of legacy code can make the kernel more clean.

Thanks
Yafang