* [PATCH net-next] net: dsa: allow port mirroring towards foreign interfaces @ 2021-02-05 22:33 Vladimir Oltean 2021-02-05 22:42 ` Florian Fainelli 0 siblings, 1 reply; 6+ messages in thread From: Vladimir Oltean @ 2021-02-05 22:33 UTC (permalink / raw) To: David S . Miller, Jakub Kicinski, netdev Cc: Andrew Lunn, Florian Fainelli, Vivien Didelot From: Vladimir Oltean <vladimir.oltean@nxp.com> To a DSA switch, port mirroring towards a foreign interface is the same as mirroring towards the CPU port, since all non-DSA interfaces are reachable through that. Tell the hardware to send the packets to the CPU port and let the mirred action deal with them in software. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- net/dsa/slave.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/net/dsa/slave.c b/net/dsa/slave.c index b0571ab4e5a7..913a4a5e32a9 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -936,19 +936,19 @@ dsa_slave_add_cls_matchall_mirred(struct net_device *dev, if (!act->dev) return -EINVAL; - if (!dsa_slave_dev_check(act->dev)) - return -EOPNOTSUPP; - mall_tc_entry = kzalloc(sizeof(*mall_tc_entry), GFP_KERNEL); if (!mall_tc_entry) return -ENOMEM; + if (dsa_slave_dev_check(act->dev)) + to_dp = dsa_slave_to_port(act->dev); + else + to_dp = dp->cpu_dp; + mall_tc_entry->cookie = cls->cookie; mall_tc_entry->type = DSA_PORT_MALL_MIRROR; mirror = &mall_tc_entry->mirror; - to_dp = dsa_slave_to_port(act->dev); - mirror->to_local_port = to_dp->index; mirror->ingress = ingress; -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: dsa: allow port mirroring towards foreign interfaces 2021-02-05 22:33 [PATCH net-next] net: dsa: allow port mirroring towards foreign interfaces Vladimir Oltean @ 2021-02-05 22:42 ` Florian Fainelli 2021-02-05 23:05 ` Vladimir Oltean 0 siblings, 1 reply; 6+ messages in thread From: Florian Fainelli @ 2021-02-05 22:42 UTC (permalink / raw) To: Vladimir Oltean, David S . Miller, Jakub Kicinski, netdev Cc: Andrew Lunn, Vivien Didelot On 2/5/21 2:33 PM, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > To a DSA switch, port mirroring towards a foreign interface is the same > as mirroring towards the CPU port, since all non-DSA interfaces are > reachable through that. Tell the hardware to send the packets to the CPU > port and let the mirred action deal with them in software. Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> How does the mirred action deal with that case? How does it know that packets delivered to the DSA master should be sent towards a foreign address, do I need to set-up two mirred rules? One that set-ups the filter on say sw0p0 to redirect egress to eth0 (DSA master) and another one to ingress filter on eth0 and egress mirror to eth1 (USB ethernet dongle)? -- Florian ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: dsa: allow port mirroring towards foreign interfaces 2021-02-05 22:42 ` Florian Fainelli @ 2021-02-05 23:05 ` Vladimir Oltean 2021-02-06 23:58 ` Jakub Kicinski 0 siblings, 1 reply; 6+ messages in thread From: Vladimir Oltean @ 2021-02-05 23:05 UTC (permalink / raw) To: Florian Fainelli Cc: David S . Miller, Jakub Kicinski, netdev, Andrew Lunn, Vivien Didelot On Fri, Feb 05, 2021 at 02:42:55PM -0800, Florian Fainelli wrote: > How does the mirred action deal with that case? How does it know that > packets delivered to the DSA master should be sent towards a foreign > address, do I need to set-up two mirred rules? One that set-ups the > filter on say sw0p0 to redirect egress to eth0 (DSA master) and another > one to ingress filter on eth0 and egress mirror to eth1 (USB ethernet > dongle)? [ I should have posted this as RFC, somebody asked me if it's possible, I only tested ingress mirroring, saw something come out, and posted this. I didn't even study act_mirred.c to see why I got anything at all ] For ingress mirroring there should be nothing special about the mirror packets, it's just more traffic in the ingress data path where the qdisc hook already exists. For egress mirroring I don't think there's really any way for the mirred action to take over the packets from what is basically the ingress qdisc and into the egress qdisc of the DSA interface such that they will be redirected to the selected mirror. I hadn't even thought about egress mirroring. I suppose with more API, we could have DSA do introspection into the frame header, see it's an egress-mirrored packet, and inject it into the egress qdisc of the net device instead of doing netif_rx. The idea with 2 mirrors might work however it's not amazing and I was thinking that if we bother to do something at all, we could as well try to think it through and come up with something that's seamless for the user. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: dsa: allow port mirroring towards foreign interfaces 2021-02-05 23:05 ` Vladimir Oltean @ 2021-02-06 23:58 ` Jakub Kicinski 2021-02-07 0:16 ` Vladimir Oltean 0 siblings, 1 reply; 6+ messages in thread From: Jakub Kicinski @ 2021-02-06 23:58 UTC (permalink / raw) To: Vladimir Oltean Cc: Florian Fainelli, David S . Miller, netdev, Andrew Lunn, Vivien Didelot On Sat, 6 Feb 2021 01:05:21 +0200 Vladimir Oltean wrote: > On Fri, Feb 05, 2021 at 02:42:55PM -0800, Florian Fainelli wrote: > > How does the mirred action deal with that case? How does it know that > > packets delivered to the DSA master should be sent towards a foreign > > address, do I need to set-up two mirred rules? One that set-ups the > > filter on say sw0p0 to redirect egress to eth0 (DSA master) and another > > one to ingress filter on eth0 and egress mirror to eth1 (USB ethernet > > dongle)? > > [ I should have posted this as RFC, somebody asked me if it's possible, > I only tested ingress mirroring, saw something come out, and posted this. > I didn't even study act_mirred.c to see why I got anything at all ] Let me mark it as RFC, then :) > For ingress mirroring there should be nothing special about the mirror > packets, it's just more traffic in the ingress data path where the qdisc > hook already exists. For ingress the only possible corner case seems to be if the filter has SKIP_SW set, then HW will send to CPU but SW will ignore. That's assuming the frame still comes on the CPU appropriately tagged. > For egress mirroring I don't think there's really any way for the mirred > action to take over the packets from what is basically the ingress qdisc > and into the egress qdisc of the DSA interface such that they will be > redirected to the selected mirror. I hadn't even thought about egress > mirroring. I suppose with more API, we could have DSA do introspection > into the frame header, see it's an egress-mirrored packet, and inject it > into the egress qdisc of the net device instead of doing netif_rx. IMHO it's not very pretty but FWIW some "SmartNIC" drivers already do a similar thing. But to be clear that's just an optimization, right? The SW should still be able to re-process and come to the same decisions as the switch, provided SKIP_SW was not set? > The idea with 2 mirrors might work however it's not amazing and I was > thinking that if we bother to do something at all, we could as well try > to think it through and come up with something that's seamless for the > user. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: dsa: allow port mirroring towards foreign interfaces 2021-02-06 23:58 ` Jakub Kicinski @ 2021-02-07 0:16 ` Vladimir Oltean 2021-02-07 0:43 ` Jakub Kicinski 0 siblings, 1 reply; 6+ messages in thread From: Vladimir Oltean @ 2021-02-07 0:16 UTC (permalink / raw) To: Jakub Kicinski Cc: Florian Fainelli, David S . Miller, netdev, Andrew Lunn, Vivien Didelot On Sat, Feb 06, 2021 at 03:58:57PM -0800, Jakub Kicinski wrote: > > For ingress mirroring there should be nothing special about the mirror > > packets, it's just more traffic in the ingress data path where the qdisc > > hook already exists. > > For ingress the only possible corner case seems to be if the filter has > SKIP_SW set, then HW will send to CPU but SW will ignore. Correct, but I'm not sure if this requirement can be enforced at driver level though. > That's assuming the frame still comes on the CPU appropriately tagged. For ingress mirroring I think the assumption that it does is reasonable, since the packet should be mirrored before the forwarding took place, it can only have one DSA tag and that would be the tag where the source port is the ingress port. For egress mirroring, software would need to see the mirrored packet as coming from the egress port, and this would mean that the source port in the DSA frame header would have to be equal to the egress port. > > For egress mirroring I don't think there's really any way for the mirred > > action to take over the packets from what is basically the ingress qdisc > > and into the egress qdisc of the DSA interface such that they will be > > redirected to the selected mirror. I hadn't even thought about egress > > mirroring. I suppose with more API, we could have DSA do introspection > > into the frame header, see it's an egress-mirrored packet, and inject it > > into the egress qdisc of the net device instead of doing netif_rx. > > IMHO it's not very pretty but FWIW some "SmartNIC" drivers already do > a similar thing. But to be clear that's just an optimization, right? > The SW should still be able to re-process and come to the same > decisions as the switch, provided SKIP_SW was not set? I guess what would need to happen is that we'd need to do something like this, from the DSA tagging protocol files: if (is_egress_mirror(skb)) { skb_get(skb); skb_push(skb, ETH_ALEN); skb = sch_handle_egress(skb, &err, skb->dev); if (skb) consume_skb(skb); return NULL; } basically just run whatever tc filters there might be on that packet (in our case mirred), then discard it. It's not an optimization thing. Egress mirrored traffic on a DSA switch is still ingress traffic from software's perspective, so it won't match on any mirred action on any egress qdisc. Only packets sent from the network stack would match the mirred egress mirror rule, however there might be lots of offloaded flows which don't. Or I might just be misunderstanding. > > The idea with 2 mirrors might work however it's not amazing and I was > > thinking that if we bother to do something at all, we could as well try > > to think it through and come up with something that's seamless for the > > user. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: dsa: allow port mirroring towards foreign interfaces 2021-02-07 0:16 ` Vladimir Oltean @ 2021-02-07 0:43 ` Jakub Kicinski 0 siblings, 0 replies; 6+ messages in thread From: Jakub Kicinski @ 2021-02-07 0:43 UTC (permalink / raw) To: Vladimir Oltean Cc: Florian Fainelli, David S . Miller, netdev, Andrew Lunn, Vivien Didelot On Sun, 7 Feb 2021 02:16:17 +0200 Vladimir Oltean wrote: > On Sat, Feb 06, 2021 at 03:58:57PM -0800, Jakub Kicinski wrote: > > > For ingress mirroring there should be nothing special about the mirror > > > packets, it's just more traffic in the ingress data path where the qdisc > > > hook already exists. > > > > For ingress the only possible corner case seems to be if the filter has > > SKIP_SW set, then HW will send to CPU but SW will ignore. > > Correct, but I'm not sure if this requirement can be enforced at driver > level though. True, we'd need to add more info to struct flow_cls_common_offload. > > That's assuming the frame still comes on the CPU appropriately tagged. > > For ingress mirroring I think the assumption that it does is reasonable, > since the packet should be mirrored before the forwarding took place, it > can only have one DSA tag and that would be the tag where the source > port is the ingress port. > For egress mirroring, software would need to see the mirrored packet as > coming from the egress port, and this would mean that the source port in > the DSA frame header would have to be equal to the egress port. > > > > For egress mirroring I don't think there's really any way for the mirred > > > action to take over the packets from what is basically the ingress qdisc > > > and into the egress qdisc of the DSA interface such that they will be > > > redirected to the selected mirror. I hadn't even thought about egress > > > mirroring. I suppose with more API, we could have DSA do introspection > > > into the frame header, see it's an egress-mirrored packet, and inject it > > > into the egress qdisc of the net device instead of doing netif_rx. > > > > IMHO it's not very pretty but FWIW some "SmartNIC" drivers already do > > a similar thing. But to be clear that's just an optimization, right? > > The SW should still be able to re-process and come to the same > > decisions as the switch, provided SKIP_SW was not set? > > I guess what would need to happen is that we'd need to do something like > this, from the DSA tagging protocol files: > > if (is_egress_mirror(skb)) { > skb_get(skb); > skb_push(skb, ETH_ALEN); > skb = sch_handle_egress(skb, &err, skb->dev); > if (skb) > consume_skb(skb); > return NULL; > } > > basically just run whatever tc filters there might be on that packet (in > our case mirred), then discard it. > > It's not an optimization thing. Egress mirrored traffic on a DSA switch > is still ingress traffic from software's perspective, so it won't match > on any mirred action on any egress qdisc. Only packets sent from the > network stack would match the mirred egress mirror rule, however there > might be lots of offloaded flows which don't. > > Or I might just be misunderstanding. Okay, that makes sense, sounds like we just can't expect the DSA tag with ingress port info preserved when frames are trapped at egress. All depends on HW capabilities then. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-07 0:45 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-05 22:33 [PATCH net-next] net: dsa: allow port mirroring towards foreign interfaces Vladimir Oltean 2021-02-05 22:42 ` Florian Fainelli 2021-02-05 23:05 ` Vladimir Oltean 2021-02-06 23:58 ` Jakub Kicinski 2021-02-07 0:16 ` Vladimir Oltean 2021-02-07 0:43 ` Jakub Kicinski
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.