Re: multicast route counter
Claudio Jeker <[email protected]>
| Newsgroups | gmane.os.openbsd.tech |
|---|---|
| Message-ID | <[email protected]> |
On Sat, Jun 27, 2026 at 09:50:44PM +0200, Alexander Bluhm wrote:
> On Sat, Jun 27, 2026 at 10:10:09AM +1000, David Gwynne wrote:
> > On Fri, Jun 26, 2026 at 10:42:44PM +0200, Alexander Bluhm wrote:
> > > Hi,
> > >
> > > I want to use per CPU counter for the multicast routes.
> >
> > why?
>
> I thought per cpu counter for packets is the way to go. This case
> is new as we get the counters per route. But still we can create
> per cpu memory per route. How expensive is it? I would have
> expected that it multiplies the memory by the number of CPU.
I think per cpu counter only make sense for global counters that are
updated in very critical paths where contention is high.
Even then they come with a high overhead and are not a golden ticket.
In this case it makes less sense. If you have a counter per route
then use a mutex, atomic instructions or if there are close to no
consumers a pc lock (per cpu counters are a special kind of pc
lock which allow concurrent updates from many CPUs concurrently).
As for the overhead. The system allocates ncpus * cpumem struct and then
another ncpus * cachelines (in this case for 2 long counters). So doing
that per route is a probably 5 times memory usage increase.
I think mutex or atomic instructions are fine here since the each
multicast route will be mostly bound to a single CPU so there is little
overhead since the locks / atomic ops are not operating on busy
cachelines.
On top of this multicast forwaring is a very edge case and it does not
need to be overly optimised until there is a concrete need.
> > the memory overhead of per cpu counters is non-trivial. are concurrent
> > accesses to these structs common enough to justify the memory
> > overhead compared to just using a per struct mutex or prod/cons
> > lock here?
>
> A mutex per packet just for counting is usually a bad idea. Also
> atomic increment has impact on performace. In general I try to
> avoid the complexity prod/cons locks. That is overengineered for
> a counter.
The prod/cons locks are exactly built for counters. There is nothing bad
about mutexes, they are simple and easy to understand. This is also valid
for atomic operations on unshared cache lines.
The performance impact on either of those is only problematic if the cache
line is accessed by many cpus concurrently. I really don't see that here.
> I don't know the performance impact. Multi CPU multicast routing
> is not finished. I wanted to start with the low hanging fruits
> like counters.
>
> I see to alternatives forward:
> 1. use per cpu counter, if they are not too expensive
> 2. use atomic increments, if they are not too slow
>
> In practice both variants have little impact. Multicast routers
> are rarely used. I prefer 1.
I certainly prefer 2 over 1, it is much simpler to argue about and does
not require extended APIs, error cases and high memory usage.
Do not over optimise counters before having the big ticket locks in place.
It is well possible that other locks will suddenly cover the counters and
drop the need for anything special.
I think there is option number 3:
3. don't do anything and accept the update glitches for now.
Once the scope of the issue can be assessed on can come back and use the
right tool for this bit of the code.
> > > Note that
> > > I have to call counters_alloc() with M_NOWAIT, so I extended the
> > > API.
> >
> > this introduces failure points that you don't handle.
>
> Thanks for spotting, updated diff.
>
> bluhm
>
> Index: kern/subr_percpu.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/subr_percpu.c,v
> diff -u -p -r1.11 subr_percpu.c
> --- kern/subr_percpu.c 16 Sep 2023 09:33:27 -0000 1.11
> +++ kern/subr_percpu.c 27 Jun 2026 19:46:04 -0000
> @@ -59,17 +59,28 @@ cpumem_put(struct pool *pp, struct cpume
> }
>
> struct cpumem *
> -cpumem_malloc(size_t sz, int type)
> +cpumem_malloc_wait(size_t sz, int type, int wait)
> {
> struct cpumem *cm;
> unsigned int cpu;
>
> sz = roundup(sz, CACHELINESIZE);
>
> - cm = pool_get(&cpumem_pl, PR_WAITOK);
> + cm = pool_get(&cpumem_pl, wait == M_WAITOK ? PR_WAITOK : PR_NOWAIT);
> + if (cm == NULL)
> + return (NULL);
>
> - for (cpu = 0; cpu < ncpusfound; cpu++)
> - cm[cpu].mem = malloc(sz, type, M_WAITOK | M_ZERO);
> + for (cpu = 0; cpu < ncpusfound; cpu++) {
> + cm[cpu].mem = malloc(sz, type, wait | M_ZERO);
> + if (cm[cpu].mem == NULL) {
> + while (cpu > 0) {
> + cpu--;
> + free(cm[cpu].mem, type, sz);
> + }
> + pool_put(&cpumem_pl, cm);
> + return (NULL);
> + }
> + }
>
> return (cm);
> }
> @@ -124,7 +135,7 @@ cpumem_next(struct cpumem_iter *i, struc
> }
>
> struct cpumem *
> -counters_alloc(unsigned int n)
> +counters_alloc_wait(unsigned int n, int wait)
> {
> struct cpumem *cm;
> struct cpumem_iter cmi;
> @@ -134,7 +145,9 @@ counters_alloc(unsigned int n)
> KASSERT(n > 0);
>
> n++; /* add space for a generation number */
> - cm = cpumem_malloc(n * sizeof(uint64_t), M_COUNTERS);
> + cm = cpumem_malloc_wait(n * sizeof(uint64_t), M_COUNTERS, wait);
> + if (cm == NULL)
> + return (NULL);
>
> CPUMEM_FOREACH(counters, &cmi, cm) {
> for (i = 0; i < n; i++)
> @@ -257,9 +270,9 @@ cpumem_put(struct pool *pp, struct cpume
> }
>
> struct cpumem *
> -cpumem_malloc(size_t sz, int type)
> +cpumem_malloc_wait(size_t sz, int type, int wait)
> {
> - return (malloc(sz, type, M_WAITOK | M_ZERO));
> + return (malloc(sz, type, wait | M_ZERO));
> }
>
> struct cpumem *
> @@ -287,11 +300,11 @@ cpumem_next(struct cpumem_iter *i, struc
> }
>
> struct cpumem *
> -counters_alloc(unsigned int n)
> +counters_alloc_wait(unsigned int n, int wait)
> {
> KASSERT(n > 0);
>
> - return (cpumem_malloc(n * sizeof(uint64_t), M_COUNTERS));
> + return (cpumem_malloc_wait(n * sizeof(uint64_t), M_COUNTERS, wait));
> }
>
> struct cpumem *
> @@ -339,3 +352,15 @@ counters_zero(struct cpumem *cm, unsigne
> }
>
> #endif /* MULTIPROCESSOR */
> +
> +struct cpumem *
> +cpumem_malloc(size_t sz, int type)
> +{
> + return (cpumem_malloc_wait(sz, type, M_WAITOK));
> +}
> +
> +struct cpumem *
> +counters_alloc(unsigned int n)
> +{
> + return (counters_alloc_wait(n, M_WAITOK));
> +}
> Index: netinet/ip_mroute.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_mroute.c,v
> diff -u -p -r1.153 ip_mroute.c
> --- netinet/ip_mroute.c 24 Jun 2026 12:33:49 -0000 1.153
> +++ netinet/ip_mroute.c 27 Jun 2026 19:46:04 -0000
> @@ -318,6 +318,8 @@ get_sg_cnt(unsigned int rtableid, struct
>
> req->pktcnt = req->bytecnt = req->wrong_if = 0;
> do {
> + uint64_t count[mfc_ncounters], scratch[mfc_ncounters];
> +
> /* Don't consider non multicast routes. */
> if (ISSET(rt->rt_flags, RTF_HOST | RTF_MULTICAST) !=
> (RTF_HOST | RTF_MULTICAST))
> @@ -327,9 +329,10 @@ get_sg_cnt(unsigned int rtableid, struct
> if (mfc == NULL)
> continue;
>
> - req->pktcnt += mfc->mfc_pkt_cnt;
> - req->bytecnt += mfc->mfc_byte_cnt;
> - req->wrong_if += mfc->mfc_wrong_if;
> + counters_read(mfc->mfc_counter, count, mfc_ncounters, scratch);
> + req->pktcnt += count[mfc_packets];
> + req->bytecnt += count[mfc_bytes];
> + req->wrong_if += count[mfc_wrong_if];
> } while ((rt = rtable_iterate(rt)) != NULL);
>
> return (0);
> @@ -474,6 +477,8 @@ mrt_rtwalk_mfcsysctl(struct rtentry *rt,
> (uint8_t *)(minfo + 1) <=
> (uint8_t *)msa->msa_minfos + msa->msa_len;
> minfo++) {
> + uint64_t count[mfc_ncounters], scratch[mfc_ncounters];
> +
> /* Find a new entry or update old entry. */
> if (minfo->mfc_origin.s_addr !=
> satosin(rt->rt_gateway)->sin_addr.s_addr ||
> @@ -489,8 +494,9 @@ mrt_rtwalk_mfcsysctl(struct rtentry *rt,
> minfo->mfc_origin = satosin(rt->rt_gateway)->sin_addr;
> minfo->mfc_mcastgrp = satosin(rt_key(rt))->sin_addr;
> minfo->mfc_parent = mfc->mfc_parent;
> - minfo->mfc_pkt_cnt += mfc->mfc_pkt_cnt;
> - minfo->mfc_byte_cnt += mfc->mfc_byte_cnt;
> + counters_read(mfc->mfc_counter, count, mfc_ncounters, scratch);
> + minfo->mfc_pkt_cnt += count[mfc_packets];
> + minfo->mfc_byte_cnt += count[mfc_bytes];
> minfo->mfc_ttls[v->v_id] = mfc->mfc_ttl;
> break;
> }
> @@ -895,11 +901,13 @@ mfc_add_route(struct ifnet *ifp, struct
>
> mfc = malloc(sizeof(*mfc), M_MRTABLE, wait | M_ZERO);
> if (mfc == NULL) {
> - DPRINTF("origin %#08X group %#08X parent %d (%s) "
> - "malloc failed",
> - satosin(origin)->sin_addr.s_addr,
> - satosin(group)->sin_addr.s_addr,
> - mfccp->mfcc_parent, ifp->if_xname);
> + mrt_mcast_del(rt, rtableid);
> + rtfree(rt);
> + return (ENOMEM);
> + }
> + mfc->mfc_counter = counters_alloc_wait(mfc_ncounters, wait);
> + if (mfc->mfc_counter == NULL) {
> + free(mfc, M_MRTABLE, sizeof(struct mfc));
> mrt_mcast_del(rt, rtableid);
> rtfree(rt);
> return (ENOMEM);
> @@ -910,9 +918,6 @@ mfc_add_route(struct ifnet *ifp, struct
> rt_timer_add(rt, &ip_mrouterq, rtableid);
>
> mfc->mfc_parent = mfccp->mfcc_parent;
> - mfc->mfc_pkt_cnt = 0;
> - mfc->mfc_byte_cnt = 0;
> - mfc->mfc_wrong_if = 0;
> mfc->mfc_ttl = mfccp->mfcc_ttls[v->v_id];
> mfc->mfc_flags = mfccp->mfcc_flags[v->v_id] & mrt_api_config &
> MRT_MFC_FLAGS_ALL;
> @@ -1278,7 +1283,7 @@ ip_mdq(struct mbuf *m, struct ifnet *ifp
> if (mfc->mfc_parent != v->v_id) {
> /* came in the wrong interface */
> mrtstat_inc(mrts_wrong_if);
> - mfc->mfc_wrong_if++;
> + counters_inc(mfc->mfc_counter, mfc_wrong_if);
> rtfree(rt);
> return (0);
> }
> @@ -1308,8 +1313,8 @@ ip_mdq(struct mbuf *m, struct ifnet *ifp
> if (mfc == NULL)
> continue;
>
> - mfc->mfc_pkt_cnt++;
> - mfc->mfc_byte_cnt += m->m_pkthdr.len;
> + counters_pkt(mfc->mfc_counter, mfc_packets, mfc_bytes,
> + m->m_pkthdr.len);
>
> /* Don't let this route expire. */
> mfc->mfc_expire = 0;
> @@ -1414,13 +1419,16 @@ void
> mrt_mcast_del(struct rtentry *rt, unsigned int rtableid)
> {
> struct ifnet *ifp;
> + struct mfc *mfc;
> int error;
>
> /* Remove all timers related to this route. */
> rt_timer_remove_all(rt);
>
> - free(rt->rt_llinfo, M_MRTABLE, sizeof(struct mfc));
> + mfc = (struct mfc *)rt->rt_llinfo;
> rt->rt_llinfo = NULL;
> + counters_free(mfc->mfc_counter, mfc_ncounters);
> + free(mfc, M_MRTABLE, sizeof(struct mfc));
>
> ifp = if_get(rt->rt_ifidx);
> if (ifp == NULL)
> Index: netinet/ip_mroute.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_mroute.h,v
> diff -u -p -r1.35 ip_mroute.h
> --- netinet/ip_mroute.h 24 Jun 2026 12:33:49 -0000 1.35
> +++ netinet/ip_mroute.h 27 Jun 2026 19:46:04 -0000
> @@ -224,14 +224,19 @@ struct vif {
> * at a future point.)
> */
> struct mfc {
> + struct cpumem *mfc_counter; /* counters for src-grp */
> + u_long mfc_expire; /* expire timer */
> + struct in_addr mfc_rp; /* the RP address */
> vifi_t mfc_parent; /* incoming vif */
> - u_long mfc_pkt_cnt; /* pkt count for src-grp */
> - u_long mfc_byte_cnt; /* byte count for src-grp */
> - u_long mfc_wrong_if; /* wrong if for src-grp */
> uint8_t mfc_ttl; /* route interface ttl */
> uint8_t mfc_flags; /* MRT_MFC_FLAGS_* flags */
> - struct in_addr mfc_rp; /* the RP address */
> - u_long mfc_expire; /* expire timer */
> +};
> +
> +enum mfc_counters {
> + mfc_packets, /* packet count for src-grp */
> + mfc_bytes, /* byte count for src-grp */
> + mfc_wrong_if, /* wrong if for src-grp */
> + mfc_ncounters
> };
>
> /*
> Index: sys/percpu.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/sys/percpu.h,v
> diff -u -p -r1.9 percpu.h
> --- sys/percpu.h 16 Sep 2023 09:33:27 -0000 1.9
> +++ sys/percpu.h 27 Jun 2026 19:23:52 -0000
> @@ -54,6 +54,7 @@ struct cpumem *cpumem_get(struct pool *)
> void cpumem_put(struct pool *, struct cpumem *);
>
> struct cpumem *cpumem_malloc(size_t, int);
> +struct cpumem *cpumem_malloc_wait(size_t, int, int);
> struct cpumem *cpumem_malloc_ncpus(struct cpumem *, size_t, int);
> void cpumem_free(struct cpumem *, int, size_t);
>
> @@ -111,6 +112,7 @@ static struct { \
> */
>
> struct cpumem *counters_alloc(unsigned int);
> +struct cpumem *counters_alloc_wait(unsigned int, int);
> struct cpumem *counters_alloc_ncpus(struct cpumem *, unsigned int);
> void counters_free(struct cpumem *, unsigned int);
> void counters_read(struct cpumem *, uint64_t *, unsigned int,
>
--
:wq Claudio