issue: 1897191 Improve extra TCP statistics

- 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]>

src/stats/stats_data_reader.h

@@ -52,12 +52,9 @@ class stats_data_reader : public timer_handler
                 void    handle_timer_expired(void *ctx);
                 void    register_to_timer();
                 void    add_data_reader(void* local_addr, void* shm_addr, int size);
-                void*   find_data_reader(void* local_addr);
                 void*   pop_data_reader(void* local_addr);
 
         private:
-                void*   find_data_reader_unlocked(void* local_addr);
-
                 void*  m_timer_handler;
                 stats_read_map_t m_data_map;
                 lock_spin m_lock_data_map;

src/stats/stats_printer.cpp

@@ -199,6 +199,7 @@ void print_full_stats(socket_stats_t* p_si_stats, mc_grp_info_t* p_mc_grp_info,
 
 #ifdef DEFINED_EXTRA_STATS
 	if (p_si_stats->socket_type == SOCK_STREAM && b_any_activity) {
+		fprintf(filename, "TCP --------------------------------\n");
 		fprintf(filename, "TCP n_rto: %u\n", p_si_stats->tcp_stats.n_rto);
 		fprintf(filename, "TCP n_rtx_fast: %u\n", p_si_stats->tcp_stats.n_rtx_fast);
 		fprintf(filename, "TCP n_rtx_rto: %u\n", p_si_stats->tcp_stats.n_rtx_rto);

src/stats/stats_publisher.cpp

@@ -138,30 +138,13 @@ void stats_data_reader::add_data_reader(void* local_addr, void* shm_addr, int si
 	m_lock_data_map.unlock();
 }
 
-void* stats_data_reader::find_data_reader_unlocked(void* local_addr)
-{
-	stats_read_map_t::iterator iter = m_data_map.find(local_addr);
-	if (iter != m_data_map.end()) {//found
-		return SHM_DATA_ADDRESS;
-	}
-	return NULL;
-}
-
-void* stats_data_reader::find_data_reader(void* local_addr)
-{
-	void* rv;
-	m_lock_data_map.lock();
-	rv = find_data_reader_unlocked(local_addr);
-	m_lock_data_map.unlock();
-	return rv;
-}
-
 void* stats_data_reader::pop_data_reader(void* local_addr)
 {       
-	void* rv;
+	void* rv = NULL;
 	m_lock_data_map.lock();
-	rv = find_data_reader_unlocked(local_addr);
-	if (rv) {
+	stats_read_map_t::iterator iter = m_data_map.find(local_addr);
+	if (iter != m_data_map.end()) {//found
+		rv = SHM_DATA_ADDRESS;
 		m_data_map.erase(local_addr);
 	}
 	m_lock_data_map.unlock();
@@ -398,25 +381,6 @@ void vma_stats_instance_remove_socket_block(socket_stats_t* local_addr)
 	g_lock_skt_inst_arr.unlock();
 }
 
-#ifdef DEFINED_EXTRA_STATS
-void vma_stats_instance_add_tcp(socket_tcp_stats_t* tcp_stats_addr, socket_stats_t* stats_addr)
-{
-	socket_stats_t* p_skt_stats;
-
-	p_skt_stats = (socket_stats_t*)g_p_stats_data_reader->find_data_reader(stats_addr);
-	if (p_skt_stats) {
-		g_p_stats_data_reader->add_data_reader(tcp_stats_addr,
-						       &p_skt_stats->tcp_stats,
-						       sizeof(socket_tcp_stats_t));
-	}
-}
-
-void vma_stats_instance_del_tcp(socket_tcp_stats_t* tcp_stats_addr)
-{
-	(void)g_p_stats_data_reader->pop_data_reader(tcp_stats_addr);
-}
-#endif /* DEFINED_EXTRA_STATS */
-
 void vma_stats_mc_group_add(in_addr_t mc_grp, socket_stats_t* p_socket_stats)
 {
 	int empty_entry = -1; 

src/vma/lwip/stats.h

@@ -153,7 +153,7 @@ typedef struct socket_tcp_stats socket_tcp_stats_t;
 #define EXTRA_STATS_INC(x) do {} while (0)
 #endif /* DEFINED_EXTRA_STATS */
 
-#define PCB_STATS_INC(x) EXTRA_STATS_INC(pcb->stats.x)
+#define PCB_STATS_INC(x) EXTRA_STATS_INC(pcb->p_stats->x)
 
 #ifdef __cplusplus
 }

src/vma/lwip/tcp.c

@@ -109,30 +109,39 @@ const u8_t tcp_persist_backoff[7] = { 3, 6, 12, 24, 48, 96, 120 };
 struct tcp_pcb *tcp_tmp_pcb;
 
 #ifdef DEFINED_EXTRA_STATS
+void register_tcp_stats_instance(struct tcp_pcb *pcb, socket_tcp_stats_t *stats)
+{
+    pcb->p_stats = stats;
+}
+
 static void copy_tcp_metrics(struct tcp_pcb *pcb)
 {
   struct tcp_seg *seg;
+  socket_tcp_stats_t *stats = pcb->p_stats;
   u32_t n;
 
-  pcb->stats.n_mss = pcb->mss;
-  pcb->stats.n_rto_timer = pcb->rto * slow_tmr_interval;
-  pcb->stats.n_snd_wnd = pcb->snd_wnd;
-  pcb->stats.n_cwnd = pcb->cwnd;
-  pcb->stats.n_ssthresh = pcb->ssthresh;
-  pcb->stats.n_snd_nxt = pcb->snd_nxt;
-  pcb->stats.n_lastack = pcb->lastack;
+  if (stats == NULL)
+    return;
+
+  stats->n_mss = pcb->mss;
+  stats->n_rto_timer = pcb->rto * slow_tmr_interval;
+  stats->n_snd_wnd = pcb->snd_wnd;
+  stats->n_cwnd = pcb->cwnd;
+  stats->n_ssthresh = pcb->ssthresh;
+  stats->n_snd_nxt = pcb->snd_nxt;
+  stats->n_lastack = pcb->lastack;
 
   for (seg = pcb->unsent, n = 0; seg != NULL; seg = seg->next, ++n);
-  pcb->stats.n_unsent_q = n;
+  stats->n_unsent_q = n;
   for (seg = pcb->unacked, n = 0; seg != NULL; seg = seg->next, ++n);
-  pcb->stats.n_unacked_q = n;
+  stats->n_unacked_q = n;
   for (seg = pcb->ooseq, n = 0; seg != NULL; seg = seg->next, ++n);
-  pcb->stats.n_ooseq_q = n;
+  stats->n_ooseq_q = n;
 }
 #else /* DEFINED_EXTRA_STATS */
 static void copy_tcp_metrics(struct tcp_pcb *pcb)
 {
-  /* Do nothing is extra statistics is off. */
+  /* Do nothing if extra statistics is off. */
   (void)pcb;
 }
 #endif /* DEFINED_EXTRA_STATS */

src/vma/lwip/tcp.h

@@ -295,7 +295,7 @@ struct tcp_pcb {
 #define TF_WND_SCALE   ((u16_t)0x0100U) /* Window Scale option enabled */
 
 #ifdef DEFINED_EXTRA_STATS
-  socket_tcp_stats_t stats;
+  socket_tcp_stats_t *p_stats;
 #endif /* DEFINED_EXTRA_STATS */
 
   /* the rest of the fields are in host byte order
@@ -483,6 +483,11 @@ err_t lwip_tcp_event(void *arg, struct tcp_pcb *pcb,
 /*Initialization of tcp_pcb structure*/
 void tcp_pcb_init (struct tcp_pcb* pcb, u8_t prio);
 
+#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 */
+
 void             tcp_arg     		(struct tcp_pcb *pcb, void *arg);
 void             tcp_ip_output          (struct tcp_pcb *pcb, ip_output_fn ip_output);
 void             tcp_accept  		(struct tcp_pcb *pcb, tcp_accept_fn accept);

src/vma/sock/sockinfo_tcp.cpp

@@ -255,9 +255,8 @@ sockinfo_tcp::sockinfo_tcp(int fd):
 	si_tcp_logdbg("tcp socket created");
 
 	tcp_pcb_init(&m_pcb, TCP_PRIO_NORMAL);
-
 #ifdef DEFINED_EXTRA_STATS
-	vma_stats_instance_add_tcp(&m_pcb.stats, m_p_socket_stats);
+	register_tcp_stats_instance(&m_pcb, &m_p_socket_stats->tcp_stats);
 #endif /* DEFINED_EXTRA_STATS */
 
 	si_tcp_logdbg("new pcb %p pcb state %d", &m_pcb, get_tcp_state(&m_pcb));
@@ -333,10 +332,6 @@ sockinfo_tcp::~sockinfo_tcp()
 		prepare_to_close();
 	}
 
-#ifdef DEFINED_EXTRA_STATS
-	vma_stats_instance_del_tcp(&m_pcb.stats);
-#endif /* DEFINED_EXTRA_STATS */
-
 	do_wakeup();
 
 	destructor_helper();
@@ -856,7 +851,7 @@ ssize_t sockinfo_tcp::tx(const tx_call_t call_type, const iovec* p_iov, const ss
 					errno = ECONNRESET;
 					goto err;
 				}
-				EXTRA_STATS_INC(m_pcb.stats.n_blocked_sndbuf);
+				EXTRA_STATS_INC(m_pcb.p_stats->n_blocked_sndbuf);
 				//force out TCP data before going on wait()
 				tcp_output(&m_pcb);
 
@@ -989,6 +984,13 @@ ssize_t sockinfo_tcp::tx(const tx_call_t call_type, const iovec* p_iov, const ss
 	return ret;
 }
 
+void sockinfo_tcp::on_retransmit()
+{
+	m_p_socket_stats->counters.n_tx_retransmits++;
+	if (m_pcb.cwnd < m_pcb.ssthresh)
+		EXTRA_STATS_INC(m_pcb.p_stats->n_rtx_ss);
+}
+
 #ifdef DEFINED_TSO
 err_t sockinfo_tcp::ip_output(struct pbuf *p, void* v_p_conn, uint16_t flags)
 {
@@ -1027,7 +1029,7 @@ err_t sockinfo_tcp::ip_output(struct pbuf *p, void* v_p_conn, uint16_t flags)
 	}
 
 	if (is_set(attr.flags, VMA_TX_PACKET_REXMIT)) {
-		p_si_tcp->m_p_socket_stats->counters.n_tx_retransmits++;
+		p_si_tcp->on_retransmit();
 	}
 
 	return ERR_OK;
@@ -1067,7 +1069,7 @@ err_t sockinfo_tcp::ip_output_syn_ack(struct pbuf *p, void* v_p_conn, uint16_t f
 
 	attr = (vma_wr_tx_packet_attr)flags;
 	if (is_set(attr, VMA_TX_PACKET_REXMIT))
-		p_si_tcp->m_p_socket_stats->counters.n_tx_retransmits++;
+		p_si_tcp->on_retransmit();
 
 	((dst_entry_tcp*)p_dst)->slow_send_neigh(p_iovec, count, p_si_tcp->m_so_ratelimit);
 
@@ -1113,9 +1115,7 @@ err_t sockinfo_tcp::ip_output(struct pbuf *p, void* v_p_conn, int is_rexmit, uin
 	}
 
 	if (is_rexmit) {
-		p_si_tcp->m_p_socket_stats->counters.n_tx_retransmits++;
-		if (p_si_tcp->m_pcb.cwnd < p_si_tcp->m_pcb.ssthresh)
-			EXTRA_STATS_INC(p_si_tcp->m_pcb.stats.n_rtx_ss);
+		p_si_tcp->on_retransmit();
 	}
 
 	return ERR_OK;
@@ -1155,7 +1155,7 @@ err_t sockinfo_tcp::ip_output_syn_ack(struct pbuf *p, void* v_p_conn, int is_rex
 	}
 
 	if (is_rexmit)
-		p_si_tcp->m_p_socket_stats->counters.n_tx_retransmits++;
+		p_si_tcp->on_retransmit();
 
 	((dst_entry_tcp*)p_dst)->slow_send_neigh(p_iovec, count, p_si_tcp->m_so_ratelimit);
 

src/vma/sock/sockinfo_tcp.h

@@ -315,6 +315,9 @@ class sockinfo_tcp : public sockinfo, public timer_handler
 	inline void unlock_tcp_con();
 	void tcp_timer();
 
+	// Increment retransmit counters
+	void on_retransmit();
+
 	bool prepare_listen_to_close();
 
 	//Builds rfs key

src/vma/util/vma_stats.h

@@ -381,10 +381,6 @@ void vma_shmem_stats_close();
 
 void vma_stats_instance_create_socket_block(socket_stats_t*);
 void vma_stats_instance_remove_socket_block(socket_stats_t*);
-#ifdef DEFINED_EXTRA_STATS
-void vma_stats_instance_add_tcp(socket_tcp_stats_t*, socket_stats_t*);
-void vma_stats_instance_del_tcp(socket_tcp_stats_t*);
-#endif /* DEFINED_EXTRA_STATS */
 
 void vma_stats_mc_group_add(in_addr_t mc_grp, socket_stats_t* p_socket_stats);
 void vma_stats_mc_group_remove(in_addr_t mc_grp, socket_stats_t* p_socket_stats);