Received-SPF: pass (google.com: domain of linux-kernel+bounces-132167-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <4c28d59e-0c4f-462c-8a1c-d4bd72e25115@gmail.com>
Date: Thu, 4 Apr 2024 17:35:41 -0400
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH RFC net-next 00/10] MC Flood disable and snooping
To: Andrew Lunn <andrew@lunn.ch>, Joseph Huang <Joseph.Huang@garmin.com>
Cc: netdev@vger.kernel.org, Florian Fainelli <f.fainelli@gmail.com>,
 Vladimir Oltean <olteanv@gmail.com>, "David S. Miller"
 <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>,
 Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
 Roopa Prabhu <roopa@nvidia.com>, Nikolay Aleksandrov <razor@blackwall.org>,
 =?UTF-8?Q?Linus_L=C3=BCssing?= <linus.luessing@c0d3.blue>,
 linux-kernel@vger.kernel.org, bridge@lists.linux.dev
References: <20240402001137.2980589-1-Joseph.Huang@garmin.com>
 <f5512e7b-b572-4444-9d56-1eed9bbcda6b@lunn.ch>
Content-Language: en-US
From: Joseph Huang <joseph.huang.2024@gmail.com>
In-Reply-To: <f5512e7b-b572-4444-9d56-1eed9bbcda6b@lunn.ch>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Hi Andrew,

On 4/2/2024 8:43 AM, Andrew Lunn wrote:
> On Mon, Apr 01, 2024 at 08:10:59PM -0400, Joseph Huang wrote:
>> There is a use case where one would like to enable multicast snooping
>> on a bridge but disable multicast flooding on all bridge ports so that
>> registered multicast traffic will only reach the intended recipients and
>> unregistered multicast traffic will be dropped. However, with existing
>> bridge ports' mcast_flood flag implementation, it doesn't work as desired.
>> 
>> This patchset aims to make multicast snooping work even when multicast
>> flooding is disabled on the bridge ports, without changing the semantic of
>> the mcast_flood flag too much. Patches 1 to 4 attempt to address this issue.
>> 
>> Also, in a network where more than one multicast snooping capable bridges
>> are interconnected without multicast routers being present, multicast
>> snooping fails if:
>> 
>>   1. The source is not directly attached to the Querier
>>   2. The listener is beyond the mrouter port of the bridge where the
>>      source is directly attached
>>   3. A hardware offloading switch is involved
> 
> I've not studied the details here, but that last point makes me think
> the offload driver is broken. There should not be any difference
> between software bridging and hardware bridging. The whole idea is
> that you take what Linux can do in software and accelerate it by
> offloading to hardware. Doing acceleration should not change the
> behaviour.
> 

In patch 10 I gave a little more detail about the fix, but basically 
this is what happened.

Assuming we have a soft bridge like the following:

             bp1 +------------+
   Querier <---- |   bridge   |
                 +------------+
                bp2 |      | bp3
                    |      |
                    v      v
             MC Source    MC Listener

Here bp1 is the mrouter port, bp2 is connected to the multicast source, 
and bp3 is connected to the multicast listener who wishes to receive 
multicast traffic for that group.

After some Query/Report exchange, the snooping code in the bridge is 
going to learn about the Listener from bp3, and is going to create an 
MDB group which includes bp3 as the sole member. When the bridge 
receives a multicast packet for that group from bp2, the bridge will do 
a look up to find the members of that group (in this case, bp3) and 
forward the packet to every single member in that group. At the same 
time, the bridge will also forward the packet to every mrouter port so 
that listeners beyond mrouter ports can receive that multicast packet as 
well.

Now consider the same scenario, but with a hardware-offloaded switch:

                 +------------+
                 |   bridge   |
                 +------------+
                       ^
                       |
                       | p6 (Host CPU port)
          p1/bp1 +------------+
   Querier <---- |     sw     |
                 +------------+
             p2/bp2 |      | p3/bp3
                    |      |
                    v      v
             MC Source    MC Listener

Same Query/Report exchange, same MDB group, except that this time around 
the MDB group will be offloaded to the switch as well. So in the 
switch's ATU we will now have an entry for the multicast group and with 
p3 being the only member of that ATU. When the multicast packet arrives 
at the switch from p2, the switch will do an ATU lookup, and forward the 
packet to p3 only. This means that the Host CPU (p6) will not get a copy 
of the packet, and so the soft bridge will not have the opportunity to 
forward that packet to the mrouter port. This is what patch 10 attempts 
to address.

One possible solution of course is to add p6 to every MDB group, however 
that's probably not very desirable. Besides, it will be more efficient 
if the packet is forwarded to the mrouter port by the switch in hardware 
(i.e., offload mrouter forwarding), vs. being forwarded in the bridge by 
software.

>> The patches were developed against 5.15, and forward-ported to 6.8.
>> Tests were done on a Pi 4B + Marvell 6393X Eval board with a single
>> switch chip with no VLAN.
> 
> So what is the mv88e6xxx driver doing wrong?
> 
> 	Andrew
> 

The mv88e6xxx driver did nothing differently than the other DSA drivers. 
I chose the mv88e6xxx driver to implement the fix simply because that's 
the only platform I have at hand to verify the solution.