MIME-Version: 1.0
References: <20230211065153.54116-1-kerneljasonxing@gmail.com> <20230213172939.39449-1-kuniyu@amazon.com>
In-Reply-To: <20230213172939.39449-1-kuniyu@amazon.com>
From:   Jason Xing <kerneljasonxing@gmail.com>
Date:   Tue, 14 Feb 2023 10:15:24 +0800
Message-ID: <CAL+tcoAMByaw8Bdj8kBbzq7Sz8wNJW67Swvw49NQnD0Ks3GWtg@mail.gmail.com>
Subject: Re: [PATCH net-next] net: Kconfig.debug: wrap socket refcnt debug
 into an option
To:     Kuniyuki Iwashima <kuniyu@amazon.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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Feb 14, 2023 at 1:30 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> 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.

Yes.

>
>
> > 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.

I agree with you since it seems no one is developing more features
based on it.  I just did check every caller which may call those debug
interfaces and now confirm we surely can use bpf/kprobe related tools
to track. So I decided to remove the whole feature in the next
submission.

Thanks,
Jason

>
>
> >
> > 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