Re: [PATCH v5 1/1] examples/l2fwd-jobstats: fix lock availability

[Stephen Hemminger <stephen@networkplumber.org>]

On Sun, 11 Aug 2024 21:29:57 +0530
Rakesh Kudurumalla <rkudurumalla@marvell.com> wrote:

> Race condition between jobstats and time metrics
> for forwarding and flushing is maintained using spinlock.
> Timer metrics are not displayed properly due to the
> frequent unavailability of the lock.This patch fixes
> the issue by introducing a delay before acquiring
> the lock in the loop. This delay allows for betteravailability
> of the lock, ensuring that show_lcore_stats() can
> periodically update the statistics even when forwarding
> jobs are running.
> 
> Fixes: 204896f8d66c ("examples/l2fwd-jobstats: add new example")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Rakesh Kudurumalla <rkudurumalla@marvell.com>
> ---

I prodded AI to review this patch and it had some good observations.
You could go farther with rte_wait_until_equal which has even better power performance.

Let me see the critical `l2fwd_main_loop` function to understand the full locking context:Now I have a complete picture. Let me provide my review.

---

## Patch Review: examples/l2fwd-jobstats: fix lock availability

### Commit Message Issues

**Errors:**
- Line 84: Missing space before "betteravailability" → should be "better availability"
- Line 82: Missing space after period: "lock.This" → should be "lock. This"

**Style (minor):** The commit message is a bit verbose for what it's doing.

### Technical Analysis

The patch addresses a real problem. Looking at the code flow:

```c
/* Main loop holds lock almost continuously */
for (;;) {
    rte_spinlock_lock(&qconf->lock);      // Acquire
    
    do {
        /* ... lots of work ... */
        stats_read_pending = rte_atomic_load_explicit(...);
    } while (likely(stats_read_pending == 0));
    
    rte_spinlock_unlock(&qconf->lock);    // Release
    rte_pause();                          // <-- This is too short!
    // Immediately re-acquires lock...
}
```

Meanwhile, `show_lcore_stats()` sets the flag and waits on the lock:
```c
rte_atomic_store_explicit(&qconf->stats_read_pending, 1, ...);
rte_spinlock_lock(&qconf->lock);  // Starved - main loop re-acquires too fast
```

**The fix works**, but it's suboptimal:
- The 10µs delay is an arbitrary magic number
- It delays **unconditionally**, even when no stats reader is waiting
- Wastes cycles when the system is idle

### Better Option: Wait for stats completion

The `stats_read_pending` flag already exists to coordinate this handshake but isn't used fully. The cleaner fix is to make the main loop wait until the stats reader has finished:

```c
rte_spinlock_unlock(&qconf->lock);
/* Wait for stats reader to complete before reacquiring lock */
while (rte_atomic_load_explicit(&qconf->stats_read_pending, 
                                 rte_memory_order_relaxed) != 0)
    rte_pause();
```

This approach:
- Uses the existing synchronization mechanism as intended
- Only yields when stats are actually being read
- Has zero overhead when no stats read is pending
- Doesn't introduce arbitrary magic constants
- Guarantees the stats reader gets the lock (not probabilistic)

### Alternative: Conditional delay

If you want to keep the delay approach, at least make it conditional:

```c
rte_spinlock_unlock(&qconf->lock);
if (stats_read_pending)
    rte_delay_us(10);
else
    rte_pause();
```

This avoids the unconditional 10µs penalty on every iteration.

### Verdict

**NAK as-is.** The fix works but the better solution is to properly use the existing `stats_read_pending` handshake. Would suggest the author revise to use the wait-for-completion pattern instead of an arbitrary delay.