On Wed, Oct 21, 2026 at 10:57 AM David Laight
<[email protected]> wrote:
> > Apply it to all e1000e parts: limit the first data descriptor of a TSO
> > packet to 3 * (max_read_request - 4) bytes (372 on the I219, where
> > pcie_get_readrq() returns 128). The rest of the linear part goes into
> > the next descriptor, which e1000_xmit_frame() already reserves;
>
> Aren't you using an additional descriptor over and above those the code
> might already use?
> So either it was reserving one too many, or you need to reserve another one.
You're right that the split can use one more descriptor than before.
The count already covers it, though: the bare count++ in
e1000_xmit_frame() is never used by e1000_tx_map(). It comes from e1000,
where that descriptor pays for the 4-byte TSO sentinel ("premature desc
write-backs in TSO mode"); e1000e dropped the sentinel but kept the
count++. With this patch the linear part takes
1 + DIV_ROUND_UP(len - 372, tx_fifo_limit) descriptors instead of
DIV_ROUND_UP(len, tx_fifo_limit), at most one more, so it uses exactly
that spare.
If we want the count to say so, I see two ways:
a) Count the split explicitly and keep the spare:
if ((mss) || (skb->ip_summed == CHECKSUM_PARTIAL))
count++;
count++;
+ /* the headers go in a first buffer of at most tx_hdr_buf_max */
+ if (mss && len > adapter->tx_hdr_buf_max)
+ count++;
count += DIV_ROUND_UP(len, adapter->tx_fifo_limit);
The split pays for its own descriptor and the old spare stays a
spare.
b) Replace the spare with the explicit count:
if ((mss) || (skb->ip_summed == CHECKSUM_PARTIAL))
count++;
- count++;
+ if (mss && len > adapter->tx_hdr_buf_max)
+ count++;
count += DIV_ROUND_UP(len, adapter->tx_fifo_limit);
The count becomes exact, and one lower for every other packet.
Neither fixes a bug I have seen: the reservation already covers the
split, and in about 3.5 hours of test traffic on the patched driver the
ring reached its stop threshold over 300,000 times without a stall. b
also changes the reservation for every other packet, which I haven't
tested. So I'd keep the patch as it is, the smallest change that fixes
the bug, and leave the descriptor accounting for a separate cleanup.
If you'd rather have the dependency explicit, I'm happy to fold a) into
a v3; it only reserves one more descriptor for those TSO packets.
> I'm not actually sure all the logic there is actually needed.
> The code calculates the number of descriptors the packet is expected to need
> and gets the packet requeued if there aren't enough, but there is also the
> 'post transmit' check that stops the transmit if there isn't enough room
> for a full length packet.
> If the latter is valid you don't need the former.
> Although a sanity check that the ring isn't being wrapped might not do any
> harm.
The post-transmit check reserves (MAX_SKB_FRAGS + 1) *
DIV_ROUND_UP(PAGE_SIZE, tx_fifo_limit) + 4, which assumes no buffer is
larger than a page. The linear part and high-order frags can be, so the
per-packet count is still needed.
Andre
