All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH iproute2 0/6] Clarifications to bridge man page
@ 2021-02-11 10:44 Vladimir Oltean
  2021-02-11 10:44 ` [PATCH iproute2 1/6] man8/bridge.8: document the "permanent" flag for "bridge fdb add" Vladimir Oltean
                   ` (5 more replies)
  0 siblings, 6 replies; 12+ messages in thread
From: Vladimir Oltean @ 2021-02-11 10:44 UTC (permalink / raw)
  To: David Ahern, netdev
  Cc: Andrew Lunn, Florian Fainelli, Vivien Didelot, Jiri Pirko,
	Ido Schimmel, DENG Qingfang, Tobias Waldekranz, Roopa Prabhu,
	Nikolay Aleksandrov, Stephen Hemminger

From: Vladimir Oltean <vladimir.oltean@nxp.com>

There are several explanations missing in the bridge man page which have
led people in the past to make incorrect assumptions. Try to improve the
situation a little bit.

Vladimir Oltean (6):
  man8/bridge.8: document the "permanent" flag for "bridge fdb add"
  man8/bridge.8: document that "local" is default for "bridge fdb add"
  man8/bridge.8: explain what a local FDB entry is
  man8/bridge.8: fix which one of self/master is default for "bridge
    fdb"
  man8/bridge.8: explain self vs master for "bridge fdb add"
  man8/bridge.8: be explicit that "flood" is an egress setting

 man/man8/bridge.8 | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH iproute2 1/6] man8/bridge.8: document the "permanent" flag for "bridge fdb add"
  2021-02-11 10:44 [PATCH iproute2 0/6] Clarifications to bridge man page Vladimir Oltean
@ 2021-02-11 10:44 ` Vladimir Oltean
  2021-02-11 10:44 ` [PATCH iproute2 2/6] man8/bridge.8: document that "local" is default " Vladimir Oltean
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Vladimir Oltean @ 2021-02-11 10:44 UTC (permalink / raw)
  To: David Ahern, netdev
  Cc: Andrew Lunn, Florian Fainelli, Vivien Didelot, Jiri Pirko,
	Ido Schimmel, DENG Qingfang, Tobias Waldekranz, Roopa Prabhu,
	Nikolay Aleksandrov, Stephen Hemminger

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The bridge program parses "local" and "permanent" in just the same way,
so it makes sense to tell that to users:

fdb_modify:
		} else if (matches(*argv, "local") == 0 ||
			   matches(*argv, "permanent") == 0) {
			req.ndm.ndm_state |= NUD_PERMANENT;

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 man/man8/bridge.8 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/man/man8/bridge.8 b/man/man8/bridge.8
index b3414ae31faf..12c09a56563d 100644
--- a/man/man8/bridge.8
+++ b/man/man8/bridge.8
@@ -517,6 +517,10 @@ the interface to which this address is associated.
 - is a local permanent fdb entry
 .sp
 
+.B permanent
+- this is a synonym for "local"
+.sp
+
 .B static
 - is a static (no arp) fdb entry
 .sp
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH iproute2 2/6] man8/bridge.8: document that "local" is default for "bridge fdb add"
  2021-02-11 10:44 [PATCH iproute2 0/6] Clarifications to bridge man page Vladimir Oltean
  2021-02-11 10:44 ` [PATCH iproute2 1/6] man8/bridge.8: document the "permanent" flag for "bridge fdb add" Vladimir Oltean
@ 2021-02-11 10:44 ` Vladimir Oltean
  2021-02-11 10:44 ` [PATCH iproute2 3/6] man8/bridge.8: explain what a local FDB entry is Vladimir Oltean
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Vladimir Oltean @ 2021-02-11 10:44 UTC (permalink / raw)
  To: David Ahern, netdev
  Cc: Andrew Lunn, Florian Fainelli, Vivien Didelot, Jiri Pirko,
	Ido Schimmel, DENG Qingfang, Tobias Waldekranz, Roopa Prabhu,
	Nikolay Aleksandrov, Stephen Hemminger

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The bridge does this:

fdb_modify:
	/* Assume permanent */
	if (!(req.ndm.ndm_state&(NUD_PERMANENT|NUD_REACHABLE)))
		req.ndm.ndm_state |= NUD_PERMANENT;

So let's make the user aware of the fact that if they don't want local
entries, they need to specify some other flag like "static".

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 man/man8/bridge.8 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/man/man8/bridge.8 b/man/man8/bridge.8
index 12c09a56563d..223e65d64757 100644
--- a/man/man8/bridge.8
+++ b/man/man8/bridge.8
@@ -514,7 +514,8 @@ the Ethernet MAC address.
 the interface to which this address is associated.
 
 .B local
-- is a local permanent fdb entry
+- is a local permanent fdb entry. This flag is default unless "static" or
+  "dynamic" are explicitly specified.
 .sp
 
 .B permanent
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH iproute2 3/6] man8/bridge.8: explain what a local FDB entry is
  2021-02-11 10:44 [PATCH iproute2 0/6] Clarifications to bridge man page Vladimir Oltean
  2021-02-11 10:44 ` [PATCH iproute2 1/6] man8/bridge.8: document the "permanent" flag for "bridge fdb add" Vladimir Oltean
  2021-02-11 10:44 ` [PATCH iproute2 2/6] man8/bridge.8: document that "local" is default " Vladimir Oltean
@ 2021-02-11 10:44 ` Vladimir Oltean
  2021-02-11 10:45 ` [PATCH iproute2 4/6] man8/bridge.8: fix which one of self/master is default for "bridge fdb" Vladimir Oltean
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Vladimir Oltean @ 2021-02-11 10:44 UTC (permalink / raw)
  To: David Ahern, netdev
  Cc: Andrew Lunn, Florian Fainelli, Vivien Didelot, Jiri Pirko,
	Ido Schimmel, DENG Qingfang, Tobias Waldekranz, Roopa Prabhu,
	Nikolay Aleksandrov, Stephen Hemminger

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Explaining the "local" flag by saying that it is "a local permanent fdb
entry" is not very helpful, be more specific.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 man/man8/bridge.8 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/man/man8/bridge.8 b/man/man8/bridge.8
index 223e65d64757..b629c52b8341 100644
--- a/man/man8/bridge.8
+++ b/man/man8/bridge.8
@@ -514,8 +514,10 @@ the Ethernet MAC address.
 the interface to which this address is associated.
 
 .B local
-- is a local permanent fdb entry. This flag is default unless "static" or
-  "dynamic" are explicitly specified.
+- is a local permanent fdb entry, which means that the bridge will not forward
+frames with this destination MAC address and VLAN ID, but terminate them
+locally. This flag is default unless "static" or "dynamic" are explicitly
+specified.
 .sp
 
 .B permanent
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH iproute2 4/6] man8/bridge.8: fix which one of self/master is default for "bridge fdb"
  2021-02-11 10:44 [PATCH iproute2 0/6] Clarifications to bridge man page Vladimir Oltean
                   ` (2 preceding siblings ...)
  2021-02-11 10:44 ` [PATCH iproute2 3/6] man8/bridge.8: explain what a local FDB entry is Vladimir Oltean
@ 2021-02-11 10:45 ` Vladimir Oltean
  2021-02-11 10:45 ` [PATCH iproute2 5/6] man8/bridge.8: explain self vs master for "bridge fdb add" Vladimir Oltean
  2021-02-11 10:45 ` [PATCH iproute2 6/6] man8/bridge.8: be explicit that "flood" is an egress setting Vladimir Oltean
  5 siblings, 0 replies; 12+ messages in thread
From: Vladimir Oltean @ 2021-02-11 10:45 UTC (permalink / raw)
  To: David Ahern, netdev
  Cc: Andrew Lunn, Florian Fainelli, Vivien Didelot, Jiri Pirko,
	Ido Schimmel, DENG Qingfang, Tobias Waldekranz, Roopa Prabhu,
	Nikolay Aleksandrov, Stephen Hemminger

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The bridge program does:

fdb_modify:
	/* Assume self */
	if (!(req.ndm.ndm_flags&(NTF_SELF|NTF_MASTER)))
		req.ndm.ndm_flags |= NTF_SELF;

which is clearly against the documented behavior. The only thing we can
do, sadly, is update the documentation.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 man/man8/bridge.8 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/man/man8/bridge.8 b/man/man8/bridge.8
index b629c52b8341..1dc0aec83f09 100644
--- a/man/man8/bridge.8
+++ b/man/man8/bridge.8
@@ -533,11 +533,12 @@ specified.
 .sp
 
 .B self
-- the address is associated with the port drivers fdb. Usually hardware.
+- the address is associated with the port drivers fdb. Usually hardware
+  (default).
 .sp
 
 .B master
-- the address is associated with master devices fdb. Usually software (default).
+- the address is associated with master devices fdb. Usually software.
 .sp
 
 .B router
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH iproute2 5/6] man8/bridge.8: explain self vs master for "bridge fdb add"
  2021-02-11 10:44 [PATCH iproute2 0/6] Clarifications to bridge man page Vladimir Oltean
                   ` (3 preceding siblings ...)
  2021-02-11 10:45 ` [PATCH iproute2 4/6] man8/bridge.8: fix which one of self/master is default for "bridge fdb" Vladimir Oltean
@ 2021-02-11 10:45 ` Vladimir Oltean
  2021-02-15  8:22   ` Alexandra Winter
  2021-02-11 10:45 ` [PATCH iproute2 6/6] man8/bridge.8: be explicit that "flood" is an egress setting Vladimir Oltean
  5 siblings, 1 reply; 12+ messages in thread
From: Vladimir Oltean @ 2021-02-11 10:45 UTC (permalink / raw)
  To: David Ahern, netdev
  Cc: Andrew Lunn, Florian Fainelli, Vivien Didelot, Jiri Pirko,
	Ido Schimmel, DENG Qingfang, Tobias Waldekranz, Roopa Prabhu,
	Nikolay Aleksandrov, Stephen Hemminger

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The "usually hardware" and "usually software" distinctions make no
sense, try to clarify what these do based on the actual kernel behavior.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 man/man8/bridge.8 | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/man/man8/bridge.8 b/man/man8/bridge.8
index 1dc0aec83f09..d0bcd708bb61 100644
--- a/man/man8/bridge.8
+++ b/man/man8/bridge.8
@@ -533,12 +533,21 @@ specified.
 .sp
 
 .B self
-- the address is associated with the port drivers fdb. Usually hardware
-  (default).
+- the operation is fulfilled directly by the driver for the specified network
+device. If the network device belongs to a master like a bridge, then the
+bridge is bypassed and not notified of this operation (and if the device does
+notify the bridge, it is driver-specific behavior and not mandated by this
+flag, check the driver for more details). The "bridge fdb add" command can also
+be used on the bridge device itself, and in this case, the added fdb entries
+will be locally terminated (not forwarded). In the latter case, the "self" flag
+is mandatory. The flag is set by default if "master" is not specified.
 .sp
 
 .B master
-- the address is associated with master devices fdb. Usually software.
+- if the specified network device is a port that belongs to a master device
+such as a bridge, the operation is fulfilled by the master device's driver,
+which may in turn notify the port driver too of the address. If the specified
+device is a master itself, such as a bridge, this flag is invalid.
 .sp
 
 .B router
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH iproute2 6/6] man8/bridge.8: be explicit that "flood" is an egress setting
  2021-02-11 10:44 [PATCH iproute2 0/6] Clarifications to bridge man page Vladimir Oltean
                   ` (4 preceding siblings ...)
  2021-02-11 10:45 ` [PATCH iproute2 5/6] man8/bridge.8: explain self vs master for "bridge fdb add" Vladimir Oltean
@ 2021-02-11 10:45 ` Vladimir Oltean
  5 siblings, 0 replies; 12+ messages in thread
From: Vladimir Oltean @ 2021-02-11 10:45 UTC (permalink / raw)
  To: David Ahern, netdev
  Cc: Andrew Lunn, Florian Fainelli, Vivien Didelot, Jiri Pirko,
	Ido Schimmel, DENG Qingfang, Tobias Waldekranz, Roopa Prabhu,
	Nikolay Aleksandrov, Stephen Hemminger

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Talking to varios people, it became apparent that there is a certain
ambiguity in the description of these flags. They refer to egress
flooding, which should perhaps be stated more clearly.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 man/man8/bridge.8 | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/man/man8/bridge.8 b/man/man8/bridge.8
index d0bcd708bb61..9d8663bd23cc 100644
--- a/man/man8/bridge.8
+++ b/man/man8/bridge.8
@@ -397,7 +397,8 @@ bridge FDB.
 
 .TP
 .BR "flood on " or " flood off "
-Controls whether a given port will flood unicast traffic for which there is no FDB entry. By default this flag is on.
+Controls whether unicast traffic for which there is no FDB entry will be
+flooded towards this given port. By default this flag is on.
 
 .TP
 .B hwmode
@@ -413,8 +414,8 @@ switch.
 
 .TP
 .BR "mcast_flood on " or " mcast_flood off "
-Controls whether a given port will flood multicast traffic for which
-there is no MDB entry. By default this flag is on.
+Controls whether multicast traffic for which there is no MDB entry will be
+flooded towards this given port. By default this flag is on.
 
 .TP
 .BR "mcast_to_unicast on " or " mcast_to_unicast off "
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH iproute2 5/6] man8/bridge.8: explain self vs master for "bridge fdb add"
  2021-02-11 10:45 ` [PATCH iproute2 5/6] man8/bridge.8: explain self vs master for "bridge fdb add" Vladimir Oltean
@ 2021-02-15  8:22   ` Alexandra Winter
  2021-02-15 10:32     ` Vladimir Oltean
  0 siblings, 1 reply; 12+ messages in thread
From: Alexandra Winter @ 2021-02-15  8:22 UTC (permalink / raw)
  To: Vladimir Oltean, David Ahern, netdev
  Cc: Andrew Lunn, Florian Fainelli, Vivien Didelot, Jiri Pirko,
	Ido Schimmel, DENG Qingfang, Tobias Waldekranz, Roopa Prabhu,
	Nikolay Aleksandrov, Stephen Hemminger

Thank you very much Vladimir for improving this man page. I am still struggling with the meaning of the bridge attributes and sometimes
the man page has caused more confusion.

In the section about 'bridge link set' Self vs master mention physical device vs software bridge. Would it make sense to use the same
terminology here?

The attributes are listed under 'bridge fdb add' not under 'bridge fdb show'. Is it correct that the attributes displayed by 'show'
are a 1-to-1 representation of the ones set by 'add'? What about the entries that are not manually set, like bridge learned adresses?
Is it possible to add some explanation about those as well?

On 11.02.21 11:45, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> The "usually hardware" and "usually software" distinctions make no
> sense, try to clarify what these do based on the actual kernel behavior.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  man/man8/bridge.8 | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/man/man8/bridge.8 b/man/man8/bridge.8
> index 1dc0aec83f09..d0bcd708bb61 100644
> --- a/man/man8/bridge.8
> +++ b/man/man8/bridge.8
> @@ -533,12 +533,21 @@ specified.
>  .sp
>  
>  .B self
> -- the address is associated with the port drivers fdb. Usually hardware
> -  (default).
> +- the operation is fulfilled directly by the driver for the specified network
> +device. If the network device belongs to a master like a bridge, then the
> +bridge is bypassed and not notified of this operation (and if the device does
> +notify the bridge, it is driver-specific behavior and not mandated by this
> +flag, check the driver for more details). The "bridge fdb add" command can also
> +be used on the bridge device itself, and in this case, the added fdb entries
> +will be locally terminated (not forwarded). In the latter case, the "self" flag
> +is mandatory. 
Maybe I misunderstand this sentence, but I can do a 'bridge fdb add' without 'self'
on the bridge device. And the address shows up under 'bridge fdb show'.
So what does mandatory mean here?
The flag is set by default if "master" is not specified.
>  .sp
>  
>  .B master
> -- the address is associated with master devices fdb. Usually software.
> +- if the specified network device is a port that belongs to a master device
> +such as a bridge, the operation is fulfilled by the master device's driver,
> +which may in turn notify the port driver too of the address. If the specified
> +device is a master itself, such as a bridge, this flag is invalid.
>  .sp
>  
>  .B router
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH iproute2 5/6] man8/bridge.8: explain self vs master for "bridge fdb add"
  2021-02-15  8:22   ` Alexandra Winter
@ 2021-02-15 10:32     ` Vladimir Oltean
  2021-02-15 10:53       ` Alexandra Winter
  0 siblings, 1 reply; 12+ messages in thread
From: Vladimir Oltean @ 2021-02-15 10:32 UTC (permalink / raw)
  To: Alexandra Winter
  Cc: David Ahern, netdev, Andrew Lunn, Florian Fainelli,
	Vivien Didelot, Jiri Pirko, Ido Schimmel, DENG Qingfang,
	Tobias Waldekranz, Roopa Prabhu, Nikolay Aleksandrov,
	Stephen Hemminger

Hi Alexandra,

On Mon, Feb 15, 2021 at 09:22:47AM +0100, Alexandra Winter wrote:
> In the section about 'bridge link set' Self vs master mention physical
> device vs software bridge. Would it make sense to use the same
> terminology here?

You mean like this?

.TP
.B self
operation is fulfilled by the driver of the specified network interface.

.TP
.B master
operation is fulfilled by the specified interface's master, for example
a bridge, which in turn may or may not notify the underlying network
interface driver. This flag is considered implicit by the kernel if
'self' was not specified.

> The attributes are listed under 'bridge fdb add' not under 'bridge fdb
> show'. Is it correct that the attributes displayed by 'show' are a
> 1-to-1 representation of the ones set by 'add'?

Bah, not quite. I'll try to summarize below.

> What about the entries that are not manually set, like bridge learned
> adresses? Is it possible to add some explanation about those as well?

Ok, challenge accepted. Here's my take on 'bridge fdb show', I haven't
used most of these options (I'm commenting solely based on code
inspection) so if anybody with more experience could chime in, I'd be
happy to adjust the wording.


.SS bridge fdb show - list forwarding entries.

This command displays the current forwarding table. By default all FDB
entries in the system are shown. The following options can be used to
reduce the number of displayed entries:

.TP
.B br
Filter the output to contain only the FDB entries of the specified
bridge, or belonging to ports of the specified bridge (optional).

.B brport
Filter the output to contain only the FDB entries present on the
specified network interface (bridge port). This flag is optional.

.B dev
Same as "brport".

.B vlan
Filter the output to contain only the FDB entries with the specified
VLAN ID (optional).

.B dynamic
Filter out the local/permanent (not forwarded) FDB entries.

.B state
Filter the output to contain only the FDB entries having the specified
state. The bridge FDB is modeled as a neighbouring protocol for
PF_BRIDGE (similar to what ARP is for IPv4 and ND is for IPv6).
Therefore, an FDB entry has a NUD ("Network Unreachability Detection")
state given by the generic neighbouring layer.

The following are the valid components of an FDB entry state (more than
one may be valid at the same time):

.B permanent
Associated with the generic NUD_PERMANENT state, which means that the L2
address of the neighbor has been statically configured by the user and
therefore there is no need for a neighbour resolution.
For the bridge FDB, it means that an FDB entry is 'local', i.e. the L2
address belongs to a local interface.

.B reachable
Associated with the generic NUD_REACHABLE state, which means that the L2
address has been resolved by the neighbouring protocol. A reachable
bridge FDB entry can have two sub-states (static and dynamic) detailed
below.

.B static
Associated with the generic NUD_NOARP state, which is used to denote a
neighbour for which no protocol is needed to resolve the mapping between
the L3 address and L2 address. For the bridge FDB, the neighbour
resolution protocol is source MAC address learning, therefore a static
FDB entry is one that has not been learnt.

.B dynamic
Is a NUD_REACHABLE entry that lacks the NUD_NOARP state, therefore has
been resolved through address learning.

.B stale
Associated with the generic NUD_STALE state. Denotes an FDB entry that
was last updated longer ago than the bridge's hold time, but not yet
removed. The hold time is equal to the forward_delay (if the STP
topology is still changing) or to the ageing_time otherwise.


.PP
In the resulting output, each FDB entry can have one or more of the
following flags:

.B self
This entry is present in the FDB of the specified network interface driver.

.B router
???

.B extern_learn
This entry has been added to the master interface's FDB by the lower
port driver, as a result of hardware address learning.

.B offload
This entry is present in the hardware FDB of a lower port and also
associated with an entry of the master interface.

.B master
This entry is present in the software FDB of the master interface of
this lower port.

.B sticky
This entry cannot be migrated to another port by the address learning
process.

.PP
With the
.B -statistics
option, the command becomes verbose. It prints out the last updated
and last used time for each entry.

> >  .B self
> > -- the address is associated with the port drivers fdb. Usually hardware
> > -  (default).
> > +- the operation is fulfilled directly by the driver for the specified network
> > +device. If the network device belongs to a master like a bridge, then the
> > +bridge is bypassed and not notified of this operation (and if the device does
> > +notify the bridge, it is driver-specific behavior and not mandated by this
> > +flag, check the driver for more details). The "bridge fdb add" command can also
> > +be used on the bridge device itself, and in this case, the added fdb entries
> > +will be locally terminated (not forwarded). In the latter case, the "self" flag
> > +is mandatory. 
> Maybe I misunderstand this sentence, but I can do a 'bridge fdb add' without 'self'
> on the bridge device. And the address shows up under 'bridge fdb show'.
> So what does mandatory mean here?

It's right in the next sentence:

> The flag is set by default if "master" is not specified.

It's mandatory and implicit if "master" is not specified, ergo 'bridge
fdb add dev br0' will work because 'master' is not specified (it is
implicitly 'bridge fdb add dev br0 self'. But 'bridge fdb add dev br0
master' will fail, because the 'self' flag is no longer implicit (since
'master' was specified) but mandatory and absent.

I'm not sure what I can do to improve this.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH iproute2 5/6] man8/bridge.8: explain self vs master for "bridge fdb add"
  2021-02-15 10:32     ` Vladimir Oltean
@ 2021-02-15 10:53       ` Alexandra Winter
  2021-02-15 12:13         ` Vladimir Oltean
  0 siblings, 1 reply; 12+ messages in thread
From: Alexandra Winter @ 2021-02-15 10:53 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: David Ahern, netdev, Andrew Lunn, Florian Fainelli,
	Vivien Didelot, Jiri Pirko, Ido Schimmel, DENG Qingfang,
	Tobias Waldekranz, Roopa Prabhu, Nikolay Aleksandrov,
	Stephen Hemminger



On 15.02.21 11:32, Vladimir Oltean wrote:
> Hi Alexandra,
> 
> On Mon, Feb 15, 2021 at 09:22:47AM +0100, Alexandra Winter wrote:
>> In the section about 'bridge link set' Self vs master mention physical
>> device vs software bridge. Would it make sense to use the same
>> terminology here?
> 
> You mean like this?
> 
> .TP
> .B self
> operation is fulfilled by the driver of the specified network interface.
> 
> .TP
> .B master
> operation is fulfilled by the specified interface's master, for example
> a bridge, which in turn may or may not notify the underlying network
> interface driver. This flag is considered implicit by the kernel if
> 'self' was not specified.
> 
Actually, I found your first (more verbose) proposal more helpful. 

>> The attributes are listed under 'bridge fdb add' not under 'bridge fdb
>> show'. Is it correct that the attributes displayed by 'show' are a
>> 1-to-1 representation of the ones set by 'add'?
> 
> Bah, not quite. I'll try to summarize below.
> 
>> What about the entries that are not manually set, like bridge learned
>> adresses? Is it possible to add some explanation about those as well?
> 
> Ok, challenge accepted. Here's my take on 'bridge fdb show', I haven't
> used most of these options (I'm commenting solely based on code
> inspection) so if anybody with more experience could chime in, I'd be
> happy to adjust the wording.
> 
> 
> .SS bridge fdb show - list forwarding entries.
> 
> This command displays the current forwarding table. By default all FDB
> entries in the system are shown. The following options can be used to
> reduce the number of displayed entries:
> 
> .TP
> .B br
> Filter the output to contain only the FDB entries of the specified
> bridge, or belonging to ports of the specified bridge (optional).
> 
> .B brport
> Filter the output to contain only the FDB entries present on the
> specified network interface (bridge port). This flag is optional.
> 
> .B dev
> Same as "brport".
> 
> .B vlan
> Filter the output to contain only the FDB entries with the specified
> VLAN ID (optional).
> 
> .B dynamic
> Filter out the local/permanent (not forwarded) FDB entries.
> 
> .B state
> Filter the output to contain only the FDB entries having the specified
> state. The bridge FDB is modeled as a neighbouring protocol for
> PF_BRIDGE (similar to what ARP is for IPv4 and ND is for IPv6).
> Therefore, an FDB entry has a NUD ("Network Unreachability Detection")
> state given by the generic neighbouring layer.
> 
> The following are the valid components of an FDB entry state (more than
> one may be valid at the same time):
> 
> .B permanent
> Associated with the generic NUD_PERMANENT state, which means that the L2
> address of the neighbor has been statically configured by the user and
> therefore there is no need for a neighbour resolution.
> For the bridge FDB, it means that an FDB entry is 'local', i.e. the L2
> address belongs to a local interface.
> 
> .B reachable
> Associated with the generic NUD_REACHABLE state, which means that the L2
> address has been resolved by the neighbouring protocol. A reachable
> bridge FDB entry can have two sub-states (static and dynamic) detailed
> below.
> 
> .B static
> Associated with the generic NUD_NOARP state, which is used to denote a
> neighbour for which no protocol is needed to resolve the mapping between
> the L3 address and L2 address. For the bridge FDB, the neighbour
> resolution protocol is source MAC address learning, therefore a static
> FDB entry is one that has not been learnt.
> 
> .B dynamic
> Is a NUD_REACHABLE entry that lacks the NUD_NOARP state, therefore has
> been resolved through address learning.
> 
> .B stale
> Associated with the generic NUD_STALE state. Denotes an FDB entry that
> was last updated longer ago than the bridge's hold time, but not yet
> removed. The hold time is equal to the forward_delay (if the STP
> topology is still changing) or to the ageing_time otherwise.
> 
> 
> .PP
> In the resulting output, each FDB entry can have one or more of the
> following flags:
> 
> .B self
> This entry is present in the FDB of the specified network interface driver.
> 
> .B router
> ???
> 
> .B extern_learn
> This entry has been added to the master interface's FDB by the lower
> port driver, as a result of hardware address learning.
> 
> .B offload
> This entry is present in the hardware FDB of a lower port and also
> associated with an entry of the master interface.
> 
> .B master
> This entry is present in the software FDB of the master interface of
> this lower port.
> 
> .B sticky
> This entry cannot be migrated to another port by the address learning
> process.
> 
> .PP
> With the
> .B -statistics
> option, the command becomes verbose. It prints out the last updated
> and last used time for each entry.
> 
Thank you so much!! This will be very helpful.

>>>  .B self
>>> -- the address is associated with the port drivers fdb. Usually hardware
>>> -  (default).
>>> +- the operation is fulfilled directly by the driver for the specified network
>>> +device. If the network device belongs to a master like a bridge, then the
>>> +bridge is bypassed and not notified of this operation (and if the device does
>>> +notify the bridge, it is driver-specific behavior and not mandated by this
>>> +flag, check the driver for more details). The "bridge fdb add" command can also
>>> +be used on the bridge device itself, and in this case, the added fdb entries
>>> +will be locally terminated (not forwarded). In the latter case, the "self" flag
>>> +is mandatory. 
>> Maybe I misunderstand this sentence, but I can do a 'bridge fdb add' without 'self'
>> on the bridge device. And the address shows up under 'bridge fdb show'.
>> So what does mandatory mean here?
> 
> It's right in the next sentence:
> 
>> The flag is set by default if "master" is not specified.
> 
> It's mandatory and implicit if "master" is not specified, ergo 'bridge
> fdb add dev br0' will work because 'master' is not specified (it is
> implicitly 'bridge fdb add dev br0 self'. But 'bridge fdb add dev br0
> master' will fail, because the 'self' flag is no longer implicit (since
> 'master' was specified) but mandatory and absent.
> 
> I'm not sure what I can do to improve this.
> 
Maybe the sentence under 'master':
" If the specified
+device is a master itself, such as a bridge, this flag is invalid."
is sufficient to defien this situation. And no need to explain mandatory implicit defaults
in the first paragraph?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH iproute2 5/6] man8/bridge.8: explain self vs master for "bridge fdb add"
  2021-02-15 10:53       ` Alexandra Winter
@ 2021-02-15 12:13         ` Vladimir Oltean
  2021-02-15 12:33           ` Alexandra Winter
  0 siblings, 1 reply; 12+ messages in thread
From: Vladimir Oltean @ 2021-02-15 12:13 UTC (permalink / raw)
  To: Alexandra Winter
  Cc: David Ahern, netdev, Andrew Lunn, Florian Fainelli,
	Vivien Didelot, Jiri Pirko, Ido Schimmel, DENG Qingfang,
	Tobias Waldekranz, Roopa Prabhu, Nikolay Aleksandrov,
	Stephen Hemminger

On Mon, Feb 15, 2021 at 11:53:42AM +0100, Alexandra Winter wrote:
> Actually, I found your first (more verbose) proposal more helpful.

Sorry, I don't understand. Do you want me to copy the whole explanation
from bridge fdb add to bridge link set?

> >> Maybe I misunderstand this sentence, but I can do a 'bridge fdb add' without 'self'
> >> on the bridge device. And the address shows up under 'bridge fdb show'.
> >> So what does mandatory mean here?
> >
> > It's right in the next sentence:
> >
> >> The flag is set by default if "master" is not specified.
> >
> > It's mandatory and implicit if "master" is not specified, ergo 'bridge
> > fdb add dev br0' will work because 'master' is not specified (it is
> > implicitly 'bridge fdb add dev br0 self'. But 'bridge fdb add dev br0
> > master' will fail, because the 'self' flag is no longer implicit (since
> > 'master' was specified) but mandatory and absent.
> >
> > I'm not sure what I can do to improve this.
> >
> Maybe the sentence under 'master':
> " If the specified
> +device is a master itself, such as a bridge, this flag is invalid."
> is sufficient to defien this situation. And no need to explain mandatory implicit defaults
> in the first paragraph?

I don't understand this either. Could you paste here how you think this
paragraph should read?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH iproute2 5/6] man8/bridge.8: explain self vs master for "bridge fdb add"
  2021-02-15 12:13         ` Vladimir Oltean
@ 2021-02-15 12:33           ` Alexandra Winter
  0 siblings, 0 replies; 12+ messages in thread
From: Alexandra Winter @ 2021-02-15 12:33 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: David Ahern, netdev, Andrew Lunn, Florian Fainelli,
	Vivien Didelot, Jiri Pirko, Ido Schimmel, DENG Qingfang,
	Tobias Waldekranz, Roopa Prabhu, Nikolay Aleksandrov,
	Stephen Hemminger



On 15.02.21 13:13, Vladimir Oltean wrote:
> On Mon, Feb 15, 2021 at 11:53:42AM +0100, Alexandra Winter wrote:
>> Actually, I found your first (more verbose) proposal more helpful.
> 
> Sorry, I don't understand. Do you want me to copy the whole explanation
> from bridge fdb add to bridge link set?
> 
>>>> Maybe I misunderstand this sentence, but I can do a 'bridge fdb add' without 'self'
>>>> on the bridge device. And the address shows up under 'bridge fdb show'.
>>>> So what does mandatory mean here?
>>>
>>> It's right in the next sentence:
>>>
>>>> The flag is set by default if "master" is not specified.
>>>
>>> It's mandatory and implicit if "master" is not specified, ergo 'bridge
>>> fdb add dev br0' will work because 'master' is not specified (it is
>>> implicitly 'bridge fdb add dev br0 self'. But 'bridge fdb add dev br0
>>> master' will fail, because the 'self' flag is no longer implicit (since
>>> 'master' was specified) but mandatory and absent.
>>>
>>> I'm not sure what I can do to improve this.
>>>
>> Maybe the sentence under 'master':
>> " If the specified
>> +device is a master itself, such as a bridge, this flag is invalid."
>> is sufficient to defien this situation. And no need to explain mandatory implicit defaults
>> in the first paragraph?
> 
> I don't understand this either. Could you paste here how you think this
> paragraph should read?
> 
Sorry, I did not mean to cause confusion. Your original proposal:
 .B self
-- the address is associated with the port drivers fdb. Usually hardware
-  (default).
+- the operation is fulfilled directly by the driver for the specified network
+device. If the network device belongs to a master like a bridge, then the
+bridge is bypassed and not notified of this operation (and if the device does
+notify the bridge, it is driver-specific behavior and not mandated by this
+flag, check the driver for more details). The "bridge fdb add" command can also
+be used on the bridge device itself, and in this case, the added fdb entries
+will be locally terminated (not forwarded). In the latter case, the "self" flag
+is mandatory. The flag is set by default if "master" is not specified.
 .sp
 
 .B master
-- the address is associated with master devices fdb. Usually software.
+- if the specified network device is a port that belongs to a master device
+such as a bridge, the operation is fulfilled by the master device's driver,
+which may in turn notify the port driver too of the address. If the specified
+device is a master itself, such as a bridge, this flag is invalid.
 .sp


The above is fine with me and IMHO much better than it is today.
But if you ask me I would change it to:

 .B self
- the operation is fulfilled directly by the driver for the specified physical device. 
If the network device belongs to a master like a bridge, then the
bridge is bypassed and not notified of this operation (and if the device does
notify the bridge, it is driver-specific behavior and not mandated by this
flag, check the driver for more details). The "bridge fdb add" command can also
be used on the bridge device itself, and in this case, the added fdb entries
will be locally terminated (not forwarded). The flag is set by default if "master" 
is not specified.
 .sp
 
 .B master
- if the specified network device is a port that belongs to a master device
such as a software bridge, the operation is fulfilled by the master device's driver,
which may in turn notify the port driver too of the address. If the specified
device is a master itself, such as a bridge, this flag is invalid.
 .sp



^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-02-15 12:35 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-11 10:44 [PATCH iproute2 0/6] Clarifications to bridge man page Vladimir Oltean
2021-02-11 10:44 ` [PATCH iproute2 1/6] man8/bridge.8: document the "permanent" flag for "bridge fdb add" Vladimir Oltean
2021-02-11 10:44 ` [PATCH iproute2 2/6] man8/bridge.8: document that "local" is default " Vladimir Oltean
2021-02-11 10:44 ` [PATCH iproute2 3/6] man8/bridge.8: explain what a local FDB entry is Vladimir Oltean
2021-02-11 10:45 ` [PATCH iproute2 4/6] man8/bridge.8: fix which one of self/master is default for "bridge fdb" Vladimir Oltean
2021-02-11 10:45 ` [PATCH iproute2 5/6] man8/bridge.8: explain self vs master for "bridge fdb add" Vladimir Oltean
2021-02-15  8:22   ` Alexandra Winter
2021-02-15 10:32     ` Vladimir Oltean
2021-02-15 10:53       ` Alexandra Winter
2021-02-15 12:13         ` Vladimir Oltean
2021-02-15 12:33           ` Alexandra Winter
2021-02-11 10:45 ` [PATCH iproute2 6/6] man8/bridge.8: be explicit that "flood" is an egress setting Vladimir Oltean

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.