Skip to content

Commit

Permalink
netdev-linux: Avoid deadlock in netdev_get_speed.
Browse files Browse the repository at this point in the history 
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: b8f8fad ("netdev-linux: Use speed as max rate in tc classes.")
Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2024-January/052912.html
Reported-by: Daryl Wang <darylywang@google.com>
Suggested-by: Ilya Maximets <i.maximets@ovn.org>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
  • Loading branch information
@amorenoz @igsilya
amorenoz authored and igsilya committed Feb 13, 2024
1 parent 0061a48 commit 19cffe3
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 29 deletions.
32 changes: 20 additions & 12 deletions lib/netdev-linux.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2721,16 +2721,11 @@ netdev_linux_get_features(const struct netdev *netdev_,
}

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);
Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down
53 changes: 36 additions & 17 deletions tests/system-traffic.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2541,34 +2541,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
Expand Down

0 comments on commit 19cffe3

Please sign in to comment.