| Message ID | 20240205120553.684210-1-amorenoz@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev] netdev-linux: Avoid deadlock in netdev_get_speed. | 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 Mon, Feb 05, 2024 at 01:02:10PM +0100, Adrian Moreno wrote: > netdev_linux_get_speed needs to lock netdev_linux->mutex, and so do the > internal tc operations. Therefore, the former cannot be called from the > latter. > > Create a lock-free version of netdev_linux_get_speed() and call it from > tc operations. > > Also expand the unit test to cover queues where ceil is determined by > the maximum link speed. > > Fixes: b8f8fad86435 ("netdev-linux: Use speed as max rate in tc classes.") > Reported-by: Daryl Wang <darylywang@google.com> > Suggested-by: Ilya Maximets <i.maximets@ovn.org> > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> Acked-by: Simon Horman <horms@ovn.org>
On 2/5/24 13:02, Adrian Moreno wrote: > netdev_linux_get_speed needs to lock netdev_linux->mutex, and so do the > internal tc operations. Therefore, the former cannot be called from the > latter. > > Create a lock-free version of netdev_linux_get_speed() and call it from > tc operations. > > Also expand the unit test to cover queues where ceil is determined by > the maximum link speed. > > Fixes: b8f8fad86435 ("netdev-linux: Use speed as max rate in tc classes.") > Reported-by: Daryl Wang <darylywang@google.com> > Suggested-by: Ilya Maximets <i.maximets@ovn.org> > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > --- > lib/netdev-linux.c | 32 +++++++++++++++---------- > tests/system-traffic.at | 53 ++++++++++++++++++++++++++++------------- > 2 files changed, 56 insertions(+), 29 deletions(-) > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index 1b2e5b6c2..00df7f634 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -2721,16 +2721,11 @@ exit: > } > > static int > -netdev_linux_get_speed(const struct netdev *netdev_, uint32_t *current, > - uint32_t *max) > +netdev_linux_get_speed_locked(struct netdev_linux *netdev, > + uint32_t *current, uint32_t *max) > { > - struct netdev_linux *netdev = netdev_linux_cast(netdev_); > - int error; > - > - ovs_mutex_lock(&netdev->mutex); > if (netdev_linux_netnsid_is_remote(netdev)) { > - error = EOPNOTSUPP; > - goto exit; > + return EOPNOTSUPP; > } > > netdev_linux_read_features(netdev); > @@ -2740,9 +2735,18 @@ netdev_linux_get_speed(const struct netdev *netdev_, uint32_t *current, > *max = MIN(UINT32_MAX, > netdev_features_to_bps(netdev->supported, 0) / 1000000ULL); > } > - error = netdev->get_features_error; > + return netdev->get_features_error; > +} > > -exit: > +static int > +netdev_linux_get_speed(const struct netdev *netdev_, uint32_t *current, > + uint32_t *max) > +{ > + struct netdev_linux *netdev = netdev_linux_cast(netdev_); > + int error; > + > + ovs_mutex_lock(&netdev->mutex); > + error = netdev_linux_get_speed_locked(netdev, current, max); > ovs_mutex_unlock(&netdev->mutex); > return error; > } > @@ -4954,8 +4958,10 @@ htb_parse_qdisc_details__(struct netdev *netdev, const struct smap *details, > hc->max_rate = smap_get_ullong(details, "max-rate", 0) / 8; > if (!hc->max_rate) { > uint32_t current_speed; > + uint32_t max_speed OVS_UNUSED; > > - netdev_get_speed(netdev, ¤t_speed, NULL); > + netdev_linux_get_speed_locked(netdev_linux_cast(netdev), > + ¤t_speed, &max_speed); > hc->max_rate = current_speed ? current_speed / 8 * 1000000ULL > : NETDEV_DEFAULT_BPS / 8; > } > @@ -5424,8 +5430,10 @@ hfsc_parse_qdisc_details__(struct netdev *netdev, const struct smap *details, > uint32_t max_rate = smap_get_ullong(details, "max-rate", 0) / 8; > if (!max_rate) { > uint32_t current_speed; > + uint32_t max_speed OVS_UNUSED; > > - netdev_get_speed(netdev, ¤t_speed, NULL); Oh, wow. This thing not only deadlocks, it also crashes on the NULL, right? I see we call netdev_get_speed(..., NULL) a lot in other palces. Should those be fixed as well? Maybe the function should be fixed to allow NULL instead? > + netdev_linux_get_speed_locked(netdev_linux_cast(netdev), > + ¤t_speed, &max_speed); > max_rate = current_speed ? current_speed / 8 * 1000000ULL > : NETDEV_DEFAULT_BPS / 8; > } > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index f363a778c..d90717d1b 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -2458,34 +2458,53 @@ AT_BANNER([QoS]) > > AT_SETUP([QoS - basic configuration]) > OVS_CHECK_TC_QDISC() > +AT_SKIP_IF([test $HAVE_ETHTOOL = "no"]) > OVS_TRAFFIC_VSWITCHD_START() > > -ADD_NAMESPACES(at_ns0, at_ns1) > +AT_CHECK([ip tuntap add tap0 mode tap]) > +on_exit 'ip link del tap0' > +AT_CHECK([ip tuntap add tap1 mode tap]) > +on_exit 'ip link del tap1' tapN is a fairly common name. Even though we run in a separate namespace in GHA, it may not be always the case. Maybe rename the ports into ovs-tapN ? > > -ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") > -ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") > +dnl Set maximum link speed to 5Gb. > +AT_CHECK([ethtool -s tap0 speed 5000 duplex full]) > +AT_CHECK([ip link set dev tap0 up]) > +AT_CHECK([ethtool -s tap1 speed 5000 duplex full]) > +AT_CHECK([ip link set dev tap1 up]) > > -dnl Adding a custom qdisc to ovs-p1, ovs-p0 will have the default qdisc. > -AT_CHECK([tc qdisc add dev ovs-p1 root noqueue]) > -AT_CHECK([tc qdisc show dev ovs-p1 | grep -q noqueue]) > +AT_CHECK([ovs-vsctl add-port br0 tap0 -- set int tap0 type=tap]) > +AT_CHECK([ovs-vsctl add-port br0 tap1 -- set int tap1 type=tap]) > > -dnl Configure the same QoS for both ports. > -AT_CHECK([ovs-vsctl set port ovs-p0 qos=@qos -- set port ovs-p1 qos=@qos dnl > - -- --id=@qos create qos dnl > - type=linux-htb other-config:max-rate=3000000 queues:0=@queue dnl > - -- --id=@queue create queue dnl > +dnl Adding a custom qdisc to tap1, tap0 will have the default qdisc. > +AT_CHECK([tc qdisc add dev tap1 root noqueue]) > +AT_CHECK([tc qdisc show dev tap1 | grep -q noqueue]) > + > +dnl Configure the same QoS for both ports: > +dnl queue0 uses fixed max-rate. > +dnl queue1 relies on underlying link speed. > +AT_CHECK([ovs-vsctl dnl > + -- --id=@queue0 create queue dnl > other_config:min-rate=2000000 other_config:max-rate=3000000 dnl > - other_config:burst=3000000], > + other_config:burst=3000000 dnl > + -- --id=@queue1 create queue dnl > + other_config:min-rate=4000000 other_config:burst=4000000 dnl > + -- --id=@qos create qos dnl > + type=linux-htb queues:0=@queue0 dnl > + queues:1=@queue1 -- dnl > + -- set port tap0 qos=@qos -- set port tap1 qos=@qos], > [ignore], [ignore]) > > dnl Wait for qdiscs to be applied. > -OVS_WAIT_UNTIL([tc qdisc show dev ovs-p0 | grep -q htb]) > -OVS_WAIT_UNTIL([tc qdisc show dev ovs-p1 | grep -q htb]) > +OVS_WAIT_UNTIL([tc qdisc show dev tap0 | grep -q htb]) > +OVS_WAIT_UNTIL([tc qdisc show dev tap1 | grep -q htb]) > > dnl Check the configuration. > -m4_define([HTB_CONF], [rate 2Mbit ceil 3Mbit burst 375000b cburst 375000b]) > -AT_CHECK([tc class show dev ovs-p0 | grep -q 'class htb .* HTB_CONF']) > -AT_CHECK([tc class show dev ovs-p1 | grep -q 'class htb .* HTB_CONF']) > +m4_define([HTB_CONF0], [rate 2Mbit ceil 3Mbit burst 375000b cburst 375000b]) > +m4_define([HTB_CONF1], [rate 4Mbit ceil 5Gbit burst 500000b cburst 500000b]) > +AT_CHECK([tc class show dev tap0 | grep -q 'class htb .* HTB_CONF0']) > +AT_CHECK([tc class show dev tap0 | grep -q 'class htb .* HTB_CONF1']) > +AT_CHECK([tc class show dev tap1 | grep -q 'class htb .* HTB_CONF0']) > +AT_CHECK([tc class show dev tap1 | grep -q 'class htb .* HTB_CONF1']) > > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP
On 2/7/24 19:05, Ilya Maximets wrote: > On 2/5/24 13:02, Adrian Moreno wrote: >> netdev_linux_get_speed needs to lock netdev_linux->mutex, and so do the >> internal tc operations. Therefore, the former cannot be called from the >> latter. >> >> Create a lock-free version of netdev_linux_get_speed() and call it from >> tc operations. >> >> Also expand the unit test to cover queues where ceil is determined by >> the maximum link speed. >> >> Fixes: b8f8fad86435 ("netdev-linux: Use speed as max rate in tc classes.") >> Reported-by: Daryl Wang <darylywang@google.com> >> Suggested-by: Ilya Maximets <i.maximets@ovn.org> >> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >> --- >> lib/netdev-linux.c | 32 +++++++++++++++---------- >> tests/system-traffic.at | 53 ++++++++++++++++++++++++++++------------- >> 2 files changed, 56 insertions(+), 29 deletions(-) >> >> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c >> index 1b2e5b6c2..00df7f634 100644 >> --- a/lib/netdev-linux.c >> +++ b/lib/netdev-linux.c >> @@ -2721,16 +2721,11 @@ exit: >> } >> >> static int >> -netdev_linux_get_speed(const struct netdev *netdev_, uint32_t *current, >> - uint32_t *max) >> +netdev_linux_get_speed_locked(struct netdev_linux *netdev, >> + uint32_t *current, uint32_t *max) >> { >> - struct netdev_linux *netdev = netdev_linux_cast(netdev_); >> - int error; >> - >> - ovs_mutex_lock(&netdev->mutex); >> if (netdev_linux_netnsid_is_remote(netdev)) { >> - error = EOPNOTSUPP; >> - goto exit; >> + return EOPNOTSUPP; >> } >> >> netdev_linux_read_features(netdev); >> @@ -2740,9 +2735,18 @@ netdev_linux_get_speed(const struct netdev *netdev_, uint32_t *current, >> *max = MIN(UINT32_MAX, >> netdev_features_to_bps(netdev->supported, 0) / 1000000ULL); >> } >> - error = netdev->get_features_error; >> + return netdev->get_features_error; >> +} >> >> -exit: >> +static int >> +netdev_linux_get_speed(const struct netdev *netdev_, uint32_t *current, >> + uint32_t *max) >> +{ >> + struct netdev_linux *netdev = netdev_linux_cast(netdev_); >> + int error; >> + >> + ovs_mutex_lock(&netdev->mutex); >> + error = netdev_linux_get_speed_locked(netdev, current, max); >> ovs_mutex_unlock(&netdev->mutex); >> return error; >> } >> @@ -4954,8 +4958,10 @@ htb_parse_qdisc_details__(struct netdev *netdev, const struct smap *details, >> hc->max_rate = smap_get_ullong(details, "max-rate", 0) / 8; >> if (!hc->max_rate) { >> uint32_t current_speed; >> + uint32_t max_speed OVS_UNUSED; >> >> - netdev_get_speed(netdev, ¤t_speed, NULL); >> + netdev_linux_get_speed_locked(netdev_linux_cast(netdev), >> + ¤t_speed, &max_speed); >> hc->max_rate = current_speed ? current_speed / 8 * 1000000ULL >> : NETDEV_DEFAULT_BPS / 8; >> } >> @@ -5424,8 +5430,10 @@ hfsc_parse_qdisc_details__(struct netdev *netdev, const struct smap *details, >> uint32_t max_rate = smap_get_ullong(details, "max-rate", 0) / 8; >> if (!max_rate) { >> uint32_t current_speed; >> + uint32_t max_speed OVS_UNUSED; >> >> - netdev_get_speed(netdev, ¤t_speed, NULL); > > Oh, wow. This thing not only deadlocks, it also crashes on the NULL, right? > netdev_get_speed() defined in lib/netdev.c does support the pointer being NULL. netdev_linux_get_speed{,_locked}() do not. I didn't add such logic now precisely to avoid duplicating logic since it's similar to get_features_*. > I see we call netdev_get_speed(..., NULL) a lot in other palces. Should > those be fixed as well? Maybe the function should be fixed to allow NULL > instead? > >> + netdev_linux_get_speed_locked(netdev_linux_cast(netdev), >> + ¤t_speed, &max_speed); >> max_rate = current_speed ? current_speed / 8 * 1000000ULL >> : NETDEV_DEFAULT_BPS / 8; >> } >> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >> index f363a778c..d90717d1b 100644 >> --- a/tests/system-traffic.at >> +++ b/tests/system-traffic.at >> @@ -2458,34 +2458,53 @@ AT_BANNER([QoS]) >> >> AT_SETUP([QoS - basic configuration]) >> OVS_CHECK_TC_QDISC() >> +AT_SKIP_IF([test $HAVE_ETHTOOL = "no"]) >> OVS_TRAFFIC_VSWITCHD_START() >> >> -ADD_NAMESPACES(at_ns0, at_ns1) >> +AT_CHECK([ip tuntap add tap0 mode tap]) >> +on_exit 'ip link del tap0' >> +AT_CHECK([ip tuntap add tap1 mode tap]) >> +on_exit 'ip link del tap1' > > tapN is a fairly common name. Even though we run in a separate > namespace in GHA, it may not be always the case. Maybe rename > the ports into ovs-tapN ? > Sure. I'll respin the patch with the changed name. >> >> -ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") >> -ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") >> +dnl Set maximum link speed to 5Gb. >> +AT_CHECK([ethtool -s tap0 speed 5000 duplex full]) >> +AT_CHECK([ip link set dev tap0 up]) >> +AT_CHECK([ethtool -s tap1 speed 5000 duplex full]) >> +AT_CHECK([ip link set dev tap1 up]) >> >> -dnl Adding a custom qdisc to ovs-p1, ovs-p0 will have the default qdisc. >> -AT_CHECK([tc qdisc add dev ovs-p1 root noqueue]) >> -AT_CHECK([tc qdisc show dev ovs-p1 | grep -q noqueue]) >> +AT_CHECK([ovs-vsctl add-port br0 tap0 -- set int tap0 type=tap]) >> +AT_CHECK([ovs-vsctl add-port br0 tap1 -- set int tap1 type=tap]) >> >> -dnl Configure the same QoS for both ports. >> -AT_CHECK([ovs-vsctl set port ovs-p0 qos=@qos -- set port ovs-p1 qos=@qos dnl >> - -- --id=@qos create qos dnl >> - type=linux-htb other-config:max-rate=3000000 queues:0=@queue dnl >> - -- --id=@queue create queue dnl >> +dnl Adding a custom qdisc to tap1, tap0 will have the default qdisc. >> +AT_CHECK([tc qdisc add dev tap1 root noqueue]) >> +AT_CHECK([tc qdisc show dev tap1 | grep -q noqueue]) >> + >> +dnl Configure the same QoS for both ports: >> +dnl queue0 uses fixed max-rate. >> +dnl queue1 relies on underlying link speed. >> +AT_CHECK([ovs-vsctl dnl >> + -- --id=@queue0 create queue dnl >> other_config:min-rate=2000000 other_config:max-rate=3000000 dnl >> - other_config:burst=3000000], >> + other_config:burst=3000000 dnl >> + -- --id=@queue1 create queue dnl >> + other_config:min-rate=4000000 other_config:burst=4000000 dnl >> + -- --id=@qos create qos dnl >> + type=linux-htb queues:0=@queue0 dnl >> + queues:1=@queue1 -- dnl >> + -- set port tap0 qos=@qos -- set port tap1 qos=@qos], >> [ignore], [ignore]) >> >> dnl Wait for qdiscs to be applied. >> -OVS_WAIT_UNTIL([tc qdisc show dev ovs-p0 | grep -q htb]) >> -OVS_WAIT_UNTIL([tc qdisc show dev ovs-p1 | grep -q htb]) >> +OVS_WAIT_UNTIL([tc qdisc show dev tap0 | grep -q htb]) >> +OVS_WAIT_UNTIL([tc qdisc show dev tap1 | grep -q htb]) >> >> dnl Check the configuration. >> -m4_define([HTB_CONF], [rate 2Mbit ceil 3Mbit burst 375000b cburst 375000b]) >> -AT_CHECK([tc class show dev ovs-p0 | grep -q 'class htb .* HTB_CONF']) >> -AT_CHECK([tc class show dev ovs-p1 | grep -q 'class htb .* HTB_CONF']) >> +m4_define([HTB_CONF0], [rate 2Mbit ceil 3Mbit burst 375000b cburst 375000b]) >> +m4_define([HTB_CONF1], [rate 4Mbit ceil 5Gbit burst 500000b cburst 500000b]) >> +AT_CHECK([tc class show dev tap0 | grep -q 'class htb .* HTB_CONF0']) >> +AT_CHECK([tc class show dev tap0 | grep -q 'class htb .* HTB_CONF1']) >> +AT_CHECK([tc class show dev tap1 | grep -q 'class htb .* HTB_CONF0']) >> +AT_CHECK([tc class show dev tap1 | grep -q 'class htb .* HTB_CONF1']) >> >> OVS_TRAFFIC_VSWITCHD_STOP >> AT_CLEANUP >
On 2/7/24 22:07, Adrian Moreno wrote: > > > On 2/7/24 19:05, Ilya Maximets wrote: >> On 2/5/24 13:02, Adrian Moreno wrote: >>> netdev_linux_get_speed needs to lock netdev_linux->mutex, and so do the >>> internal tc operations. Therefore, the former cannot be called from the >>> latter. >>> >>> Create a lock-free version of netdev_linux_get_speed() and call it from >>> tc operations. >>> >>> Also expand the unit test to cover queues where ceil is determined by >>> the maximum link speed. >>> >>> Fixes: b8f8fad86435 ("netdev-linux: Use speed as max rate in tc classes.") >>> Reported-by: Daryl Wang <darylywang@google.com> >>> Suggested-by: Ilya Maximets <i.maximets@ovn.org> >>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >>> --- >>> lib/netdev-linux.c | 32 +++++++++++++++---------- >>> tests/system-traffic.at | 53 ++++++++++++++++++++++++++++------------- >>> 2 files changed, 56 insertions(+), 29 deletions(-) >>> >>> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c >>> index 1b2e5b6c2..00df7f634 100644 >>> --- a/lib/netdev-linux.c >>> +++ b/lib/netdev-linux.c >>> @@ -2721,16 +2721,11 @@ exit: >>> } >>> >>> static int >>> -netdev_linux_get_speed(const struct netdev *netdev_, uint32_t *current, >>> - uint32_t *max) >>> +netdev_linux_get_speed_locked(struct netdev_linux *netdev, >>> + uint32_t *current, uint32_t *max) >>> { >>> - struct netdev_linux *netdev = netdev_linux_cast(netdev_); >>> - int error; >>> - >>> - ovs_mutex_lock(&netdev->mutex); >>> if (netdev_linux_netnsid_is_remote(netdev)) { >>> - error = EOPNOTSUPP; >>> - goto exit; >>> + return EOPNOTSUPP; >>> } >>> >>> netdev_linux_read_features(netdev); >>> @@ -2740,9 +2735,18 @@ netdev_linux_get_speed(const struct netdev *netdev_, uint32_t *current, >>> *max = MIN(UINT32_MAX, >>> netdev_features_to_bps(netdev->supported, 0) / 1000000ULL); >>> } >>> - error = netdev->get_features_error; >>> + return netdev->get_features_error; >>> +} >>> >>> -exit: >>> +static int >>> +netdev_linux_get_speed(const struct netdev *netdev_, uint32_t *current, >>> + uint32_t *max) >>> +{ >>> + struct netdev_linux *netdev = netdev_linux_cast(netdev_); >>> + int error; >>> + >>> + ovs_mutex_lock(&netdev->mutex); >>> + error = netdev_linux_get_speed_locked(netdev, current, max); >>> ovs_mutex_unlock(&netdev->mutex); >>> return error; >>> } >>> @@ -4954,8 +4958,10 @@ htb_parse_qdisc_details__(struct netdev *netdev, const struct smap *details, >>> hc->max_rate = smap_get_ullong(details, "max-rate", 0) / 8; >>> if (!hc->max_rate) { >>> uint32_t current_speed; >>> + uint32_t max_speed OVS_UNUSED; >>> >>> - netdev_get_speed(netdev, ¤t_speed, NULL); >>> + netdev_linux_get_speed_locked(netdev_linux_cast(netdev), >>> + ¤t_speed, &max_speed); >>> hc->max_rate = current_speed ? current_speed / 8 * 1000000ULL >>> : NETDEV_DEFAULT_BPS / 8; >>> } >>> @@ -5424,8 +5430,10 @@ hfsc_parse_qdisc_details__(struct netdev *netdev, const struct smap *details, >>> uint32_t max_rate = smap_get_ullong(details, "max-rate", 0) / 8; >>> if (!max_rate) { >>> uint32_t current_speed; >>> + uint32_t max_speed OVS_UNUSED; >>> >>> - netdev_get_speed(netdev, ¤t_speed, NULL); >> >> Oh, wow. This thing not only deadlocks, it also crashes on the NULL, right? >> > > netdev_get_speed() defined in lib/netdev.c does support the pointer being NULL. > netdev_linux_get_speed{,_locked}() do not. I didn't add such logic now precisely > to avoid duplicating logic since it's similar to get_features_*. Ah, I missed the check in netdev_get_speed(). Thanks for explanation! > >> I see we call netdev_get_speed(..., NULL) a lot in other palces. Should >> those be fixed as well? Maybe the function should be fixed to allow NULL >> instead? >> >>> + netdev_linux_get_speed_locked(netdev_linux_cast(netdev), >>> + ¤t_speed, &max_speed); >>> max_rate = current_speed ? current_speed / 8 * 1000000ULL >>> : NETDEV_DEFAULT_BPS / 8; >>> } >>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >>> index f363a778c..d90717d1b 100644 >>> --- a/tests/system-traffic.at >>> +++ b/tests/system-traffic.at >>> @@ -2458,34 +2458,53 @@ AT_BANNER([QoS]) >>> >>> AT_SETUP([QoS - basic configuration]) >>> OVS_CHECK_TC_QDISC() >>> +AT_SKIP_IF([test $HAVE_ETHTOOL = "no"]) >>> OVS_TRAFFIC_VSWITCHD_START() >>> >>> -ADD_NAMESPACES(at_ns0, at_ns1) >>> +AT_CHECK([ip tuntap add tap0 mode tap]) >>> +on_exit 'ip link del tap0' >>> +AT_CHECK([ip tuntap add tap1 mode tap]) >>> +on_exit 'ip link del tap1' >> >> tapN is a fairly common name. Even though we run in a separate >> namespace in GHA, it may not be always the case. Maybe rename >> the ports into ovs-tapN ? >> > > Sure. I'll respin the patch with the changed name. Thanks! > > >>> >>> -ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") >>> -ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") >>> +dnl Set maximum link speed to 5Gb. >>> +AT_CHECK([ethtool -s tap0 speed 5000 duplex full]) >>> +AT_CHECK([ip link set dev tap0 up]) >>> +AT_CHECK([ethtool -s tap1 speed 5000 duplex full]) >>> +AT_CHECK([ip link set dev tap1 up]) >>> >>> -dnl Adding a custom qdisc to ovs-p1, ovs-p0 will have the default qdisc. >>> -AT_CHECK([tc qdisc add dev ovs-p1 root noqueue]) >>> -AT_CHECK([tc qdisc show dev ovs-p1 | grep -q noqueue]) >>> +AT_CHECK([ovs-vsctl add-port br0 tap0 -- set int tap0 type=tap]) >>> +AT_CHECK([ovs-vsctl add-port br0 tap1 -- set int tap1 type=tap]) >>> >>> -dnl Configure the same QoS for both ports. >>> -AT_CHECK([ovs-vsctl set port ovs-p0 qos=@qos -- set port ovs-p1 qos=@qos dnl >>> - -- --id=@qos create qos dnl >>> - type=linux-htb other-config:max-rate=3000000 queues:0=@queue dnl >>> - -- --id=@queue create queue dnl >>> +dnl Adding a custom qdisc to tap1, tap0 will have the default qdisc. >>> +AT_CHECK([tc qdisc add dev tap1 root noqueue]) >>> +AT_CHECK([tc qdisc show dev tap1 | grep -q noqueue]) >>> + >>> +dnl Configure the same QoS for both ports: >>> +dnl queue0 uses fixed max-rate. >>> +dnl queue1 relies on underlying link speed. >>> +AT_CHECK([ovs-vsctl dnl >>> + -- --id=@queue0 create queue dnl >>> other_config:min-rate=2000000 other_config:max-rate=3000000 dnl >>> - other_config:burst=3000000], >>> + other_config:burst=3000000 dnl >>> + -- --id=@queue1 create queue dnl >>> + other_config:min-rate=4000000 other_config:burst=4000000 dnl >>> + -- --id=@qos create qos dnl >>> + type=linux-htb queues:0=@queue0 dnl >>> + queues:1=@queue1 -- dnl >>> + -- set port tap0 qos=@qos -- set port tap1 qos=@qos], >>> [ignore], [ignore]) >>> >>> dnl Wait for qdiscs to be applied. >>> -OVS_WAIT_UNTIL([tc qdisc show dev ovs-p0 | grep -q htb]) >>> -OVS_WAIT_UNTIL([tc qdisc show dev ovs-p1 | grep -q htb]) >>> +OVS_WAIT_UNTIL([tc qdisc show dev tap0 | grep -q htb]) >>> +OVS_WAIT_UNTIL([tc qdisc show dev tap1 | grep -q htb]) >>> >>> dnl Check the configuration. >>> -m4_define([HTB_CONF], [rate 2Mbit ceil 3Mbit burst 375000b cburst 375000b]) >>> -AT_CHECK([tc class show dev ovs-p0 | grep -q 'class htb .* HTB_CONF']) >>> -AT_CHECK([tc class show dev ovs-p1 | grep -q 'class htb .* HTB_CONF']) >>> +m4_define([HTB_CONF0], [rate 2Mbit ceil 3Mbit burst 375000b cburst 375000b]) >>> +m4_define([HTB_CONF1], [rate 4Mbit ceil 5Gbit burst 500000b cburst 500000b]) >>> +AT_CHECK([tc class show dev tap0 | grep -q 'class htb .* HTB_CONF0']) >>> +AT_CHECK([tc class show dev tap0 | grep -q 'class htb .* HTB_CONF1']) >>> +AT_CHECK([tc class show dev tap1 | grep -q 'class htb .* HTB_CONF0']) >>> +AT_CHECK([tc class show dev tap1 | grep -q 'class htb .* HTB_CONF1']) >>> >>> OVS_TRAFFIC_VSWITCHD_STOP >>> AT_CLEANUP >> >
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 1b2e5b6c2..00df7f634 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -2721,16 +2721,11 @@ exit: } static int -netdev_linux_get_speed(const struct netdev *netdev_, uint32_t *current, - uint32_t *max) +netdev_linux_get_speed_locked(struct netdev_linux *netdev, + uint32_t *current, uint32_t *max) { - struct netdev_linux *netdev = netdev_linux_cast(netdev_); - int error; - - ovs_mutex_lock(&netdev->mutex); if (netdev_linux_netnsid_is_remote(netdev)) { - error = EOPNOTSUPP; - goto exit; + return EOPNOTSUPP; } netdev_linux_read_features(netdev); @@ -2740,9 +2735,18 @@ netdev_linux_get_speed(const struct netdev *netdev_, uint32_t *current, *max = MIN(UINT32_MAX, netdev_features_to_bps(netdev->supported, 0) / 1000000ULL); } - error = netdev->get_features_error; + return netdev->get_features_error; +} -exit: +static int +netdev_linux_get_speed(const struct netdev *netdev_, uint32_t *current, + uint32_t *max) +{ + struct netdev_linux *netdev = netdev_linux_cast(netdev_); + int error; + + ovs_mutex_lock(&netdev->mutex); + error = netdev_linux_get_speed_locked(netdev, current, max); ovs_mutex_unlock(&netdev->mutex); return error; } @@ -4954,8 +4958,10 @@ htb_parse_qdisc_details__(struct netdev *netdev, const struct smap *details, hc->max_rate = smap_get_ullong(details, "max-rate", 0) / 8; if (!hc->max_rate) { uint32_t current_speed; + uint32_t max_speed OVS_UNUSED; - netdev_get_speed(netdev, ¤t_speed, NULL); + netdev_linux_get_speed_locked(netdev_linux_cast(netdev), + ¤t_speed, &max_speed); hc->max_rate = current_speed ? current_speed / 8 * 1000000ULL : NETDEV_DEFAULT_BPS / 8; } @@ -5424,8 +5430,10 @@ hfsc_parse_qdisc_details__(struct netdev *netdev, const struct smap *details, uint32_t max_rate = smap_get_ullong(details, "max-rate", 0) / 8; if (!max_rate) { uint32_t current_speed; + uint32_t max_speed OVS_UNUSED; - netdev_get_speed(netdev, ¤t_speed, NULL); + netdev_linux_get_speed_locked(netdev_linux_cast(netdev), + ¤t_speed, &max_speed); max_rate = current_speed ? current_speed / 8 * 1000000ULL : NETDEV_DEFAULT_BPS / 8; } diff --git a/tests/system-traffic.at b/tests/system-traffic.at index f363a778c..d90717d1b 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -2458,34 +2458,53 @@ AT_BANNER([QoS]) AT_SETUP([QoS - basic configuration]) OVS_CHECK_TC_QDISC() +AT_SKIP_IF([test $HAVE_ETHTOOL = "no"]) OVS_TRAFFIC_VSWITCHD_START() -ADD_NAMESPACES(at_ns0, at_ns1) +AT_CHECK([ip tuntap add tap0 mode tap]) +on_exit 'ip link del tap0' +AT_CHECK([ip tuntap add tap1 mode tap]) +on_exit 'ip link del tap1' -ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") -ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") +dnl Set maximum link speed to 5Gb. +AT_CHECK([ethtool -s tap0 speed 5000 duplex full]) +AT_CHECK([ip link set dev tap0 up]) +AT_CHECK([ethtool -s tap1 speed 5000 duplex full]) +AT_CHECK([ip link set dev tap1 up]) -dnl Adding a custom qdisc to ovs-p1, ovs-p0 will have the default qdisc. -AT_CHECK([tc qdisc add dev ovs-p1 root noqueue]) -AT_CHECK([tc qdisc show dev ovs-p1 | grep -q noqueue]) +AT_CHECK([ovs-vsctl add-port br0 tap0 -- set int tap0 type=tap]) +AT_CHECK([ovs-vsctl add-port br0 tap1 -- set int tap1 type=tap]) -dnl Configure the same QoS for both ports. -AT_CHECK([ovs-vsctl set port ovs-p0 qos=@qos -- set port ovs-p1 qos=@qos dnl - -- --id=@qos create qos dnl - type=linux-htb other-config:max-rate=3000000 queues:0=@queue dnl - -- --id=@queue create queue dnl +dnl Adding a custom qdisc to tap1, tap0 will have the default qdisc. +AT_CHECK([tc qdisc add dev tap1 root noqueue]) +AT_CHECK([tc qdisc show dev tap1 | grep -q noqueue]) + +dnl Configure the same QoS for both ports: +dnl queue0 uses fixed max-rate. +dnl queue1 relies on underlying link speed. +AT_CHECK([ovs-vsctl dnl + -- --id=@queue0 create queue dnl other_config:min-rate=2000000 other_config:max-rate=3000000 dnl - other_config:burst=3000000], + other_config:burst=3000000 dnl + -- --id=@queue1 create queue dnl + other_config:min-rate=4000000 other_config:burst=4000000 dnl + -- --id=@qos create qos dnl + type=linux-htb queues:0=@queue0 dnl + queues:1=@queue1 -- dnl + -- set port tap0 qos=@qos -- set port tap1 qos=@qos], [ignore], [ignore]) dnl Wait for qdiscs to be applied. -OVS_WAIT_UNTIL([tc qdisc show dev ovs-p0 | grep -q htb]) -OVS_WAIT_UNTIL([tc qdisc show dev ovs-p1 | grep -q htb]) +OVS_WAIT_UNTIL([tc qdisc show dev tap0 | grep -q htb]) +OVS_WAIT_UNTIL([tc qdisc show dev tap1 | grep -q htb]) dnl Check the configuration. -m4_define([HTB_CONF], [rate 2Mbit ceil 3Mbit burst 375000b cburst 375000b]) -AT_CHECK([tc class show dev ovs-p0 | grep -q 'class htb .* HTB_CONF']) -AT_CHECK([tc class show dev ovs-p1 | grep -q 'class htb .* HTB_CONF']) +m4_define([HTB_CONF0], [rate 2Mbit ceil 3Mbit burst 375000b cburst 375000b]) +m4_define([HTB_CONF1], [rate 4Mbit ceil 5Gbit burst 500000b cburst 500000b]) +AT_CHECK([tc class show dev tap0 | grep -q 'class htb .* HTB_CONF0']) +AT_CHECK([tc class show dev tap0 | grep -q 'class htb .* HTB_CONF1']) +AT_CHECK([tc class show dev tap1 | grep -q 'class htb .* HTB_CONF0']) +AT_CHECK([tc class show dev tap1 | grep -q 'class htb .* HTB_CONF1']) OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP
netdev_linux_get_speed needs to lock netdev_linux->mutex, and so do the internal tc operations. Therefore, the former cannot be called from the latter. Create a lock-free version of netdev_linux_get_speed() and call it from tc operations. Also expand the unit test to cover queues where ceil is determined by the maximum link speed. Fixes: b8f8fad86435 ("netdev-linux: Use speed as max rate in tc classes.") Reported-by: Daryl Wang <darylywang@google.com> Suggested-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> --- lib/netdev-linux.c | 32 +++++++++++++++---------- tests/system-traffic.at | 53 ++++++++++++++++++++++++++++------------- 2 files changed, 56 insertions(+), 29 deletions(-)