[PATCH grout] l2: optimize bridge FDB source MAC learning

[Morten Brørup]

> From: Robin Jarry [mailto:rjarry at redhat.com]
> Sent: Friday, 22 May 2026 13.41
> 
> Morten Brørup, May 21, 2026 at 23:09:
> > We started off with something simple as that. But races started
> > showing up, where e.g. a packet could seemingly spend negative time
> in
> > a shaper if the enqueueing thread had snapshotted its clock after the
> > dequeueing thread had snapshotted its clock - although the enqueueing
> > thread had obviously put the packet in the queue before the
> dequeueing
> > thread took it out of the queue.
> >
> > So we kept the RTE_PER_LCORE() timestamps in various formats, and
> > introduced a "void per_lcore_clock_update(void)" function to update
> > the snapshots of the calling thread. Now, the trick is: Any burst
> > processing function relying on timestamps must call this update
> > function first. Thinking about the need for this requires the same
> way
> > of thinking as for locks; you need to always consider ordering and
> > races.
> 
> Ah yes, if you do cross worker processing, you'll need a way to
> synchronize clocks.
> 
> > If I added a "clock" module exposing these features for public use
> > (not only internal use), where in the tree should I put the files?
> > There's going to be a C file with the gr_per_lcore_clock_update()
> > function implementation and the RTE_DEFINE_PER_LCORE(gr_clock_xyz)
> > timestamps in various formats, and an accompanying public header file
> > with the function declaration and the
> > RTE_DECLARE_PER_LCORE(gr_clock_xyz) timestamps in various formats.
> >
> > Then I can update the bridge FDB patch to use this central clock
> > instead. And for now, I think updating the clock is only required
> > where you put it in your example above.
> >
> > I'm not yet familiar with Grout naming conventions. The "void
> > gr_per_lcore_clock_update(void)" function updates the RTE_PER_LCORE()
> > timestamp variables for the calling thread. Any guidance for how to
> > name the function?
> 
> Only symbols that are exposed via the API headers (gr_*.h) should start
> with the gr_ prefix. Symbols internal to the grout daemon or grcli
> should not. There are some unfortunate exceptions (one of them being
> gr_datapath_loop) but we should avoid these.
> 
> How about adding the functions to modules/infra/datapath/datapath.h and
> find a common prefix for them:
> 
> diff --git a/modules/infra/datapath/datapath.h
> b/modules/infra/datapath/datapath.h
> index 2c1e58e5f8ec..4bdb9532498c 100644
> --- a/modules/infra/datapath/datapath.h
> +++ b/modules/infra/datapath/datapath.h
> @@ -3,4 +3,21 @@
> 
>  #pragma once
> 
> +#include <rte_per_lcore.h>
> +
> +#include <time.h>
> +
>  void *gr_datapath_loop(void *priv);
> +
> +// Per worker timestamp value maintained by gr_datapath_loop().
> +RTE_DECLARE_PER_LCORE(clock_t, timestamp_us);
> +
> +// Get the value of the current thread timestamp.
> +static inline clock_t timestamp_us_get(void) {
> +	return RTE_PER_LCORE(timestamp_us);
> +}
> +
> +// Update the value of the current thread timestamp.
> +static inline void timestamp_us_set(clock_t us) {
> +	RTE_PER_LCORE(timestamp_us) = us;
> +}
> diff --git a/modules/infra/datapath/main_loop.c
> b/modules/infra/datapath/main_loop.c
> index f462cfbd42dd..423a4eb91ba7 100644
> --- a/modules/infra/datapath/main_loop.c
> +++ b/modules/infra/datapath/main_loop.c
> @@ -10,6 +10,8 @@
>  #include "vec.h"
>  #include "worker.h"
> 
> +#include <gr_clock.h>
> +
>  #include <rte_common.h>
>  #include <rte_eal.h>
>  #include <rte_errno.h>
> @@ -182,6 +184,8 @@ err:
> 
>  static struct rte_rcu_qsbr *rcu;
> 
> +RTE_DEFINE_PER_LCORE(clock_t, timestamp_us);
> +
>  void *gr_datapath_loop(void *priv) {
>  	struct stats_context ctx = {
>  		.stats = NULL,
> @@ -257,6 +261,7 @@ reconfig:
>  	loop = 0;
>  	sleep = 0;
>  	timestamp = rte_rdtsc();
> +	timestamp_us_set(gr_clock_us());
>  	for (;;) {
>  		rte_graph_walk(graph);
> 
> @@ -292,6 +297,7 @@ reconfig:
>  			ctx.w_stats->total_cycles += cycles;
>  			ctx.w_stats->loop_cycles += cycles;
>  			ctx.w_stats->n_loops += HOUSEKEEPING_INTERVAL;
> +			timestamp_us_set(gr_clock_us());
>  		}
>  	}
> 
> diff --git a/modules/l2/control/fdb.c b/modules/l2/control/fdb.c
> index 3982cce11854..bb0fb86cc42d 100644
> --- a/modules/l2/control/fdb.c
> +++ b/modules/l2/control/fdb.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: BSD-3-Clause
>  // Copyright (c) 2026 Robin Jarry
> 
> +#include "datapath.h"
>  #include "event.h"
>  #include "iface.h"
>  #include "l2.h"
> @@ -138,7 +139,7 @@ void fdb_learn(
>  		fdb = data;
>  	}
> 
> -	fdb->last_seen = gr_clock_us();
> +	fdb->last_seen = timestamp_us_get();
> 
>  	if ((fdb->flags & GR_FDB_F_LEARN)
>  	    && (fdb->iface_id != iface_id || !l3_addr_eq(&fdb->vtep,
> vtep))) {
> 
> Does that fit your needs?

Thanks for the feedback.
I will post a v2 patch.
But first, I have sent a patch to replace clock_t (which has implementation defined type and granularity) with a new gr_clock_ns_t (with nanosecond granularity, and also using 64 bit).
My v2 of my performance optimization patch will depend on the gr_clock_ns_t. And since they are not directly related, I have not sent a series containing both.