On Mon, 15 Jun 2026 14:01:42 -0700
Mark Blasko <[email protected]> wrote:
> That appears to be the AI feedback from V3 (which was addressed in V4).
> I also looked at the AI feedback in the mail archive for V4 and it does not
> look like there's anything actionable.
>
> Do you have any other feedback for the V4 patches?
Let me re-run it.
Sorry got wrong part of long AI thread.
Re-reviewed v4 against current main. The two correctness errors and the
interrupt-thread warning from v3 are all resolved:
- 4/6: the periodic read now runs on a dedicated control thread via
rte_thread_create_internal_control(), sleeping in 10ms increments
with a relaxed stop flag, instead of blocking the shared EAL
interrupt thread. Teardown sets the stop flag, joins the thread,
then frees the memzone, so the join keeps the thread off the freed
memzone. Good.
- 5/6: nic_ts_lock is now initialized before gve_init_priv() (so the
setup-time gve_read_nic_clock() locks an initialized mutex) and
destroyed only after gve_teardown_device_resources() has joined the
sync thread. Both ordering bugs are fixed, and the gve_init_priv()
failure path destroys both mutexes correctly.
One item remains:
[PATCH v4 4/6] net/gve: add periodic NIC clock synchronization
Warning: the commit message body still describes the old implementation.
It says the sync "runs every 250ms using rte_alarm" and "the alarm is
still rescheduled", but the code no longer uses rte_alarm - it is a
dedicated control thread (gve_nic_ts_thread) that polls with
rte_delay_us_sleep() and a stop flag. The v4 changelog notes the switch,
but the commit message body itself was not updated. Please reword it to
match the thread-based implementation.
Patches 1, 2, 3, 5, and 6 look good. With the 4/6 commit message
corrected, the series is ready.
