Re: [PATCH net 2/2] ionic: limit TSO segments to the Tx ring size

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 1 · 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).

- [High] A Tx ring shrink on a running interface can still permanently
  stall a Tx queue, which is the failure this fix is meant to remove.
- [Medium] The new tso_max_segs comes only from the regular Tx ring size
  (ntxq_descs - 1: 1023 by default, up to 8191).
- [Low] The commit message understates what the fix addresses.

--

ionic: limit TSO segments to the Tx ring size

This patch advertises the Tx ring size minus one as tso_max_segs, so the
stack stops building GSO skbs with more segments than the ring can hold.
The limit is updated on ring resize through a new
ionic_lif_set_ntxq_descs() helper and in ionic_cmb_reconfig(). A features
change event is sent after an ethtool Tx ring resize.

> The driver never tells the stack this limit, so such skbs are built
> and handed to it instead of being segmented in software.

[Severity: Low]
Could the changelog also describe what users see when this happens?

When gso_segs is larger than num_descs - 1, ionic_start_xmit() hits:

	if (!netif_txq_maybe_stop(q_to_ndq(netdev, q),
				  ionic_q_space_avail(q),
				  ndescs, ndescs))
		return NETDEV_TX_BUSY;

ionic_q_space_avail() can never reach ndescs. The queue stops, the qdisc
requeues the skb and keeps retrying it until dev_watchdog fires, and the
tx timeout recovery then replays the same skb.

This is easy to hit with a 64-entry ring (the kdump default
IONIC_MIN_TXRX_DESC, or ethtool -G tx 64) and a 64KB TSO skb with MSS
below about 1040, or with UDP GSO.

Would it help stable backporters if the message said this fixes a Tx
queue hang, rather than only a missed software segmentation?

> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> index 7f0da2b0defdf..ce1e51f318e01 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c

[ ... ]

> @@ -3065,7 +3069,7 @@ int ionic_reconfigure_queues(struct ionic_lif *lif,
>  
>  	/* swap new desc_info and rings, keeping existing interrupt config */
>  	if (tx_qcqs) {
> -		lif->ntxq_descs = qparam->ntxq_descs;
> +		ionic_lif_set_ntxq_descs(lif, qparam->ntxq_descs);

[Severity: High]
Can shrinking the Tx ring on a running interface still stall a Tx queue
permanently?

The lower gso_max_segs only takes effect in validate_xmit_skb(), which
runs on skbs freshly dequeued from the qdisc. Skbs that were requeued
into q->gso_skb skip that check:

net/sched/sch_generic.c:dequeue_skb() {
    ...
		/* skb in gso_skb were already validated */
		*validate = false;
    ...
}

Here is one possible sequence:

1. The ring has 1024 entries, so gso_max_segs is 1023, and an skb with
   100 segments passes validation.

2. The ring is briefly full, so ionic_start_xmit() returns
   NETDEV_TX_BUSY and the skb is requeued into q->gso_skb. An skb can
   also end up there when sch_direct_xmit() finds the txq frozen after
   netif_device_detach().

3. The admin runs ethtool -G tx 64. Inside ionic_reconfigure_queues():
   - ionic_stop_queues_reconfig() detaches and stops the queues but
     never resets the qdisc.
   - The 64-entry rings are swapped in.
   - ionic_lif_set_ntxq_descs() lowers the limit to 63.
   - ionic_start_queues_reconfig()->netif_device_attach() wakes the
     queue.

4. The stale skb is dequeued without validation. ionic_tx_descs_needed()
   returns 100, but ionic_q_space_avail() can never be more than 63.
   ionic_start_xmit() stops the queue and returns NETDEV_TX_BUSY. The
   ring is empty, so no completion ever wakes the queue.

5. dev_watchdog fires, and ionic_tx_timeout_work() runs the stop/start
   reconfig again. That does not purge q->gso_skb either, so the same
   skb blocks the queue again. Only qdisc_reset() on ifdown clears it.

ionic_cmb_reconfig() appears to have the same window.

Is a driver-side check needed to fully close this? For example,
ionic_start_xmit() could drop or software-segment an skb whose ndescs is
more than q->num_descs - 1, instead of returning NETDEV_TX_BUSY.

>  		for (i = 0; i < qparam->nxqs; i++)
>  			ionic_swap_queues(lif->txqcqs[i], tx_qcqs[i]);
>  	}

[ ... ]

> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> index e790f6b9441e2..14aaaeb360a27 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> @@ -290,6 +290,16 @@ static inline void ionic_init_queue_params(struct ionic_lif *lif,
>  	qparam->cmb_rx = test_bit(IONIC_LIF_F_CMB_RX_RINGS, lif->state);
>  }
>  
> +static inline void ionic_lif_set_ntxq_descs(struct ionic_lif *lif,
> +					    unsigned int descs)
> +{
> +	if (descs == lif->ntxq_descs)
> +		return;
> +
> +	lif->ntxq_descs = descs;
> +	netif_set_tso_max_segs(lif->netdev, descs - 1);

[Severity: Medium]
Does this limit also need to cover the hwstamp Tx ring?

With Tx hardware timestamping enabled, ionic_start_xmit() sends every
SKBTX_HW_TSTAMP skb to the hwstamp queue:

	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
		if (lif->hwstamp_txq && lif->phc->ts_config_tx_mode)
			return ionic_start_hwstamp_xmit(skb, netdev);

ionic_lif_create_hwstamp_txq() always creates that ring with:

	num_desc = IONIC_MIN_TXRX_DESC;

So at most 63 entries are usable there. The tso_max_segs set here is
ntxq_descs - 1, which is 1023 by default and can be up to 8191.

A timestamped TSO skb with 64 to ntxq_descs - 1 segments passes the core
gso_max_segs check. ionic_start_hwstamp_xmit() then drops it every time:

	if (unlikely(!ionic_q_has_space(q, ndescs)))
		goto err_out_drop;

The ionic_cmb_reconfig() hunk computes the limit the same way. Any
SO_TIMESTAMPING TX_HARDWARE user with MSS below about 1040 would hit
these drops, for example after setting TCP_MAXSEG or over a small path
MTU.

The commit message says the goal is to stop skbs that can never be
posted from reaching the driver. Doesn't the hwstamp ring, the smallest
Tx ring the driver posts to, still receive them?

> +}
> +

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107-b4-ionic-tso-max-segs-v1-0-67c248a5b681%40amd.com