| Message ID | 20201016200226.23994-2-ceggers@arri.de |
|---|---|
| State | New |
| Headers | show |
| Series | net: dsa: move skb reallocation to dsa_slave_xmit | expand |
On Fri, Oct 16, 2020 at 10:02:24PM +0200, Christian Eggers wrote: > Ensure that the skb is not cloned and has enough tail room for the tail > tag. This code will be removed from the drivers in the next commits. > > Signed-off-by: Christian Eggers <ceggers@arri.de> > --- Does 1588 work for you using this change, or you haven't finished implementing it yet? If you haven't, I would suggest finishing that part first. The post-reallocation skb looks nothing like the one before. Before: skb len=68 headroom=2 headlen=68 tailroom=186 mac=(2,14) net=(16,-1) trans=-1 shinfo(txflags=1 nr_frags=0 gso(size=0 type=0 segs=0)) csum(0x0 ip_summed=0 complete_sw=0 valid=0 level=0) hash(0x9d6927ec sw=1 l4=0) proto=0x88f7 pkttype=0 iif=0 dev name=swp2 feat=0x0x0002000000005020 sk family=17 type=3 proto=0 After: skb len=68 headroom=2 headlen=68 tailroom=186 mac=(2,16) net=(18,-17) trans=1 shinfo(txflags=0 nr_frags=0 gso(size=0 type=0 segs=0)) csum(0x0 ip_summed=0 complete_sw=0 valid=0 level=0) hash(0x0 sw=0 l4=0) proto=0x0000 pkttype=0 iif=0 Notice how you've changed shinfo(txflags), among other things. Which proves that you can't just copy&paste whatever you found in tag_trailer.c. I am not yet sure whether there is any helper that can be used instead of this crazy open-coding. Right now, not having tested anything yet, my candidates of choice would be pskb_expand_head or __pskb_pull_tail. You should probably also try to cater here for the potential reallocation done in the skb_cow_head() of non-tail taggers. Which would lean the balance towards pskb_expand_head(), I believe. Also, if the result is going to be longer than ~20 lines of code, I strongly suggest moving the reallocation to a separate function so you don't clutter dsa_slave_xmit. Also, please don't redeclare struct sk_buff *nskb, you don't need to.
On 10/16/2020 1:02 PM, Christian Eggers wrote: > Ensure that the skb is not cloned and has enough tail room for the tail > tag. This code will be removed from the drivers in the next commits. > > Signed-off-by: Christian Eggers <ceggers@arri.de> > --- [snip] > + /* We have to pad he packet to the minimum Ethernet frame size, > + * if necessary, before adding the trailer (tail tagging only). > + */ > + padlen = (skb->len >= ETH_ZLEN) ? 0 : ETH_ZLEN - skb->len; > + > + /* To keep the slave's xmit() methods simple, don't pass cloned skbs to > + * them. Additionally ensure, that suitable room for tail tagging is > + * available. > + */ > + if (skb_cloned(skb) || > + (p->tail_tag && skb_tailroom(skb) < (padlen + p->overhead))) { > + struct sk_buff *nskb; > + > + nskb = alloc_skb(NET_IP_ALIGN + skb->len + > + padlen + p->overhead, GFP_ATOMIC); > + if (!nskb) { > + kfree_skb(skb); > + return NETDEV_TX_OK; > + } > + skb_reserve(nskb, NET_IP_ALIGN); > + > + skb_reset_mac_header(nskb); > + skb_set_network_header(nskb, > + skb_network_header(skb) - skb->head); > + skb_set_transport_header(nskb, > + skb_transport_header(skb) - skb->head); > + skb_copy_and_csum_dev(skb, skb_put(nskb, skb->len)); > + consume_skb(skb); > + > + if (padlen) > + skb_put_zero(nskb, padlen); > + > + skb = nskb; > + } Given the low number of tail taggers, maybe this should be a helper function that is used by them where applicable? If nothing else you may want to sprinkle unlikely() conditions to sort of hing the processor that these are unlikely conditions. -- Florian
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index 12998bf04e55..975001c625b1 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -77,6 +77,9 @@ struct dsa_slave_priv { /* Copy of CPU port xmit for faster access in slave transmit hot path */ struct sk_buff * (*xmit)(struct sk_buff *skb, struct net_device *dev); + /* same for tail_tag and overhead */ + bool tail_tag; + unsigned int overhead; struct pcpu_sw_netstats __percpu *stats64; diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 3bc5ca40c9fb..49a19a3b0736 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -553,6 +553,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev) struct dsa_slave_priv *p = netdev_priv(dev); struct pcpu_sw_netstats *s; struct sk_buff *nskb; + int padlen; s = this_cpu_ptr(p->stats64); u64_stats_update_begin(&s->syncp); @@ -567,6 +568,41 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev) */ dsa_skb_tx_timestamp(p, skb); + /* We have to pad he packet to the minimum Ethernet frame size, + * if necessary, before adding the trailer (tail tagging only). + */ + padlen = (skb->len >= ETH_ZLEN) ? 0 : ETH_ZLEN - skb->len; + + /* To keep the slave's xmit() methods simple, don't pass cloned skbs to + * them. Additionally ensure, that suitable room for tail tagging is + * available. + */ + if (skb_cloned(skb) || + (p->tail_tag && skb_tailroom(skb) < (padlen + p->overhead))) { + struct sk_buff *nskb; + + nskb = alloc_skb(NET_IP_ALIGN + skb->len + + padlen + p->overhead, GFP_ATOMIC); + if (!nskb) { + kfree_skb(skb); + return NETDEV_TX_OK; + } + skb_reserve(nskb, NET_IP_ALIGN); + + skb_reset_mac_header(nskb); + skb_set_network_header(nskb, + skb_network_header(skb) - skb->head); + skb_set_transport_header(nskb, + skb_transport_header(skb) - skb->head); + skb_copy_and_csum_dev(skb, skb_put(nskb, skb->len)); + consume_skb(skb); + + if (padlen) + skb_put_zero(nskb, padlen); + + skb = nskb; + } + /* Transmit function may have to reallocate the original SKB, * in which case it must have freed it. Only free it here on error. */ @@ -1814,6 +1850,8 @@ int dsa_slave_create(struct dsa_port *port) p->dp = port; INIT_LIST_HEAD(&p->mall_tc_list); p->xmit = cpu_dp->tag_ops->xmit; + p->tail_tag = cpu_dp->tag_ops->tail_tag; + p->overhead = cpu_dp->tag_ops->overhead; port->slave = slave_dev; rtnl_lock();
Ensure that the skb is not cloned and has enough tail room for the tail tag. This code will be removed from the drivers in the next commits. Signed-off-by: Christian Eggers <ceggers@arri.de> --- net/dsa/dsa_priv.h | 3 +++ net/dsa/slave.c | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+)