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 <[email protected]> Suggested-by: Ilya Maximets <[email protected]> Signed-off-by: Adrian Moreno <[email protected]> --- 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_STOPAT_CLEANUP
-- Adrián Moreno _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
