Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 26 Aug 2021 03:00:30 +0300
From:   Vladimir Oltean <olteanv@gmail.com>
To:     DENG Qingfang <dqfext@gmail.com>
Cc:     Sean Wang <sean.wang@mediatek.com>,
        Landen Chao <Landen.Chao@mediatek.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC net-next 0/2] DSA slave with customise netdev features
Message-ID: <20210826000030.dhiwv4puqiqyh3re@skbuf>
References: <20210825083832.2425886-1-dqfext@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210825083832.2425886-1-dqfext@gmail.com>
Precedence: bulk

On Wed, Aug 25, 2021 at 04:38:29PM +0800, DENG Qingfang wrote:
> Some taggers, such as tag_dsa.c, combine VLAN tags with DSA tags, which
> currently has a few problems:
>
> 1. Unnecessary reallocation on TX:
>
> A central TX reallocation has been used since commit a3b0b6479700
> ("net: dsa: implement a central TX reallocation procedure"), but for
> VLAN-tagged frames, the actual headroom required for DSA taggers which
> combine with VLAN tags is smaller.

If true, this would be a major failure of the central TX reallocation idea.

However, for this to fail, it would mean that there is a code path in
the network stack that routinely allocates skbs with skb_needed_headroom(skb)
smaller than dev->needed_headroom. That's the only thing that would trigger
reallocs (beside cloned skbs).

The fact that we ask for a dev->needed_headroom that is a bit larger
than what is needed in some cases is not an issue. We should declare the
largest dev->needed_headroom that should cover all cases.

Would you please let me know what is the stack trace when you apply this
patch?

-----------------------------[ cut here ]-----------------------------
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 6d6f1aebf1ca..1924025ac136 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -600,6 +600,10 @@ static int dsa_realloc_skb(struct sk_buff *skb, struct net_device *dev)
 		/* No reallocation needed, yay! */
 		return 0;
 
+	netdev_err(dev, "%s: skb realloc: headroom %u, tailroom %u, cloned %d, needed_headroom %d, needed_tailroom %d\n",
+		  __func__, skb_headroom(skb), skb_tailroom(skb), skb_cloned(skb), needed_headroom, needed_tailroom);
+	WARN_ON(!skb_cloned(skb));
+
 	return pskb_expand_head(skb, needed_headroom, needed_tailroom,
 				GFP_ATOMIC);
 }
-----------------------------[ cut here ]-----------------------------

>
> 2. Repeated memmoves:
>
> If a both Marvell EDSA and VLAN tagged frame is received, the current
> code will move the (DA,SA) twice: the first in dsa_rcv_ll to convert the
> frame to a normal 802.1Q frame, and the second to strip off the 802.1Q
> tag. The similar thing happens on TX.

I don't think a lot of people use 8021q uppers with mv88e6xxx. At least
the error messages I've seen during the few times when I've booted the
Turris Mox would seem to suggest that.

The code that converts DSA 'tagged' frames to VLAN tags originates from
Lennert Buytenhek himself.

>
> For these tags, it is better to handle DSA and VLAN tags at the same time
> in DSA taggers.

No objection there.

>
> This patch set allows taggers to add custom netdev features to DSA
> slaves so they can advertise VLAN offload, and converts tag_mtk to use
> the TX VLAN offload.
>
> DENG Qingfang (2):
>   net: dsa: allow taggers to customise netdev features
>   net: dsa: tag_mtk: handle VLAN tag insertion on TX
>
>  include/net/dsa.h |  2 ++
>  net/dsa/slave.c   |  3 ++-
>  net/dsa/tag_mtk.c | 46 ++++++++++++++++++++++------------------------
>  3 files changed, 26 insertions(+), 25 deletions(-)
>
> --
> 2.25.1
>