diff mbox series

[mptcp-next,v2,3/4] mptcp: add a ftrace callback for tcp_set_state

Message ID 4323a04943500dce8c072c49ce50b21591357876.1701230012.git.geliang.tang@suse.com (mailing list archive)
State Superseded, archived
Headers show
Series add MPTCP_MIB_CURRESTAB | expand

Checks

Context Check Description
matttbe/checkpatch warning total: 0 errors, 2 warnings, 0 checks, 68 lines checked
matttbe/build warning Build error with: make C=1 net/mptcp/trace.o

Commit Message

Geliang Tang Nov. 29, 2023, 4:08 a.m. UTC 
This patch adds a new function mptcp_check_state(), in it if switch from
or to ESTABLISH state, increment or decrement the newly added counter
MPTCP_MIB_CURRESTAB.

Instead of invoking mptcp_check_state() in tcp_set_state() directly, here
add a new file trace.c, in it use ftrace to hook a callback function to
tcp_set_state(), named mptcp_state_callback(). mptcp_check_state() is
invoked in the callback function.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/460
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/Makefile |  1 +
 net/mptcp/trace.c  | 64 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)
 create mode 100644 net/mptcp/trace.c

Comments

Paolo Abeni Nov. 29, 2023, 9:08 a.m. UTC | #1 
On Wed, 2023-11-29 at 12:08 +0800, Geliang Tang wrote:
> This patch adds a new function mptcp_check_state(), in it if switch from
> or to ESTABLISH state, increment or decrement the newly added counter
> MPTCP_MIB_CURRESTAB.
> 
> Instead of invoking mptcp_check_state() in tcp_set_state() directly, here
> add a new file trace.c, in it use ftrace to hook a callback function to
> tcp_set_state(), named mptcp_state_callback(). mptcp_check_state() is
> invoked in the callback function.
> 
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/460
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/Makefile |  1 +
>  net/mptcp/trace.c  | 64 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
>  create mode 100644 net/mptcp/trace.c
> 
> diff --git a/net/mptcp/Makefile b/net/mptcp/Makefile
> index 67cd565bb321..bf88bdb222b3 100644
> --- a/net/mptcp/Makefile
> +++ b/net/mptcp/Makefile
> @@ -14,3 +14,4 @@ mptcp_token_test-objs := token_test.o
>  obj-$(CONFIG_MPTCP_KUNIT_TEST) += mptcp_crypto_test.o mptcp_token_test.o
>  
>  obj-$(CONFIG_BPF_SYSCALL) += bpf.o
> +obj-$(CONFIG_FUNCTION_TRACER) += trace.o
> diff --git a/net/mptcp/trace.c b/net/mptcp/trace.c
> new file mode 100644
> index 000000000000..7dc33ff88813
> --- /dev/null
> +++ b/net/mptcp/trace.c
> @@ -0,0 +1,64 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Multipath TCP
> + *
> + * Copyright (c) 2023, SUSE.
> + */
> +
> +#define pr_fmt(fmt) "MPTCP: " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/ftrace.h>
> +#include <net/sock.h>
> +#include <net/mptcp.h>
> +#include "protocol.h"
> +#include "mib.h"
> +
> +static void mptcp_check_state(struct sock *sk, int oldstate, int state)
> +{
> +	switch (state) {
> +	case TCP_ESTABLISHED:
> +		if (oldstate != TCP_ESTABLISHED)
> +			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_CURRESTAB);
> +		break;
> +
> +	default:
> +		if (oldstate == TCP_ESTABLISHED)
> +			MPTCP_DEC_STATS(sock_net(sk), MPTCP_MIB_CURRESTAB);
> +	}
> +}
> +
> +static void mptcp_state_callback(unsigned long ip,
> +				 unsigned long parent_ip,
> +				 struct ftrace_ops *op,
> +				 struct ftrace_regs *fregs)
> +{
> +	int oldstate, state;
> +	struct sock *sk;
> +
> +	sk = (struct sock *)ftrace_regs_get_argument(fregs, 0);
> +	if (!sk)
> +		return;
> +
> +	oldstate = sk->sk_state;
> +	state = (int)ftrace_regs_get_argument(fregs, 1);
> +
> +	if (sk_is_mptcp(sk))
> +		mptcp_check_state(sk, oldstate, state);
> +}
> +
> +static struct ftrace_ops mptcp_state_ops = {
> +	.func		= mptcp_state_callback,
> +	.flags		= FTRACE_OPS_FL_SAVE_REGS,
> +};
> +
> +static __init int mptcp_ftrace_init(void)
> +{
> +	char *func_name = "tcp_set_state";
> +	int ret;
> +
> +	ret = ftrace_set_filter(&mptcp_state_ops, func_name,
> +				strlen(func_name), 0);
> +	return ret ?: register_ftrace_function(&mptcp_state_ops);
> +}
> +late_initcall(mptcp_ftrace_init);

I think this is not the correct approach.

I think we must avoid using ftrace to plug an hook from a builtin
feature into another builtin kernel feature.

Additionally this is tracing tcp socket state change, not mptcp
sockets.

A cleaner solution would be adding in protocol.c a mptcp_set_state()
helper that does set the msk state to the provided value and does the
MPTCP_MIB_CURRESTAB accounting.

Then replacing all the 'inet_sk_state_store()' calls under net/mptcp
with the new helper call. I already checked all of them are under the
msk socket lock, so such substitution is safe.

There is only one exception:

	inet_sk_state_store(newsk, TCP_LISTEN);

in mptcp_pm_nl_create_listen_socket(). You can either:

* avoid replacing such call with the new helper, as the old status is
known to be TCP_CLOSE, hence will not affect the count (just drop a
comment there)

* (possibly better) do the replace even there, adding the relevant msk
socket lock around such call.

Cheers,

Paolo
kernel test robot Nov. 29, 2023, 12:01 p.m. UTC | #2 
Hi Geliang,

kernel test robot noticed the following build warnings:

[auto build test WARNING on mptcp/export]
[also build test WARNING on mptcp/export-net linus/master v6.7-rc3 next-20231129]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Geliang-Tang/mptcp-add-a-current-established-counter/20231129-131030
base:   https://github.com/multipath-tcp/mptcp_net-next.git export
patch link:    https://lore.kernel.org/r/4323a04943500dce8c072c49ce50b21591357876.1701230012.git.geliang.tang%40suse.com
patch subject: [PATCH mptcp-next v2 3/4] mptcp: add a ftrace callback for tcp_set_state
config: x86_64-randconfig-005-20231129 (https://download.01.org/0day-ci/archive/20231129/202311291902.gD0Y21iG-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231129/202311291902.gD0Y21iG-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311291902.gD0Y21iG-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> net/mptcp/trace.c:57:8: warning: unused variable 'func_name' [-Wunused-variable]
           char *func_name = "tcp_set_state";
                 ^
   1 warning generated.


vim +/func_name +57 net/mptcp/trace.c

    54	
    55	static __init int mptcp_ftrace_init(void)
    56	{
  > 57		char *func_name = "tcp_set_state";
kernel test robot Nov. 29, 2023, 9:36 p.m. UTC | #3 
Hi Geliang,

kernel test robot noticed the following build errors:

[auto build test ERROR on mptcp/export]
[also build test ERROR on mptcp/export-net linus/master v6.7-rc3 next-20231129]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Geliang-Tang/mptcp-add-a-current-established-counter/20231129-131030
base:   https://github.com/multipath-tcp/mptcp_net-next.git export
patch link:    https://lore.kernel.org/r/4323a04943500dce8c072c49ce50b21591357876.1701230012.git.geliang.tang%40suse.com
patch subject: [PATCH mptcp-next v2 3/4] mptcp: add a ftrace callback for tcp_set_state
config: sh-allyesconfig (https://download.01.org/0day-ci/archive/20231130/202311300110.9kJQ1JjE-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231130/202311300110.9kJQ1JjE-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311300110.9kJQ1JjE-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from net/mptcp/trace.c:11:
   net/mptcp/trace.c: In function 'mptcp_state_callback':
>> include/linux/ftrace.h:158:9: error: implicit declaration of function 'regs_get_kernel_argument'; did you mean 'regs_get_kernel_stack_nth'? [-Werror=implicit-function-declaration]
     158 |         regs_get_kernel_argument(ftrace_get_regs(fregs), n)
         |         ^~~~~~~~~~~~~~~~~~~~~~~~
   net/mptcp/trace.c:39:29: note: in expansion of macro 'ftrace_regs_get_argument'
      39 |         sk = (struct sock *)ftrace_regs_get_argument(fregs, 0);
         |                             ^~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +158 include/linux/ftrace.h

94d095ffa0e16b Mark Rutland 2022-11-03  153  
94d095ffa0e16b Mark Rutland 2022-11-03  154  #ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
94d095ffa0e16b Mark Rutland 2022-11-03  155  #define ftrace_regs_get_instruction_pointer(fregs) \
94d095ffa0e16b Mark Rutland 2022-11-03  156  	instruction_pointer(ftrace_get_regs(fregs))
94d095ffa0e16b Mark Rutland 2022-11-03  157  #define ftrace_regs_get_argument(fregs, n) \
94d095ffa0e16b Mark Rutland 2022-11-03 @158  	regs_get_kernel_argument(ftrace_get_regs(fregs), n)
94d095ffa0e16b Mark Rutland 2022-11-03  159  #define ftrace_regs_get_stack_pointer(fregs) \
94d095ffa0e16b Mark Rutland 2022-11-03  160  	kernel_stack_pointer(ftrace_get_regs(fregs))
94d095ffa0e16b Mark Rutland 2022-11-03  161  #define ftrace_regs_return_value(fregs) \
94d095ffa0e16b Mark Rutland 2022-11-03  162  	regs_return_value(ftrace_get_regs(fregs))
94d095ffa0e16b Mark Rutland 2022-11-03  163  #define ftrace_regs_set_return_value(fregs, ret) \
94d095ffa0e16b Mark Rutland 2022-11-03  164  	regs_set_return_value(ftrace_get_regs(fregs), ret)
94d095ffa0e16b Mark Rutland 2022-11-03  165  #define ftrace_override_function_with_return(fregs) \
94d095ffa0e16b Mark Rutland 2022-11-03  166  	override_function_with_return(ftrace_get_regs(fregs))
94d095ffa0e16b Mark Rutland 2022-11-03  167  #define ftrace_regs_query_register_offset(name) \
94d095ffa0e16b Mark Rutland 2022-11-03  168  	regs_query_register_offset(name)
94d095ffa0e16b Mark Rutland 2022-11-03  169  #endif
94d095ffa0e16b Mark Rutland 2022-11-03  170
kernel test robot Nov. 29, 2023, 11:15 p.m. UTC | #4 
Hi Geliang,

kernel test robot noticed the following build errors:

[auto build test ERROR on mptcp/export]
[also build test ERROR on mptcp/export-net linus/master v6.7-rc3 next-20231129]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Geliang-Tang/mptcp-add-a-current-established-counter/20231129-131030
base:   https://github.com/multipath-tcp/mptcp_net-next.git export
patch link:    https://lore.kernel.org/r/4323a04943500dce8c072c49ce50b21591357876.1701230012.git.geliang.tang%40suse.com
patch subject: [PATCH mptcp-next v2 3/4] mptcp: add a ftrace callback for tcp_set_state
config: arm-randconfig-r111-20231129 (https://download.01.org/0day-ci/archive/20231130/202311300545.66qcYRv2-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce: (https://download.01.org/0day-ci/archive/20231130/202311300545.66qcYRv2-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311300545.66qcYRv2-lkp@intel.com/

All errors (new ones prefixed by >>):

>> net/mptcp/trace.c:39:22: error: call to undeclared function 'regs_get_kernel_argument'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      39 |         sk = (struct sock *)ftrace_regs_get_argument(fregs, 0);
         |                             ^
   include/linux/ftrace.h:158:2: note: expanded from macro 'ftrace_regs_get_argument'
     158 |         regs_get_kernel_argument(ftrace_get_regs(fregs), n)
         |         ^
   net/mptcp/trace.c:39:22: note: did you mean 'regs_get_kernel_stack_nth'?
   include/linux/ftrace.h:158:2: note: expanded from macro 'ftrace_regs_get_argument'
     158 |         regs_get_kernel_argument(ftrace_get_regs(fregs), n)
         |         ^
   arch/arm/include/asm/ptrace.h:131:22: note: 'regs_get_kernel_stack_nth' declared here
     131 | extern unsigned long regs_get_kernel_stack_nth(struct pt_regs *regs,
         |                      ^
   1 error generated.


vim +/regs_get_kernel_argument +39 net/mptcp/trace.c

    30	
    31	static void mptcp_state_callback(unsigned long ip,
    32					 unsigned long parent_ip,
    33					 struct ftrace_ops *op,
    34					 struct ftrace_regs *fregs)
    35	{
    36		int oldstate, state;
    37		struct sock *sk;
    38	
  > 39		sk = (struct sock *)ftrace_regs_get_argument(fregs, 0);
    40		if (!sk)
    41			return;
    42	
    43		oldstate = sk->sk_state;
    44		state = (int)ftrace_regs_get_argument(fregs, 1);
    45	
    46		if (sk_is_mptcp(sk))
    47			mptcp_check_state(sk, oldstate, state);
    48	}
    49
diff mbox series

Patch

diff --git a/net/mptcp/Makefile b/net/mptcp/Makefile
index 67cd565bb321..bf88bdb222b3 100644
--- a/net/mptcp/Makefile
+++ b/net/mptcp/Makefile
@@ -14,3 +14,4 @@  mptcp_token_test-objs := token_test.o
 obj-$(CONFIG_MPTCP_KUNIT_TEST) += mptcp_crypto_test.o mptcp_token_test.o
 
 obj-$(CONFIG_BPF_SYSCALL) += bpf.o
+obj-$(CONFIG_FUNCTION_TRACER) += trace.o
diff --git a/net/mptcp/trace.c b/net/mptcp/trace.c
new file mode 100644
index 000000000000..7dc33ff88813
--- /dev/null
+++ b/net/mptcp/trace.c
@@ -0,0 +1,64 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Multipath TCP
+ *
+ * Copyright (c) 2023, SUSE.
+ */
+
+#define pr_fmt(fmt) "MPTCP: " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/ftrace.h>
+#include <net/sock.h>
+#include <net/mptcp.h>
+#include "protocol.h"
+#include "mib.h"
+
+static void mptcp_check_state(struct sock *sk, int oldstate, int state)
+{
+	switch (state) {
+	case TCP_ESTABLISHED:
+		if (oldstate != TCP_ESTABLISHED)
+			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_CURRESTAB);
+		break;
+
+	default:
+		if (oldstate == TCP_ESTABLISHED)
+			MPTCP_DEC_STATS(sock_net(sk), MPTCP_MIB_CURRESTAB);
+	}
+}
+
+static void mptcp_state_callback(unsigned long ip,
+				 unsigned long parent_ip,
+				 struct ftrace_ops *op,
+				 struct ftrace_regs *fregs)
+{
+	int oldstate, state;
+	struct sock *sk;
+
+	sk = (struct sock *)ftrace_regs_get_argument(fregs, 0);
+	if (!sk)
+		return;
+
+	oldstate = sk->sk_state;
+	state = (int)ftrace_regs_get_argument(fregs, 1);
+
+	if (sk_is_mptcp(sk))
+		mptcp_check_state(sk, oldstate, state);
+}
+
+static struct ftrace_ops mptcp_state_ops = {
+	.func		= mptcp_state_callback,
+	.flags		= FTRACE_OPS_FL_SAVE_REGS,
+};
+
+static __init int mptcp_ftrace_init(void)
+{
+	char *func_name = "tcp_set_state";
+	int ret;
+
+	ret = ftrace_set_filter(&mptcp_state_ops, func_name,
+				strlen(func_name), 0);
+	return ret ?: register_ftrace_function(&mptcp_state_ops);
+}
+late_initcall(mptcp_ftrace_init);