ipoe: encapsulate statistics counters

Group the IPOE starting, active, and delayed offer statistics in struct ipoe_stat_t and keep the storage private to ipoe.c instead of exposing writable stat_* globals. This keeps ownership inside the IPOE control code while preserving the existing CLI and ACCEL-PPP-MIB counter semantics.

Route counter updates through ipoe_stat_*() helpers. Session setup, activation, teardown, and delayed offer queue paths no longer open-code individual counter increments/decrements; the update policy now lives beside the IPOE-owned storage and uses relaxed atomic operations for the simple state counters.

Make the CLI show-stat path render from a local snapshot and convert the IPOE SNMP starting/active scalars from watched raw pointers to scalar handlers. SNMP now reads through ipoe_stat_starting() and ipoe_stat_active(), removing the old ipoe_get_stat() pointer escape hatch.

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

3 files changed, 132 insertions, 72 deletions

diff --git a/accel-pppd/ctrl/ipoe/ipoe.c b/accel-pppd/ctrl/ipoe/ipoe.c
index e215c565..42d142fe 100644
--- a/accel-pppd/ctrl/ipoe/ipoe.c
+++ b/accel-pppd/ctrl/ipoe/ipoe.c
@@ -180,9 +180,7 @@ static int conf_check_mac_change;
 static int conf_soft_terminate;
 static int conf_calling_sid = SID_MAC;
 
-static unsigned int stat_starting;
-static unsigned int stat_active;
-static unsigned int stat_delayed_offer;
+static struct ipoe_stat_t ipoe_stat;
 
 static mempool_t ses_pool;
 static mempool_t disc_item_pool;
@@ -225,6 +223,39 @@ static struct ipoe_session *ipoe_session_create_up(struct ipoe_serv *serv, struc
 static void __terminate(struct ap_session *ses);
 static void ipoe_ipv6_disable(struct ipoe_serv *serv);
 
+void __export ipoe_stat_get(struct ipoe_stat_t *stat)
+{
+	stat->starting = __atomic_load_n(&ipoe_stat.starting, __ATOMIC_RELAXED);
+	stat->active = __atomic_load_n(&ipoe_stat.active, __ATOMIC_RELAXED);
+	stat->delayed_offer = __atomic_load_n(&ipoe_stat.delayed_offer, __ATOMIC_RELAXED);
+}
+
+unsigned int __export ipoe_stat_starting(void)
+{
+	return __atomic_load_n(&ipoe_stat.starting, __ATOMIC_RELAXED);
+}
+
+unsigned int __export ipoe_stat_active(void)
+{
+	return __atomic_load_n(&ipoe_stat.active, __ATOMIC_RELAXED);
+}
+
+static void ipoe_stat_inc(unsigned int *stat)
+{
+	__atomic_add_fetch(stat, 1, __ATOMIC_RELAXED);
+}
+
+static void ipoe_stat_dec(unsigned int *stat)
+{
+	__atomic_sub_fetch(stat, 1, __ATOMIC_RELAXED);
+}
+
+static void ipoe_stat_move(unsigned int *from, unsigned int *to)
+{
+	ipoe_stat_dec(from);
+	ipoe_stat_inc(to);
+}
+
 static void ipoe_ctx_switch(struct triton_context_t *ctx, void *arg)
 {
 	if (arg) {
@@ -741,7 +772,7 @@ static void ipoe_session_start(struct ipoe_session *ses)
 		}
 	}
 
-	__sync_add_and_fetch(&stat_starting, 1);
+	ipoe_stat_inc(&ipoe_stat.starting);
 
 	assert(!ses->ses.username);
 
@@ -1058,8 +1089,7 @@ static void __ipoe_session_activate(struct ipoe_session *ses)
 		}
 	}
 
-	__sync_sub_and_fetch(&stat_starting, 1);
-	__sync_add_and_fetch(&stat_active, 1);
+	ipoe_stat_move(&ipoe_stat.starting, &ipoe_stat.active);
 	ses->started = 1;
 
 	ap_session_activate(&ses->ses);
@@ -1184,9 +1214,9 @@ static void ipoe_session_started(struct ap_session *s)
 static void ipoe_session_free(struct ipoe_session *ses)
 {
 	if (ses->started)
-		__sync_sub_and_fetch(&stat_active, 1);
+		ipoe_stat_dec(&ipoe_stat.active);
 	else
-		__sync_sub_and_fetch(&stat_starting, 1);
+		ipoe_stat_dec(&ipoe_stat.starting);
 
 	if (ses->timer.tpd)
 		triton_timer_del(&ses->timer);
@@ -1632,7 +1662,7 @@ static void ipoe_serv_disc_timer(struct triton_timer_t *t)
 		list_del(&d->entry);
 		mempool_free(d);
 
-		__sync_sub_and_fetch(&stat_delayed_offer, 1);
+		ipoe_stat_dec(&ipoe_stat.delayed_offer);
 	}
 
 	while (!list_empty(&serv->arp_list)) {
@@ -1651,7 +1681,7 @@ static void ipoe_serv_disc_timer(struct triton_timer_t *t)
 		list_del(&d->entry);
 		mempool_free(d);
 
-		__sync_sub_and_fetch(&stat_delayed_offer, 1);
+		ipoe_stat_dec(&ipoe_stat.delayed_offer);
 	}
 
 	if (list_empty(&serv->disc_list) && list_empty(&serv->arp_list))
@@ -1672,7 +1702,7 @@ static void ipoe_serv_add_disc_arp(struct ipoe_serv *serv, struct _arphdr *arph,
 	if (!d)
 		return;
 
-	__sync_add_and_fetch(&stat_delayed_offer, 1);
+	ipoe_stat_inc(&ipoe_stat.delayed_offer);
 
 	memcpy(&d->arph, arph, sizeof(*arph));
 	clock_gettime(CLOCK_MONOTONIC, &d->ts);
@@ -1692,7 +1722,7 @@ static void ipoe_serv_add_disc(struct ipoe_serv *serv, struct dhcpv4_packet *pac
 	if (!d)
 		return;
 
-	__sync_add_and_fetch(&stat_delayed_offer, 1);
+	ipoe_stat_inc(&ipoe_stat.delayed_offer);
 
 	dhcpv4_packet_ref(pack);
 	d->pack = pack;
@@ -1721,7 +1751,7 @@ static int ipoe_serv_check_disc(struct ipoe_serv *serv, struct dhcpv4_packet *pa
 		dhcpv4_packet_free(d->pack);
 		mempool_free(d);
 
-		__sync_sub_and_fetch(&stat_delayed_offer, 1);
+		ipoe_stat_dec(&ipoe_stat.delayed_offer);
 
 		return 1;
 	}
@@ -1869,7 +1899,7 @@ static void __ipoe_recv_dhcpv4(struct dhcpv4_serv *dhcpv4, struct dhcpv4_packet
 			if (!ses)
 				goto out;
 
-			ses->weight = weight = serv->opt_weight >= 0 ? serv->sess_cnt * serv->opt_weight : (stat_active + 1) * conf_weight;
+			ses->weight = weight = serv->opt_weight >= 0 ? serv->sess_cnt * serv->opt_weight : (ipoe_stat_active() + 1) * conf_weight;
 		}	else {
 			if (ses->terminate) {
 				triton_context_call(ses->ctrl.ctx, (triton_event_func)ipoe_session_terminated, ses);
@@ -2327,7 +2357,7 @@ void ipoe_serv_recv_arp(struct ipoe_serv *serv, struct _arphdr *arph)
 				list_del(&d->entry);
 				mempool_free(d);
 
-				__sync_sub_and_fetch(&stat_delayed_offer, 1);
+				ipoe_stat_dec(&ipoe_stat.delayed_offer);
 
 				break;
 			}
@@ -2624,14 +2654,14 @@ static void ipoe_serv_release(struct ipoe_serv *serv)
 		list_del(&d->entry);
 		dhcpv4_packet_free(d->pack);
 		mempool_free(d);
-		__sync_sub_and_fetch(&stat_delayed_offer, 1);
+		ipoe_stat_dec(&ipoe_stat.delayed_offer);
 	}
 
 	while (!list_empty(&serv->arp_list)) {
 		struct arp_item *d = list_entry(serv->arp_list.next, typeof(*d), entry);
 		list_del(&d->entry);
 		mempool_free(d);
-		__sync_sub_and_fetch(&stat_delayed_offer, 1);
+		ipoe_stat_dec(&ipoe_stat.delayed_offer);
 	}
 
 	while (!list_empty(&serv->req_list)) {
@@ -2704,10 +2734,14 @@ static void l4_redirect_ctx_close(struct triton_context_t *ctx)
 
 static int show_stat_exec(const char *cmd, char * const *fields, int fields_cnt, void *client)
 {
+	struct ipoe_stat_t stat;
+
+	ipoe_stat_get(&stat);
+
 	cli_send(client, "ipoe:\r\n");
-	cli_sendv(client,"  starting: %u\r\n", stat_starting);
-	cli_sendv(client,"  active: %u\r\n", stat_active);
-	cli_sendv(client,"  delayed: %u\r\n", stat_delayed_offer);
+	cli_sendv(client,"  starting: %u\r\n", stat.starting);
+	cli_sendv(client,"  active: %u\r\n", stat.active);
+	cli_sendv(client,"  delayed: %u\r\n", stat.delayed_offer);
 
 	return CLI_CMD_OK;
 }
@@ -2725,12 +2759,6 @@ static void print_session_type(struct ap_session *s, char *buf)
 		*buf = 0;
 }
 
-void __export ipoe_get_stat(unsigned int **starting, unsigned int **active)
-{
-	*starting = &stat_starting;
-	*active = &stat_active;
-}
-
 static void __terminate(struct ap_session *ses)
 {
 	ap_session_terminate(ses, TERM_NAS_REQUEST, 1);
@@ -2768,9 +2796,10 @@ struct ipoe_serv *ipoe_find_serv(const char *ifname)
 static int get_offer_delay()
 {
 	struct delay *r, *prev = NULL;
+	unsigned int active = ipoe_stat_active();
 
 	list_for_each_entry(r, &conf_offer_delay, entry) {
-		if (!prev || stat_active >= r->conn_cnt) {
+		if (!prev || active >= r->conn_cnt) {
 			prev = r;
 			continue;
 		}
diff --git a/accel-pppd/ctrl/ipoe/ipoe.h b/accel-pppd/ctrl/ipoe/ipoe.h
index 38f521e1..37c26a63 100644
--- a/accel-pppd/ctrl/ipoe/ipoe.h
+++ b/accel-pppd/ctrl/ipoe/ipoe.h
@@ -130,6 +130,17 @@ struct ipoe_session_info {
 	uint32_t peer_addr;
 };
 
+struct ipoe_stat_t
+{
+	unsigned int starting;
+	unsigned int active;
+	unsigned int delayed_offer;
+};
+
+void ipoe_stat_get(struct ipoe_stat_t *stat);
+unsigned int ipoe_stat_starting(void);
+unsigned int ipoe_stat_active(void);
+
 int ipoe_ipv6_nd_start(struct ipoe_serv *serv);
 
 #ifdef USE_LUA
diff --git a/accel-pppd/extra/net-snmp/statIPOE.c b/accel-pppd/extra/net-snmp/statIPOE.c
index 24f154fd..33430fe6 100644
--- a/accel-pppd/extra/net-snmp/statIPOE.c
+++ b/accel-pppd/extra/net-snmp/statIPOE.c
@@ -8,18 +8,17 @@
 #include <net-snmp/agent/net-snmp-agent-includes.h>
 
 #include "triton.h"
+#include "accel-pppd/ctrl/ipoe/ipoe.h"
 #include "statIPOE.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.
- */
-
-void ipoe_get_stat(unsigned int **, unsigned int **);
-
-static unsigned int *stat_starting;
-static unsigned int *stat_active;
+static int handle_statIPOEStarting(netsnmp_mib_handler *handler,
+				   netsnmp_handler_registration *reginfo,
+				   netsnmp_agent_request_info *reqinfo,
+				   netsnmp_request_info *requests);
+static int handle_statIPOEActive(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 +27,6 @@ static unsigned int *stat_active;
 void
 init_statIPOE(void)
 {
-  netsnmp_handler_registration *reg;
-  netsnmp_watcher_info         *winfo;
-
     static oid statIPOEStarting_oid[] = { 1,3,6,1,4,1,8072,100,1,6,1 };
     static oid statIPOEActive_oid[] = { 1,3,6,1,4,1,8072,100,1,6,2 };
 
@@ -43,51 +39,75 @@ init_statIPOE(void)
 	if (!triton_module_loaded("ipoe"))
 		return;
 
-	ipoe_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(("statIPOE",
                 "Initializing statIPOEStarting scalar integer.  Default value = %d\n",
                 0));
-    reg = netsnmp_create_handler_registration(
-             "statIPOEStarting", NULL,
+    if (netsnmp_register_scalar(netsnmp_create_handler_registration(
+             "statIPOEStarting", handle_statIPOEStarting,
               statIPOEStarting_oid, OID_LENGTH(statIPOEStarting_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 statIPOEStarting" );
+              HANDLER_CAN_RONLY)) < 0 ) {
+        snmp_log( LOG_ERR, "Failed to register statIPOEStarting" );
     }
 
     DEBUGMSGTL(("statIPOE",
                 "Initializing statIPOEActive scalar integer.  Default value = %d\n",
                 0));
-    reg = netsnmp_create_handler_registration(
-             "statIPOEActive", NULL,
+    if (netsnmp_register_scalar(netsnmp_create_handler_registration(
+             "statIPOEActive", handle_statIPOEActive,
               statIPOEActive_oid, OID_LENGTH(statIPOEActive_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 statIPOEActive" );
+              HANDLER_CAN_RONLY)) < 0 ) {
+        snmp_log( LOG_ERR, "Failed to register statIPOEActive" );
     }
 
 
   DEBUGMSGTL(("statIPOE",
               "Done initalizing statIPOE module\n"));
 }
+
+static int handle_statIPOEStarting(netsnmp_mib_handler *handler,
+				   netsnmp_handler_registration *reginfo,
+				   netsnmp_agent_request_info *reqinfo,
+				   netsnmp_request_info *requests)
+{
+	long stat;
+
+	(void)handler;
+	(void)reginfo;
+
+	switch (reqinfo->mode) {
+	case MODE_GET:
+		stat = ipoe_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_statIPOEStarting\n", reqinfo->mode);
+		return SNMP_ERR_GENERR;
+	}
+
+	return SNMP_ERR_NOERROR;
+}
+
+static int handle_statIPOEActive(netsnmp_mib_handler *handler,
+				 netsnmp_handler_registration *reginfo,
+				 netsnmp_agent_request_info *reqinfo,
+				 netsnmp_request_info *requests)
+{
+	long stat;
+
+	(void)handler;
+	(void)reginfo;
+
+	switch (reqinfo->mode) {
+	case MODE_GET:
+		stat = ipoe_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_statIPOEActive\n", reqinfo->mode);
+		return SNMP_ERR_GENERR;
+	}
+
+	return SNMP_ERR_NOERROR;
+}