Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH net-next v1 1/9] net: dsa: add rcv_post call back
To:     Vladimir Oltean <olteanv@gmail.com>,
        Oleksij Rempel <o.rempel@pengutronix.de>
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Russell King <linux@armlinux.org.uk>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-mips@vger.kernel.org
References: <20210403114848.30528-1-o.rempel@pengutronix.de>
 <20210403114848.30528-2-o.rempel@pengutronix.de>
 <20210403140534.c4ydlgu5hqh7bmcq@skbuf>
 <20210403232116.knf6d7gdrvamk2lj@skbuf>
From:   Florian Fainelli <f.fainelli@gmail.com>
Message-ID: <53d84140-c072-f4ab-2f5c-af5c62abce2d@gmail.com>
Date:   Sat, 3 Apr 2021 19:32:48 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.9.0
MIME-Version: 1.0
In-Reply-To: <20210403232116.knf6d7gdrvamk2lj@skbuf>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


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