| Message ID | 20230801152138.132719-1-idosch@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [iproute2-next] bridge: Add backup nexthop ID support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
Ido Schimmel <idosch@nvidia.com> writes: > diff --git a/bridge/link.c b/bridge/link.c > index b35429866f52..c7ee5e760c08 100644 > --- a/bridge/link.c > +++ b/bridge/link.c > @@ -186,6 +186,10 @@ static void print_protinfo(FILE *fp, struct rtattr *attr) > ll_index_to_name(ifidx)); > } > > + if (prtb[IFLA_BRPORT_BACKUP_NHID]) > + print_uint(PRINT_ANY, "backup_nhid", "backup_nhid %u ", > + rta_getattr_u32(prtb[IFLA_BRPORT_BACKUP_NHID])); > + This doesn't build on current main. I think we usually send the relevant header sync patch, but maybe there's an assumption the maintainer pushes it _before_ this patch? I'm not sure, just calling it out. > if (prtb[IFLA_BRPORT_ISOLATED]) > print_on_off(PRINT_ANY, "isolated", "isolated %s ", > rta_getattr_u8(prtb[IFLA_BRPORT_ISOLATED])); > @@ -311,6 +315,7 @@ static void usage(void) > " [ mab {on | off} ]\n" > " [ hwmode {vepa | veb} ]\n" > " [ backup_port DEVICE ] [ nobackup_port ]\n" > + " [ backup_nhid NHID ]\n" I thought about whether there should be "nobackup_nhid", but no. The corresponding nobackup_port is necessary because it would be awkward to specify "backup_port ''" or something. No such issue with NHID. > " [ self ] [ master ]\n" > " bridge link show [dev DEV]\n"); > exit(-1); > @@ -330,6 +335,7 @@ static int brlink_modify(int argc, char **argv) > }; > char *d = NULL; > int backup_port_idx = -1; > + __s32 backup_nhid = -1; > __s8 neigh_suppress = -1; > __s8 neigh_vlan_suppress = -1; > __s8 learning = -1; > @@ -493,6 +499,10 @@ static int brlink_modify(int argc, char **argv) > } > } else if (strcmp(*argv, "nobackup_port") == 0) { > backup_port_idx = 0; > + } else if (strcmp(*argv, "backup_nhid") == 0) { > + NEXT_ARG(); > + if (get_s32(&backup_nhid, *argv, 0)) > + invarg("invalid backup_nhid", *argv); Not sure about that s32. NHID's are unsigned in general. I can add a NHID of 0xffffffff just fine: # ip nexthop add id 0xffffffff via 192.0.2.3 dev Xd (Though ip nexthop show then loops endlessly probably because -1 is used as a sentinel in the dump code. Oops!) IMHO the tool should allow configuring this. You allow full u32 range for the "ip" tool, no need for "bridge" to be arbitrarily limited.
On Wed, Aug 02, 2023 at 11:55:26AM +0200, Petr Machata wrote: > > Ido Schimmel <idosch@nvidia.com> writes: > > > diff --git a/bridge/link.c b/bridge/link.c > > index b35429866f52..c7ee5e760c08 100644 > > --- a/bridge/link.c > > +++ b/bridge/link.c > > @@ -186,6 +186,10 @@ static void print_protinfo(FILE *fp, struct rtattr *attr) > > ll_index_to_name(ifidx)); > > } > > > > + if (prtb[IFLA_BRPORT_BACKUP_NHID]) > > + print_uint(PRINT_ANY, "backup_nhid", "backup_nhid %u ", > > + rta_getattr_u32(prtb[IFLA_BRPORT_BACKUP_NHID])); > > + > > This doesn't build on current main. I think we usually send the relevant > header sync patch, but maybe there's an assumption the maintainer pushes > it _before_ this patch? I'm not sure, just calling it out. Not needed. David syncs the headers himself. > > > if (prtb[IFLA_BRPORT_ISOLATED]) > > print_on_off(PRINT_ANY, "isolated", "isolated %s ", > > rta_getattr_u8(prtb[IFLA_BRPORT_ISOLATED])); > > @@ -311,6 +315,7 @@ static void usage(void) > > " [ mab {on | off} ]\n" > > " [ hwmode {vepa | veb} ]\n" > > " [ backup_port DEVICE ] [ nobackup_port ]\n" > > + " [ backup_nhid NHID ]\n" > > I thought about whether there should be "nobackup_nhid", but no. The > corresponding nobackup_port is necessary because it would be awkward to > specify "backup_port ''" or something. No such issue with NHID. > > > " [ self ] [ master ]\n" > > " bridge link show [dev DEV]\n"); > > exit(-1); > > @@ -330,6 +335,7 @@ static int brlink_modify(int argc, char **argv) > > }; > > char *d = NULL; > > int backup_port_idx = -1; > > + __s32 backup_nhid = -1; > > __s8 neigh_suppress = -1; > > __s8 neigh_vlan_suppress = -1; > > __s8 learning = -1; > > @@ -493,6 +499,10 @@ static int brlink_modify(int argc, char **argv) > > } > > } else if (strcmp(*argv, "nobackup_port") == 0) { > > backup_port_idx = 0; > > + } else if (strcmp(*argv, "backup_nhid") == 0) { > > + NEXT_ARG(); > > + if (get_s32(&backup_nhid, *argv, 0)) > > + invarg("invalid backup_nhid", *argv); > > Not sure about that s32. NHID's are unsigned in general. I can add a > NHID of 0xffffffff just fine: > > # ip nexthop add id 0xffffffff via 192.0.2.3 dev Xd > > (Though ip nexthop show then loops endlessly probably because -1 is used > as a sentinel in the dump code. Oops!) > > IMHO the tool should allow configuring this. You allow full u32 range > for the "ip" tool, no need for "bridge" to be arbitrarily limited. What about the diff below? diff --git a/bridge/link.c b/bridge/link.c index c7ee5e760c08..4bf806c5be61 100644 --- a/bridge/link.c +++ b/bridge/link.c @@ -334,8 +334,9 @@ static int brlink_modify(int argc, char **argv) .ifm.ifi_family = PF_BRIDGE, }; char *d = NULL; + bool backup_nhid_set = false; + __u32 backup_nhid; int backup_port_idx = -1; - __s32 backup_nhid = -1; __s8 neigh_suppress = -1; __s8 neigh_vlan_suppress = -1; __s8 learning = -1; @@ -501,8 +502,9 @@ static int brlink_modify(int argc, char **argv) backup_port_idx = 0; } else if (strcmp(*argv, "backup_nhid") == 0) { NEXT_ARG(); - if (get_s32(&backup_nhid, *argv, 0)) + if (get_u32(&backup_nhid, *argv, 0)) invarg("invalid backup_nhid", *argv); + backup_nhid_set = true; } else { usage(); } @@ -589,7 +591,7 @@ static int brlink_modify(int argc, char **argv) addattr32(&req.n, sizeof(req), IFLA_BRPORT_BACKUP_PORT, backup_port_idx); - if (backup_nhid != -1) + if (backup_nhid_set) addattr32(&req.n, sizeof(req), IFLA_BRPORT_BACKUP_NHID, backup_nhid);
Ido Schimmel <idosch@idosch.org> writes: > On Wed, Aug 02, 2023 at 11:55:26AM +0200, Petr Machata wrote: >> >> Ido Schimmel <idosch@nvidia.com> writes: >> >> > @@ -493,6 +499,10 @@ static int brlink_modify(int argc, char **argv) >> > } >> > } else if (strcmp(*argv, "nobackup_port") == 0) { >> > backup_port_idx = 0; >> > + } else if (strcmp(*argv, "backup_nhid") == 0) { >> > + NEXT_ARG(); >> > + if (get_s32(&backup_nhid, *argv, 0)) >> > + invarg("invalid backup_nhid", *argv); >> >> Not sure about that s32. NHID's are unsigned in general. I can add a >> NHID of 0xffffffff just fine: >> >> # ip nexthop add id 0xffffffff via 192.0.2.3 dev Xd >> >> (Though ip nexthop show then loops endlessly probably because -1 is used >> as a sentinel in the dump code. Oops!) >> >> IMHO the tool should allow configuring this. You allow full u32 range >> for the "ip" tool, no need for "bridge" to be arbitrarily limited. > > What about the diff below? > > diff --git a/bridge/link.c b/bridge/link.c > index c7ee5e760c08..4bf806c5be61 100644 > --- a/bridge/link.c > +++ b/bridge/link.c > @@ -334,8 +334,9 @@ static int brlink_modify(int argc, char **argv) > .ifm.ifi_family = PF_BRIDGE, > }; > char *d = NULL; > + bool backup_nhid_set = false; > + __u32 backup_nhid; > int backup_port_idx = -1; > - __s32 backup_nhid = -1; > __s8 neigh_suppress = -1; > __s8 neigh_vlan_suppress = -1; > __s8 learning = -1; > @@ -501,8 +502,9 @@ static int brlink_modify(int argc, char **argv) > backup_port_idx = 0; > } else if (strcmp(*argv, "backup_nhid") == 0) { > NEXT_ARG(); > - if (get_s32(&backup_nhid, *argv, 0)) > + if (get_u32(&backup_nhid, *argv, 0)) > invarg("invalid backup_nhid", *argv); > + backup_nhid_set = true; > } else { > usage(); > } > @@ -589,7 +591,7 @@ static int brlink_modify(int argc, char **argv) > addattr32(&req.n, sizeof(req), IFLA_BRPORT_BACKUP_PORT, > backup_port_idx); > > - if (backup_nhid != -1) > + if (backup_nhid_set) > addattr32(&req.n, sizeof(req), IFLA_BRPORT_BACKUP_NHID, > backup_nhid); Yep, that's what I had in mind. With that: Reviewed-by: Petr Machata <petrm@nvidia.com>
On 8/2/23 6:34 AM, Ido Schimmel wrote: > On Wed, Aug 02, 2023 at 11:55:26AM +0200, Petr Machata wrote: >> >> Ido Schimmel <idosch@nvidia.com> writes: >> >>> diff --git a/bridge/link.c b/bridge/link.c >>> index b35429866f52..c7ee5e760c08 100644 >>> --- a/bridge/link.c >>> +++ b/bridge/link.c >>> @@ -186,6 +186,10 @@ static void print_protinfo(FILE *fp, struct rtattr *attr) >>> ll_index_to_name(ifidx)); >>> } >>> >>> + if (prtb[IFLA_BRPORT_BACKUP_NHID]) >>> + print_uint(PRINT_ANY, "backup_nhid", "backup_nhid %u ", >>> + rta_getattr_u32(prtb[IFLA_BRPORT_BACKUP_NHID])); >>> + >> >> This doesn't build on current main. I think we usually send the relevant >> header sync patch, but maybe there's an assumption the maintainer pushes >> it _before_ this patch? I'm not sure, just calling it out. > > Not needed. David syncs the headers himself. just pushed the update. > >> >>> if (prtb[IFLA_BRPORT_ISOLATED]) >>> print_on_off(PRINT_ANY, "isolated", "isolated %s ", >>> rta_getattr_u8(prtb[IFLA_BRPORT_ISOLATED])); >>> @@ -311,6 +315,7 @@ static void usage(void) >>> " [ mab {on | off} ]\n" >>> " [ hwmode {vepa | veb} ]\n" >>> " [ backup_port DEVICE ] [ nobackup_port ]\n" >>> + " [ backup_nhid NHID ]\n" >> >> I thought about whether there should be "nobackup_nhid", but no. The >> corresponding nobackup_port is necessary because it would be awkward to >> specify "backup_port ''" or something. No such issue with NHID. >> >>> " [ self ] [ master ]\n" >>> " bridge link show [dev DEV]\n"); >>> exit(-1); >>> @@ -330,6 +335,7 @@ static int brlink_modify(int argc, char **argv) >>> }; >>> char *d = NULL; >>> int backup_port_idx = -1; >>> + __s32 backup_nhid = -1; >>> __s8 neigh_suppress = -1; >>> __s8 neigh_vlan_suppress = -1; >>> __s8 learning = -1; >>> @@ -493,6 +499,10 @@ static int brlink_modify(int argc, char **argv) >>> } >>> } else if (strcmp(*argv, "nobackup_port") == 0) { >>> backup_port_idx = 0; >>> + } else if (strcmp(*argv, "backup_nhid") == 0) { >>> + NEXT_ARG(); >>> + if (get_s32(&backup_nhid, *argv, 0)) >>> + invarg("invalid backup_nhid", *argv); >> >> Not sure about that s32. NHID's are unsigned in general. I can add a >> NHID of 0xffffffff just fine: >> >> # ip nexthop add id 0xffffffff via 192.0.2.3 dev Xd >> >> (Though ip nexthop show then loops endlessly probably because -1 is used >> as a sentinel in the dump code. Oops!) ugh, please send a fix.
On Wed, Aug 02, 2023 at 09:35:12AM -0600, David Ahern wrote:
> ugh, please send a fix.
Will send a fix tomorrow. Seems to be fixed by [1], but we might need
something similar for the bucket dump.
[1]
diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index 93f14d39fef6..1bd92acbc6c5 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -3170,7 +3170,7 @@ static int nh_valid_dump_req(const struct nlmsghdr *nlh,
}
struct rtm_dump_nh_ctx {
- u32 idx;
+ u64 idx;
};
static struct rtm_dump_nh_ctx *
@@ -3192,7 +3192,7 @@ static int rtm_dump_walk_nexthops(struct sk_buff *skb,
void *data)
{
struct rb_node *node;
- int s_idx;
+ u64 s_idx;
int err;
s_idx = ctx->idx;
diff --git a/bridge/link.c b/bridge/link.c index b35429866f52..c7ee5e760c08 100644 --- a/bridge/link.c +++ b/bridge/link.c @@ -186,6 +186,10 @@ static void print_protinfo(FILE *fp, struct rtattr *attr) ll_index_to_name(ifidx)); } + if (prtb[IFLA_BRPORT_BACKUP_NHID]) + print_uint(PRINT_ANY, "backup_nhid", "backup_nhid %u ", + rta_getattr_u32(prtb[IFLA_BRPORT_BACKUP_NHID])); + if (prtb[IFLA_BRPORT_ISOLATED]) print_on_off(PRINT_ANY, "isolated", "isolated %s ", rta_getattr_u8(prtb[IFLA_BRPORT_ISOLATED])); @@ -311,6 +315,7 @@ static void usage(void) " [ mab {on | off} ]\n" " [ hwmode {vepa | veb} ]\n" " [ backup_port DEVICE ] [ nobackup_port ]\n" + " [ backup_nhid NHID ]\n" " [ self ] [ master ]\n" " bridge link show [dev DEV]\n"); exit(-1); @@ -330,6 +335,7 @@ static int brlink_modify(int argc, char **argv) }; char *d = NULL; int backup_port_idx = -1; + __s32 backup_nhid = -1; __s8 neigh_suppress = -1; __s8 neigh_vlan_suppress = -1; __s8 learning = -1; @@ -493,6 +499,10 @@ static int brlink_modify(int argc, char **argv) } } else if (strcmp(*argv, "nobackup_port") == 0) { backup_port_idx = 0; + } else if (strcmp(*argv, "backup_nhid") == 0) { + NEXT_ARG(); + if (get_s32(&backup_nhid, *argv, 0)) + invarg("invalid backup_nhid", *argv); } else { usage(); } @@ -579,6 +589,10 @@ static int brlink_modify(int argc, char **argv) addattr32(&req.n, sizeof(req), IFLA_BRPORT_BACKUP_PORT, backup_port_idx); + if (backup_nhid != -1) + addattr32(&req.n, sizeof(req), IFLA_BRPORT_BACKUP_NHID, + backup_nhid); + addattr_nest_end(&req.n, nest); /* IFLA_AF_SPEC nested attribute. Contains IFLA_BRIDGE_FLAGS that diff --git a/ip/iplink_bridge_slave.c b/ip/iplink_bridge_slave.c index 11ab2113fe96..dc73c86574da 100644 --- a/ip/iplink_bridge_slave.c +++ b/ip/iplink_bridge_slave.c @@ -43,6 +43,7 @@ static void print_explain(FILE *f) " [ locked {on | off} ]\n" " [ mab {on | off} ]\n" " [ backup_port DEVICE ] [ nobackup_port ]\n" + " [ backup_nhid NHID ]\n" ); } @@ -301,6 +302,10 @@ static void bridge_slave_print_opt(struct link_util *lu, FILE *f, print_string(PRINT_ANY, "backup_port", "backup_port %s ", ll_index_to_name(backup_p)); } + + if (tb[IFLA_BRPORT_BACKUP_NHID]) + print_uint(PRINT_ANY, "backup_nhid", "backup_nhid %u ", + rta_getattr_u32(tb[IFLA_BRPORT_BACKUP_NHID])); } static void bridge_slave_parse_on_off(char *arg_name, char *arg_val, @@ -436,6 +441,14 @@ static int bridge_slave_parse_opt(struct link_util *lu, int argc, char **argv, addattr32(n, 1024, IFLA_BRPORT_BACKUP_PORT, ifindex); } else if (matches(*argv, "nobackup_port") == 0) { addattr32(n, 1024, IFLA_BRPORT_BACKUP_PORT, 0); + } else if (strcmp(*argv, "backup_nhid") == 0) { + __u32 backup_nhid; + + NEXT_ARG(); + if (get_u32(&backup_nhid, *argv, 0)) + invarg("backup_nhid is invalid", *argv); + addattr32(n, 1024, IFLA_BRPORT_BACKUP_NHID, + backup_nhid); } else if (matches(*argv, "help") == 0) { explain(); return -1; diff --git a/man/man8/bridge.8 b/man/man8/bridge.8 index e05528199eab..dd0659d37df2 100644 --- a/man/man8/bridge.8 +++ b/man/man8/bridge.8 @@ -61,6 +61,8 @@ bridge \- show / manipulate bridge addresses and devices .B backup_port .IR DEVICE " ] [" .BR nobackup_port " ] [ " +.B backup_nhid +.IR NHID " ] [" .BR self " ] [ " master " ]" .ti -8 @@ -647,6 +649,13 @@ configured backup port .B nobackup_port Removes the currently configured backup port +.TP +.BI backup_nhid " NHID" +The FDB nexthop object ID (see \fBip-nexthop\fR(8)) to attach to packets being +redirected to a backup port that has VLAN tunnel mapping enabled (via the +\fBvlan_tunnel\fR option). Setting a value of 0 (default) has the effect of not +attaching any ID. + .TP .B self link setting is configured on specified physical device diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in index 6a82ddc45adf..128c855f82be 100644 --- a/man/man8/ip-link.8.in +++ b/man/man8/ip-link.8.in @@ -2539,7 +2539,10 @@ the following additional arguments are supported: ] [ .BR backup_port " DEVICE" ] [ -.BR nobackup_port " ]" +.BR nobackup_port +] [ +.BR backup_nhid " NHID" +] .in +8 .sp @@ -2678,6 +2681,12 @@ configured backup port .BR nobackup_port - removes the currently configured backup port +.BI backup_nhid " NHID" +- the FDB nexthop object ID (see \fBip-nexthop\fR(8)) to attach to packets +being redirected to a backup port that has VLAN tunnel mapping enabled (via the +\fBvlan_tunnel\fR option). Setting a value of 0 (default) has the effect of not +attaching any ID. + .in -8 .TP
Extend the bridge and ip utilities to set and show the backup nexthop ID bridge port attribute. A value of 0 (default) disables the feature, in which case the attribute is not printed since it is not emitted by the kernel. Example: # bridge -d link show dev swp1 | grep -o "backup_nhid [0-9]*" # bridge -d -j -p link show dev swp1 | jq '.[]["backup_nhid"]' null # bridge link set dev swp1 backup_nhid 10 # bridge -d link show dev swp1 | grep -o "backup_nhid [0-9]*" backup_nhid 10 # bridge -d -j -p link show dev swp1 | jq '.[]["backup_nhid"]' 10 # bridge link set dev swp1 backup_nhid 0 # bridge -d link show dev swp1 | grep -o "backup_nhid [0-9]*" # bridge -d -j -p link show dev swp1 | jq '.[]["backup_nhid"]' null # ip -d link show dev swp1 | grep -o "backup_nhid [0-9]*" # ip -d -j -p lin show dev swp1 | jq '.[]["linkinfo"]["info_slave_data"]["backup_nhid"]' null # ip link set dev swp1 type bridge_slave backup_nhid 10 # ip -d link show dev swp1 | grep -o "backup_nhid [0-9]*" backup_nhid 10 # ip -d -j -p lin show dev swp1 | jq '.[]["linkinfo"]["info_slave_data"]["backup_nhid"]' 10 # ip link set dev swp1 type bridge_slave backup_nhid 0 # ip -d link show dev swp1 | grep -o "backup_nhid [0-9]*" # ip -d -j -p lin show dev swp1 | jq '.[]["linkinfo"]["info_slave_data"]["backup_nhid"]' null Signed-off-by: Ido Schimmel <idosch@nvidia.com> --- bridge/link.c | 14 ++++++++++++++ ip/iplink_bridge_slave.c | 13 +++++++++++++ man/man8/bridge.8 | 9 +++++++++ man/man8/ip-link.8.in | 11 ++++++++++- 4 files changed, 46 insertions(+), 1 deletion(-)