> -----Original Message----- > From: Vladimir Oltean <olteanv@xxxxxxxxx> > Sent: 2021年4月27日 2:40 > To: Richard Cochran <richardcochran@xxxxxxxxx> > Cc: Y.b. Lu <yangbo.lu@xxxxxxx>; netdev@xxxxxxxxxxxxxxx; Vladimir Oltean > <vladimir.oltean@xxxxxxx>; David S . Miller <davem@xxxxxxxxxxxxx>; Jakub > Kicinski <kuba@xxxxxxxxxx>; Jonathan Corbet <corbet@xxxxxxx>; Kurt > Kanzenbach <kurt@xxxxxxxxxxxxx>; Andrew Lunn <andrew@xxxxxxx>; Vivien > Didelot <vivien.didelot@xxxxxxxxx>; Florian Fainelli <f.fainelli@xxxxxxxxx>; > Claudiu Manoil <claudiu.manoil@xxxxxxx>; Alexandre Belloni > <alexandre.belloni@xxxxxxxxxxx>; UNGLinuxDriver@xxxxxxxxxxxxx; > linux-doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > Subject: Re: [net-next, v2, 3/7] net: dsa: free skb->cb usage in core driver > > On Mon, Apr 26, 2021 at 06:38:46AM -0700, Richard Cochran wrote: > > On Mon, Apr 26, 2021 at 05:37:58PM +0800, Yangbo Lu wrote: > > > @@ -624,7 +623,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff > > > *skb, struct net_device *dev) > > > > > > dev_sw_netstats_tx_add(dev, 1, skb->len); > > > > > > - DSA_SKB_CB(skb)->clone = NULL; > > > + memset(skb->cb, 0, 48); > > > > Replace hard coded 48 with sizeof() please. > > You mean just a trivial change like this, right? > > memset(skb->cb, 0, sizeof(skb->cb)); > > And not what I had suggested in v1, which would have looked something like > this: > > -----------------------------[cut here]----------------------------- > diff --git a/include/net/dsa.h b/include/net/dsa.h index > e1a2610a0e06..c75b249e846f 100644 > --- a/include/net/dsa.h > +++ b/include/net/dsa.h > @@ -92,6 +92,7 @@ struct dsa_device_ops { > */ > bool (*filter)(const struct sk_buff *skb, struct net_device *dev); > unsigned int overhead; > + unsigned int skb_cb_size; > const char *name; > enum dsa_tag_protocol proto; > /* Some tagging protocols either mangle or shift the destination MAC diff > --git a/net/dsa/slave.c b/net/dsa/slave.c index 2033d8bac23d..2230596b48b7 > 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -610,11 +610,14 @@ static int dsa_realloc_skb(struct sk_buff *skb, struct > net_device *dev) static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, > struct net_device *dev) { > struct dsa_slave_priv *p = netdev_priv(dev); > + const struct dsa_device_ops *tag_ops; > struct sk_buff *nskb; > > dev_sw_netstats_tx_add(dev, 1, skb->len); > > - memset(skb->cb, 0, 48); > + tag_ops = p->dp->cpu_dp->tag_ops; > + if (tag_ops->skb_cb_size) > + memset(skb->cb, 0, tag_ops->skb_cb_size); > > /* Handle tx timestamp if any */ > dsa_skb_tx_timestamp(p, skb); > diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c index > 50496013cdb7..1b337fa104dc 100644 > --- a/net/dsa/tag_sja1105.c > +++ b/net/dsa/tag_sja1105.c > @@ -365,6 +365,7 @@ static const struct dsa_device_ops > sja1105_netdev_ops = { > .overhead = VLAN_HLEN, > .flow_dissect = sja1105_flow_dissect, > .promisc_on_master = true, > + .skb_cb_size = sizeof(struct sja1105_skb_cb), > }; > > MODULE_LICENSE("GPL v2"); > -----------------------------[cut here]----------------------------- > > I wanted to see how badly impacted would the performance be, so I created > an IPv4 forwarding setup on the NXP LS1021A-TSN board (gianfar + > sja1105): > > #!/bin/bash > > ETH0=swp3 > ETH1=swp2 > > systemctl stop ptp4l # runs a BPF classifier on every packet systemctl stop > phc2sys > > echo 1 > /proc/sys/net/ipv4/ip_forward > ip addr flush $ETH0 && ip addr add 192.168.100.1/24 dev $ETH0 && ip link > set $ETH0 up ip addr flush $ETH1 && ip addr add 192.168.200.1/24 dev $ETH1 > && ip link set $ETH1 up > > arp -s 192.168.100.2 00:04:9f:06:00:09 dev $ETH0 arp -s 192.168.200.2 > 00:04:9f:06:00:0a dev $ETH1 > > ethtool --config-nfc eth2 flow-type ether dst 00:1f:7b:63:01:d4 m ff:ff:ff:ff:ff:ff > action 0 > > and I got the following results on 1 CPU, 64B UDP packets (yes, I know the > baseline results suck, I haven't investigated why that is, but nonetheless, it > should still be relevant as far as comparative results > go): > > Unpatched net-next: > proto 17: 65695 pkt/s > proto 17: 65725 pkt/s > proto 17: 65732 pkt/s > proto 17: 65720 pkt/s > proto 17: 65695 pkt/s > proto 17: 65725 pkt/s > proto 17: 65732 pkt/s > proto 17: 65720 pkt/s > > > After patch 1: > proto 17: 72679 pkt/s > proto 17: 72677 pkt/s > proto 17: 72669 pkt/s > proto 17: 72707 pkt/s > proto 17: 72696 pkt/s > proto 17: 72699 pkt/s > > After patch 2: > proto 17: 72292 pkt/s > proto 17: 72425 pkt/s > proto 17: 72485 pkt/s > proto 17: 72478 pkt/s > > After patch 4 (as 3 doesn't build): > proto 17: 72437 pkt/s > proto 17: 72510 pkt/s > proto 17: 72479 pkt/s > proto 17: 72499 pkt/s > proto 17: 72497 pkt/s > proto 17: 72427 pkt/s > > With the change I pasted above: > proto 17: 71891 pkt/s > proto 17: 71810 pkt/s > proto 17: 71850 pkt/s > proto 17: 71826 pkt/s > proto 17: 71798 pkt/s > proto 17: 71786 pkt/s > proto 17: 71814 pkt/s > proto 17: 71814 pkt/s > proto 17: 72010 pkt/s > > So basically, not only are we better off just zero-initializing the complete > skb->cb instead of looking up the tagger's skb_cb_size, but zero-initializing the > skb->cb isn't even all that bad. Yangbo's change is an overall win anyway, all > things considered. So just change the memset as Richard suggested, make sure > all patches compile, and we should be good to go. Ah... I had thought 48bytes memset was acceptable for now by you. I actually didn't consider the performance affected by this. I just did this because I believed the direction was right:) That's really good testing. Thank you very much, Vladimir. With the data, we can feel free to use the changes.