Re: [RFC Draft PATCHv2 net-next] Doc: update bridge doc

[Hangbin Liu <liuhangbin@gmail.com>]

Hi Nikolay,

On Wed, Nov 01, 2023 at 01:29:34PM +0200, Nikolay Aleksandrov wrote:
> Hi,
> I have written some initial comments, there will definitely be more.
> One general thing - please split this in 2 patches at least. 1 for the
> documentation, and 1 for the netlink uAPI changes. You can even split it

Sure, I will.
> further into logical parts if you'd like, it will make it easier to
> review and people can focus on different parts better. Please CC DSA
> folks as well.
> 
> > diff --git a/Documentation/networking/bridge.rst b/Documentation/networking/bridge.rst
> > index c859f3c1636e..b36bd737c05e 100644
> > --- a/Documentation/networking/bridge.rst
> > +++ b/Documentation/networking/bridge.rst
> > -The bridge-utilities are maintained at:
> > -   git://git.kernel.org/pub/scm/linux/kernel/git/shemminger/bridge-utils.git
> > +Bridge internals
> > +================
> > -Additionally, the iproute2 utilities can be used to configure
> > -bridge devices.
> > +Here are the core structs of bridge code.
> 
> the core structs? These are outdated structures used in ioctl.

Ah, I just notice this. In next patch I will use `struct net_bridge_vlan`
as example.

> > +Bridge sysfs
> > +------------
> > +
> > +All the sysfs parameters are also exported via the bridge netlink API.
> > +Here you can find the explanation based on the correspond netlink attributes.
> 
> I don't get this one?

Some users/admins may still config bridge via sysfs. So I added this part
to let users know what's the meaning/usage of each sysfs field.

Do you want to remove this part for the doc?

> Also please mention the sysfs interface is deprecated and should not be
> extended if new options are added.

OK, I will add this note if we will keep this part.
> > +
> > +Switchdev
> > +=========
> > +
> > +Linux Bridge Switchdev is a feature in the Linux kernel that extends the
> > +capabilities of the traditional Linux bridge to work more efficiently with
> > +hardware switches that support switchdev. This technology is particularly
> > +useful in data center and networking environments where high-performance
> > +and low-latency packet forwarding is essential.
> 
> The last sentence is misleading, switchdev is used for many different types
> of devices.
> > +
> > +With Linux Bridge Switchdev, certain networking functions like forwarding,
> > +filtering, and learning of Ethernet frames can be offloaded to the hardware
> 
> "to a hardware switch"
> 
> > +switch. This offloading reduces the burden on the Linux kernel and CPU,
> > +leading to improved network performance and lower latency.
> > +
> > +To use Linux Bridge Switchdev, you need hardware switches that support the
> > +switchdev interface. This means that the switch hardware needs to have the
> > +necessary drivers and functionality to work in conjunction with the Linux
> > +kernel.
> 
> I'd add DSA maintainers to the CC list, and also ask switchdev driver
> maintainers to add more here. Switchdev can be explained much better.

Yes, you are right. I will add them in next version.
> > +
> > +Is it protocol independent?
> 
> Unclear, what layer?

How about "Is it L3/L4 protocol independent?"

> > +---------------------------
> > +
> > +Yes. The bridge knows nothing about protocols, it only sees Ethernet frames.
> 
> It sees all frames, it *uses* only L2 headers/information.
> 
> > diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
> > index f95326fce6bb..63e39de1055b 100644
> > --- a/include/uapi/linux/if_bridge.h
> > +++ b/include/uapi/linux/if_bridge.h
> > + *
> > + * @IFLA_BR_MCAST_STARTUP_QUERY_CNT
> > + *   Set the number of IGMP queries to send during startup phase.
> 
> What is a startup phase?

https://datatracker.ietf.org/doc/html/rfc2236#section-8.7

I think it means when the bridge/switch up.
> 
> > + *
> > + *   The default value is 1.
> 
> What is 1?

1 second.

> 
> > + *
> > + * @IFLA_BR_MCAST_MEMBERSHIP_INTVL
> > + *   The interval after which the bridge will leave a group, if no membership
> > + *   reports for this group are received.
> > + *
> > + *   The default value is 260.
> 
> What is 260? Please be more specific.

OK, I will.

Thanks
Hangbin