Re: [MPTCP][PATCH v2 mptcp-next 2/5] mptcp: add tracepoint in mptcp_subflow_get_send

[Paolo Abeni <pabeni@redhat.com>]

On Wed, 2021-03-24 at 16:51 +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 | 43 ++++++++++++++++++++++++++++++++++++
>  net/mptcp/protocol.c         |  4 ++++
>  2 files changed, 47 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..06908ea6afb4
> --- /dev/null
> +++ b/include/trace/events/mptcp.h
> @@ -0,0 +1,43 @@
> +/* 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(struct mptcp_subflow_context *subflow, struct sock *ssk),
> +
> +	TP_ARGS(subflow, ssk),
> +
> +	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 = mptcp_subflow_active(subflow);
> +		__entry->free = sk_stream_memory_free(subflow->tcp_sock);

I'm sorry for not noticing the thing before, but subflow->tcp_sock can
be potentially a TIME_WAIT socket. We need to do:

	sk = sk_fullsock(subflow->tcp_sock);

and checking for sk being != NULL before accessing any tcp_sock or sock
fields.

Note that the caller don't need such thing explicitly, because it
accesses such fields only for mptcp_subflow_active() sockets - which
are NOT TIME_WAIT ones.

> +		__entry->snd_wnd = tcp_sk(ssk)->snd_wnd;
> +		__entry->pace = ssk->sk_pacing_rate;
> +		__entry->backup = subflow->backup;
> +		__entry->ratio = div_u64((u64)ssk->sk_wmem_queued << 32, __entry->pace);

Here we need to check for !__entry->pace, to avoid possible division by
0.

Thanks!

Paolo