On 4/3/2021 16:21, Vladimir Oltean wrote:

On Sat, Apr 03, 2021 at 05:05:34PM +0300, Vladimir Oltean wrote:

quoted

On Sat, Apr 03, 2021 at 01:48:40PM +0200, Oleksij Rempel wrote:

quoted

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