On Tue, Oct 25, 2022 at 12:38 PM Ilya Maximets <[email protected]> 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 <[email protected]>
I think this is a reasonable change, 100MB has mostly been relegated to embedded devices for a long time. Acked-by: Mike Pattrick <[email protected]> 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
