Re: [MPTCP][PATCH v3 mptcp-next 1/4] mptcp: add tracepoint in mptcp_subflow_get_send

[Paolo Abeni <pabeni@redhat.com>]

On Tue, 2021-03-30 at 17:16 +0800, Geliang Tang wrote:
> This patch added a tracepoint in the packet scheduler function
> mptcp_subflow_get_send().
> 
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
>  include/trace/events/mptcp.h | 45 ++++++++++++++++++++++++++++++++++++
>  net/mptcp/protocol.c         |  7 ++++++
>  2 files changed, 52 insertions(+)
>  create mode 100644 include/trace/events/mptcp.h
> 
> diff --git a/include/trace/events/mptcp.h b/include/trace/events/mptcp.h
> new file mode 100644
> index 000000000000..80c9498b4ad9
> --- /dev/null
> +++ b/include/trace/events/mptcp.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM mptcp
> +
> +#if !defined(_TRACE_MPTCP_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_MPTCP_H
> +
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(mptcp_subflow_get_send,
> +
> +	TP_PROTO(bool active, bool free, u32 snd_wnd,
> +		 u32 pace, u8 backup, u64 ratio),
> +
> +	TP_ARGS(active, free, snd_wnd,
> +		pace, backup, ratio),
> +
> +	TP_STRUCT__entry(
> +		__field(bool, active)
> +		__field(bool, free)
> +		__field(u32, snd_wnd)
> +		__field(u32, pace)
> +		__field(u8, backup)
> +		__field(u64, ratio)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->active = active;
> +		__entry->free = free;
> +		__entry->snd_wnd = snd_wnd;
> +		__entry->pace = pace;
> +		__entry->backup = backup;
> +		__entry->ratio = ratio;
> +	),
> +
> +	TP_printk("active=%d free=%d snd_wnd=%u pace=%u backup=%u ratio=%llu",
> +		  __entry->active, __entry->free, __entry->snd_wnd,
> +		  __entry->pace, __entry->backup,
> +		  __entry->ratio)
> +);
> +
> +#endif /* _TRACE_MPTCP_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 9d7e7e13fba8..eb64d373a7ee 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -25,6 +25,9 @@
>  #include "protocol.h"
>  #include "mib.h"
>  
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/mptcp.h>
> +
>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>  struct mptcp6_sock {
>  	struct mptcp_sock msk;
> @@ -1411,6 +1414,10 @@ static struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
>  			send_info[subflow->backup].ssk = ssk;
>  			send_info[subflow->backup].ratio = ratio;
>  		}
> +		trace_mptcp_subflow_get_send(mptcp_subflow_active(subflow),
> +					     sk_stream_memory_free(subflow->tcp_sock),
> +					     tcp_sk(ssk)->snd_wnd, pace,
> +					     subflow->backup, ratio);

If I read correctly, here 'mptcp_subflow_active(subflow)' should be
always true. That means we actually don't trace some data: we don'
trace not active subflows, subflows with 0 pacing_rate etc.

We should instead get the info even for them.

I think it would be better move the tracepoint just after the loop
start, move mptcp_subflow_active() to the header file, and do the
relevant check inside the tracepoint.

Thanks!

Side note: the other patches LGTM, thanks !

Paolo