Re: [PATCH net-next 2/2] net: enetc: add support for software TSO

On Thu, Oct 07, 2021 at 09:06:45AM +0000, Claudiu Manoil wrote:

quoted

-----Original Message-----
From: Ioana Ciornei <ioana.ciornei@nxp.com>
Sent: Thursday, October 7, 2021 11:33 AM
To: Claudiu Manoil <claudiu.manoil@nxp.com>
Cc: davem@davemloft.net; kuba@kernel.org; netdev@vger.kernel.org;
Vladimir Oltean [off-list ref]
Subject: Re: [PATCH net-next 2/2] net: enetc: add support for software TSO

On Thu, Oct 07, 2021 at 07:59:25AM +0000, Claudiu Manoil wrote:

quoted

quoted

-----Original Message-----
From: Ioana Ciornei <ioana.ciornei@nxp.com>
Sent: Wednesday, October 6, 2021 11:13 PM

[...]

quoted

+static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct
sk_buff *skb)
+{
+	int hdr_len, total_len, data_len;
+	struct enetc_tx_swbd *tx_swbd;
+	union enetc_tx_bd *txbd;
+	struct tso_t tso;
+	__wsum csum, csum2;
+	int count = 0, pos;
+	int err, i;
+
+	/* Check that we have enough BDs for this skb */
+	if (enetc_bd_unused(tx_ring) < tso_count_descs(skb)) {
+		if (net_ratelimit())
+			netdev_err(tx_ring->ndev, "Not enough BDs for TSO!\n");
+		return 0;
+	}
+

On this path, in case the interface is congested, you will drop the packet in the driver,
and the stack will think transmission was successful and will continue to deliver skbs
to the driver. Is this the right thing to do?

Good point. I should have mimicked the non-GSO code path when
congestion occurs and stop the subqueue.

For symmetry I'll also move this check outside of the
enetc_map_tx_tso_buffs() to get the code looking somewhat like this:


	if (skb_is_gso(skb)) {
		if (enetc_bd_unused(tx_ring) < tso_count_descs(skb)) {
			netif_stop_subqueue(ndev, tx_ring->index);
			return NETDEV_TX_BUSY;
		}

		enetc_lock_mdio();
		count = enetc_map_tx_tso_buffs(tx_ring, skb);
		enetc_unlock_mdio();G
	} else {
		if (unlikely(skb_shinfo(skb)->nr_frags > ENETC_MAX_SKB_FRAGS))
			if (unlikely(skb_linearize(skb)))
				goto drop_packet_err;

Ok for handling congestion, the idea is good, but now another issue emerges.
The ENETC_MAX_SKB_FRAGS check is due to a hardware limitation. Enetc cannot
handle more than 15 chained buffer descriptors for transmission (i.e. 13 frags + 1
for the linear part + 1 optional extension BD). This limitation is specified in the
hardware manual now (after I've hit it during development).

On the TSO processing case this is way less likely to happen since the
resulted segment would have to need 15 chained BDs, not the entire skb.
At a maximum, I have seen one frame needing 4-5 BDs: 1 for the header,
1 extention BD in case of VLAN and 1-3 for the data part.

So you should add this check on the TSO processing path too, but adapted to that
case, of course, since skb_linearize() would not work with TSO.

Anyhow, I'll add this check directly in the while loop which creates the
chain of BDs for a frame, since that is the only time that I know how
many BDs are needed for a resulted frame.

Thanks.