Re: [RFC PATCH 02/13] net: dsa: implement a central TX reallocation procedure

[Florian Fainelli <f.fainelli@gmail.com>]

On 10/17/2020 3:01 PM, Vladimir Oltean wrote:
> On Sun, Oct 18, 2020 at 12:36:00AM +0300, Vladimir Oltean wrote:
>> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>> index d4326940233c..790f5c8deb13 100644
>> --- a/net/dsa/slave.c
>> +++ b/net/dsa/slave.c
>> @@ -548,6 +548,36 @@ netdev_tx_t dsa_enqueue_skb(struct sk_buff *skb, struct net_device *dev)
>>   }
>>   EXPORT_SYMBOL_GPL(dsa_enqueue_skb);
>>   
>> +static int dsa_realloc_skb(struct sk_buff *skb, struct net_device *dev)
> 
> I forgot to actually pad the skb here, if it's a tail tagger, silly me.
> The following changes should do the trick.
> 
>> +{
>> +	struct net_device *master = dsa_slave_to_master(dev);
> 
> The addition of master->needed_headroom and master->needed_tailroom used
> to be here, that's why this unused variable is here.
> 
>> +	struct dsa_slave_priv *p = netdev_priv(dev);
>> +	struct dsa_slave_stats *e;
>> +	int headroom, tailroom;
> 	int padlen = 0, err;
>> +
>> +	headroom = dev->needed_headroom;
>> +	tailroom = dev->needed_tailroom;
>> +	/* For tail taggers, we need to pad short frames ourselves, to ensure
>> +	 * that the tail tag does not fail at its role of being at the end of
>> +	 * the packet, once the master interface pads the frame.
>> +	 */
>> +	if (unlikely(tailroom && skb->len < ETH_ZLEN))
>> +		tailroom += ETH_ZLEN - skb->len;
> 		~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 		padlen = ETH_ZLEN - skb->len;
> 	tailroom += padlen;
>> +
>> +	if (likely(skb_headroom(skb) >= headroom &&
>> +		   skb_tailroom(skb) >= tailroom) &&
>> +		   !skb_cloned(skb))
>> +		/* No reallocation needed, yay! */
>> +		return 0;
>> +
>> +	e = this_cpu_ptr(p->extra_stats);
>> +	u64_stats_update_begin(&e->syncp);
>> +	e->tx_reallocs++;
>> +	u64_stats_update_end(&e->syncp);
>> +
>> +	return pskb_expand_head(skb, headroom, tailroom, GFP_ATOMIC);
> 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 	err = pskb_expand_head(skb, headroom, tailroom, GFP_ATOMIC);
> 	if (err < 0 || !padlen)
> 		return err;
> 
> 	return __skb_put_padto(skb, padlen, false);
>> +}
>> +
>>   static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
>>   {
>>   	struct dsa_slave_priv *p = netdev_priv(dev);
>> @@ -567,6 +597,11 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
>>   	 */
>>   	dsa_skb_tx_timestamp(p, skb);
>>   
>> +	if (dsa_realloc_skb(skb, dev)) {
>> +		kfree_skb(skb);
>> +		return NETDEV_TX_OK;
>> +	}
>> +
>>   	/* Transmit function may have to reallocate the original SKB,
>>   	 * in which case it must have freed it. Only free it here on error.
>>   	 */
>> @@ -1802,6 +1837,14 @@ int dsa_slave_create(struct dsa_port *port)
>>   	slave_dev->netdev_ops = &dsa_slave_netdev_ops;
>>   	if (ds->ops->port_max_mtu)
>>   		slave_dev->max_mtu = ds->ops->port_max_mtu(ds, port->index);
>> +	/* Try to save one extra realloc later in the TX path (in the master)
>> +	 * by also inheriting the master's needed headroom and tailroom.
>> +	 * The 8021q driver also does this.
>> +	 */
> 
> Also, this comment is bogus given the current code. It should be removed
> from here, and...
> 
>> +	if (cpu_dp->tag_ops->tail_tag)
>> +		slave_dev->needed_tailroom = cpu_dp->tag_ops->overhead;
>> +	else
>> +		slave_dev->needed_headroom = cpu_dp->tag_ops->overhead;
> ...put here, along with:
> 	slave_dev->needed_headroom += master->needed_headroom;
> 	slave_dev->needed_tailroom += master->needed_tailroom;

Not positive you need that because you may be account for more head or 
tail room than necessary.

For instance with tag_brcm.c and systemport.c we need 4 bytes of head 
room for the Broadcom tag and an additional 8 bytes for pushing the 
transmit status block descriptor in front of the Ethernet frame about to 
be transmitted. These additional 8 bytes are a requirement of the DSA 
master here and exist regardless of DSA being used, but we should not be 
propagating them to the DSA slave.
-- 
Florian