Re: [PATCH net-next v1 1/9] net: dsa: add rcv_post call back

[Florian Fainelli]

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@xxxxxxxxxxxxxx>
> > > ---
> >
> > 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