LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
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
Date: Thu, 26 Aug 2021 03:00:30 +0300	[thread overview]
Message-ID: <20210826000030.dhiwv4puqiqyh3re@skbuf> (raw)
In-Reply-To: <20210825083832.2425886-1-dqfext@gmail.com>

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
>

      parent reply	other threads:[~2021-08-26  0:00 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-25  8:38 DENG Qingfang
2021-08-25  8:38 ` [RFC net-next 1/2] net: dsa: allow taggers to " DENG Qingfang
2021-08-26  0:01   ` Vladimir Oltean
2021-08-26 11:28   ` Florian Fainelli
2021-08-25  8:38 ` [RFC net-next 2/2] net: dsa: tag_mtk: handle VLAN tag insertion on TX DENG Qingfang
2021-08-26  0:03   ` Vladimir Oltean
2021-08-26  5:29     ` DENG Qingfang
2021-08-26 11:37       ` Florian Fainelli
2021-08-26 14:13       ` Vladimir Oltean
2021-08-26 15:27         ` DENG Qingfang
2021-08-26  0:00 ` Vladimir Oltean [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210826000030.dhiwv4puqiqyh3re@skbuf \
    --to=olteanv@gmail.com \
    --cc=Landen.Chao@mediatek.com \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=dqfext@gmail.com \
    --cc=f.fainelli@gmail.com \
    --cc=kuba@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=sean.wang@mediatek.com \
    --cc=vivien.didelot@gmail.com \
    --subject='Re: [RFC net-next 0/2] DSA slave with customise netdev features' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).