From:   Kuniyuki Iwashima <kuniyu@amazon.com>
To:     <kerneljasonxing@gmail.com>
CC:     <bpf@vger.kernel.org>, <davem@davemloft.net>, <dsahern@kernel.org>,
        <edumazet@google.com>, <kernelxing@tencent.com>, <kuba@kernel.org>,
        <linux-kernel@vger.kernel.org>, <netdev@vger.kernel.org>,
        <pabeni@redhat.com>, <kuniyu@amazon.com>
Subject: Re: [PATCH net-next] net: Kconfig.debug: wrap socket refcnt debug into an option
Date:   Mon, 13 Feb 2023 09:29:39 -0800
Message-ID: <20230213172939.39449-1-kuniyu@amazon.com>
In-Reply-To: <20230211065153.54116-1-kerneljasonxing@gmail.com>
References: <20230211065153.54116-1-kerneljasonxing@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
Precedence: bulk

From:   Jason Xing <kerneljasonxing@gmail.com>
Date:   Sat, 11 Feb 2023 14:51:53 +0800
> From: Jason Xing <kernelxing@tencent.com>
> 
> Since commit 463c84b97f24 ("[NET]: Introduce inet_connection_sock")
> commented out the definition of SOCK_REFCNT_DEBUG and later another
> patch deleted it,

e48c414ee61f ("[INET]: Generalise the TCP sock ID lookup routines")
is the commit which commented out SOCK_REFCNT_DEBUG, and 463c84b97f24
removed it.


> we need to enable it through defining it manually
> somewhere. Wrapping it into an option in Kconfig.debug could make
> it much clearer and easier for some developers to do things based
> on this change.

Considering SOCK_REFCNT_DEBUG is removed in 2005, how about removing
the whole feature?  I think we can track the same info easily with
bpftrace + kprobe.


> 
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>  include/net/sock.h            | 8 ++++----
>  net/Kconfig.debug             | 8 ++++++++
>  net/ipv4/inet_timewait_sock.c | 2 +-
>  3 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/include/net/sock.h b/include/net/sock.h
> index dcd72e6285b2..1b001efeb9b5 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1349,7 +1349,7 @@ struct proto {
>  	char			name[32];
>  
>  	struct list_head	node;
> -#ifdef SOCK_REFCNT_DEBUG
> +#ifdef CONFIG_SOCK_REFCNT_DEBUG
>  	atomic_t		socks;
>  #endif
>  	int			(*diag_destroy)(struct sock *sk, int err);
> @@ -1359,7 +1359,7 @@ int proto_register(struct proto *prot, int alloc_slab);
>  void proto_unregister(struct proto *prot);
>  int sock_load_diag_module(int family, int protocol);
>  
> -#ifdef SOCK_REFCNT_DEBUG
> +#ifdef CONFIG_SOCK_REFCNT_DEBUG
>  static inline void sk_refcnt_debug_inc(struct sock *sk)
>  {
>  	atomic_inc(&sk->sk_prot->socks);
> @@ -1378,11 +1378,11 @@ static inline void sk_refcnt_debug_release(const struct sock *sk)
>  		printk(KERN_DEBUG "Destruction of the %s socket %p delayed, refcnt=%d\n",
>  		       sk->sk_prot->name, sk, refcount_read(&sk->sk_refcnt));
>  }
> -#else /* SOCK_REFCNT_DEBUG */
> +#else /* CONFIG_SOCK_REFCNT_DEBUG */
>  #define sk_refcnt_debug_inc(sk) do { } while (0)
>  #define sk_refcnt_debug_dec(sk) do { } while (0)
>  #define sk_refcnt_debug_release(sk) do { } while (0)
> -#endif /* SOCK_REFCNT_DEBUG */
> +#endif /* CONFIG_SOCK_REFCNT_DEBUG */
>  
>  INDIRECT_CALLABLE_DECLARE(bool tcp_stream_memory_free(const struct sock *sk, int wake));
>  
> diff --git a/net/Kconfig.debug b/net/Kconfig.debug
> index 5e3fffe707dd..667396d70e10 100644
> --- a/net/Kconfig.debug
> +++ b/net/Kconfig.debug
> @@ -18,6 +18,14 @@ config NET_NS_REFCNT_TRACKER
>  	  Enable debugging feature to track netns references.
>  	  This adds memory and cpu costs.
>  
> +config SOCK_REFCNT_DEBUG
> +	bool "Enable socket refcount debug"
> +	depends on DEBUG_KERNEL && NET
> +	default n
> +	help
> +	  Enable debugging feature to track socket references.
> +	  This adds memory and cpu costs.
> +
>  config DEBUG_NET
>  	bool "Add generic networking debug"
>  	depends on DEBUG_KERNEL && NET
> diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
> index beed32fff484..e313516b64ce 100644
> --- a/net/ipv4/inet_timewait_sock.c
> +++ b/net/ipv4/inet_timewait_sock.c
> @@ -77,7 +77,7 @@ void inet_twsk_free(struct inet_timewait_sock *tw)
>  {
>  	struct module *owner = tw->tw_prot->owner;
>  	twsk_destructor((struct sock *)tw);
> -#ifdef SOCK_REFCNT_DEBUG
> +#ifdef CONFIG_SOCK_REFCNT_DEBUG
>  	pr_debug("%s timewait_sock %p released\n", tw->tw_prot->name, tw);
>  #endif
>  	kmem_cache_free(tw->tw_prot->twsk_prot->twsk_slab, tw);
> -- 
> 2.37.3