Re: [PATCH net-next v2 1/3] dsa: add support for Arrow XRS700x tag trailer

On Thu, Nov 26, 2020 at 03:01:26PM +0100, Andrew Lunn wrote:

On Thu, Nov 26, 2020 at 03:50:04PM +0200, Vladimir Oltean wrote:

quoted

On Wed, Nov 25, 2020 at 09:34:29PM +0100, Andrew Lunn wrote:

quoted

quoted

+static struct sk_buff *xrs700x_rcv(struct sk_buff *skb, struct net_device *dev,
+				   struct packet_type *pt)
+{
+	int source_port;
+	u8 *trailer;
+
+	if (skb_linearize(skb))
+		return NULL;

Something for Vladimir:

Could this linearise be moved into the core, depending on the
tail_tag?

Honestly I believe that the skb_linearize is not needed at all.

Humm

I'm assuming this is here in case the frame is in fragments, and the
trailer could be spread over two fragments? If so, you cannot access
the trailer using straight pointers. Linearize should copy it into one
buffer.

For the normal case of a 1500 byte frame, i doubt we have hardware
which uses multiple scatter/gather buffers. But for jumbo frames?

In this particular case, I don't think that one byte can be in multiple
fragments at once, unless it is a quantum byte. So skb_linearize should
still be removed.

Are you saying that we should do something like this? I would need to
performance-test it:

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index a1b1dc8a4d87..4c2065bec8d5 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c

@@ -212,6 +212,13 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
 	if (!skb)
 		return 0;
 
+	if (cpu_dp->tag_ops->tail_tag && cpu_dp->tag_ops->overhead > 1) {
+		if (unlikely(skb_linearize(skb))) {
+			dev_kfree_skb_any(skb);
+			return 0;
+		}
+	}
+
 	nskb = cpu_dp->rcv(skb, dev, pt);
 	if (!nskb) {
 		kfree_skb(skb);

Also, since we are now finally touching the cacheline with tag_ops,
maybe we could remove the copy of rcv() from struct dsa_port? Although
there is a chance that this might destabilize things and could need a
bit of tweaking to get right.

quoted

quoted

quoted

+	if (pskb_trim_rcsum(skb, skb->len - 1))
+		return NULL;

And the overhead is also in dsa_devlink_ops, so maybe this can be
moved as well?

Sorry, I don't understand this comment.

I'm meaning, could that also be moved into the core? We seem to have
the needed information to do it in the core.

Ok, I got confused by the devlink reference.
Currently the tag is always stripped by the tagger driver, because there
are 3 cases:
- tag is before Ethernet MAC DA
- tag is before Ethernet Type
- tag is before FCS
We do not have a way to distinguish between cases 1 and 2 such that DSA
could strip the tag in all cases and provide a uniform interface to all
types of taggers.