Re: [PATCH net-next v43 6/8] eea: implement packet transmit logic

[Xuan Zhuo <xuanzhuo@linux.alibaba.com>]

From sashiko:


commit cffb539ce8b8ad9935febc105007a1405d95169b
Author: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
eea: implement packet transmit logic
This commit fills in the previously stubbed eea_tx_xmit() and
eea_poll_tx() with the full TX path: building descriptors from skbs,
handling DMA mapping/unmapping, GSO and checksum offload setup, and
completion processing.
> diff --git a/drivers/net/ethernet/alibaba/eea/eea_tx.c b/drivers/net/ethernet/alibaba/eea/eea_tx.c
> index 5a8babfce5de..8b7fc75ead8a 100644
> --- a/drivers/net/ethernet/alibaba/eea/eea_tx.c
> +++ b/drivers/net/ethernet/alibaba/eea/eea_tx.c
[ ... ]
> +static int eea_fill_desc_from_skb(const struct sk_buff *skb,
> +				  struct eea_tx_desc *desc)
> +{
> +	if (skb_is_gso(skb)) {
> +		struct skb_shared_info *sinfo = skb_shinfo(skb);
> +
> +		desc->gso_size = cpu_to_le16(sinfo->gso_size);
> +		if (sinfo->gso_type & SKB_GSO_TCPV4)
> +			desc->gso_type = EEA_TX_GSO_TCPV4;
> +
> +		else if (sinfo->gso_type & SKB_GSO_TCPV6)
> +			desc->gso_type = EEA_TX_GSO_TCPV6;
> +
> +		else if (sinfo->gso_type & SKB_GSO_UDP_L4)
> +			desc->gso_type = EEA_TX_GSO_UDP_L4;
> +
> +		else
> +			return -EINVAL;
> +
> +		if (sinfo->gso_type & SKB_GSO_TCP_ECN)
> +			desc->gso_type |= EEA_TX_GSO_ECN;
> +	} else {
> +		desc->gso_type = EEA_TX_GSO_NONE;
> +	}
> +
> +	if (skb->ip_summed == CHECKSUM_PARTIAL) {
> +		desc->csum_start = cpu_to_le16(skb_checksum_start_offset(skb));
> +		desc->csum_offset = cpu_to_le16(skb->csum_offset);
> +	}
> +
> +	return 0;
> +}
Can stale fields in a reused descriptor slot be sent to the device?
The SQ ring slots are reused as the ring wraps. Several eea_tx_desc
fields are only conditionally written:
  - csum_start and csum_offset are only set when
    skb->ip_summed == CHECKSUM_PARTIAL
  - gso_size is only set when skb_is_gso(skb)
  - gso_type is set here for the head descriptor, but
    eea_tx_add_skb_frag() and the >USHRT_MAX split path in
    eea_tx_desc_fill() never write gso_type, gso_size, csum_start
    or csum_offset on continuation descriptors
  - reserved1, reserved2 and reserved3 are never written
After the first ring wrap each reused slot will carry whatever values
the previous occupant wrote. Does correctness here rely on the device
gating each optional field on EEA_DESC_F_DO_CSUM and on the
"more descriptors" flag, and ignoring all reserved bits?
If so, would it be worth either zeroing the descriptor at fill time
(for example a memset before populating it) or adding a comment that
documents the hardware contract being relied on?
What is the silent drop signal when an unsupported gso_type appears?
When skb_is_gso(skb) is true but the type is none of TCPV4, TCPV6 or
UDP_L4, this returns -EINVAL, which propagates up through
eea_tx_post_skb() into err_cancel and ultimately to
dev_kfree_skb_any() in eea_tx_xmit(). There is no kfree_skb_reason(),
no per-queue counter and no log message.
Today the four supported types match what is advertised in
hw_features and features, so this branch should be unreachable. If a
future feature flag is advertised without updating this switch (for
example a tunnel GSO type), would the resulting drops be observable
from userspace at all?
Would kfree_skb_reason() with a specific drop reason, or restructuring
so the unsupported case is structurally unreachable, make this easier
to debug?
> +static int eea_tx_check_free_num(struct eea_net_tx *tx,
> +				 struct netdev_queue *txq)
> +{
> +	int n;
> +
> +	/* MAX_SKB_FRAGS + 1: Covers the skb linear head and all paged fragments
> +	 * 1: Extra slot for a head or fragment that exceeds 64KB.
> +	 */
> +	n = MAX_SKB_FRAGS + 2;
> +	return netif_txq_maybe_stop(txq, tx->ering->num_free, n, n);
>  }
>
>  netdev_tx_t eea_tx_xmit(struct sk_buff *skb, struct net_device *netdev)
>  {
> -	/* Empty function; will be implemented in a subsequent commit. */
> -	dev_kfree_skb_any(skb);
> +	struct eea_net *enet = netdev_priv(netdev);
> +	int qnum = skb_get_queue_mapping(skb);
> +	struct eea_net_tx *tx = &enet->tx[qnum];
> +	struct netdev_queue *txq;
> +	int err, enable;
> +
> +	txq = netdev_get_tx_queue(netdev, qnum);
> +
> +	enable = eea_tx_check_free_num(tx, txq);
> +	if (!enable)
> +		return NETDEV_TX_BUSY;
> +
> +	err = eea_tx_post_skb(tx, skb);
> +	if (unlikely(err)) {
> +		dev_kfree_skb_any(skb);
> +	} else {
> +		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
> +			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +		skb_tx_timestamp(skb);
> +	}
> +
> +	/* NETDEV_TX_BUSY is expensive. So stop advancing the TX queue. */
> +	eea_tx_check_free_num(tx, txq);
> +
> +	if (!netdev_xmit_more() || netif_xmit_stopped(txq))
> +		eea_tx_kick(tx);
> +
>  	return NETDEV_TX_OK;
>  }
Is BQL intended to be wired up here?
eea_tx_check_free_num() uses netif_txq_maybe_stop(), which is the
BQL-aware helper, but the xmit path never calls
netdev_tx_sent_queue() and the completion path in eea_clean_tx() /
eea_poll_tx() never calls netdev_tx_completed_queue(). Without the
paired byte updates, BQL has no view of in-flight bytes.
Also, the stop and start thresholds passed to netif_txq_maybe_stop()
are both MAX_SKB_FRAGS + 2, so there is no hysteresis between
queue-stop and queue-wake. eea_poll_tx() uses the same threshold for
its wake check:
	if (netif_tx_queue_stopped(txq) &&
	    tx->ering->num_free >= MAX_SKB_FRAGS + 2)
		netif_tx_wake_queue(txq);
Was a stop/start gap (e.g. start at 2 * stop) considered to dampen
wake/stop oscillation under sustained pressure?