diff mbox series

[ovs-dev,v2] netdev-linux: Avoid deadlock in netdev_get_speed.

Message ID 20240212081833.3698087-1-amorenoz@redhat.com
State Accepted
Commit 19cffe30cfdad6f3a0fcc57069d6fb005233ee92
Headers show
Series [ovs-dev,v2] netdev-linux: Avoid deadlock in netdev_get_speed. | expand

Checks

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

Commit Message

Adrian Moreno Feb. 12, 2024, 8:18 a.m. UTC 
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(-)

Comments

Ilya Maximets Feb. 13, 2024, 10:31 p.m. UTC | #1 
On 2/12/24 09:18, 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(-)

Thanks!  Applied and backported down to 3.2.

Best regards, Ilya Maximets.
diff mbox series

Patch

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, &current_speed, NULL);
+        netdev_linux_get_speed_locked(netdev_linux_cast(netdev),
+                                      &current_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, &current_speed, NULL);
+        netdev_linux_get_speed_locked(netdev_linux_cast(netdev),
+                                      &current_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..1376ab0f0 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 ovs-tap0 mode tap])
+on_exit 'ip link del ovs-tap0'
+AT_CHECK([ip tuntap add ovs-tap1 mode tap])
+on_exit 'ip link del ovs-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 ovs-tap0 speed 5000 duplex full])
+AT_CHECK([ip link set dev ovs-tap0 up])
+AT_CHECK([ethtool -s ovs-tap1 speed 5000 duplex full])
+AT_CHECK([ip link set dev ovs-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 ovs-tap0 -- set int ovs-tap0 type=tap])
+AT_CHECK([ovs-vsctl add-port br0 ovs-tap1 -- set int ovs-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 ovs-tap1, ovs-tap0 will have the default qdisc.
+AT_CHECK([tc qdisc add dev ovs-tap1 root noqueue])
+AT_CHECK([tc qdisc show dev ovs-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 ovs-tap0 qos=@qos -- set port ovs-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 ovs-tap0 | grep -q htb])
+OVS_WAIT_UNTIL([tc qdisc show dev ovs-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 ovs-tap0 | grep -q 'class htb .* HTB_CONF0'])
+AT_CHECK([tc class show dev ovs-tap0 | grep -q 'class htb .* HTB_CONF1'])
+AT_CHECK([tc class show dev ovs-tap1 | grep -q 'class htb .* HTB_CONF0'])
+AT_CHECK([tc class show dev ovs-tap1 | grep -q 'class htb .* HTB_CONF1'])
 
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP