pppoe: encapsulate statistics counters

Group the PPPoE statistics in struct pppoe_stat_t and keep the storage private to pppoe.c instead of exporting writable counter globals through pppoe.h.  The CLI now reads a snapshot with pppoe_stat_get(), while the packet/control paths update the counters through the PPPoE-owned storage using relaxed atomic operations.

Convert the PPPoE SNMP starting/active scalars from watched raw pointers to scalar handlers.  This removes the old pppoe_get_stat() pointer escape hatch and makes SNMP read the counters through pppoe_stat_starting() and pppoe_stat_active(), so the synchronization policy is applied consistently outside the PPPoE module.

This also fixes the long-standing PPPoE starting counter behavior.  PPPoE used to expose starting in the CLI and ACCEL-PPP-MIB, but never updated it, so it always reported zero.  Track a per-connection ppp_starting state, increment starting when the controller begins channel setup, move the session from starting to active after establish_ppp() succeeds, and decrement starting on setup failure before PPP becomes active.  This matches the state accounting used by the other PPP controllers.

Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>

6 files changed, 145 insertions, 94 deletions

diff --git a/accel-pppd/ctrl/pppoe/cli.c b/accel-pppd/ctrl/pppoe/cli.c
index d8399543..453c0cf3 100644
--- a/accel-pppd/ctrl/pppoe/cli.c
+++ b/accel-pppd/ctrl/pppoe/cli.c
@@ -88,16 +88,20 @@ help:
 
 static int show_stat_exec(const char *cmd, char * const *fields, int fields_cnt, void *client)
 {
+	struct pppoe_stat_t stat;
+
+	pppoe_stat_get(&stat);
+
 	cli_send(client, "pppoe:\r\n");
-	cli_sendv(client, "  starting: %u\r\n", stat_starting);
-	cli_sendv(client, "  active: %u\r\n", stat_active);
-	cli_sendv(client, "  delayed PADO: %u\r\n", stat_delayed_pado);
-	cli_sendv(client, "  recv PADI: %lu\r\n", stat_PADI_recv);
-	cli_sendv(client, "  drop PADI: %lu\r\n", stat_PADI_drop);
-	cli_sendv(client, "  sent PADO: %lu\r\n", stat_PADO_sent);
-	cli_sendv(client, "  recv PADR(dup): %lu(%lu)\r\n", stat_PADR_recv, stat_PADR_dup_recv);
-	cli_sendv(client, "  sent PADS: %lu\r\n", stat_PADS_sent);
-	cli_sendv(client, "  filtered: %lu\r\n", stat_filtered);
+	cli_sendv(client, "  starting: %u\r\n", stat.starting);
+	cli_sendv(client, "  active: %u\r\n", stat.active);
+	cli_sendv(client, "  delayed PADO: %u\r\n", stat.delayed_PADO);
+	cli_sendv(client, "  recv PADI: %lu\r\n", stat.PADI_recv);
+	cli_sendv(client, "  drop PADI: %lu\r\n", stat.PADI_drop);
+	cli_sendv(client, "  sent PADO: %lu\r\n", stat.PADO_sent);
+	cli_sendv(client, "  recv PADR(dup): %lu(%lu)\r\n", stat.PADR_recv, stat.PADR_dup_recv);
+	cli_sendv(client, "  sent PADS: %lu\r\n", stat.PADS_sent);
+	cli_sendv(client, "  filtered: %lu\r\n", stat.filtered);
 
 	return CLI_CMD_OK;
 }
diff --git a/accel-pppd/ctrl/pppoe/disc.c b/accel-pppd/ctrl/pppoe/disc.c
index 8a82e1d2..e673a97a 100644
--- a/accel-pppd/ctrl/pppoe/disc.c
+++ b/accel-pppd/ctrl/pppoe/disc.c
@@ -329,7 +329,7 @@ static int disc_read(struct triton_md_handler_t *h)
 		}
 
 		if (mac_filter_check(ethhdr->h_source)) {
-			__sync_add_and_fetch(&stat_filtered, 1);
+			pppoe_stat_add_filtered();
 			continue;
 		}
 
diff --git a/accel-pppd/ctrl/pppoe/dpado.c b/accel-pppd/ctrl/pppoe/dpado.c
index 71faa130..351a43dc 100644
--- a/accel-pppd/ctrl/pppoe/dpado.c
+++ b/accel-pppd/ctrl/pppoe/dpado.c
@@ -77,6 +77,7 @@ int dpado_parse(const char *str)
 {
 	char *str1 = _strdup(str);
 	char *ptr1, *ptr2, *ptr3, *endptr;
+	unsigned int active = pppoe_stat_active();
 	LIST_HEAD(range_list);
 	struct dpado_range_t *r;
 
@@ -131,7 +132,7 @@ int dpado_parse(const char *str)
 	dpado_range_prev = NULL;
 
 	list_for_each_entry(r, &dpado_range_list, entry) {
-		if (!dpado_range_prev || stat_active >= r->conn_cnt) {
+		if (!dpado_range_prev || active >= r->conn_cnt) {
 			dpado_range_prev = r;
 			if (r->entry.next != &dpado_range_list)
 				dpado_range_next = list_entry(r->entry.next, typeof(*r), entry);
diff --git a/accel-pppd/ctrl/pppoe/pppoe.c b/accel-pppd/ctrl/pppoe/pppoe.c
index 6ec07c9d..475ee530 100644
--- a/accel-pppd/ctrl/pppoe/pppoe.c
+++ b/accel-pppd/ctrl/pppoe/pppoe.c
@@ -43,6 +43,7 @@ struct pppoe_conn_t {
 	struct pppoe_serv_t *serv;
 	uint16_t sid;
 	uint8_t addr[ETH_ALEN];
+	unsigned int ppp_starting:1;
 	unsigned int ppp_started:1;
 
 	struct pppoe_tag *relay_sid;
@@ -109,17 +110,8 @@ static mempool_t conn_pool;
 static mempool_t pado_pool;
 static mempool_t padi_pool;
 
-unsigned int stat_starting;
-unsigned int stat_active;
-unsigned int stat_delayed_pado;
-unsigned long stat_PADI_recv;
-unsigned long stat_PADI_drop;
-unsigned long stat_PADO_sent;
-unsigned long stat_PADR_recv;
-unsigned long stat_PADR_dup_recv;
-unsigned long stat_PADS_sent;
+static struct pppoe_stat_t pppoe_stat;
 unsigned int total_padi_cnt;
-unsigned long stat_filtered;
 
 pthread_rwlock_t serv_lock = PTHREAD_RWLOCK_INITIALIZER;
 LIST_HEAD(serv_list);
@@ -131,6 +123,35 @@ static unsigned long *sid_map;
 static unsigned long *sid_ptr;
 static int sid_idx;
 
+void __export pppoe_stat_get(struct pppoe_stat_t *stat)
+{
+	stat->starting = __atomic_load_n(&pppoe_stat.starting, __ATOMIC_RELAXED);
+	stat->active = __atomic_load_n(&pppoe_stat.active, __ATOMIC_RELAXED);
+	stat->delayed_PADO = __atomic_load_n(&pppoe_stat.delayed_PADO, __ATOMIC_RELAXED);
+	stat->PADI_recv = __atomic_load_n(&pppoe_stat.PADI_recv, __ATOMIC_RELAXED);
+	stat->PADI_drop = __atomic_load_n(&pppoe_stat.PADI_drop, __ATOMIC_RELAXED);
+	stat->PADO_sent = __atomic_load_n(&pppoe_stat.PADO_sent, __ATOMIC_RELAXED);
+	stat->PADR_recv = __atomic_load_n(&pppoe_stat.PADR_recv, __ATOMIC_RELAXED);
+	stat->PADR_dup_recv = __atomic_load_n(&pppoe_stat.PADR_dup_recv, __ATOMIC_RELAXED);
+	stat->PADS_sent = __atomic_load_n(&pppoe_stat.PADS_sent, __ATOMIC_RELAXED);
+	stat->filtered = __atomic_load_n(&pppoe_stat.filtered, __ATOMIC_RELAXED);
+}
+
+unsigned int __export pppoe_stat_starting(void)
+{
+	return __atomic_load_n(&pppoe_stat.starting, __ATOMIC_RELAXED);
+}
+
+unsigned int __export pppoe_stat_active(void)
+{
+	return __atomic_load_n(&pppoe_stat.active, __ATOMIC_RELAXED);
+}
+
+void __export pppoe_stat_add_filtered(void)
+{
+	__atomic_add_fetch(&pppoe_stat.filtered, 1, __ATOMIC_RELAXED);
+}
+
 static uint8_t bc_addr[ETH_ALEN] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
 
 static void pppoe_send_PADT(struct pppoe_conn_t *conn);
@@ -168,9 +189,12 @@ static void disconnect(struct pppoe_conn_t *conn)
 	struct pppoe_serv_t *serv = conn->serv;
 
 	if (conn->ppp_started) {
-		dpado_check_prev(__sync_fetch_and_sub(&stat_active, 1));
+		dpado_check_prev(__atomic_fetch_sub(&pppoe_stat.active, 1, __ATOMIC_RELAXED));
 		conn->ppp_started = 0;
 		ap_session_terminate(&conn->ppp.ses, TERM_USER_REQUEST, 1);
+	} else if (conn->ppp_starting) {
+		__atomic_sub_fetch(&pppoe_stat.starting, 1, __ATOMIC_RELAXED);
+		conn->ppp_starting = 0;
 	}
 
 	pppoe_send_PADT(conn);
@@ -227,7 +251,7 @@ static void ppp_finished(struct ap_session *ses)
 	log_ppp_debug("pppoe: ppp finished\n");
 
 	if (conn->ppp_started) {
-		dpado_check_prev(__sync_fetch_and_sub(&stat_active, 1));
+		dpado_check_prev(__atomic_fetch_sub(&pppoe_stat.active, 1, __ATOMIC_RELAXED));
 		conn->ppp_started = 0;
 		triton_context_call(&conn->ctx, (triton_event_func)disconnect, conn);
 	}
@@ -445,6 +469,9 @@ static void connect_channel(struct pppoe_conn_t *conn)
 	struct sockaddr_pppox sp;
 
 	triton_event_fire(EV_CTRL_STARTING, &conn->ppp.ses);
+	conn->ppp_starting = 1;
+	__atomic_add_fetch(&pppoe_stat.starting, 1, __ATOMIC_RELAXED);
+
 	triton_event_fire(EV_CTRL_STARTED, &conn->ppp.ses);
 
 	sock = net->socket(AF_PPPOX, SOCK_DGRAM, PX_PROTO_OE);
@@ -481,9 +508,11 @@ static void connect_channel(struct pppoe_conn_t *conn)
 	}
 #endif
 
+	conn->ppp_starting = 0;
 	conn->ppp_started = 1;
 
-	dpado_check_next(__sync_add_and_fetch(&stat_active, 1));
+	__atomic_sub_fetch(&pppoe_stat.starting, 1, __ATOMIC_RELAXED);
+	dpado_check_next(__atomic_add_fetch(&pppoe_stat.active, 1, __ATOMIC_RELAXED));
 
 	return;
 
@@ -817,7 +846,7 @@ static void pppoe_send_PADO(struct pppoe_serv_t *serv, const uint8_t *addr, cons
 	if (conf_verbose)
 		print_packet(serv->ifname, "send", pack);
 
-	__sync_add_and_fetch(&stat_PADO_sent, 1);
+	__atomic_add_fetch(&pppoe_stat.PADO_sent, 1, __ATOMIC_RELAXED);
 	pppoe_send(serv, pack);
 }
 
@@ -866,7 +895,7 @@ static void pppoe_send_PADS(struct pppoe_conn_t *conn)
 	if (conf_verbose)
 		print_packet(conn->serv->ifname, "send", pack);
 
-	__sync_add_and_fetch(&stat_PADS_sent, 1);
+	__atomic_add_fetch(&pppoe_stat.PADS_sent, 1, __ATOMIC_RELAXED);
 	pppoe_send(conn->serv, pack);
 }
 
@@ -893,7 +922,7 @@ static void free_delayed_pado(struct delayed_pado_t *pado)
 {
 	triton_timer_del(&pado->timer);
 
-	__sync_sub_and_fetch(&stat_delayed_pado, 1);
+	__atomic_sub_fetch(&pppoe_stat.delayed_PADO, 1, __ATOMIC_RELAXED);
 	list_del(&pado->entry);
 
 	if (pado->host_uniq)
@@ -979,7 +1008,7 @@ static void pppoe_recv_PADI(struct pppoe_serv_t *serv, uint8_t *pack, int size)
 	struct timespec ts;
 	uint16_t ppp_max_payload = 0;
 
-	__sync_add_and_fetch(&stat_PADI_recv, 1);
+	__atomic_add_fetch(&pppoe_stat.PADI_recv, 1, __ATOMIC_RELAXED);
 
 	if (ap_shutdown || pado_delay == -1)
 		return;
@@ -991,7 +1020,7 @@ static void pppoe_recv_PADI(struct pppoe_serv_t *serv, uint8_t *pack, int size)
 		return;
 
 	if (check_padi_limit(serv, ethhdr->h_source)) {
-		__sync_add_and_fetch(&stat_PADI_drop, 1);
+		__atomic_add_fetch(&pppoe_stat.PADI_drop, 1, __ATOMIC_RELAXED);
 		if (conf_verbose) {
 			clock_gettime(CLOCK_MONOTONIC, &ts);
 			if (ts.tv_sec - 60 >= serv->last_padi_limit_warn) {
@@ -1094,7 +1123,7 @@ tags_done:
 		triton_timer_add(&serv->ctx, &pado->timer, 0);
 
 		list_add_tail(&pado->entry, &serv->pado_list);
-		__sync_add_and_fetch(&stat_delayed_pado, 1);
+		__atomic_add_fetch(&pppoe_stat.delayed_PADO, 1, __ATOMIC_RELAXED);
 	} else
 		pppoe_send_PADO(serv, ethhdr->h_source, host_uniq_tag, relay_sid_tag, service_name_tag, ppp_max_payload);
 }
@@ -1114,7 +1143,7 @@ static void pppoe_recv_PADR(struct pppoe_serv_t *serv, uint8_t *pack, int size)
 	int vendor_id;
 	uint16_t ppp_max_payload = 0;
 
-	__sync_add_and_fetch(&stat_PADR_recv, 1);
+	__atomic_add_fetch(&pppoe_stat.PADR_recv, 1, __ATOMIC_RELAXED);
 
 	if (ap_shutdown)
 		return;
@@ -1227,7 +1256,7 @@ padr_tags_done:
 	pthread_mutex_lock(&serv->lock);
 	conn = find_channel(serv, (uint8_t *)ac_cookie_tag->tag_data);
 	if (conn && !conn->ppp.ses.username) {
-		__sync_add_and_fetch(&stat_PADR_dup_recv, 1);
+		__atomic_add_fetch(&pppoe_stat.PADR_dup_recv, 1, __ATOMIC_RELAXED);
 		pppoe_send_PADS(conn);
 	}
 	pthread_mutex_unlock(&serv->lock);
@@ -1643,12 +1672,6 @@ void pppoe_server_stop(const char *ifname)
 	pthread_rwlock_unlock(&serv_lock);
 }
 
-void __export pppoe_get_stat(unsigned int **starting, unsigned int **active)
-{
-	*starting = &stat_starting;
-	*active = &stat_active;
-}
-
 static int init_secret(struct pppoe_serv_t *serv)
 {
 	DES_cblock key;
diff --git a/accel-pppd/ctrl/pppoe/pppoe.h b/accel-pppd/ctrl/pppoe/pppoe.h
index 2510c320..42067590 100644
--- a/accel-pppd/ctrl/pppoe/pppoe.h
+++ b/accel-pppd/ctrl/pppoe/pppoe.h
@@ -109,16 +109,24 @@ extern int conf_accept_any_service;
 extern char *conf_ac_name;
 extern char *conf_pado_delay;
 
-extern unsigned int stat_starting;
-extern unsigned int stat_active;
-extern unsigned int stat_delayed_pado;
-extern unsigned long stat_PADI_recv;
-extern unsigned long stat_PADO_sent;
-extern unsigned long stat_PADR_recv;
-extern unsigned long stat_PADR_dup_recv;
-extern unsigned long stat_PADS_sent;
-extern unsigned long stat_PADI_drop;
-extern unsigned long stat_filtered;
+struct pppoe_stat_t
+{
+	unsigned int starting;
+	unsigned int active;
+	unsigned int delayed_PADO;
+	unsigned long PADI_recv;
+	unsigned long PADI_drop;
+	unsigned long PADO_sent;
+	unsigned long PADR_recv;
+	unsigned long PADR_dup_recv;
+	unsigned long PADS_sent;
+	unsigned long filtered;
+};
+
+void pppoe_stat_get(struct pppoe_stat_t *stat);
+unsigned int pppoe_stat_starting(void);
+unsigned int pppoe_stat_active(void);
+void pppoe_stat_add_filtered(void);
 
 extern pthread_rwlock_t serv_lock;
 extern struct list_head serv_list;
@@ -142,4 +150,3 @@ int tr101_send_access_request(struct pppoe_tag *tr101, struct rad_packet_t *pack
 int tr101_send_accounting_request(struct pppoe_tag *tr101, struct rad_packet_t *pack);
 
 #endif
-
diff --git a/accel-pppd/extra/net-snmp/statPPPOE.c b/accel-pppd/extra/net-snmp/statPPPOE.c
index 6042dc5b..5cc7e872 100644
--- a/accel-pppd/extra/net-snmp/statPPPOE.c
+++ b/accel-pppd/extra/net-snmp/statPPPOE.c
@@ -10,16 +10,17 @@
 #include "triton.h"
 #include "statPPPOE.h"
 
-/*
- * The variables we want to tie the relevant OIDs to.
- * The agent will handle all GET and (if applicable) SET requests
- * to these variables automatically, changing the values as needed.
- */
+unsigned int pppoe_stat_starting(void);
+unsigned int pppoe_stat_active(void);
 
-void pppoe_get_stat(unsigned int **, unsigned int **);
-
-static unsigned int *stat_starting;
-static unsigned int *stat_active;
+static int handle_statPPPOEStarting(netsnmp_mib_handler *handler,
+				    netsnmp_handler_registration *reginfo,
+				    netsnmp_agent_request_info *reqinfo,
+				    netsnmp_request_info *requests);
+static int handle_statPPPOEActive(netsnmp_mib_handler *handler,
+				  netsnmp_handler_registration *reginfo,
+				  netsnmp_agent_request_info *reqinfo,
+				  netsnmp_request_info *requests);
 
 /*
  * Our initialization routine, called automatically by the agent
@@ -28,9 +29,6 @@ static unsigned int *stat_active;
 void
 init_statPPPOE(void)
 {
-  netsnmp_handler_registration *reg;
-  netsnmp_watcher_info         *winfo;
-
     static oid statPPPOEStarting_oid[] = { 1,3,6,1,4,1,8072,100,1,5,1 };
     static oid statPPPOEActive_oid[] = { 1,3,6,1,4,1,8072,100,1,5,2 };
 
@@ -43,51 +41,69 @@ init_statPPPOE(void)
 	if (!triton_module_loaded("pppoe"))
 		return;
 
-	pppoe_get_stat(&stat_starting, &stat_active);
-
-    /*
-     * Register scalar watchers for each of the MIB objects.
-     * The ASN type and RO/RW status are taken from the MIB definition,
-     * but can be adjusted if needed.
-     *
-     * In most circumstances, the scalar watcher will handle all
-     * of the necessary processing.  But the NULL parameter in the
-     * netsnmp_create_handler_registration() call can be used to
-     * supply a user-provided handler if necessary.
-     *
-     * This approach can also be used to handle Counter64, string-
-     * and OID-based watched scalars (although variable-sized writeable
-     * objects will need some more specialised initialisation).
-     */
     DEBUGMSGTL(("statPPPOE",
                 "Initializing statPPPOEStarting scalar integer.  Default value = %d\n",
                 0));
-    reg = netsnmp_create_handler_registration(
-             "statPPPOEStarting", NULL,
+    if (netsnmp_register_scalar(netsnmp_create_handler_registration(
+             "statPPPOEStarting", handle_statPPPOEStarting,
               statPPPOEStarting_oid, OID_LENGTH(statPPPOEStarting_oid),
-              HANDLER_CAN_RONLY);
-    winfo = netsnmp_create_watcher_info(
-                stat_starting, sizeof(*stat_starting),
-                 ASN_INTEGER, WATCHER_FIXED_SIZE);
-    if (netsnmp_register_watched_scalar( reg, winfo ) < 0 ) {
-        snmp_log( LOG_ERR, "Failed to register watched statPPPOEStarting" );
+              HANDLER_CAN_RONLY)) < 0 ) {
+        snmp_log( LOG_ERR, "Failed to register statPPPOEStarting" );
     }
 
     DEBUGMSGTL(("statPPPOE",
                 "Initializing statPPPOEActive scalar integer.  Default value = %d\n",
                 0));
-    reg = netsnmp_create_handler_registration(
-             "statPPPOEActive", NULL,
+    if (netsnmp_register_scalar(netsnmp_create_handler_registration(
+             "statPPPOEActive", handle_statPPPOEActive,
               statPPPOEActive_oid, OID_LENGTH(statPPPOEActive_oid),
-              HANDLER_CAN_RONLY);
-    winfo = netsnmp_create_watcher_info(
-                stat_active, sizeof(*stat_active),
-                 ASN_INTEGER, WATCHER_FIXED_SIZE);
-    if (netsnmp_register_watched_scalar( reg, winfo ) < 0 ) {
-        snmp_log( LOG_ERR, "Failed to register watched statPPPOEActive" );
+              HANDLER_CAN_RONLY)) < 0 ) {
+        snmp_log( LOG_ERR, "Failed to register statPPPOEActive" );
     }
 
 
   DEBUGMSGTL(("statPPPOE",
-              "Done initalizing statPPPOE module\n"));
+	              "Done initalizing statPPPOE module\n"));
+}
+
+static int handle_statPPPOEStarting(netsnmp_mib_handler *handler,
+				    netsnmp_handler_registration *reginfo,
+				    netsnmp_agent_request_info *reqinfo,
+				    netsnmp_request_info *requests)
+{
+	unsigned int stat;
+
+	switch (reqinfo->mode) {
+	case MODE_GET:
+		stat = pppoe_stat_starting();
+		snmp_set_var_typed_value(requests->requestvb, ASN_INTEGER,
+					 (u_char *)&stat, sizeof(stat));
+		break;
+	default:
+		snmp_log(LOG_ERR, "unknown mode (%d) in handle_statPPPOEStarting\n", reqinfo->mode);
+		return SNMP_ERR_GENERR;
+	}
+
+	return SNMP_ERR_NOERROR;
+}
+
+static int handle_statPPPOEActive(netsnmp_mib_handler *handler,
+				  netsnmp_handler_registration *reginfo,
+				  netsnmp_agent_request_info *reqinfo,
+				  netsnmp_request_info *requests)
+{
+	unsigned int stat;
+
+	switch (reqinfo->mode) {
+	case MODE_GET:
+		stat = pppoe_stat_active();
+		snmp_set_var_typed_value(requests->requestvb, ASN_INTEGER,
+					 (u_char *)&stat, sizeof(stat));
+		break;
+	default:
+		snmp_log(LOG_ERR, "unknown mode (%d) in handle_statPPPOEActive\n", reqinfo->mode);
+		return SNMP_ERR_GENERR;
+	}
+
+	return SNMP_ERR_NOERROR;
 }