| Message ID | 20200926173108.1230014-7-vladimir.oltean@nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Generic adjustment for flow dissector in DSA | expand |
On Sat, Sep 26, 2020 at 08:30:58PM +0300, Vladimir Oltean wrote: > For all DSA formats that don't use tail tags, it looks like behind the > obscure number crunching they're all doing the same thing: locating the > real EtherType behind the DSA tag. Nonetheless, this is not immediately > obvious, so create a generic helper for those DSA taggers that put the > header before the EtherType. This is interesting, and opens up the possibility of an optimisation on the hot path. net/core/flow_dissector.c: bool __skb_flow_dissect(const struct net *net, const struct sk_buff *skb, struct flow_dissector *flow_dissector, void *target_container, void *data, __be16 proto, int nhoff, int hlen, unsigned int flags) { ... #if IS_ENABLED(CONFIG_NET_DSA) if (unlikely(skb->dev && netdev_uses_dsa(skb->dev) && proto == htons(ETH_P_XDSA))) { const struct dsa_device_ops *ops; int offset = 0; ops = skb->dev->dsa_ptr->tag_ops; if (ops->flow_dissect && !ops->flow_dissect(skb, &proto, &offset)) { hlen -= offset; nhoff += offset; } } #endif Given spectra and meltdown etc, jumping through a pointer is expensive and we try to avoid it on the hot path. Given most of the taggers are going to use the generic version, maybe add a test here, is ops->flow_dissect the generic version, and if so, call it directly, rather than go through the pointer. Or only set ops->flow_dissect if the generic version cannot be used. Andrew
> +void dsa_tag_generic_flow_dissect(const struct sk_buff *skb, __be16 *proto, > + int *offset) > +{ > + const struct dsa_device_ops *ops = skb->dev->dsa_ptr->tag_ops; > + int tag_len = ops->overhead; > + > + *offset = tag_len; > + *proto = ((__be16 *)skb->data)[(tag_len / 2) - 1]; > +} > +EXPORT_SYMBOL(dsa_tag_generic_flow_dissect); If you look where this is used: #if IS_ENABLED(CONFIG_NET_DSA) if (unlikely(skb->dev && netdev_uses_dsa(skb->dev) && proto == htons(ETH_P_XDSA))) { const struct dsa_device_ops *ops; int offset = 0; ops = skb->dev->dsa_ptr->tag_ops; if (ops->flow_dissect && !ops->flow_dissect(skb, &proto, &offset)) { hlen -= offset; nhoff += offset; } } #endif We have already done ops = skb->dev->dsa_ptr->tag_ops once. But since it is in a different compilation unit, the optimise has no idea about this. So building on my last comment, it would be nice to make this an inline function to help out the optimiser. Andrew
On Sat, Sep 26, 2020 at 08:05:45PM +0200, Andrew Lunn wrote: > Given spectra and meltdown etc, jumping through a pointer is expensive > and we try to avoid it on the hot path. Given most of the taggers are > going to use the generic version, maybe add a test here, is > ops->flow_dissect the generic version, and if so, call it directly, > rather than go through the pointer. Or only set ops->flow_dissect if > the generic version cannot be used. Agree about the motivation to eliminate an indirect call if possible. The situation is as follows: - Some taggers are before DMAC or before EtherType. These are the vast majority, and dsa_tag_generic_flow_dissect works well for them. We can keep the .flow_dissect callback as an override, but if this is absent, then the flow dissector can call dsa_tag_generic_flow_dissect directly. - Some taggers use tail tags. These don't need any massaging at all. But we need to tell the flow dissector to not call dsa_tag_generic_flow_dissect if it doesn't find a function pointer. I'm thinking about adding another "bool tail_tag" in struct dsa_device_ops, for this purpose and not only*. *Usually tunnel interfaces need to set dev->needed_headroom for the memory allocator. But DSA doesn't request that, and needs to check manually in the xmit function if the headroom is large enough to push a tag. BUT! tail tags don't need dev->needed_headroom, they need dev->needed_tailroom, I think. So I was thinking about adding this bool anyway, to distinguish between these 2 cases. What do you think?
On Sat, Sep 26, 2020 at 09:18:15PM +0300, Vladimir Oltean wrote: > On Sat, Sep 26, 2020 at 08:05:45PM +0200, Andrew Lunn wrote: > > Given spectra and meltdown etc, jumping through a pointer is expensive > > and we try to avoid it on the hot path. Given most of the taggers are > > going to use the generic version, maybe add a test here, is > > ops->flow_dissect the generic version, and if so, call it directly, > > rather than go through the pointer. Or only set ops->flow_dissect if > > the generic version cannot be used. > > Agree about the motivation to eliminate an indirect call if possible. > > The situation is as follows: > - Some taggers are before DMAC or before EtherType. These are the vast > majority, and dsa_tag_generic_flow_dissect works well for them. We can > keep the .flow_dissect callback as an override, but if this is absent, > then the flow dissector can call dsa_tag_generic_flow_dissect > directly. > - Some taggers use tail tags. These don't need any massaging at all. But > we need to tell the flow dissector to not call > dsa_tag_generic_flow_dissect if it doesn't find a function pointer. > I'm thinking about adding another "bool tail_tag" in struct > dsa_device_ops, for this purpose and not only*. > *Usually tunnel interfaces need to set dev->needed_headroom for the > memory allocator. But DSA doesn't request that, and needs to check > manually in the xmit function if the headroom is large enough to push > a tag. BUT! tail tags don't need dev->needed_headroom, they need > dev->needed_tailroom, I think. So I was thinking about adding this > bool anyway, to distinguish between these 2 cases. > > What do you think? Sounds good. Andrew
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c index 5c18c0214aac..aa925eac7888 100644 --- a/net/dsa/dsa.c +++ b/net/dsa/dsa.c @@ -305,6 +305,31 @@ bool dsa_schedule_work(struct work_struct *work) return queue_work(dsa_owq, work); } +/* All DSA tags that push the EtherType to the right (basically all except tail + * tags, which don't break dissection) can be treated the same from the + * perspective of the flow dissector. + * + * We need to return: + * - offset: the (B - A) difference between: + * A. the position of the real EtherType and + * B. the current skb->data (aka ETH_HLEN bytes into the frame, aka 2 bytes + * after the normal EtherType was supposed to be) + * The offset in bytes is exactly equal to the tagger overhead (and half of + * that, in __be16 shorts). + * + * - proto: the value of the real EtherType. + */ +void dsa_tag_generic_flow_dissect(const struct sk_buff *skb, __be16 *proto, + int *offset) +{ + const struct dsa_device_ops *ops = skb->dev->dsa_ptr->tag_ops; + int tag_len = ops->overhead; + + *offset = tag_len; + *proto = ((__be16 *)skb->data)[(tag_len / 2) - 1]; +} +EXPORT_SYMBOL(dsa_tag_generic_flow_dissect); + static ATOMIC_NOTIFIER_HEAD(dsa_notif_chain); int register_dsa_notifier(struct notifier_block *nb) diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index 0348dbab4131..61120145679d 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -99,6 +99,8 @@ void dsa_tag_driver_put(const struct dsa_device_ops *ops); bool dsa_schedule_work(struct work_struct *work); const char *dsa_tag_protocol_to_str(const struct dsa_device_ops *ops); +void dsa_tag_generic_flow_dissect(const struct sk_buff *skb, __be16 *proto, + int *offset); int dsa_legacy_fdb_add(struct ndmsg *ndm, struct nlattr *tb[], struct net_device *dev,
For all DSA formats that don't use tail tags, it looks like behind the obscure number crunching they're all doing the same thing: locating the real EtherType behind the DSA tag. Nonetheless, this is not immediately obvious, so create a generic helper for those DSA taggers that put the header before the EtherType. Another assumption for the generic function is that the DSA tags are of equal length on RX and on TX. Prior to the previous patch, this was not true for ocelot and for gswip. The problem was resolved for ocelot, but for gswip it still remains, so that can't use this helper yet. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- net/dsa/dsa.c | 25 +++++++++++++++++++++++++ net/dsa/dsa_priv.h | 2 ++ 2 files changed, 27 insertions(+)