Re: [PATCH net-next 1/4] net: rmnet: use fast monotonic time for tx aggregation

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 3 (2) · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message describes the new clock source wrongly and
  gives no measurements for its performance claims.
- [Medium] The patch calls ktime_get_mono_fast_ns() from a driver and
  describes it as monotonic.
- [Medium] rmnet: stale timestamp assigned to agg_time causes premature
  tx flush
- [Low] Dropping the `diff.tv_sec > 0` term quietly changes the in-path
  flush behaviour of rmnet_map_tx_aggregate() for tx_aggr_time_usecs of…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is a
  time-of-check/time-of-use race in rmnet_map_tx_aggregate() that leaks…
- [Medium] This problem wasn't introduced by this patch, but
  rmnet_map_update_ul_agg_config() computes…
- [Medium] This problem wasn't introduced by this patch, but both flush
  branches of rmnet_map_tx_aggregate() (lines 637-645 and 658-667) call…

--

net: rmnet: use fast monotonic time for tx aggregation

This commit replaces ktime_get_real_ts64() with ktime_get_mono_fast_ns()
in rmnet_map_tx_aggregate(). It also changes agg_time and agg_last in
struct rmnet_port from struct timespec64 to u64 nanoseconds, so the bypass
and flush age checks become plain u64 subtractions.

> Calling a wall-clock time function on the transmit hot path introduces
> significant performance bottlenecks, causing cacheline bouncing and pipeline
> stalls when processing high packet volumes.

[ ... ]

> Replace ktime_get_real_ts64() with ktime_get_mono_fast_ns(). This reads a
> lockless, per-CPU timestamp directly in nanoseconds, returning a simple u64.

[ ... ]

> This optimization significantly reduces CPU overhead, eliminates struct
> timespec64 math, and provides a much faster, cache-friendly timekeeping
> mechanism for high-throughput egress traffic.

[Severity: Medium]
Is this description of ktime_get_mono_fast_ns() accurate? In
kernel/time/timekeeping.c it reads from a single global object:

static struct tk_fast tk_fast_mono ____cacheline_aligned = {

u64 notrace ktime_get_mono_fast_ns(void)
{
	return __ktime_get_fast_ns(&tk_fast_mono);
}

__ktime_get_fast_ns() then runs a latch seqcount retry loop and reads the
hardware clocksource. ktime_get_real_ts64() does the same:

	do {
		seq = read_seqcount_latch(&tkf->seq);
		tkr = tkf->base + (seq & 0x01);
		now = ktime_to_ns(tkr->base);
		now += timekeeping_get_ns(tkr);
	} while (read_seqcount_latch_retry(&tkf->seq, seq));

Nothing in this path is per-CPU. Seqcount readers also never write the
shared cacheline, so the old ktime_get_real_ts64() reader path wouldn't
cause cacheline bouncing either.

The real savings appear to be a latch seqcount in place of a plain
seqcount, plus no timespec64 normalisation and no two-field compare.

Could the commit message describe that instead? Could it also include
measurements to support "significantly reduces CPU overhead"?

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index 39d6d084e73f0..2eafb1d969c1a 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c

[ ... ]

> @@ -591,21 +591,23 @@ static enum hrtimer_restart rmnet_map_flush_tx_packet_queue(struct hrtimer *t)
>  unsigned int rmnet_map_tx_aggregate(struct sk_buff *skb, struct rmnet_port *port,
>  				    struct net_device *orig_dev)
>  {

[ ... ]

>  	spin_lock_bh(&port->agg_lock);
> -	memcpy(&last, &port->agg_last, sizeof(struct timespec64));
> -	ktime_get_real_ts64(&port->agg_last);
> +	last = port->agg_last;
> +
> +	now = ktime_get_mono_fast_ns();
> +	port->agg_last = now;

[Severity: Medium]
Is ktime_get_mono_fast_ns() a good choice for a driver? Its kernel-doc
says it is not strictly monotonic:

 * This timestamp is not guaranteed to be monotonic across an update.
 ...
 * So reader 6 will observe time going backwards versus reader 5.
 ...
 * Callers need to be aware of that and deal with it.

Documentation/core-api/timekeeping.rst also says:

	machine check reporting, but most drivers should never call them,
	since the time is allowed to jump under certain conditions.

port->agg_last and port->agg_time are shared across CPUs.
rmnet_vnd_setup() sets rmnet_dev->lltx = true, and several muxed rmnet
devices can share one rmnet_port. As a result, rmnet_map_tx_aggregate()
runs on several CPUs at once. agg_lock serialises the transmitters, but
it does not serialise them against update_fast_timekeeper().

Suppose CPU A reads now from the old base and stores it in agg_last. CPU B
then reads from the new base after the latch switch. B's now can be
smaller than last.

Neither unsigned subtraction guards against that:

	diff = now - last;
	...
	diff = now - port->agg_time;

In that case diff wraps to roughly U64_MAX. That either forces the
sparse traffic bypass (diff > RMNET_AGG_BYPASS_TIME_NSEC) or flushes a
partial aggregate early (diff > egress_agg_params.time_nsec).

The effect is small: one packet skips aggregation, or one aggregate is
flushed early. The old timespec64_sub() code took the same paths on a
negative delta. Even so, the commit message calls this clock monotonic.

Would ktime_get_ns() fit better here? It is monotonic, returns a u64, and
still avoids the timespec64 math.

>  
>  	if (!port->skbagg_head) {
>  		/* Check to see if we should agg first. If the traffic is very
>  		 * sparse, don't aggregate.
>  		 */
>  new_packet:
> -		diff = timespec64_sub(port->agg_last, last);
> +		diff = now - last;
>  		size = port->egress_agg_params.bytes - skb->len;

[ ... ]

> @@ -625,11 +626,12 @@ unsigned int rmnet_map_tx_aggregate(struct sk_buff *skb, struct rmnet_port *port
>  		dev_kfree_skb_any(skb);
>  		port->skbagg_head->protocol = htons(ETH_P_MAP);
>  		port->agg_count = 1;
> -		ktime_get_real_ts64(&port->agg_time);
> +		port->agg_time = now;
>  		skb_frag_list_init(port->skbagg_head);
>  		goto schedule;
>  	}
> -	diff = timespec64_sub(port->agg_last, port->agg_time);
> +
> +	diff = now - port->agg_time;
>  	size = port->egress_agg_params.bytes - port->skbagg_head->len;
>  
>  	if (skb->len > size) {

[Severity: Medium]
Does this change when a new aggregate's age starts counting on the
goto new_packet path?

now is read once, at the top of rmnet_map_tx_aggregate(). Sometimes
the incoming skb does not fit in the current aggregate. The function
then drops agg_lock, calls hrtimer_cancel() and rmnet_send_skb() for
the old aggregate, re-takes agg_lock and jumps back to new_packet. A
new aggregate started there gets agg_time set to the timestamp read
before all of that.

The old code called ktime_get_real_ts64(&port->agg_time) at this
point, so agg_time was a fresh reading taken after the flush. With
this patch, agg_time is backdated by however long hrtimer_cancel()
and the nested dev_queue_xmit() took. hrtimer_cancel() can spin until
a running rmnet_map_flush_tx_packet_queue() finishes.
dev_queue_xmit() on the real device can contend on the qdisc lock.

The next packet computes now - port->agg_time against that stale
value. It is therefore more likely to exceed
egress_agg_params.time_nsec, so the new aggregate is flushed earlier
than configured. The hrtimer armed at schedule: uses an expiry
relative to the current time. The timer and the age check therefore
no longer agree on when the aggregate started.

This path runs on every size-triggered flush. Under sustained traffic
that overflows aggregates, it seems to work against the throughput
goal of this patch.

Could agg_time take a fresh ktime_get_mono_fast_ns() reading after
agg_lock is re-acquired on this path, as the old code effectively
did? The bypass check at new_packet compares against last. It used
the entry-time agg_last in the old code too, so only agg_time seems
to need the fresh value.

[Severity: High]
This isn't a bug introduced by this patch, but can this branch leak an
aggregate that another CPU built?

	if (skb->len > size) {
		agg_skb = port->skbagg_head;
		reset_aggr_params(port);
		spin_unlock_bh(&port->agg_lock);
		hrtimer_cancel(&port->hrtimer);
		rmnet_send_skb(port, agg_skb);
		spin_lock_bh(&port->agg_lock);
		goto new_packet;
	}

The new_packet label is inside the if (!port->skbagg_head) block. After
agg_lock is retaken, the goto skips that check.

With lltx = true and several muxed devices sharing one rmnet_port,
another CPU can run while the lock is dropped:

CPU A                              CPU B
reset_aggr_params()
  skbagg_head = NULL
spin_unlock_bh()
hrtimer_cancel()                   spin_lock_bh()
rmnet_send_skb()                   sees skbagg_head == NULL
                                   small diff, A just set agg_last
                                   skbagg_head = skb_copy_expand(...)
                                   spin_unlock_bh()
spin_lock_bh()
goto new_packet

If A's diff is small when it resumes, A overwrites the head without
checking it first:

		port->skbagg_head = skb_copy_expand(skb, 0, size, GFP_ATOMIC);

Nothing references B's head skb or any frag_list entries appended to it
after that, so they leak. Those packets were already counted as
transmitted in rmnet_vnd_tx_fixup_len(), but they are never sent.

If A's diff exceeds the bypass threshold instead, A sends its skb directly
ahead of B's pending aggregate, so packets are reordered.

Could port->skbagg_head be re-checked after the lock is re-acquired? For
example, the goto could target a label placed before the
if (!port->skbagg_head) test.

[ ... ]

> @@ -653,7 +655,7 @@ unsigned int rmnet_map_tx_aggregate(struct sk_buff *skb, struct rmnet_port *port
>  	port->skbagg_tail = skb;
>  	port->agg_count++;
>  
> -	if (diff.tv_sec > 0 || diff.tv_nsec > port->egress_agg_params.time_nsec ||
> +	if (diff > port->egress_agg_params.time_nsec ||

[Severity: Low]
Is it intended that removing the diff.tv_sec > 0 term changes flush
behaviour for timeouts of one second or more?

tv_nsec used to be normalised below 1e9, so the next packet flushed any
aggregate that was at least 1s old. In effect, a configured time_nsec of
1s or more was capped at 1s when a packet arrived.

The full configured timeout now applies. rmnet_set_coalesce() passes
tx_aggr_time_usecs through without an upper bound, and the netlink
policy for ETHTOOL_A_COALESCE_TX_AGGR_TIME_USECS is plain NLA_U32.

With a 2s timeout, a packet that arrives when the aggregate is 1.1s old
used to trigger a flush and no longer does.

The new behaviour matches the hrtimer, which was always armed with the
full time_nsec. Could the commit message mention this change?

[Severity: Medium]
This isn't a bug introduced by this patch, but on 32-bit builds the
time_nsec value compared here can be wrong.
rmnet_map_update_ul_agg_config() does:

	port->egress_agg_params.time_nsec = time * NSEC_PER_USEC;

time is u32 and NSEC_PER_USEC is 1000L. On 32-bit, long is 32 bits, so
the product is computed as a 32-bit unsigned long. It wraps before it is
stored in the u64 time_nsec.

rmnet_set_coalesce() bounds tx_aggr_max_frames and tx_aggr_max_bytes,
but not tx_aggr_time_usecs. Any value above about 4294967 usec wraps.
For example, 5000000 usec is stored as 705032704 ns, and
rmnet_get_coalesce() reports 705032 usec back.

Both the hrtimer and this age check use the wrapped value.

>  	    port->agg_count >= port->egress_agg_params.count ||
>  	    port->skbagg_head->len == port->egress_agg_params.bytes) {
>  		agg_skb = port->skbagg_head;

[Severity: Medium]
This is a pre-existing issue, but can a new aggregate end up with no timer
armed? Both flush branches do this:

		reset_aggr_params(port);
		spin_unlock_bh(&port->agg_lock);
		hrtimer_cancel(&port->hrtimer);

reset_aggr_params() sets agg_state to 0, and the lock is released while
the old hrtimer is still queued.

Suppose CPU B creates a new aggregate in that window and reaches
schedule:. The old timer is still active, so this check fails:

	if (!hrtimer_active(&port->hrtimer) && port->agg_state != -EINPROGRESS) {

B then neither arms a timer nor sets agg_state. CPU A cancels the old
timer next, which leaves B's aggregate with no pending hrtimer and no
queued work. It stays buffered until the next egress packet on the port.

If the port is torn down first, rmnet_map_tx_aggregate_exit() only frees
skbagg_head when agg_state is -EINPROGRESS:

	if (port->agg_state == -EINPROGRESS) {
		if (port->skbagg_head) {
			dev_kfree_skb_any(port->skbagg_head);

In that case the stranded aggregate also leaks.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002143529.3217189-1-koen.vandeputte%40citymesh.com