Messages in this thread | | | Subject | Re: [PATCH net-next v1 1/9] net: dsa: add rcv_post call back | From | Florian Fainelli <> | Date | Sat, 3 Apr 2021 19:32:48 -0700 |
| |
On 4/3/2021 16:21, Vladimir Oltean wrote: > On Sat, Apr 03, 2021 at 05:05:34PM +0300, Vladimir Oltean wrote: >> On Sat, Apr 03, 2021 at 01:48:40PM +0200, Oleksij Rempel wrote: >>> Some switches (for example ar9331) do not provide enough information >>> about forwarded packets. If the switch decision was made based on IPv4 >>> or IPv6 header, we need to analyze it and set proper flag. >>> >>> Potentially we can do it in existing rcv path, on other hand we can >>> avoid part of duplicated work and let the dsa framework set skb header >>> pointers and then use preprocessed skb one step later withing the rcv_post >>> call back. >>> >>> This patch is needed for ar9331 switch. >>> >>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> >>> --- >> >> I don't necessarily disagree with this, perhaps we can even move >> Florian's dsa_untag_bridge_pvid() call inside a rcv_post() method >> implemented by the DSA_TAG_PROTO_BRCM_LEGACY, DSA_TAG_PROTO_BRCM_PREPEND >> and DSA_TAG_PROTO_BRCM taggers. Or even better, because Oleksij's >> rcv_post is already prototype-compatible with dsa_untag_bridge_pvid, we >> can already do: >> >> .rcv_post = dsa_untag_bridge_pvid, >> >> This should be generally useful for stuff that DSA taggers need to do >> which is easiest done after eth_type_trans() was called. > > I had some fun with an alternative method of parsing the frame for IGMP > so that you can clear skb->offload_fwd_mark, which doesn't rely on the > introduction of a new method in DSA. It should also have several other > advantages compared to your solution such as the fact that it should > work with VLAN-tagged packets. > > Background: we made Receive Packet Steering work on DSA master interfaces > (echo 3 > /sys/class/net/eth0/queues/rx-1/rps_cpus) even when the DSA > tag shifts to the right the IP headers and everything that comes > afterwards. The flow dissector had to be patched for that, just grep for > DSA in net/core/flow_dissector.c. > > The problem you're facing is that you can't parse the IP and IGMP > headers in the tagger's rcv() method, since the network header, > transport header offsets and skb->protocol are all messed up, since > eth_type_trans hasn't been called yet. > > And that's the trick right there, you're between a rock and a hard > place: too early because eth_type_trans wasn't called yet, and too late > because skb->dev was changed and no longer points to the DSA master, so > the flow dissector adjustment we made doesn't apply. > > But if you call the flow dissector _before_ you call "skb->dev = > dsa_master_find_slave" (and yes, while the DSA tag is still there), then > it's virtually as if you had called that while the skb belonged to the > DSA master, so it should work with __skb_flow_dissect. > > In fact I prototyped this idea below. I wanted to check whether I can > match something as fine-grained as an IGMPv2 Membership Report message, > and I could. > > I prototyped it inside the ocelot tagging protocol driver because that's > what I had handy. I used __skb_flow_dissect with my own flow dissector > which had to be initialized at the tagger module_init time, even though > I think I could have probably just called skb_flow_dissect_flow_keys > with a standard dissector, and that would have removed the need for the > custom module_init in tag_ocelot.c. One thing that is interesting is > that I had to add the bits for IGMP parsing to the flow dissector > myself (based on the existing ICMP code). I was too lazy to do that for > MLD as well, but it is really not hard. Or even better, if you don't > need to look at all inside the IGMP/MLD header, I think you can even > omit adding this parsing code to the flow dissector and just look at > basic.n_proto and basic.ip_proto. > > See the snippet below. Hope it helps.
This looks a lot better than introducing hooks at various points in dsa_switch_rcv(). -- Florian
| |