| Message ID | 20221025163741.6811-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Commit | b22c4d84038c3eceab9486984e601b2f979ebe6d |
| Headers | show |
| Series | [ovs-dev] netdev: Assume default link speed to be 10 Gbps instead of 100 Mbps. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Tue, Oct 25, 2022 at 12:38 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > 100 Mbps was a fair assumption 13 years ago. Modern days 10 Gbps seems > like a good value in case no information is available otherwise. > > The change mainly affects QoS which is currently limited to 100 Mbps if > the user didn't specify 'max-rate' and the card doesn't report the > speed or OVS doesn't have a predefined enumeration for the speed > reported by the NIC. > > Calculation of the path cost for STP/RSTP is also affected if OVS is > unable to determine the link speed. > > Lower link speed adapters are typically good at reporting their speed, > so chances for overshoot should be low. But newer high-speed adapters, > for which there is no speed enumeration or if there are some other > issues, will not suffer that much. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> I think this is a reasonable change, 100MB has mostly been relegated to embedded devices for a long time. Acked-by: Mike Pattrick <mkp@redhat.com> Slightly related, I noticed that we're missing support for 20 and 56Gbps, we should probably add those as well. -M > --- > NEWS | 4 ++++ > include/openvswitch/netdev.h | 2 ++ > lib/netdev-linux.c | 4 ++-- > lib/rstp.c | 2 +- > lib/rstp.h | 2 +- > lib/stp.c | 4 ++-- > tests/stp.at | 14 +++++++------- > tests/test-rstp.c | 7 +++++-- > vswitchd/bridge.c | 4 ++-- > vswitchd/vswitch.xml | 2 +- > 10 files changed, 27 insertions(+), 18 deletions(-) > > diff --git a/NEWS b/NEWS > index ff77ee404..3ae6882d5 100644 > --- a/NEWS > +++ b/NEWS > @@ -23,6 +23,10 @@ Post-v3.0.0 > bug and CVE fixes addressed since its release. > If a user wishes to benefit from these fixes it is recommended to use > DPDK 21.11.2. > + - For the QoS max-rate and STP/RSTP path-cost configuration OVS now assumes > + 10 Gbps link speed by default in case the actual link speed cannot be > + determined. Previously it was 10 Mbps. Values can still be overridden > + by specifying 'max-rate' or '[r]stp-path-cost' accordingly. > > > v3.0.0 - 15 Aug 2022 > diff --git a/include/openvswitch/netdev.h b/include/openvswitch/netdev.h > index 0c10f7b48..cf48f8691 100644 > --- a/include/openvswitch/netdev.h > +++ b/include/openvswitch/netdev.h > @@ -121,6 +121,8 @@ enum netdev_features { > NETDEV_F_PAUSE_ASYM = 1 << 15, /* Asymmetric pause. */ > }; > > +#define NETDEV_DEFAULT_BPS UINT64_C(10 * 1000 * 1000 * 1000) > + > int netdev_get_features(const struct netdev *, > enum netdev_features *current, > enum netdev_features *advertised, > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index cdc66246c..801ecd065 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -4696,7 +4696,7 @@ htb_parse_qdisc_details__(struct netdev *netdev_, > > netdev_linux_read_features(netdev); > current = !netdev->get_features_error ? netdev->current : 0; > - hc->max_rate = netdev_features_to_bps(current, 100 * 1000 * 1000) / 8; > + hc->max_rate = netdev_features_to_bps(current, NETDEV_DEFAULT_BPS) / 8; > } > hc->min_rate = hc->max_rate; > hc->burst = 0; > @@ -5168,7 +5168,7 @@ hfsc_parse_qdisc_details__(struct netdev *netdev_, const struct smap *details, > > netdev_linux_read_features(netdev); > current = !netdev->get_features_error ? netdev->current : 0; > - max_rate = netdev_features_to_bps(current, 100 * 1000 * 1000) / 8; > + max_rate = netdev_features_to_bps(current, NETDEV_DEFAULT_BPS) / 8; > } > > class->min_rate = max_rate; > diff --git a/lib/rstp.c b/lib/rstp.c > index 7e351bf32..2f01966f7 100644 > --- a/lib/rstp.c > +++ b/lib/rstp.c > @@ -784,7 +784,7 @@ rstp_convert_speed_to_cost(unsigned int speed) > : speed >= 100 ? 200000 /* 100 Mb/s. */ > : speed >= 10 ? 2000000 /* 10 Mb/s. */ > : speed >= 1 ? 20000000 /* 1 Mb/s. */ > - : RSTP_DEFAULT_PORT_PATH_COST; /* 100 Mb/s. */ > + : RSTP_DEFAULT_PORT_PATH_COST; /* 10 Gb/s. */ > > return value; > } > diff --git a/lib/rstp.h b/lib/rstp.h > index 39a13b58c..13af20195 100644 > --- a/lib/rstp.h > +++ b/lib/rstp.h > @@ -84,7 +84,7 @@ struct dp_packet; > /* Port path cost [Table 17-3] */ > #define RSTP_MIN_PORT_PATH_COST 1 > #define RSTP_MAX_PORT_PATH_COST 200000000 > -#define RSTP_DEFAULT_PORT_PATH_COST 200000 > +#define RSTP_DEFAULT_PORT_PATH_COST 2000 > > /* RSTP Bridge identifier [9.2.5]. Top four most significant bits are a > * priority value. The next most significant twelve bits are a locally > diff --git a/lib/stp.c b/lib/stp.c > index a869b5f39..f37337992 100644 > --- a/lib/stp.c > +++ b/lib/stp.c > @@ -313,7 +313,7 @@ stp_create(const char *name, stp_identifier bridge_id, > for (p = stp->ports; p < &stp->ports[ARRAY_SIZE(stp->ports)]; p++) { > p->stp = stp; > p->port_id = (stp_port_no(p) + 1) | (STP_DEFAULT_PORT_PRIORITY << 8); > - p->path_cost = 19; /* Recommended default for 100 Mb/s link. */ > + p->path_cost = 2; /* Recommended default for 10 Gb/s link. */ > stp_initialize_port(p, STP_DISABLED); > } > ovs_refcount_init(&stp->ref_cnt); > @@ -989,7 +989,7 @@ stp_convert_speed_to_cost(unsigned int speed) > : speed >= 16 ? 62 /* 16 Mb/s. */ > : speed >= 10 ? 100 /* 10 Mb/s. */ > : speed >= 4 ? 250 /* 4 Mb/s. */ > - : 19; /* 100 Mb/s (guess). */ > + : 2; /* 10 Gb/s (guess). */ > ovs_mutex_unlock(&mutex); > return ret; > } > diff --git a/tests/stp.at b/tests/stp.at > index 7ddacfc3a..93da056ac 100644 > --- a/tests/stp.at > +++ b/tests/stp.at > @@ -620,10 +620,10 @@ ovs-appctl time/stop > ovs-appctl time/warp 31000 1000 > > AT_CHECK([ovs-appctl stp/show br0 | grep p1], [0], [dnl > - p1 designated forwarding 19 128.1 > + p1 designated forwarding 2 128.1 > ]) > AT_CHECK([ovs-appctl stp/show br0 | grep p2], [0], [dnl > - p2 designated forwarding 19 128.2 > + p2 designated forwarding 2 128.2 > ]) > > # add a stp port > @@ -637,10 +637,10 @@ ovs-appctl netdev-dummy/set-admin-state p3 down > > # We should not show the p3 because its link-state is down > AT_CHECK([ovs-appctl stp/show br0 | grep p1], [0], [dnl > - p1 designated forwarding 19 128.1 > + p1 designated forwarding 2 128.1 > ]) > AT_CHECK([ovs-appctl stp/show br0 | grep p2], [0], [dnl > - p2 designated forwarding 19 128.2 > + p2 designated forwarding 2 128.2 > ]) > AT_CHECK([ovs-appctl stp/show br0 | grep p3], [1], [dnl > ]) > @@ -648,13 +648,13 @@ AT_CHECK([ovs-appctl stp/show br0 | grep p3], [1], [dnl > ovs-appctl netdev-dummy/set-admin-state p3 up > > AT_CHECK([ovs-appctl stp/show br0 | grep p1], [0], [dnl > - p1 designated forwarding 19 128.1 > + p1 designated forwarding 2 128.1 > ]) > AT_CHECK([ovs-appctl stp/show br0 | grep p2], [0], [dnl > - p2 designated forwarding 19 128.2 > + p2 designated forwarding 2 128.2 > ]) > AT_CHECK([ovs-appctl stp/show br0 | grep p3], [0], [dnl > - p3 designated listening 19 128.3 > + p3 designated listening 2 128.3 > ]) > > > diff --git a/tests/test-rstp.c b/tests/test-rstp.c > index 01aeaf847..9c1026ec1 100644 > --- a/tests/test-rstp.c > +++ b/tests/test-rstp.c > @@ -107,6 +107,8 @@ send_bpdu(struct dp_packet *pkt, void *port_, void *b_) > dp_packet_delete(pkt); > } > > +#define RSTP_PORT_PATH_COST_100M 200000 > + > static struct bridge * > new_bridge(struct test_case *tc, int id) > { > @@ -122,6 +124,7 @@ new_bridge(struct test_case *tc, int id) > for (i = 1; i < MAX_PORTS; i++) { > p = rstp_add_port(b->rstp); > rstp_port_set_aux(p, p); > + rstp_port_set_path_cost(p, RSTP_PORT_PATH_COST_100M); > rstp_port_set_state(p, RSTP_DISABLED); > rstp_port_set_mac_operational(p, true); > } > @@ -544,8 +547,8 @@ test_rstp_main(int argc, char *argv[]) > } > get_token(); > > - path_cost = match(":") ? must_get_int() : > - RSTP_DEFAULT_PORT_PATH_COST; > + path_cost = match(":") ? must_get_int() > + : RSTP_PORT_PATH_COST_100M; > if (port_no < bridge->n_ports) { > /* Enable port. */ > reinitialize_port(p); > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index 25ce45e3d..28cbad514 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -1678,7 +1678,7 @@ port_configure_stp(const struct ofproto *ofproto, struct port *port, > unsigned int mbps; > > netdev_get_features(iface->netdev, ¤t, NULL, NULL, NULL); > - mbps = netdev_features_to_bps(current, 100 * 1000 * 1000) / 1000000; > + mbps = netdev_features_to_bps(current, NETDEV_DEFAULT_BPS) / 1000000; > port_s->path_cost = stp_convert_speed_to_cost(mbps); > } > > @@ -1761,7 +1761,7 @@ port_configure_rstp(const struct ofproto *ofproto, struct port *port, > unsigned int mbps; > > netdev_get_features(iface->netdev, ¤t, NULL, NULL, NULL); > - mbps = netdev_features_to_bps(current, 100 * 1000 * 1000) / 1000000; > + mbps = netdev_features_to_bps(current, NETDEV_DEFAULT_BPS) / 1000000; > port_s->path_cost = rstp_convert_speed_to_cost(mbps); > } > > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > index 36388e3c4..0b5dc42c4 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -4776,7 +4776,7 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ > Maximum rate shared by all queued traffic, in bit/s. Optional. If not > specified, for physical interfaces, the default is the link rate. For > other interfaces or if the link rate cannot be determined, the default > - is currently 100 Mbps. > + is currently 10 Gbps. > </column> > </group> > > -- > 2.37.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 11/29/22 15:37, Mike Pattrick wrote: > On Tue, Oct 25, 2022 at 12:38 PM Ilya Maximets <i.maximets@ovn.org> wrote: >> >> 100 Mbps was a fair assumption 13 years ago. Modern days 10 Gbps seems >> like a good value in case no information is available otherwise. >> >> The change mainly affects QoS which is currently limited to 100 Mbps if >> the user didn't specify 'max-rate' and the card doesn't report the >> speed or OVS doesn't have a predefined enumeration for the speed >> reported by the NIC. >> >> Calculation of the path cost for STP/RSTP is also affected if OVS is >> unable to determine the link speed. >> >> Lower link speed adapters are typically good at reporting their speed, >> so chances for overshoot should be low. But newer high-speed adapters, >> for which there is no speed enumeration or if there are some other >> issues, will not suffer that much. >> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > > I think this is a reasonable change, 100MB has mostly been relegated > to embedded devices for a long time. > > Acked-by: Mike Pattrick <mkp@redhat.com> Applied. Thanks! > > Slightly related, I noticed that we're missing support for 20 and > 56Gbps, we should probably add those as well. I've never see 56Gbps Ethernet NIC in practice, they tend to all be Infiniband ones, but maybe I'm just wrong. In any case, maybe you, Adrian, can fix that as well while working on support for 25/50/etc link speeds detection? Should be simple enough. Best regards, Ilya Maximets.
diff --git a/NEWS b/NEWS index ff77ee404..3ae6882d5 100644 --- a/NEWS +++ b/NEWS @@ -23,6 +23,10 @@ Post-v3.0.0 bug and CVE fixes addressed since its release. If a user wishes to benefit from these fixes it is recommended to use DPDK 21.11.2. + - For the QoS max-rate and STP/RSTP path-cost configuration OVS now assumes + 10 Gbps link speed by default in case the actual link speed cannot be + determined. Previously it was 10 Mbps. Values can still be overridden + by specifying 'max-rate' or '[r]stp-path-cost' accordingly. v3.0.0 - 15 Aug 2022 diff --git a/include/openvswitch/netdev.h b/include/openvswitch/netdev.h index 0c10f7b48..cf48f8691 100644 --- a/include/openvswitch/netdev.h +++ b/include/openvswitch/netdev.h @@ -121,6 +121,8 @@ enum netdev_features { NETDEV_F_PAUSE_ASYM = 1 << 15, /* Asymmetric pause. */ }; +#define NETDEV_DEFAULT_BPS UINT64_C(10 * 1000 * 1000 * 1000) + int netdev_get_features(const struct netdev *, enum netdev_features *current, enum netdev_features *advertised, diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index cdc66246c..801ecd065 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -4696,7 +4696,7 @@ htb_parse_qdisc_details__(struct netdev *netdev_, netdev_linux_read_features(netdev); current = !netdev->get_features_error ? netdev->current : 0; - hc->max_rate = netdev_features_to_bps(current, 100 * 1000 * 1000) / 8; + hc->max_rate = netdev_features_to_bps(current, NETDEV_DEFAULT_BPS) / 8; } hc->min_rate = hc->max_rate; hc->burst = 0; @@ -5168,7 +5168,7 @@ hfsc_parse_qdisc_details__(struct netdev *netdev_, const struct smap *details, netdev_linux_read_features(netdev); current = !netdev->get_features_error ? netdev->current : 0; - max_rate = netdev_features_to_bps(current, 100 * 1000 * 1000) / 8; + max_rate = netdev_features_to_bps(current, NETDEV_DEFAULT_BPS) / 8; } class->min_rate = max_rate; diff --git a/lib/rstp.c b/lib/rstp.c index 7e351bf32..2f01966f7 100644 --- a/lib/rstp.c +++ b/lib/rstp.c @@ -784,7 +784,7 @@ rstp_convert_speed_to_cost(unsigned int speed) : speed >= 100 ? 200000 /* 100 Mb/s. */ : speed >= 10 ? 2000000 /* 10 Mb/s. */ : speed >= 1 ? 20000000 /* 1 Mb/s. */ - : RSTP_DEFAULT_PORT_PATH_COST; /* 100 Mb/s. */ + : RSTP_DEFAULT_PORT_PATH_COST; /* 10 Gb/s. */ return value; } diff --git a/lib/rstp.h b/lib/rstp.h index 39a13b58c..13af20195 100644 --- a/lib/rstp.h +++ b/lib/rstp.h @@ -84,7 +84,7 @@ struct dp_packet; /* Port path cost [Table 17-3] */ #define RSTP_MIN_PORT_PATH_COST 1 #define RSTP_MAX_PORT_PATH_COST 200000000 -#define RSTP_DEFAULT_PORT_PATH_COST 200000 +#define RSTP_DEFAULT_PORT_PATH_COST 2000 /* RSTP Bridge identifier [9.2.5]. Top four most significant bits are a * priority value. The next most significant twelve bits are a locally diff --git a/lib/stp.c b/lib/stp.c index a869b5f39..f37337992 100644 --- a/lib/stp.c +++ b/lib/stp.c @@ -313,7 +313,7 @@ stp_create(const char *name, stp_identifier bridge_id, for (p = stp->ports; p < &stp->ports[ARRAY_SIZE(stp->ports)]; p++) { p->stp = stp; p->port_id = (stp_port_no(p) + 1) | (STP_DEFAULT_PORT_PRIORITY << 8); - p->path_cost = 19; /* Recommended default for 100 Mb/s link. */ + p->path_cost = 2; /* Recommended default for 10 Gb/s link. */ stp_initialize_port(p, STP_DISABLED); } ovs_refcount_init(&stp->ref_cnt); @@ -989,7 +989,7 @@ stp_convert_speed_to_cost(unsigned int speed) : speed >= 16 ? 62 /* 16 Mb/s. */ : speed >= 10 ? 100 /* 10 Mb/s. */ : speed >= 4 ? 250 /* 4 Mb/s. */ - : 19; /* 100 Mb/s (guess). */ + : 2; /* 10 Gb/s (guess). */ ovs_mutex_unlock(&mutex); return ret; } diff --git a/tests/stp.at b/tests/stp.at index 7ddacfc3a..93da056ac 100644 --- a/tests/stp.at +++ b/tests/stp.at @@ -620,10 +620,10 @@ ovs-appctl time/stop ovs-appctl time/warp 31000 1000 AT_CHECK([ovs-appctl stp/show br0 | grep p1], [0], [dnl - p1 designated forwarding 19 128.1 + p1 designated forwarding 2 128.1 ]) AT_CHECK([ovs-appctl stp/show br0 | grep p2], [0], [dnl - p2 designated forwarding 19 128.2 + p2 designated forwarding 2 128.2 ]) # add a stp port @@ -637,10 +637,10 @@ ovs-appctl netdev-dummy/set-admin-state p3 down # We should not show the p3 because its link-state is down AT_CHECK([ovs-appctl stp/show br0 | grep p1], [0], [dnl - p1 designated forwarding 19 128.1 + p1 designated forwarding 2 128.1 ]) AT_CHECK([ovs-appctl stp/show br0 | grep p2], [0], [dnl - p2 designated forwarding 19 128.2 + p2 designated forwarding 2 128.2 ]) AT_CHECK([ovs-appctl stp/show br0 | grep p3], [1], [dnl ]) @@ -648,13 +648,13 @@ AT_CHECK([ovs-appctl stp/show br0 | grep p3], [1], [dnl ovs-appctl netdev-dummy/set-admin-state p3 up AT_CHECK([ovs-appctl stp/show br0 | grep p1], [0], [dnl - p1 designated forwarding 19 128.1 + p1 designated forwarding 2 128.1 ]) AT_CHECK([ovs-appctl stp/show br0 | grep p2], [0], [dnl - p2 designated forwarding 19 128.2 + p2 designated forwarding 2 128.2 ]) AT_CHECK([ovs-appctl stp/show br0 | grep p3], [0], [dnl - p3 designated listening 19 128.3 + p3 designated listening 2 128.3 ]) diff --git a/tests/test-rstp.c b/tests/test-rstp.c index 01aeaf847..9c1026ec1 100644 --- a/tests/test-rstp.c +++ b/tests/test-rstp.c @@ -107,6 +107,8 @@ send_bpdu(struct dp_packet *pkt, void *port_, void *b_) dp_packet_delete(pkt); } +#define RSTP_PORT_PATH_COST_100M 200000 + static struct bridge * new_bridge(struct test_case *tc, int id) { @@ -122,6 +124,7 @@ new_bridge(struct test_case *tc, int id) for (i = 1; i < MAX_PORTS; i++) { p = rstp_add_port(b->rstp); rstp_port_set_aux(p, p); + rstp_port_set_path_cost(p, RSTP_PORT_PATH_COST_100M); rstp_port_set_state(p, RSTP_DISABLED); rstp_port_set_mac_operational(p, true); } @@ -544,8 +547,8 @@ test_rstp_main(int argc, char *argv[]) } get_token(); - path_cost = match(":") ? must_get_int() : - RSTP_DEFAULT_PORT_PATH_COST; + path_cost = match(":") ? must_get_int() + : RSTP_PORT_PATH_COST_100M; if (port_no < bridge->n_ports) { /* Enable port. */ reinitialize_port(p); diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 25ce45e3d..28cbad514 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -1678,7 +1678,7 @@ port_configure_stp(const struct ofproto *ofproto, struct port *port, unsigned int mbps; netdev_get_features(iface->netdev, ¤t, NULL, NULL, NULL); - mbps = netdev_features_to_bps(current, 100 * 1000 * 1000) / 1000000; + mbps = netdev_features_to_bps(current, NETDEV_DEFAULT_BPS) / 1000000; port_s->path_cost = stp_convert_speed_to_cost(mbps); } @@ -1761,7 +1761,7 @@ port_configure_rstp(const struct ofproto *ofproto, struct port *port, unsigned int mbps; netdev_get_features(iface->netdev, ¤t, NULL, NULL, NULL); - mbps = netdev_features_to_bps(current, 100 * 1000 * 1000) / 1000000; + mbps = netdev_features_to_bps(current, NETDEV_DEFAULT_BPS) / 1000000; port_s->path_cost = rstp_convert_speed_to_cost(mbps); } diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index 36388e3c4..0b5dc42c4 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -4776,7 +4776,7 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ Maximum rate shared by all queued traffic, in bit/s. Optional. If not specified, for physical interfaces, the default is the link rate. For other interfaces or if the link rate cannot be determined, the default - is currently 100 Mbps. + is currently 10 Gbps. </column> </group>
100 Mbps was a fair assumption 13 years ago. Modern days 10 Gbps seems like a good value in case no information is available otherwise. The change mainly affects QoS which is currently limited to 100 Mbps if the user didn't specify 'max-rate' and the card doesn't report the speed or OVS doesn't have a predefined enumeration for the speed reported by the NIC. Calculation of the path cost for STP/RSTP is also affected if OVS is unable to determine the link speed. Lower link speed adapters are typically good at reporting their speed, so chances for overshoot should be low. But newer high-speed adapters, for which there is no speed enumeration or if there are some other issues, will not suffer that much. Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- NEWS | 4 ++++ include/openvswitch/netdev.h | 2 ++ lib/netdev-linux.c | 4 ++-- lib/rstp.c | 2 +- lib/rstp.h | 2 +- lib/stp.c | 4 ++-- tests/stp.at | 14 +++++++------- tests/test-rstp.c | 7 +++++-- vswitchd/bridge.c | 4 ++-- vswitchd/vswitch.xml | 2 +- 10 files changed, 27 insertions(+), 18 deletions(-)