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 <[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_*.
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
>>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
