* [PATCHv2 net-next] sctp: move trace_sctp_probe_path into sctp_outq_sack
@ 2019-12-27 8:00 Kevin Kou
2019-12-27 10:47 ` Marcelo Ricardo Leitner
0 siblings, 1 reply; 5+ messages in thread
From: Kevin Kou @ 2019-12-27 8:00 UTC (permalink / raw)
To: linux-sctp, netdev; +Cc: nhorman, marcelo.leitner, davem, qdkevin.kou
The original patch bringed in the "SCTP ACK tracking trace event"
feature was committed at Dec.20, 2017, it replaced jprobe usage
with trace events, and bringed in two trace events, one is
TRACE_EVENT(sctp_probe), another one is TRACE_EVENT(sctp_probe_path).
The original patch intended to trigger the trace_sctp_probe_path in
TRACE_EVENT(sctp_probe) as below code,
+TRACE_EVENT(sctp_probe,
+
+ TP_PROTO(const struct sctp_endpoint *ep,
+ const struct sctp_association *asoc,
+ struct sctp_chunk *chunk),
+
+ TP_ARGS(ep, asoc, chunk),
+
+ TP_STRUCT__entry(
+ __field(__u64, asoc)
+ __field(__u32, mark)
+ __field(__u16, bind_port)
+ __field(__u16, peer_port)
+ __field(__u32, pathmtu)
+ __field(__u32, rwnd)
+ __field(__u16, unack_data)
+ ),
+
+ TP_fast_assign(
+ struct sk_buff *skb = chunk->skb;
+
+ __entry->asoc = (unsigned long)asoc;
+ __entry->mark = skb->mark;
+ __entry->bind_port = ep->base.bind_addr.port;
+ __entry->peer_port = asoc->peer.port;
+ __entry->pathmtu = asoc->pathmtu;
+ __entry->rwnd = asoc->peer.rwnd;
+ __entry->unack_data = asoc->unack_data;
+
+ if (trace_sctp_probe_path_enabled()) {
+ struct sctp_transport *sp;
+
+ list_for_each_entry(sp, &asoc->peer.transport_addr_list,
+ transports) {
+ trace_sctp_probe_path(sp, asoc);
+ }
+ }
+ ),
But I found it did not work when I did testing, and trace_sctp_probe_path
had no output, I finally found that there is trace buffer lock
operation(trace_event_buffer_reserve) in include/trace/trace_events.h:
static notrace void \
trace_event_raw_event_##call(void *__data, proto) \
{ \
struct trace_event_file *trace_file = __data; \
struct trace_event_data_offsets_##call __maybe_unused __data_offsets;\
struct trace_event_buffer fbuffer; \
struct trace_event_raw_##call *entry; \
int __data_size; \
\
if (trace_trigger_soft_disabled(trace_file)) \
return; \
\
__data_size = trace_event_get_offsets_##call(&__data_offsets, args); \
\
entry = trace_event_buffer_reserve(&fbuffer, trace_file, \
sizeof(*entry) + __data_size); \
\
if (!entry) \
return; \
\
tstruct \
\
{ assign; } \
\
trace_event_buffer_commit(&fbuffer); \
}
The reason caused no output of trace_sctp_probe_path is that
trace_sctp_probe_path written in TP_fast_assign part of
TRACE_EVENT(sctp_probe), and it will be placed( { assign; } ) after the
trace_event_buffer_reserve() when compiler expands Macro,
entry = trace_event_buffer_reserve(&fbuffer, trace_file, \
sizeof(*entry) + __data_size); \
\
if (!entry) \
return; \
\
tstruct \
\
{ assign; } \
so trace_sctp_probe_path finally can not acquire trace_event_buffer
and return no output, that is to say the nest of tracepoint entry function
is not allowed. The function call flow is:
trace_sctp_probe()
-> trace_event_raw_event_sctp_probe()
-> lock buffer
-> trace_sctp_probe_path()
-> trace_event_raw_event_sctp_probe_path() --nested
-> buffer has been locked and return no output.
This patch is to remove trace_sctp_probe_path from the TP_fast_assign
part of TRACE_EVENT(sctp_probe) to avoid the nest of entry function,
and trigger sctp_probe_path_trace in sctp_outq_sack.
After this patch, you can enable both events individually,
# cd /sys/kernel/debug/tracing
# echo 1 > events/sctp/sctp_probe/enable
# echo 1 > events/sctp/sctp_probe_path/enable
Or, you can enable all the events under sctp.
# echo 1 > events/sctp/enable
v1->v2:
-add trace_sctp_probe_path_enabled check to avoid traversing the
transport list when the tracepoint is disabled as Marcelo's suggestion.
Signed-off-by: Kevin Kou <qdkevin.kou@gmail.com>
---
include/trace/events/sctp.h | 9 ---------
net/sctp/outqueue.c | 7 +++++++
2 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/include/trace/events/sctp.h b/include/trace/events/sctp.h
index 7475c7b..d4aac34 100644
--- a/include/trace/events/sctp.h
+++ b/include/trace/events/sctp.h
@@ -75,15 +75,6 @@
__entry->pathmtu = asoc->pathmtu;
__entry->rwnd = asoc->peer.rwnd;
__entry->unack_data = asoc->unack_data;
-
- if (trace_sctp_probe_path_enabled()) {
- struct sctp_transport *sp;
-
- list_for_each_entry(sp, &asoc->peer.transport_addr_list,
- transports) {
- trace_sctp_probe_path(sp, asoc);
- }
- }
),
TP_printk("asoc=%#llx mark=%#x bind_port=%d peer_port=%d pathmtu=%d "
diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index 0dab62b..83ddcfe 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -36,6 +36,7 @@
#include <net/sctp/sctp.h>
#include <net/sctp/sm.h>
#include <net/sctp/stream_sched.h>
+#include <trace/events/sctp.h>
/* Declare internal functions here. */
static int sctp_acked(struct sctp_sackhdr *sack, __u32 tsn);
@@ -1238,6 +1239,12 @@ int sctp_outq_sack(struct sctp_outq *q, struct sctp_chunk *chunk)
/* Grab the association's destination address list. */
transport_list = &asoc->peer.transport_addr_list;
+ /* SCTP path tracepoint for congestion control debugging. */
+ if (trace_sctp_probe_path_enabled()) {
+ list_for_each_entry(transport, transport_list, transports)
+ trace_sctp_probe_path(transport, asoc);
+ }
+
sack_ctsn = ntohl(sack->cum_tsn_ack);
gap_ack_blocks = ntohs(sack->num_gap_ack_blocks);
asoc->stats.gapcnt += gap_ack_blocks;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCHv2 net-next] sctp: move trace_sctp_probe_path into sctp_outq_sack
2019-12-27 8:00 [PATCHv2 net-next] sctp: move trace_sctp_probe_path into sctp_outq_sack Kevin Kou
@ 2019-12-27 10:47 ` Marcelo Ricardo Leitner
0 siblings, 0 replies; 5+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-12-27 10:47 UTC (permalink / raw)
To: Kevin Kou; +Cc: linux-sctp, netdev, nhorman, davem
On Fri, Dec 27, 2019 at 08:00:34AM +0000, Kevin Kou wrote:
> The original patch bringed in the "SCTP ACK tracking trace event"
> feature was committed at Dec.20, 2017, it replaced jprobe usage
> with trace events, and bringed in two trace events, one is
> TRACE_EVENT(sctp_probe), another one is TRACE_EVENT(sctp_probe_path).
> The original patch intended to trigger the trace_sctp_probe_path in
> TRACE_EVENT(sctp_probe) as below code,
>
> +TRACE_EVENT(sctp_probe,
> +
> + TP_PROTO(const struct sctp_endpoint *ep,
> + const struct sctp_association *asoc,
> + struct sctp_chunk *chunk),
> +
> + TP_ARGS(ep, asoc, chunk),
> +
> + TP_STRUCT__entry(
> + __field(__u64, asoc)
> + __field(__u32, mark)
> + __field(__u16, bind_port)
> + __field(__u16, peer_port)
> + __field(__u32, pathmtu)
> + __field(__u32, rwnd)
> + __field(__u16, unack_data)
> + ),
> +
> + TP_fast_assign(
> + struct sk_buff *skb = chunk->skb;
> +
> + __entry->asoc = (unsigned long)asoc;
> + __entry->mark = skb->mark;
> + __entry->bind_port = ep->base.bind_addr.port;
> + __entry->peer_port = asoc->peer.port;
> + __entry->pathmtu = asoc->pathmtu;
> + __entry->rwnd = asoc->peer.rwnd;
> + __entry->unack_data = asoc->unack_data;
> +
> + if (trace_sctp_probe_path_enabled()) {
> + struct sctp_transport *sp;
> +
> + list_for_each_entry(sp, &asoc->peer.transport_addr_list,
> + transports) {
> + trace_sctp_probe_path(sp, asoc);
> + }
> + }
> + ),
>
> But I found it did not work when I did testing, and trace_sctp_probe_path
> had no output, I finally found that there is trace buffer lock
> operation(trace_event_buffer_reserve) in include/trace/trace_events.h:
>
> static notrace void \
> trace_event_raw_event_##call(void *__data, proto) \
> { \
> struct trace_event_file *trace_file = __data; \
> struct trace_event_data_offsets_##call __maybe_unused __data_offsets;\
> struct trace_event_buffer fbuffer; \
> struct trace_event_raw_##call *entry; \
> int __data_size; \
> \
> if (trace_trigger_soft_disabled(trace_file)) \
> return; \
> \
> __data_size = trace_event_get_offsets_##call(&__data_offsets, args); \
> \
> entry = trace_event_buffer_reserve(&fbuffer, trace_file, \
> sizeof(*entry) + __data_size); \
> \
> if (!entry) \
> return; \
> \
> tstruct \
> \
> { assign; } \
> \
> trace_event_buffer_commit(&fbuffer); \
> }
>
> The reason caused no output of trace_sctp_probe_path is that
> trace_sctp_probe_path written in TP_fast_assign part of
> TRACE_EVENT(sctp_probe), and it will be placed( { assign; } ) after the
> trace_event_buffer_reserve() when compiler expands Macro,
>
> entry = trace_event_buffer_reserve(&fbuffer, trace_file, \
> sizeof(*entry) + __data_size); \
> \
> if (!entry) \
> return; \
> \
> tstruct \
> \
> { assign; } \
>
> so trace_sctp_probe_path finally can not acquire trace_event_buffer
> and return no output, that is to say the nest of tracepoint entry function
> is not allowed. The function call flow is:
>
> trace_sctp_probe()
> -> trace_event_raw_event_sctp_probe()
> -> lock buffer
> -> trace_sctp_probe_path()
> -> trace_event_raw_event_sctp_probe_path() --nested
> -> buffer has been locked and return no output.
>
> This patch is to remove trace_sctp_probe_path from the TP_fast_assign
> part of TRACE_EVENT(sctp_probe) to avoid the nest of entry function,
> and trigger sctp_probe_path_trace in sctp_outq_sack.
>
> After this patch, you can enable both events individually,
> # cd /sys/kernel/debug/tracing
> # echo 1 > events/sctp/sctp_probe/enable
> # echo 1 > events/sctp/sctp_probe_path/enable
>
> Or, you can enable all the events under sctp.
>
> # echo 1 > events/sctp/enable
>
> v1->v2:
> -add trace_sctp_probe_path_enabled check to avoid traversing the
> transport list when the tracepoint is disabled as Marcelo's suggestion.
Oh, nice, thanks for taking the time to add it back.
Dave already applied your v1, so you actually need an incremental
patch now :-)
>
> Signed-off-by: Kevin Kou <qdkevin.kou@gmail.com>
> ---
> include/trace/events/sctp.h | 9 ---------
> net/sctp/outqueue.c | 7 +++++++
> 2 files changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/include/trace/events/sctp.h b/include/trace/events/sctp.h
> index 7475c7b..d4aac34 100644
> --- a/include/trace/events/sctp.h
> +++ b/include/trace/events/sctp.h
> @@ -75,15 +75,6 @@
> __entry->pathmtu = asoc->pathmtu;
> __entry->rwnd = asoc->peer.rwnd;
> __entry->unack_data = asoc->unack_data;
> -
> - if (trace_sctp_probe_path_enabled()) {
> - struct sctp_transport *sp;
> -
> - list_for_each_entry(sp, &asoc->peer.transport_addr_list,
> - transports) {
> - trace_sctp_probe_path(sp, asoc);
> - }
> - }
> ),
>
> TP_printk("asoc=%#llx mark=%#x bind_port=%d peer_port=%d pathmtu=%d "
> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> index 0dab62b..83ddcfe 100644
> --- a/net/sctp/outqueue.c
> +++ b/net/sctp/outqueue.c
> @@ -36,6 +36,7 @@
> #include <net/sctp/sctp.h>
> #include <net/sctp/sm.h>
> #include <net/sctp/stream_sched.h>
> +#include <trace/events/sctp.h>
>
> /* Declare internal functions here. */
> static int sctp_acked(struct sctp_sackhdr *sack, __u32 tsn);
> @@ -1238,6 +1239,12 @@ int sctp_outq_sack(struct sctp_outq *q, struct sctp_chunk *chunk)
> /* Grab the association's destination address list. */
> transport_list = &asoc->peer.transport_addr_list;
>
> + /* SCTP path tracepoint for congestion control debugging. */
> + if (trace_sctp_probe_path_enabled()) {
> + list_for_each_entry(transport, transport_list, transports)
> + trace_sctp_probe_path(transport, asoc);
> + }
> +
> sack_ctsn = ntohl(sack->cum_tsn_ack);
> gap_ack_blocks = ntohs(sack->num_gap_ack_blocks);
> asoc->stats.gapcnt += gap_ack_blocks;
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCHv2 net-next] sctp: move trace_sctp_probe_path into sctp_outq_sack
@ 2019-12-27 10:47 ` Marcelo Ricardo Leitner
0 siblings, 0 replies; 5+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-12-27 10:47 UTC (permalink / raw)
To: Kevin Kou; +Cc: linux-sctp, netdev, nhorman, davem
On Fri, Dec 27, 2019 at 08:00:34AM +0000, Kevin Kou wrote:
> The original patch bringed in the "SCTP ACK tracking trace event"
> feature was committed at Dec.20, 2017, it replaced jprobe usage
> with trace events, and bringed in two trace events, one is
> TRACE_EVENT(sctp_probe), another one is TRACE_EVENT(sctp_probe_path).
> The original patch intended to trigger the trace_sctp_probe_path in
> TRACE_EVENT(sctp_probe) as below code,
>
> +TRACE_EVENT(sctp_probe,
> +
> + TP_PROTO(const struct sctp_endpoint *ep,
> + const struct sctp_association *asoc,
> + struct sctp_chunk *chunk),
> +
> + TP_ARGS(ep, asoc, chunk),
> +
> + TP_STRUCT__entry(
> + __field(__u64, asoc)
> + __field(__u32, mark)
> + __field(__u16, bind_port)
> + __field(__u16, peer_port)
> + __field(__u32, pathmtu)
> + __field(__u32, rwnd)
> + __field(__u16, unack_data)
> + ),
> +
> + TP_fast_assign(
> + struct sk_buff *skb = chunk->skb;
> +
> + __entry->asoc = (unsigned long)asoc;
> + __entry->mark = skb->mark;
> + __entry->bind_port = ep->base.bind_addr.port;
> + __entry->peer_port = asoc->peer.port;
> + __entry->pathmtu = asoc->pathmtu;
> + __entry->rwnd = asoc->peer.rwnd;
> + __entry->unack_data = asoc->unack_data;
> +
> + if (trace_sctp_probe_path_enabled()) {
> + struct sctp_transport *sp;
> +
> + list_for_each_entry(sp, &asoc->peer.transport_addr_list,
> + transports) {
> + trace_sctp_probe_path(sp, asoc);
> + }
> + }
> + ),
>
> But I found it did not work when I did testing, and trace_sctp_probe_path
> had no output, I finally found that there is trace buffer lock
> operation(trace_event_buffer_reserve) in include/trace/trace_events.h:
>
> static notrace void \
> trace_event_raw_event_##call(void *__data, proto) \
> { \
> struct trace_event_file *trace_file = __data; \
> struct trace_event_data_offsets_##call __maybe_unused __data_offsets;\
> struct trace_event_buffer fbuffer; \
> struct trace_event_raw_##call *entry; \
> int __data_size; \
> \
> if (trace_trigger_soft_disabled(trace_file)) \
> return; \
> \
> __data_size = trace_event_get_offsets_##call(&__data_offsets, args); \
> \
> entry = trace_event_buffer_reserve(&fbuffer, trace_file, \
> sizeof(*entry) + __data_size); \
> \
> if (!entry) \
> return; \
> \
> tstruct \
> \
> { assign; } \
> \
> trace_event_buffer_commit(&fbuffer); \
> }
>
> The reason caused no output of trace_sctp_probe_path is that
> trace_sctp_probe_path written in TP_fast_assign part of
> TRACE_EVENT(sctp_probe), and it will be placed( { assign; } ) after the
> trace_event_buffer_reserve() when compiler expands Macro,
>
> entry = trace_event_buffer_reserve(&fbuffer, trace_file, \
> sizeof(*entry) + __data_size); \
> \
> if (!entry) \
> return; \
> \
> tstruct \
> \
> { assign; } \
>
> so trace_sctp_probe_path finally can not acquire trace_event_buffer
> and return no output, that is to say the nest of tracepoint entry function
> is not allowed. The function call flow is:
>
> trace_sctp_probe()
> -> trace_event_raw_event_sctp_probe()
> -> lock buffer
> -> trace_sctp_probe_path()
> -> trace_event_raw_event_sctp_probe_path() --nested
> -> buffer has been locked and return no output.
>
> This patch is to remove trace_sctp_probe_path from the TP_fast_assign
> part of TRACE_EVENT(sctp_probe) to avoid the nest of entry function,
> and trigger sctp_probe_path_trace in sctp_outq_sack.
>
> After this patch, you can enable both events individually,
> # cd /sys/kernel/debug/tracing
> # echo 1 > events/sctp/sctp_probe/enable
> # echo 1 > events/sctp/sctp_probe_path/enable
>
> Or, you can enable all the events under sctp.
>
> # echo 1 > events/sctp/enable
>
> v1->v2:
> -add trace_sctp_probe_path_enabled check to avoid traversing the
> transport list when the tracepoint is disabled as Marcelo's suggestion.
Oh, nice, thanks for taking the time to add it back.
Dave already applied your v1, so you actually need an incremental
patch now :-)
>
> Signed-off-by: Kevin Kou <qdkevin.kou@gmail.com>
> ---
> include/trace/events/sctp.h | 9 ---------
> net/sctp/outqueue.c | 7 +++++++
> 2 files changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/include/trace/events/sctp.h b/include/trace/events/sctp.h
> index 7475c7b..d4aac34 100644
> --- a/include/trace/events/sctp.h
> +++ b/include/trace/events/sctp.h
> @@ -75,15 +75,6 @@
> __entry->pathmtu = asoc->pathmtu;
> __entry->rwnd = asoc->peer.rwnd;
> __entry->unack_data = asoc->unack_data;
> -
> - if (trace_sctp_probe_path_enabled()) {
> - struct sctp_transport *sp;
> -
> - list_for_each_entry(sp, &asoc->peer.transport_addr_list,
> - transports) {
> - trace_sctp_probe_path(sp, asoc);
> - }
> - }
> ),
>
> TP_printk("asoc=%#llx mark=%#x bind_port=%d peer_port=%d pathmtu=%d "
> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> index 0dab62b..83ddcfe 100644
> --- a/net/sctp/outqueue.c
> +++ b/net/sctp/outqueue.c
> @@ -36,6 +36,7 @@
> #include <net/sctp/sctp.h>
> #include <net/sctp/sm.h>
> #include <net/sctp/stream_sched.h>
> +#include <trace/events/sctp.h>
>
> /* Declare internal functions here. */
> static int sctp_acked(struct sctp_sackhdr *sack, __u32 tsn);
> @@ -1238,6 +1239,12 @@ int sctp_outq_sack(struct sctp_outq *q, struct sctp_chunk *chunk)
> /* Grab the association's destination address list. */
> transport_list = &asoc->peer.transport_addr_list;
>
> + /* SCTP path tracepoint for congestion control debugging. */
> + if (trace_sctp_probe_path_enabled()) {
> + list_for_each_entry(transport, transport_list, transports)
> + trace_sctp_probe_path(transport, asoc);
> + }
> +
> sack_ctsn = ntohl(sack->cum_tsn_ack);
> gap_ack_blocks = ntohs(sack->num_gap_ack_blocks);
> asoc->stats.gapcnt += gap_ack_blocks;
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCHv2 net-next] sctp: move trace_sctp_probe_path into sctp_outq_sack
2019-12-27 10:47 ` Marcelo Ricardo Leitner
@ 2019-12-27 11:42 ` Kevin Kou
-1 siblings, 0 replies; 5+ messages in thread
From: Kevin Kou @ 2019-12-27 11:42 UTC (permalink / raw)
To: Marcelo Ricardo Leitner; +Cc: linux-sctp, netdev, nhorman, davem
On 2019/12/27 18:47, Marcelo Ricardo Leitner wrote:
> On Fri, Dec 27, 2019 at 08:00:34AM +0000, Kevin Kou wrote:
>> The original patch bringed in the "SCTP ACK tracking trace event"
>> feature was committed at Dec.20, 2017, it replaced jprobe usage
>> with trace events, and bringed in two trace events, one is
>> TRACE_EVENT(sctp_probe), another one is TRACE_EVENT(sctp_probe_path).
>> The original patch intended to trigger the trace_sctp_probe_path in
>> TRACE_EVENT(sctp_probe) as below code,
>>
>> +TRACE_EVENT(sctp_probe,
>> +
>> + TP_PROTO(const struct sctp_endpoint *ep,
>> + const struct sctp_association *asoc,
>> + struct sctp_chunk *chunk),
>> +
>> + TP_ARGS(ep, asoc, chunk),
>> +
>> + TP_STRUCT__entry(
>> + __field(__u64, asoc)
>> + __field(__u32, mark)
>> + __field(__u16, bind_port)
>> + __field(__u16, peer_port)
>> + __field(__u32, pathmtu)
>> + __field(__u32, rwnd)
>> + __field(__u16, unack_data)
>> + ),
>> +
>> + TP_fast_assign(
>> + struct sk_buff *skb = chunk->skb;
>> +
>> + __entry->asoc = (unsigned long)asoc;
>> + __entry->mark = skb->mark;
>> + __entry->bind_port = ep->base.bind_addr.port;
>> + __entry->peer_port = asoc->peer.port;
>> + __entry->pathmtu = asoc->pathmtu;
>> + __entry->rwnd = asoc->peer.rwnd;
>> + __entry->unack_data = asoc->unack_data;
>> +
>> + if (trace_sctp_probe_path_enabled()) {
>> + struct sctp_transport *sp;
>> +
>> + list_for_each_entry(sp, &asoc->peer.transport_addr_list,
>> + transports) {
>> + trace_sctp_probe_path(sp, asoc);
>> + }
>> + }
>> + ),
>>
>> But I found it did not work when I did testing, and trace_sctp_probe_path
>> had no output, I finally found that there is trace buffer lock
>> operation(trace_event_buffer_reserve) in include/trace/trace_events.h:
>>
>> static notrace void \
>> trace_event_raw_event_##call(void *__data, proto) \
>> { \
>> struct trace_event_file *trace_file = __data; \
>> struct trace_event_data_offsets_##call __maybe_unused __data_offsets;\
>> struct trace_event_buffer fbuffer; \
>> struct trace_event_raw_##call *entry; \
>> int __data_size; \
>> \
>> if (trace_trigger_soft_disabled(trace_file)) \
>> return; \
>> \
>> __data_size = trace_event_get_offsets_##call(&__data_offsets, args); \
>> \
>> entry = trace_event_buffer_reserve(&fbuffer, trace_file, \
>> sizeof(*entry) + __data_size); \
>> \
>> if (!entry) \
>> return; \
>> \
>> tstruct \
>> \
>> { assign; } \
>> \
>> trace_event_buffer_commit(&fbuffer); \
>> }
>>
>> The reason caused no output of trace_sctp_probe_path is that
>> trace_sctp_probe_path written in TP_fast_assign part of
>> TRACE_EVENT(sctp_probe), and it will be placed( { assign; } ) after the
>> trace_event_buffer_reserve() when compiler expands Macro,
>>
>> entry = trace_event_buffer_reserve(&fbuffer, trace_file, \
>> sizeof(*entry) + __data_size); \
>> \
>> if (!entry) \
>> return; \
>> \
>> tstruct \
>> \
>> { assign; } \
>>
>> so trace_sctp_probe_path finally can not acquire trace_event_buffer
>> and return no output, that is to say the nest of tracepoint entry function
>> is not allowed. The function call flow is:
>>
>> trace_sctp_probe()
>> -> trace_event_raw_event_sctp_probe()
>> -> lock buffer
>> -> trace_sctp_probe_path()
>> -> trace_event_raw_event_sctp_probe_path() --nested
>> -> buffer has been locked and return no output.
>>
>> This patch is to remove trace_sctp_probe_path from the TP_fast_assign
>> part of TRACE_EVENT(sctp_probe) to avoid the nest of entry function,
>> and trigger sctp_probe_path_trace in sctp_outq_sack.
>>
>> After this patch, you can enable both events individually,
>> # cd /sys/kernel/debug/tracing
>> # echo 1 > events/sctp/sctp_probe/enable
>> # echo 1 > events/sctp/sctp_probe_path/enable
>>
>> Or, you can enable all the events under sctp.
>>
>> # echo 1 > events/sctp/enable
>>
>> v1->v2:
>> -add trace_sctp_probe_path_enabled check to avoid traversing the
>> transport list when the tracepoint is disabled as Marcelo's suggestion.
>
> Oh, nice, thanks for taking the time to add it back.
>
> Dave already applied your v1, so you actually need an incremental
> patch now :-)
>
Thank you for your guidance, Marcelo. I will create the incremental
patch soon. :-)
>>
>> Signed-off-by: Kevin Kou <qdkevin.kou@gmail.com>
>> ---
>> include/trace/events/sctp.h | 9 ---------
>> net/sctp/outqueue.c | 7 +++++++
>> 2 files changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/trace/events/sctp.h b/include/trace/events/sctp.h
>> index 7475c7b..d4aac34 100644
>> --- a/include/trace/events/sctp.h
>> +++ b/include/trace/events/sctp.h
>> @@ -75,15 +75,6 @@
>> __entry->pathmtu = asoc->pathmtu;
>> __entry->rwnd = asoc->peer.rwnd;
>> __entry->unack_data = asoc->unack_data;
>> -
>> - if (trace_sctp_probe_path_enabled()) {
>> - struct sctp_transport *sp;
>> -
>> - list_for_each_entry(sp, &asoc->peer.transport_addr_list,
>> - transports) {
>> - trace_sctp_probe_path(sp, asoc);
>> - }
>> - }
>> ),
>>
>> TP_printk("asoc=%#llx mark=%#x bind_port=%d peer_port=%d pathmtu=%d "
>> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
>> index 0dab62b..83ddcfe 100644
>> --- a/net/sctp/outqueue.c
>> +++ b/net/sctp/outqueue.c
>> @@ -36,6 +36,7 @@
>> #include <net/sctp/sctp.h>
>> #include <net/sctp/sm.h>
>> #include <net/sctp/stream_sched.h>
>> +#include <trace/events/sctp.h>
>>
>> /* Declare internal functions here. */
>> static int sctp_acked(struct sctp_sackhdr *sack, __u32 tsn);
>> @@ -1238,6 +1239,12 @@ int sctp_outq_sack(struct sctp_outq *q, struct sctp_chunk *chunk)
>> /* Grab the association's destination address list. */
>> transport_list = &asoc->peer.transport_addr_list;
>>
>> + /* SCTP path tracepoint for congestion control debugging. */
>> + if (trace_sctp_probe_path_enabled()) {
>> + list_for_each_entry(transport, transport_list, transports)
>> + trace_sctp_probe_path(transport, asoc);
>> + }
>> +
>> sack_ctsn = ntohl(sack->cum_tsn_ack);
>> gap_ack_blocks = ntohs(sack->num_gap_ack_blocks);
>> asoc->stats.gapcnt += gap_ack_blocks;
>> --
>> 1.8.3.1
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCHv2 net-next] sctp: move trace_sctp_probe_path into sctp_outq_sack
@ 2019-12-27 11:42 ` Kevin Kou
0 siblings, 0 replies; 5+ messages in thread
From: Kevin Kou @ 2019-12-27 11:42 UTC (permalink / raw)
To: Marcelo Ricardo Leitner; +Cc: linux-sctp, netdev, nhorman, davem
On 2019/12/27 18:47, Marcelo Ricardo Leitner wrote:
> On Fri, Dec 27, 2019 at 08:00:34AM +0000, Kevin Kou wrote:
>> The original patch bringed in the "SCTP ACK tracking trace event"
>> feature was committed at Dec.20, 2017, it replaced jprobe usage
>> with trace events, and bringed in two trace events, one is
>> TRACE_EVENT(sctp_probe), another one is TRACE_EVENT(sctp_probe_path).
>> The original patch intended to trigger the trace_sctp_probe_path in
>> TRACE_EVENT(sctp_probe) as below code,
>>
>> +TRACE_EVENT(sctp_probe,
>> +
>> + TP_PROTO(const struct sctp_endpoint *ep,
>> + const struct sctp_association *asoc,
>> + struct sctp_chunk *chunk),
>> +
>> + TP_ARGS(ep, asoc, chunk),
>> +
>> + TP_STRUCT__entry(
>> + __field(__u64, asoc)
>> + __field(__u32, mark)
>> + __field(__u16, bind_port)
>> + __field(__u16, peer_port)
>> + __field(__u32, pathmtu)
>> + __field(__u32, rwnd)
>> + __field(__u16, unack_data)
>> + ),
>> +
>> + TP_fast_assign(
>> + struct sk_buff *skb = chunk->skb;
>> +
>> + __entry->asoc = (unsigned long)asoc;
>> + __entry->mark = skb->mark;
>> + __entry->bind_port = ep->base.bind_addr.port;
>> + __entry->peer_port = asoc->peer.port;
>> + __entry->pathmtu = asoc->pathmtu;
>> + __entry->rwnd = asoc->peer.rwnd;
>> + __entry->unack_data = asoc->unack_data;
>> +
>> + if (trace_sctp_probe_path_enabled()) {
>> + struct sctp_transport *sp;
>> +
>> + list_for_each_entry(sp, &asoc->peer.transport_addr_list,
>> + transports) {
>> + trace_sctp_probe_path(sp, asoc);
>> + }
>> + }
>> + ),
>>
>> But I found it did not work when I did testing, and trace_sctp_probe_path
>> had no output, I finally found that there is trace buffer lock
>> operation(trace_event_buffer_reserve) in include/trace/trace_events.h:
>>
>> static notrace void \
>> trace_event_raw_event_##call(void *__data, proto) \
>> { \
>> struct trace_event_file *trace_file = __data; \
>> struct trace_event_data_offsets_##call __maybe_unused __data_offsets;\
>> struct trace_event_buffer fbuffer; \
>> struct trace_event_raw_##call *entry; \
>> int __data_size; \
>> \
>> if (trace_trigger_soft_disabled(trace_file)) \
>> return; \
>> \
>> __data_size = trace_event_get_offsets_##call(&__data_offsets, args); \
>> \
>> entry = trace_event_buffer_reserve(&fbuffer, trace_file, \
>> sizeof(*entry) + __data_size); \
>> \
>> if (!entry) \
>> return; \
>> \
>> tstruct \
>> \
>> { assign; } \
>> \
>> trace_event_buffer_commit(&fbuffer); \
>> }
>>
>> The reason caused no output of trace_sctp_probe_path is that
>> trace_sctp_probe_path written in TP_fast_assign part of
>> TRACE_EVENT(sctp_probe), and it will be placed( { assign; } ) after the
>> trace_event_buffer_reserve() when compiler expands Macro,
>>
>> entry = trace_event_buffer_reserve(&fbuffer, trace_file, \
>> sizeof(*entry) + __data_size); \
>> \
>> if (!entry) \
>> return; \
>> \
>> tstruct \
>> \
>> { assign; } \
>>
>> so trace_sctp_probe_path finally can not acquire trace_event_buffer
>> and return no output, that is to say the nest of tracepoint entry function
>> is not allowed. The function call flow is:
>>
>> trace_sctp_probe()
>> -> trace_event_raw_event_sctp_probe()
>> -> lock buffer
>> -> trace_sctp_probe_path()
>> -> trace_event_raw_event_sctp_probe_path() --nested
>> -> buffer has been locked and return no output.
>>
>> This patch is to remove trace_sctp_probe_path from the TP_fast_assign
>> part of TRACE_EVENT(sctp_probe) to avoid the nest of entry function,
>> and trigger sctp_probe_path_trace in sctp_outq_sack.
>>
>> After this patch, you can enable both events individually,
>> # cd /sys/kernel/debug/tracing
>> # echo 1 > events/sctp/sctp_probe/enable
>> # echo 1 > events/sctp/sctp_probe_path/enable
>>
>> Or, you can enable all the events under sctp.
>>
>> # echo 1 > events/sctp/enable
>>
>> v1->v2:
>> -add trace_sctp_probe_path_enabled check to avoid traversing the
>> transport list when the tracepoint is disabled as Marcelo's suggestion.
>
> Oh, nice, thanks for taking the time to add it back.
>
> Dave already applied your v1, so you actually need an incremental
> patch now :-)
>
Thank you for your guidance, Marcelo. I will create the incremental
patch soon. :-)
>>
>> Signed-off-by: Kevin Kou <qdkevin.kou@gmail.com>
>> ---
>> include/trace/events/sctp.h | 9 ---------
>> net/sctp/outqueue.c | 7 +++++++
>> 2 files changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/trace/events/sctp.h b/include/trace/events/sctp.h
>> index 7475c7b..d4aac34 100644
>> --- a/include/trace/events/sctp.h
>> +++ b/include/trace/events/sctp.h
>> @@ -75,15 +75,6 @@
>> __entry->pathmtu = asoc->pathmtu;
>> __entry->rwnd = asoc->peer.rwnd;
>> __entry->unack_data = asoc->unack_data;
>> -
>> - if (trace_sctp_probe_path_enabled()) {
>> - struct sctp_transport *sp;
>> -
>> - list_for_each_entry(sp, &asoc->peer.transport_addr_list,
>> - transports) {
>> - trace_sctp_probe_path(sp, asoc);
>> - }
>> - }
>> ),
>>
>> TP_printk("asoc=%#llx mark=%#x bind_port=%d peer_port=%d pathmtu=%d "
>> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
>> index 0dab62b..83ddcfe 100644
>> --- a/net/sctp/outqueue.c
>> +++ b/net/sctp/outqueue.c
>> @@ -36,6 +36,7 @@
>> #include <net/sctp/sctp.h>
>> #include <net/sctp/sm.h>
>> #include <net/sctp/stream_sched.h>
>> +#include <trace/events/sctp.h>
>>
>> /* Declare internal functions here. */
>> static int sctp_acked(struct sctp_sackhdr *sack, __u32 tsn);
>> @@ -1238,6 +1239,12 @@ int sctp_outq_sack(struct sctp_outq *q, struct sctp_chunk *chunk)
>> /* Grab the association's destination address list. */
>> transport_list = &asoc->peer.transport_addr_list;
>>
>> + /* SCTP path tracepoint for congestion control debugging. */
>> + if (trace_sctp_probe_path_enabled()) {
>> + list_for_each_entry(transport, transport_list, transports)
>> + trace_sctp_probe_path(transport, asoc);
>> + }
>> +
>> sack_ctsn = ntohl(sack->cum_tsn_ack);
>> gap_ack_blocks = ntohs(sack->num_gap_ack_blocks);
>> asoc->stats.gapcnt += gap_ack_blocks;
>> --
>> 1.8.3.1
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-12-27 11:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-27 8:00 [PATCHv2 net-next] sctp: move trace_sctp_probe_path into sctp_outq_sack Kevin Kou
2019-12-27 10:47 ` Marcelo Ricardo Leitner
2019-12-27 10:47 ` Marcelo Ricardo Leitner
2019-12-27 11:42 ` Kevin Kou
2019-12-27 11:42 ` Kevin Kou
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.