[PATCH net-next 0/5] Simplifications for Broadcom and Realtek DSA taggers

[PATCH net-next 0/5] Simplifications for Broadcom and Realtek DSA taggers

[Vladimir Oltean]

This series contains the removal of some "if" conditions from the TX hot
path in tag_brcm and tag_rtl4_a. This is made possible by the fact that
the DSA core has previously checked that memory allocations are not
necessary, so there is nothing that can fail.

Vladimir Oltean (5):
  net: dsa: tag_rtl4_a: __skb_put_padto() can never fail
  net: dsa: tag_brcm: do not account for tag length twice when padding
  net: dsa: tag_brcm: __skb_put_padto() can never fail
  net: dsa: tag_brcm: eliminate conditional based on offset from
    brcm_tag_xmit_ll
  net: dsa: tag_brcm: use dsa_etype_header_pos_tx for legacy tag

 net/dsa/tag_brcm.c   | 51 +++++++++++++++++++-------------------------
 net/dsa/tag_rtl4_a.c |  4 +---
 2 files changed, 23 insertions(+), 32 deletions(-)

-- 
2.25.1

[PATCH net-next 1/5] net: dsa: tag_rtl4_a: __skb_put_padto() can never fail

[Vladimir Oltean]

One of the purposes of the central dsa_realloc_skb() is to ensure that
the skb has a geometry which does not need to be adjusted by tagging
protocol drivers for most cases. This includes making sure that the skb
is not cloned (otherwise pskb_expand_head() is called).

So there is no reason why __skb_put_padto() is going to return an error,
otherwise it would have returned the error in dsa_realloc_skb().

Use the simple eth_skb_pad() which passes "true" to "free_on_error"
(which does not matter) and save a conditional in the TX data path.
With this, also remove the uninformative comment.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/tag_rtl4_a.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/dsa/tag_rtl4_a.c b/net/dsa/tag_rtl4_a.c
index 6d928ee3ef7a..e71d011ce4cc 100644
--- a/net/dsa/tag_rtl4_a.c
+++ b/net/dsa/tag_rtl4_a.c
@@ -39,9 +39,7 @@ static struct sk_buff *rtl4a_tag_xmit(struct sk_buff *skb,
 	u8 *tag;
 	u16 out;
 
-	/* Pad out to at least 60 bytes */
-	if (unlikely(__skb_put_padto(skb, ETH_ZLEN, false)))
-		return NULL;
+	eth_skb_pad(skb);
 
 	netdev_dbg(dev, "add realtek tag to package to port %d\n",
 		   dp->index);
-- 
2.25.1

[PATCH net-next 2/5] net: dsa: tag_brcm: do not account for tag length twice when padding

[Vladimir Oltean]

brcm_tag_xmit_ll() explains in the comment that the skb length
(sans FCS) must be at least 60 octets before adding the tag, and 64
octets after adding the tag.

But it pads the skb to 64 octets, then pushes a Broadcom tag which
increases the length to 68. With FCS, this length would be 72.

According to the comment, this is not what the minimum length is,
so just pad small packets up to 60 octets to bring them in sync.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/tag_brcm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index 96dbb8ee2fee..6cc4c4859a41 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -93,7 +93,7 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
 	 *
 	 * Let dsa_slave_xmit() free the SKB
 	 */
-	if (__skb_put_padto(skb, ETH_ZLEN + BRCM_TAG_LEN, false))
+	if (__skb_put_padto(skb, ETH_ZLEN, false))
 		return NULL;
 
 	skb_push(skb, BRCM_TAG_LEN);
@@ -223,7 +223,7 @@ static struct sk_buff *brcm_leg_tag_xmit(struct sk_buff *skb,
 	 *
 	 * Let dsa_slave_xmit() free the SKB
 	 */
-	if (__skb_put_padto(skb, ETH_ZLEN + BRCM_LEG_TAG_LEN, false))
+	if (__skb_put_padto(skb, ETH_ZLEN, false))
 		return NULL;
 
 	skb_push(skb, BRCM_LEG_TAG_LEN);
-- 
2.25.1

[PATCH net-next 3/5] net: dsa: tag_brcm: __skb_put_padto() can never fail

[Vladimir Oltean]

One of the purposes of the central dsa_realloc_skb() is to ensure that
the skb has a geometry which does not need to be adjusted by tagging
protocol drivers for most cases. This includes making sure that the skb
is not cloned (otherwise pskb_expand_head() is called).

So there is no reason why __skb_put_padto() is going to return an error,
otherwise it would have returned the error in dsa_realloc_skb().

Use the simple eth_skb_pad() which passes "true" to "free_on_error"
(which does not matter) and save a conditional in the TX data path.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/tag_brcm.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index 6cc4c4859a41..c2610d34386d 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -90,11 +90,8 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
 	 * need to make sure that packets are at least 68 bytes
 	 * (including FCS and tag) because the length verification is done after
 	 * the Broadcom tag is stripped off the ingress packet.
-	 *
-	 * Let dsa_slave_xmit() free the SKB
 	 */
-	if (__skb_put_padto(skb, ETH_ZLEN, false))
-		return NULL;
+	eth_skb_pad(skb);
 
 	skb_push(skb, BRCM_TAG_LEN);
 
@@ -220,11 +217,8 @@ static struct sk_buff *brcm_leg_tag_xmit(struct sk_buff *skb,
 	 * need to make sure that packets are at least 70 bytes
 	 * (including FCS and tag) because the length verification is done after
 	 * the Broadcom tag is stripped off the ingress packet.
-	 *
-	 * Let dsa_slave_xmit() free the SKB
 	 */
-	if (__skb_put_padto(skb, ETH_ZLEN, false))
-		return NULL;
+	eth_skb_pad(skb);
 
 	skb_push(skb, BRCM_LEG_TAG_LEN);
 
-- 
2.25.1

[PATCH net-next 4/5] net: dsa: tag_brcm: eliminate conditional based on offset from brcm_tag_xmit_ll

[Vladimir Oltean]

Simplify the low-level, common tag insertion procedure by handling the
differences between prepended headers and Ethertype headers outside it.
With this, the prepended header no longer has the check whether to call
dsa_alloc_etype_header() in its code path.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/tag_brcm.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index c2610d34386d..ac9cfd418948 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -78,27 +78,10 @@
 
 static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
 					struct net_device *dev,
-					unsigned int offset)
+					u8 *brcm_tag)
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	u16 queue = skb_get_queue_mapping(skb);
-	u8 *brcm_tag;
-
-	/* The Ethernet switch we are interfaced with needs packets to be at
-	 * least 64 bytes (including FCS) otherwise they will be discarded when
-	 * they enter the switch port logic. When Broadcom tags are enabled, we
-	 * need to make sure that packets are at least 68 bytes
-	 * (including FCS and tag) because the length verification is done after
-	 * the Broadcom tag is stripped off the ingress packet.
-	 */
-	eth_skb_pad(skb);
-
-	skb_push(skb, BRCM_TAG_LEN);
-
-	if (offset)
-		dsa_alloc_etype_header(skb, BRCM_TAG_LEN);
-
-	brcm_tag = skb->data + offset;
 
 	/* Set the ingress opcode, traffic class, tag enforcment is
 	 * deprecated
@@ -173,10 +156,21 @@ static struct sk_buff *brcm_tag_rcv_ll(struct sk_buff *skb,
 static struct sk_buff *brcm_tag_xmit(struct sk_buff *skb,
 				     struct net_device *dev)
 {
+	/* The Ethernet switch we are interfaced with needs packets to be at
+	 * least 64 bytes (including FCS) otherwise they will be discarded when
+	 * they enter the switch port logic. When Broadcom tags are enabled, we
+	 * need to make sure that packets are at least 68 bytes
+	 * (including FCS and tag) because the length verification is done after
+	 * the Broadcom tag is stripped off the ingress packet.
+	 */
+	eth_skb_pad(skb);
+
 	/* Build the tag after the MAC Source Address */
-	return brcm_tag_xmit_ll(skb, dev, 2 * ETH_ALEN);
-}
+	skb_push(skb, BRCM_TAG_LEN);
+	dsa_alloc_etype_header(skb, BRCM_TAG_LEN);
 
+	return brcm_tag_xmit_ll(skb, dev, dsa_etype_header_pos_tx(skb));
+}
 
 static struct sk_buff *brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev)
 {
@@ -282,8 +276,13 @@ MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_BRCM_LEGACY);
 static struct sk_buff *brcm_tag_xmit_prepend(struct sk_buff *skb,
 					     struct net_device *dev)
 {
+	/* See the padding comment in brcm_tag_xmit() */
+	eth_skb_pad(skb);
+
 	/* tag is prepended to the packet */
-	return brcm_tag_xmit_ll(skb, dev, 0);
+	skb_push(skb, BRCM_TAG_LEN);
+
+	return brcm_tag_xmit_ll(skb, dev, skb->data);
 }
 
 static struct sk_buff *brcm_tag_rcv_prepend(struct sk_buff *skb,
-- 
2.25.1

[PATCH net-next 5/5] net: dsa: tag_brcm: use dsa_etype_header_pos_tx for legacy tag

[Vladimir Oltean]

Use the dedicated DSA helper for identifying the header position for the
legacy Broadcom tagger.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/tag_brcm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index ac9cfd418948..9c33eea96007 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -218,7 +218,7 @@ static struct sk_buff *brcm_leg_tag_xmit(struct sk_buff *skb,
 
 	dsa_alloc_etype_header(skb, BRCM_LEG_TAG_LEN);
 
-	brcm_tag = skb->data + 2 * ETH_ALEN;
+	brcm_tag = dsa_etype_header_pos_tx(skb);
 
 	/* Broadcom tag type */
 	brcm_tag[0] = BRCM_LEG_TYPE_HI;
-- 
2.25.1

Re: [PATCH net-next 0/5] Simplifications for Broadcom and Realtek DSA taggers

[Vladimir Oltean]

On Wed, May 11, 2022 at 06:14:26PM +0300, Vladimir Oltean wrote:
> This series contains the removal of some "if" conditions from the TX hot
> path in tag_brcm and tag_rtl4_a. This is made possible by the fact that
> the DSA core has previously checked that memory allocations are not
> necessary, so there is nothing that can fail.
> 
> Vladimir Oltean (5):
>   net: dsa: tag_rtl4_a: __skb_put_padto() can never fail
>   net: dsa: tag_brcm: do not account for tag length twice when padding
>   net: dsa: tag_brcm: __skb_put_padto() can never fail
>   net: dsa: tag_brcm: eliminate conditional based on offset from
>     brcm_tag_xmit_ll
>   net: dsa: tag_brcm: use dsa_etype_header_pos_tx for legacy tag
> 
>  net/dsa/tag_brcm.c   | 51 +++++++++++++++++++-------------------------
>  net/dsa/tag_rtl4_a.c |  4 +---
>  2 files changed, 23 insertions(+), 32 deletions(-)
> 
> -- 
> 2.25.1
> 

Oh, wait, the premise behind patches 1 and 3 is invalid.
dsa_realloc_skb() only ensures there are ETH_ZLEN bytes of space in the
skb for tail taggers.

	if (unlikely(needed_tailroom && skb->len < ETH_ZLEN))
		needed_tailroom += ETH_ZLEN - skb->len;

Please treat the patch set as RFC, if we have agreement on the overall
idea I can update dsa_realloc_skb() by removing "needed_tailroom && "
from the check so the guarantee applies to all taggers.

Re: [PATCH net-next 0/5] Simplifications for Broadcom and Realtek DSA taggers

[Florian Fainelli]

On 5/11/2022 8:14 AM, Vladimir Oltean wrote:
> This series contains the removal of some "if" conditions from the TX hot
> path in tag_brcm and tag_rtl4_a. This is made possible by the fact that
> the DSA core has previously checked that memory allocations are not
> necessary, so there is nothing that can fail.

I would like to test those before they get merged, please give me a day 
to test those, thank you!
-- 
Florian

Re: [PATCH net-next 0/5] Simplifications for Broadcom and Realtek DSA taggers

[Vladimir Oltean]

On Wed, May 11, 2022 at 08:23:08AM -0700, Florian Fainelli wrote:
> On 5/11/2022 8:14 AM, Vladimir Oltean wrote:
> > This series contains the removal of some "if" conditions from the TX hot
> > path in tag_brcm and tag_rtl4_a. This is made possible by the fact that
> > the DSA core has previously checked that memory allocations are not
> > necessary, so there is nothing that can fail.
> 
> I would like to test those before they get merged, please give me a day to
> test those, thank you!
> -- 
> Florian

Sure.

RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC RFC

Re: [PATCH net-next 1/5] net: dsa: tag_rtl4_a: __skb_put_padto() can never fail

[Linus Walleij]

On Wed, May 11, 2022 at 5:14 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> One of the purposes of the central dsa_realloc_skb() is to ensure that
> the skb has a geometry which does not need to be adjusted by tagging
> protocol drivers for most cases. This includes making sure that the skb
> is not cloned (otherwise pskb_expand_head() is called).
>
> So there is no reason why __skb_put_padto() is going to return an error,
> otherwise it would have returned the error in dsa_realloc_skb().
>
> Use the simple eth_skb_pad() which passes "true" to "free_on_error"
> (which does not matter) and save a conditional in the TX data path.
> With this, also remove the uninformative comment.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

This looks correct to me!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij