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

[Vladimir Oltean <olteanv@gmail.com>]

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:
> > On Wed, Nov 25, 2020 at 09:34:29PM +0100, Andrew Lunn wrote:
> > > > +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.

> > > > +	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.