Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pasis
Copy link
Member

@pasis pasis commented Dec 2, 2019

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 [email protected]

@swx-jenkins2
Copy link

Test PASSed.
See http://hpc-master.lab.mtl.com:8080/job/accl-libvma-pr/3311/ for details (Mellanox internal link).

@swx-jenkins2
Copy link

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/accl-libvma-pr/3312/ for details (Mellanox internal link).

@swx-jenkins2
Copy link

Test PASSed.
See http://hpc-master.lab.mtl.com:8080/job/accl-libvma-pr/3313/ for details (Mellanox internal link).


typedef struct socket_tcp_stats socket_tcp_stats_t;

#define EXTRA_STATS_INC(x) ++x
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please consider (++x)

@pasis
Copy link
Member Author

pasis commented Dec 19, 2019

Possible improvement:
Expand configure option --enable-extra-stats to --with-stats-level=<level>. The level must reflect the following values:

  • off -- don't collect and don't display any statistics, should save CPU cycles when stats are not required;
  • low -- build basic stats support (as in master)
  • high -- additionally to basics stats, build extra stats support

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 <[email protected]>
- 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 <[email protected]>
@swx-jenkins2
Copy link

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/accl-libvma-pr/3357/ for details (Mellanox internal link).

@swx-jenkins2
Copy link

Test PASSed.
See http://hpc-master.lab.mtl.com:8080/job/accl-libvma-pr/3369/ for details (Mellanox internal link).

Copy link
Collaborator

@igor-ivanov igor-ivanov left a 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);
Copy link
Collaborator

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();
Copy link
Collaborator

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);
}

Copy link
Collaborator

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

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 */
Copy link
Collaborator

@igor-ivanov igor-ivanov Apr 24, 2020

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

Copy link
Collaborator

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

@igor-ivanov igor-ivanov added 9.2.x and removed 9.1.x labels Jun 16, 2020
@igor-ivanov igor-ivanov removed the 9.2.x label Nov 12, 2020
@swx-jenkins5
Copy link

Can one of the admins verify this patch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants