-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
issue: 1897191 Implement extra TCP statistics #870
base: master
Are you sure you want to change the base?
Conversation
Test PASSed. |
Test FAILed. |
Test PASSed. |
|
||
typedef struct socket_tcp_stats socket_tcp_stats_t; | ||
|
||
#define EXTRA_STATS_INC(x) ++x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please consider (++x)
Possible improvement:
|
VMA doesn't provide enough information to investigate TCP bottlenecks. Collect more counters and metrics for a TCP connection and integrate them into vma_stats. This statistics will help to see internals and describe TCP behavior. Signed-off-by: Dmytro Podgornyi <dmytrop@mellanox.com>
- Revert changes to stats_publisher and stats_data_reader. Instead, modify the stats directly in m_p_socket_stats via a pointer. This saves 1 copy operation per tick. - Move modification of the retransmit counters to a separate private method. The motivation is to reduce copy-paste and create a common execution path for both TSO and no-TSO builds. It won't require to check both builds for test coverage. Signed-off-by: Dmytro Podgornyi <dmytrop@mellanox.com>
Test FAILed. |
Test PASSed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please read my comments
@@ -132,6 +170,8 @@ tcp_tmr(struct tcp_pcb* pcb) | |||
tcp_tmr() is called. */ | |||
tcp_slowtmr(pcb); | |||
} | |||
|
|||
copy_tcp_metrics(pcb); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider
#ifdef DEFINED_EXTRA_STATS
#endif /* DEFINED_EXTRA_STATS */
as is done in other places
@@ -312,6 +312,9 @@ class sockinfo_tcp : public sockinfo, public timer_handler | |||
inline void unlock_tcp_con(); | |||
void tcp_timer(); | |||
|
|||
// Increment retransmit counters | |||
void on_retransmit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function is not big you can consider inlining
if (m_pcb.cwnd < m_pcb.ssthresh) | ||
EXTRA_STATS_INC(m_pcb.p_stats->n_rtx_ss); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is only place where attribute VMA_TX_PACKET_REXMIT = TCP_WRITE_REXMIT
is set as
Line 1751 in 7211589
flags |= (TCP_SEQ_LT(seg->seqno, pcb->snd_nxt) ? TCP_WRITE_REXMIT : 0); |
so consider collection this parameter from new lwip internal statistic structure.
It allows to reduce code below.
#ifdef DEFINED_EXTRA_STATS | ||
/* Set pointer to extra TCP stats instance */ | ||
void register_tcp_stats_instance(struct tcp_pcb *pcb, socket_tcp_stats_t *stats); | ||
#endif /* DEFINED_EXTRA_STATS */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
register_
notation is for global function mostly related vma_lwip() initialization.
I would suggest to name as tcp_stats()
else | ||
AC_MSG_RESULT([no]) | ||
fi | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please consider moving to config\m4\opt.m4
.
Look to implementation of otional log configure option at https://github.com/Mellanox/libvma/blob/master/config/m4/opt.m4
I would like to suggest --enable-opt-stats
with following values:
no | none)
;;
medium)
;;
high)
;;
*)
Probably it can be done w/o none
at this pull request
Can one of the admins verify this patch? |
VMA doesn't provide enough information to investigate TCP bottlenecks.
Collect more counters and metrics for a TCP connection and integrate
them into vma_stats. This statistics will help to see internals and
describe TCP behavior.
Signed-off-by: Dmytro Podgornyi dmytrop@mellanox.com