Re: [PATCH net-next v16 10/15] quic: add packet number space

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message of "quic: add packet number space" does not
  mention ECN anywhere, yet much of the new API is ECN accounting and…
- [Low] The pn_map layout comment above struct quic_pnspace in
  net/quic/pnspace.h gives the size-based trigger for advancing the map…

--

quic: add packet number space

This patch adds struct quic_pnspace to track per packet number space
state such as next PN, inflight bytes, loss and ACK timestamps, plus a
received-PN bitmap. It provides helpers to check and mark received packet
numbers and to build gap ACK blocks for ACK frames, with APP and EARLY
sharing one packet number space.

> It maintains the next packet number to assign, tracks the total length of
> frames currently in flight, and records the time when the next packet may
> be considered lost. It also keeps track of the largest acknowledged packet
> number, the time it was acknowledged, and when the most recent ack
> eliciting packet was sent. These fields are useful for loss detection,
> RTT estimation, and congestion control.

[Severity: Low]

This isn't a bug, but the commit message never mentions ECN. A good part
of the new API is ECN accounting and validation.

The patch adds:

- the ecn_count[QUIC_ECN_DIR_MAX][QUIC_ECN_MAX] array in struct
  quic_pnspace
- the QUIC_ECN_LOCAL/PEER/ACKED enum
- the helpers quic_pnspace_inc_ecn_local(), quic_pnspace_inc_ecn_acked(),
  quic_pnspace_reset_ecn_acked(), quic_pnspace_has_ecn_local(),
  quic_pnspace_set_ecn_peer() and quic_pnspace_validate_ecn()

quic_pnspace_validate_ecn() implements the validation rules from
rfc9000#section-13.4.2.1.

The ECN history only appears in the changelog below the --- line, and
that part is dropped when the patch is applied.

Could a sentence be added along the lines of: "It also tracks
per-codepoint ECN counts (locally received, peer-reported, newly acked)
and provides helpers to validate peer ECN counts per
rfc9000#section-13.4.2.1."?

> diff --git a/net/quic/pnspace.h b/net/quic/pnspace.h
> new file mode 100644
> index 0000000000000..67981d5f1849c
> --- /dev/null
> +++ b/net/quic/pnspace.h

[ ... ]

> +/* Packet Number Map (pn_map) Layout:
> + *
> + *     min_pn_seen -->++-----------------------+---------------------+---
> + *         base_pn -----^   last_max_pn_seen --^       max_pn_seen --^
> + *
> + * Map Advancement Logic:
> + *   - min_pn_seen = last_max_pn_seen;
> + *   - base_pn = first zero bit after last_max_pn_seen;
> + *   - last_max_pn_seen = max_pn_seen;
> + *   - last_max_pn_time = max_pn_time;
> + *
> + * Conditions to Advance pn_map:
> + *   - (max_pn_time - last_max_pn_time) >= max_time_limit, or
> + *   - (max_pn_seen - last_max_pn_seen) > QUIC_PN_MAP_LIMIT

[Severity: Low]

Does the second condition match quic_pnspace_mark()? The code limits
the reference point to base_pn:

net/quic/pnspace.c:quic_pnspace_mark() {
    ...
	last_max_pn_seen = min_t(s64, space->last_max_pn_seen, space->base_pn);
	last_max_pn_time = space->last_max_pn_time;
	if (space->max_pn_time < last_max_pn_time + space->max_time_limit &&
	    space->max_pn_seen <= last_max_pn_seen + QUIC_PN_MAP_LIMIT)
		return 0;
    ...
}

So the map advances once max_pn_seen is more than QUIC_PN_MAP_LIMIT past
min(last_max_pn_seen, base_pn).

base_pn can be lower than last_max_pn_seen. During an advance,
quic_pnspace_move() only moves base_pn to the first zero bit after the
old last_max_pn_seen. Then last_max_pn_seen is set to max_pn_seen.

If a packet between those two points is missing, base_pn stays below
last_max_pn_seen. In that state the code advances earlier than this
comment says.

This base_pn limit is what keeps pn - base_pn inside the
QUIC_PN_MAP_SIZE bitmap. Could the comment be updated to show it, for
example (max_pn_seen - min(last_max_pn_seen, base_pn)) >
QUIC_PN_MAP_LIMIT?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791227050.git.lucien.xin%40gmail.com