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.
