* [PATCH net-next 06/13] selftests: mlxsw: Add devlink KVD resource test
From: Petr Machata @ 2018-06-30 0:48 UTC (permalink / raw)
To: netdev, linux-kselftest; +Cc: jiri, idosch, shuah, davem
In-Reply-To: <cover.1530319109.git.petrm@mellanox.com>
From: Yuval Mintz <yuvalm@mellanox.com>
Add a selftest that can be used to perform basic sanity of the devlink
resource API as well as test the behavior of KVD manipulation in the
driver.
This is the first case of a HW-only test - in order to test the devlink
resource a driver capable of exposing resources has to be provided
first.
Signed-off-by: Yuval Mintz <yuvalm@mellanox.com>
[petrm@mellanox.com: Extracted two patches out of this patch. Tweaked
commit message.]
Signed-off-by: Petr Machata <petrm@mellanox.com>
---
.../net/mlxsw/spectrum/devlink_resources.sh | 117 +++++++++++++++++++++
1 file changed, 117 insertions(+)
create mode 100755 tools/testing/selftests/drivers/net/mlxsw/spectrum/devlink_resources.sh
diff --git a/tools/testing/selftests/drivers/net/mlxsw/spectrum/devlink_resources.sh b/tools/testing/selftests/drivers/net/mlxsw/spectrum/devlink_resources.sh
new file mode 100755
index 000000000000..b1fe960e398a
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/mlxsw/spectrum/devlink_resources.sh
@@ -0,0 +1,117 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+NUM_NETIFS=1
+source devlink_lib_spectrum.sh
+
+setup_prepare()
+{
+ devlink_sp_read_kvd_defaults
+}
+
+cleanup()
+{
+ pre_cleanup
+ devlink_sp_size_kvd_to_default
+}
+
+trap cleanup EXIT
+
+setup_prepare
+
+profiles_test()
+{
+ local i
+
+ log_info "Running profile tests"
+
+ for i in $KVD_PROFILES; do
+ RET=0
+ devlink_sp_resource_kvd_profile_set $i
+ log_test "'$i' profile"
+ done
+
+ # Default is explicitly tested at end to ensure it's actually applied
+ RET=0
+ devlink_sp_resource_kvd_profile_set "default"
+ log_test "'default' profile"
+}
+
+resources_min_test()
+{
+ local size
+ local i
+ local j
+
+ log_info "Running KVD-minimum tests"
+
+ for i in $KVD_CHILDREN; do
+ RET=0
+ size=$(devlink_resource_get kvd "$i" | jq '.["size_min"]')
+ devlink_resource_size_set "$size" kvd "$i"
+
+ # In case of linear, need to minimize sub-resources as well
+ if [[ "$i" == "linear" ]]; then
+ for j in $KVDL_CHILDREN; do
+ devlink_resource_size_set 0 kvd linear "$j"
+ done
+ fi
+
+ devlink_reload
+ devlink_sp_size_kvd_to_default
+ log_test "'$i' minimize [$size]"
+ done
+}
+
+resources_max_test()
+{
+ local min_size
+ local size
+ local i
+ local j
+
+ log_info "Running KVD-maximum tests"
+ for i in $KVD_CHILDREN; do
+ RET=0
+ devlink_sp_resource_minimize
+
+ # Calculate the maximum possible size for the given partition
+ size=$(devlink_resource_size_get kvd)
+ for j in $KVD_CHILDREN; do
+ if [ "$i" != "$j" ]; then
+ min_size=$(devlink_resource_get kvd "$j" | \
+ jq '.["size_min"]')
+ size=$((size - min_size))
+ fi
+ done
+
+ # Test almost maximum size
+ devlink_resource_size_set "$((size - 128))" kvd "$i"
+ devlink_reload
+ log_test "'$i' almost maximize [$((size - 128))]"
+
+ # Test above maximum size
+ devlink resource set "$DEVLINK_DEV" \
+ path "kvd/$i" size $((size + 128)) &> /dev/null
+ check_fail $? "Set kvd/$i to size $((size + 128)) should fail"
+ log_test "'$i' Overflow rejection [$((size + 128))]"
+
+ # Test maximum size
+ if [ "$i" == "hash_single" ] || [ "$i" == "hash_double" ]; then
+ echo "SKIP: Observed problem with exact max $i"
+ continue
+ fi
+
+ devlink_resource_size_set "$size" kvd "$i"
+ devlink_reload
+ log_test "'$i' maximize [$size]"
+
+ devlink_sp_size_kvd_to_default
+ done
+}
+
+profiles_test
+resources_min_test
+resources_max_test
+
+exit "$RET"
--
2.4.11
^ permalink raw reply related
* [PATCH net-next 07/13] selftests: mlxsw: Add router test
From: Petr Machata @ 2018-06-30 0:49 UTC (permalink / raw)
To: netdev, linux-kselftest; +Cc: jiri, idosch, shuah, davem
In-Reply-To: <cover.1530319109.git.petrm@mellanox.com>
From: Arkadi Sharshevsky <arkadis@mellanox.com>
This test aims for both stand alone and internal usage by the resource
infra. The test receives the number routes to offload and checks:
- The routes were offloaded correctly
- Traffic for each route.
Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
Signed-off-by: Yuval Mintz <yuvalm@mellanox.com>
Reviewed-by: Petr Machata <petrm@mellanox.com>
Signed-off-by: Petr Machata <petrm@mellanox.com>
---
.../selftests/drivers/net/mlxsw/router_scale.sh | 167 +++++++++++++++++++++
1 file changed, 167 insertions(+)
create mode 100644 tools/testing/selftests/drivers/net/mlxsw/router_scale.sh
diff --git a/tools/testing/selftests/drivers/net/mlxsw/router_scale.sh b/tools/testing/selftests/drivers/net/mlxsw/router_scale.sh
new file mode 100644
index 000000000000..d231649b4f01
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/mlxsw/router_scale.sh
@@ -0,0 +1,167 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+ROUTER_NUM_NETIFS=4
+
+router_h1_create()
+{
+ simple_if_init $h1 192.0.1.1/24
+ ip route add 193.0.0.0/8 via 192.0.1.2 dev $h1
+}
+
+router_h1_destroy()
+{
+ ip route del 193.0.0.0/8 via 192.0.1.2 dev $h1
+ simple_if_fini $h1 192.0.1.1/24
+}
+
+router_h2_create()
+{
+ simple_if_init $h2 192.0.2.1/24
+ tc qdisc add dev $h2 handle ffff: ingress
+}
+
+router_h2_destroy()
+{
+ tc qdisc del dev $h2 handle ffff: ingress
+ simple_if_fini $h2 192.0.2.1/24
+}
+
+router_create()
+{
+ ip link set dev $rp1 up
+ ip link set dev $rp2 up
+
+ ip address add 192.0.1.2/24 dev $rp1
+ ip address add 192.0.2.2/24 dev $rp2
+}
+
+router_destroy()
+{
+ ip address del 192.0.2.2/24 dev $rp2
+ ip address del 192.0.1.2/24 dev $rp1
+
+ ip link set dev $rp2 down
+ ip link set dev $rp1 down
+}
+
+router_setup_prepare()
+{
+ h1=${NETIFS[p1]}
+ rp1=${NETIFS[p2]}
+
+ rp2=${NETIFS[p3]}
+ h2=${NETIFS[p4]}
+
+ h1mac=$(mac_get $h1)
+ rp1mac=$(mac_get $rp1)
+
+ vrf_prepare
+
+ router_h1_create
+ router_h2_create
+
+ router_create
+}
+
+router_offload_validate()
+{
+ local route_count=$1
+ local offloaded_count
+
+ offloaded_count=$(ip route | grep -o 'offload' | wc -l)
+ [[ $offloaded_count -ge $route_count ]]
+}
+
+router_routes_create()
+{
+ local route_count=$1
+ local count=0
+
+ ROUTE_FILE="$(mktemp)"
+
+ for i in {0..255}
+ do
+ for j in {0..255}
+ do
+ for k in {0..255}
+ do
+ if [[ $count -eq $route_count ]]; then
+ break 3
+ fi
+
+ echo route add 193.${i}.${j}.${k}/32 via \
+ 192.0.2.1 dev $rp2 >> $ROUTE_FILE
+ ((count++))
+ done
+ done
+ done
+
+ ip -b $ROUTE_FILE &> /dev/null
+}
+
+router_routes_destroy()
+{
+ if [[ -v ROUTE_FILE ]]; then
+ rm -f $ROUTE_FILE
+ fi
+}
+
+router_test()
+{
+ local route_count=$1
+ local should_fail=$2
+ local count=0
+
+ RET=0
+
+ router_routes_create $route_count
+
+ router_offload_validate $route_count
+ check_err_fail $should_fail $? "Offload of $route_count routes"
+ if [[ $RET -ne 0 ]] || [[ $should_fail -eq 1 ]]; then
+ return
+ fi
+
+ tc filter add dev $h2 ingress protocol ip pref 1 flower \
+ skip_sw dst_ip 193.0.0.0/8 action drop
+
+ for i in {0..255}
+ do
+ for j in {0..255}
+ do
+ for k in {0..255}
+ do
+ if [[ $count -eq $route_count ]]; then
+ break 3
+ fi
+
+ $MZ $h1 -c 1 -p 64 -a $h1mac -b $rp1mac \
+ -A 192.0.1.1 -B 193.${i}.${j}.${k} \
+ -t ip -q
+ ((count++))
+ done
+ done
+ done
+
+ tc_check_packets "dev $h2 ingress" 1 $route_count
+ check_err $? "Offload mismatch"
+
+ tc filter del dev $h2 ingress protocol ip pref 1 flower \
+ skip_sw dst_ip 193.0.0.0/8 action drop
+
+ router_routes_destroy
+}
+
+router_cleanup()
+{
+ pre_cleanup
+
+ router_routes_destroy
+ router_destroy
+
+ router_h2_destroy
+ router_h1_destroy
+
+ vrf_cleanup
+}
--
2.4.11
^ permalink raw reply related
* [PATCH net-next 08/13] selftests: mlxsw: Add target for router test on spectrum
From: Petr Machata @ 2018-06-30 0:50 UTC (permalink / raw)
To: netdev, linux-kselftest; +Cc: jiri, idosch, shuah, davem
In-Reply-To: <cover.1530319109.git.petrm@mellanox.com>
From: Yuval Mintz <yuvalm@mellanox.com>
IPv4 routes in Spectrum are based on the kvd single-hash, but as it's
a hash we need to assume we cannot reach 100% of its capacity.
Add a wrapper that provides us with good/bad target numbers for the
Spectrum ASIC.
Signed-off-by: Yuval Mintz <yuvalm@mellanox.com>
Reviewed-by: Petr Machata <petrm@mellanox.com>
[petrm@mellanox.com: Drop shebang.]
Signed-off-by: Petr Machata <petrm@mellanox.com>
---
.../drivers/net/mlxsw/spectrum/router_scale.sh | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
create mode 100644 tools/testing/selftests/drivers/net/mlxsw/spectrum/router_scale.sh
diff --git a/tools/testing/selftests/drivers/net/mlxsw/spectrum/router_scale.sh b/tools/testing/selftests/drivers/net/mlxsw/spectrum/router_scale.sh
new file mode 100644
index 000000000000..21c4697d5bab
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/mlxsw/spectrum/router_scale.sh
@@ -0,0 +1,18 @@
+# SPDX-License-Identifier: GPL-2.0
+source ../router_scale.sh
+
+router_get_target()
+{
+ local should_fail=$1
+ local target
+
+ target=$(devlink_resource_size_get kvd hash_single)
+
+ if [[ $should_fail -eq 0 ]]; then
+ target=$((target * 85 / 100))
+ else
+ target=$((target + 1))
+ fi
+
+ echo $target
+}
--
2.4.11
^ permalink raw reply related
* [PATCH net-next 09/13] selftests: mlxsw: Add tc flower scale test
From: Petr Machata @ 2018-06-30 0:51 UTC (permalink / raw)
To: netdev, linux-kselftest; +Cc: jiri, idosch, shuah, davem
In-Reply-To: <cover.1530319109.git.petrm@mellanox.com>
Add test of capacity to offload flower.
This is a generic portion of the test that is meant to be called from a
driver that supplies a particular number of rules to be tested with.
Signed-off-by: Petr Machata <petrm@mellanox.com>
Reviewed-by: Yuval Mintz <yuvalm@mellanox.com>
---
.../selftests/drivers/net/mlxsw/tc_flower_scale.sh | 134 +++++++++++++++++++++
1 file changed, 134 insertions(+)
create mode 100644 tools/testing/selftests/drivers/net/mlxsw/tc_flower_scale.sh
diff --git a/tools/testing/selftests/drivers/net/mlxsw/tc_flower_scale.sh b/tools/testing/selftests/drivers/net/mlxsw/tc_flower_scale.sh
new file mode 100644
index 000000000000..a6d733d2a4b4
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/mlxsw/tc_flower_scale.sh
@@ -0,0 +1,134 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+# Test for resource limit of offloaded flower rules. The test adds a given
+# number of flower matches for different IPv6 addresses, then generates traffic,
+# and ensures each was hit exactly once. This file contains functions to set up
+# a testing topology and run the test, and is meant to be sourced from a test
+# script that calls the testing routine with a given number of rules.
+
+TC_FLOWER_NUM_NETIFS=2
+
+tc_flower_h1_create()
+{
+ simple_if_init $h1
+ tc qdisc add dev $h1 clsact
+}
+
+tc_flower_h1_destroy()
+{
+ tc qdisc del dev $h1 clsact
+ simple_if_fini $h1
+}
+
+tc_flower_h2_create()
+{
+ simple_if_init $h2
+ tc qdisc add dev $h2 clsact
+}
+
+tc_flower_h2_destroy()
+{
+ tc qdisc del dev $h2 clsact
+ simple_if_fini $h2
+}
+
+tc_flower_setup_prepare()
+{
+ h1=${NETIFS[p1]}
+ h2=${NETIFS[p2]}
+
+ vrf_prepare
+
+ tc_flower_h1_create
+ tc_flower_h2_create
+}
+
+tc_flower_cleanup()
+{
+ pre_cleanup
+
+ tc_flower_h2_destroy
+ tc_flower_h1_destroy
+
+ vrf_cleanup
+
+ if [[ -v TC_FLOWER_BATCH_FILE ]]; then
+ rm -f $TC_FLOWER_BATCH_FILE
+ fi
+}
+
+tc_flower_addr()
+{
+ local num=$1; shift
+
+ printf "2001:db8:1::%x" $num
+}
+
+tc_flower_rules_create()
+{
+ local count=$1; shift
+ local should_fail=$1; shift
+
+ TC_FLOWER_BATCH_FILE="$(mktemp)"
+
+ for ((i = 0; i < count; ++i)); do
+ cat >> $TC_FLOWER_BATCH_FILE <<-EOF
+ filter add dev $h2 ingress \
+ prot ipv6 \
+ pref 1000 \
+ flower $tcflags dst_ip $(tc_flower_addr $i) \
+ action drop
+ EOF
+ done
+
+ tc -b $TC_FLOWER_BATCH_FILE
+ check_err_fail $should_fail $? "Rule insertion"
+}
+
+__tc_flower_test()
+{
+ local count=$1; shift
+ local should_fail=$1; shift
+ local last=$((count - 1))
+
+ tc_flower_rules_create $count $should_fail
+
+ for ((i = 0; i < count; ++i)); do
+ $MZ $h1 -q -c 1 -t ip -p 20 -b bc -6 \
+ -A 2001:db8:2::1 \
+ -B $(tc_flower_addr $i)
+ done
+
+ MISMATCHES=$(
+ tc -j -s filter show dev $h2 ingress |
+ jq -r '[ .[] | select(.kind == "flower") | .options |
+ values as $rule | .actions[].stats.packets |
+ select(. != 1) | "\(.) on \($rule.keys.dst_ip)" ] |
+ join(", ")'
+ )
+
+ test -z "$MISMATCHES"
+ check_err $? "Expected to capture 1 packet for each IP, but got $MISMATCHES"
+}
+
+tc_flower_test()
+{
+ local count=$1; shift
+ local should_fail=$1; shift
+
+ # We use lower 16 bits of IPv6 address for match. Also there are only 16
+ # bits of rule priority space.
+ if ((count > 65536)); then
+ check_err 1 "Invalid count of $count. At most 65536 rules supported"
+ return
+ fi
+
+ if ! tc_offload_check $TC_FLOWER_NUM_NETIFS; then
+ check_err 1 "Could not test offloaded functionality"
+ return
+ fi
+
+ tcflags="skip_sw"
+ __tc_flower_test $count $should_fail
+}
--
2.4.11
^ permalink raw reply related
* [PATCH net-next 10/13] selftests: mlxsw: Add target for tc flower test on spectrum
From: Petr Machata @ 2018-06-30 0:51 UTC (permalink / raw)
To: netdev, linux-kselftest; +Cc: jiri, idosch, shuah, davem
In-Reply-To: <cover.1530319109.git.petrm@mellanox.com>
Add a wrapper around mlxsw/tc_flower_scale.sh that parameterizes the
generic tc flower scale test template with Spectrum-specific target
values.
Signed-off-by: Petr Machata <petrm@mellanox.com>
Reviewed-by: Yuval Mintz <yuvalm@mellanox.com>
---
.../drivers/net/mlxsw/spectrum/tc_flower_scale.sh | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
create mode 100644 tools/testing/selftests/drivers/net/mlxsw/spectrum/tc_flower_scale.sh
diff --git a/tools/testing/selftests/drivers/net/mlxsw/spectrum/tc_flower_scale.sh b/tools/testing/selftests/drivers/net/mlxsw/spectrum/tc_flower_scale.sh
new file mode 100644
index 000000000000..f9bfd8937765
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/mlxsw/spectrum/tc_flower_scale.sh
@@ -0,0 +1,19 @@
+# SPDX-License-Identifier: GPL-2.0
+source ../tc_flower_scale.sh
+
+tc_flower_get_target()
+{
+ local should_fail=$1; shift
+
+ # 6144 (6x1024) is the theoretical maximum.
+ # One bank of 512 rules is taken by the 18-byte MC router rule.
+ # One rule is the ACL catch-all.
+ # 6144 - 512 - 1 = 5631
+ local target=5631
+
+ if ((! should_fail)); then
+ echo $target
+ else
+ echo $((target + 1))
+ fi
+}
--
2.4.11
^ permalink raw reply related
* [PATCH net-next 11/13] selftests: mlxsw: Add scale test for mirror-to-gretap
From: Petr Machata @ 2018-06-30 0:52 UTC (permalink / raw)
To: netdev, linux-kselftest; +Cc: jiri, idosch, shuah, davem
In-Reply-To: <cover.1530319109.git.petrm@mellanox.com>
Test that it's possible to offload a given number of mirrors.
Signed-off-by: Petr Machata <petrm@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
.../drivers/net/mlxsw/mirror_gre_scale.sh | 197 +++++++++++++++++++++
1 file changed, 197 insertions(+)
create mode 100644 tools/testing/selftests/drivers/net/mlxsw/mirror_gre_scale.sh
diff --git a/tools/testing/selftests/drivers/net/mlxsw/mirror_gre_scale.sh b/tools/testing/selftests/drivers/net/mlxsw/mirror_gre_scale.sh
new file mode 100644
index 000000000000..6f3a70df63bc
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/mlxsw/mirror_gre_scale.sh
@@ -0,0 +1,197 @@
+# SPDX-License-Identifier: GPL-2.0
+
+# Test offloading a number of mirrors-to-gretap. The test creates a number of
+# tunnels. Then it adds one flower mirror for each of the tunnels, matching a
+# given host IP. Then it generates traffic at each of the host IPs and checks
+# that the traffic has been mirrored at the appropriate tunnel.
+#
+# +--------------------------+ +--------------------------+
+# | H1 | | H2 |
+# | + $h1 | | $h2 + |
+# | | 2001:db8:1:X::1/64 | | 2001:db8:1:X::2/64 | |
+# +-----|--------------------+ +--------------------|-----+
+# | |
+# +-----|-------------------------------------------------------------|-----+
+# | SW o--> mirrors | |
+# | +---|-------------------------------------------------------------|---+ |
+# | | + $swp1 BR $swp2 + | |
+# | +---------------------------------------------------------------------+ |
+# | |
+# | + $swp3 + gt6-<X> (ip6gretap) |
+# | | 2001:db8:2:X::1/64 : loc=2001:db8:2:X::1 |
+# | | : rem=2001:db8:2:X::2 |
+# | | : ttl=100 |
+# | | : tos=inherit |
+# | | : |
+# +-----|--------------------------------:----------------------------------+
+# | :
+# +-----|--------------------------------:----------------------------------+
+# | H3 + $h3 + h3-gt6-<X> (ip6gretap) |
+# | 2001:db8:2:X::2/64 loc=2001:db8:2:X::2 |
+# | rem=2001:db8:2:X::1 |
+# | ttl=100 |
+# | tos=inherit |
+# | |
+# +-------------------------------------------------------------------------+
+
+source ../../../../net/forwarding/mirror_lib.sh
+
+MIRROR_NUM_NETIFS=6
+
+mirror_gre_ipv6_addr()
+{
+ local net=$1; shift
+ local num=$1; shift
+
+ printf "2001:db8:%x:%x" $net $num
+}
+
+mirror_gre_tunnels_create()
+{
+ local count=$1; shift
+ local should_fail=$1; shift
+
+ MIRROR_GRE_BATCH_FILE="$(mktemp)"
+ for ((i=0; i < count; ++i)); do
+ local match_dip=$(mirror_gre_ipv6_addr 1 $i)::2
+ local htun=h3-gt6-$i
+ local tun=gt6-$i
+
+ ((mirror_gre_tunnels++))
+
+ ip address add dev $h1 $(mirror_gre_ipv6_addr 1 $i)::1/64
+ ip address add dev $h2 $(mirror_gre_ipv6_addr 1 $i)::2/64
+
+ ip address add dev $swp3 $(mirror_gre_ipv6_addr 2 $i)::1/64
+ ip address add dev $h3 $(mirror_gre_ipv6_addr 2 $i)::2/64
+
+ tunnel_create $tun ip6gretap \
+ $(mirror_gre_ipv6_addr 2 $i)::1 \
+ $(mirror_gre_ipv6_addr 2 $i)::2 \
+ ttl 100 tos inherit allow-localremote
+
+ tunnel_create $htun ip6gretap \
+ $(mirror_gre_ipv6_addr 2 $i)::2 \
+ $(mirror_gre_ipv6_addr 2 $i)::1
+ ip link set $htun vrf v$h3
+ matchall_sink_create $htun
+
+ cat >> $MIRROR_GRE_BATCH_FILE <<-EOF
+ filter add dev $swp1 ingress pref 1000 \
+ protocol ipv6 \
+ flower $tcflags dst_ip $match_dip \
+ action mirred egress mirror dev $tun
+ EOF
+ done
+
+ tc -b $MIRROR_GRE_BATCH_FILE
+ check_err_fail $should_fail $? "Mirror rule insertion"
+}
+
+mirror_gre_tunnels_destroy()
+{
+ local count=$1; shift
+
+ for ((i=0; i < count; ++i)); do
+ local htun=h3-gt6-$i
+ local tun=gt6-$i
+
+ ip address del dev $h3 $(mirror_gre_ipv6_addr 2 $i)::2/64
+ ip address del dev $swp3 $(mirror_gre_ipv6_addr 2 $i)::1/64
+
+ ip address del dev $h2 $(mirror_gre_ipv6_addr 1 $i)::2/64
+ ip address del dev $h1 $(mirror_gre_ipv6_addr 1 $i)::1/64
+
+ tunnel_destroy $htun
+ tunnel_destroy $tun
+ done
+}
+
+__mirror_gre_test()
+{
+ local count=$1; shift
+ local should_fail=$1; shift
+
+ mirror_gre_tunnels_create $count $should_fail
+ if ((should_fail)); then
+ return
+ fi
+
+ sleep 5
+
+ for ((i = 0; i < count; ++i)); do
+ local dip=$(mirror_gre_ipv6_addr 1 $i)::2
+ local htun=h3-gt6-$i
+ local message
+
+ icmp6_capture_install $htun
+ mirror_test v$h1 "" $dip $htun 100 10
+ icmp6_capture_uninstall $htun
+ done
+}
+
+mirror_gre_test()
+{
+ local count=$1; shift
+ local should_fail=$1; shift
+
+ if ! tc_offload_check $TC_FLOWER_NUM_NETIFS; then
+ check_err 1 "Could not test offloaded functionality"
+ return
+ fi
+
+ tcflags="skip_sw"
+ __mirror_gre_test $count $should_fail
+}
+
+mirror_gre_setup_prepare()
+{
+ h1=${NETIFS[p1]}
+ swp1=${NETIFS[p2]}
+
+ swp2=${NETIFS[p3]}
+ h2=${NETIFS[p4]}
+
+ swp3=${NETIFS[p5]}
+ h3=${NETIFS[p6]}
+
+ mirror_gre_tunnels=0
+
+ vrf_prepare
+
+ simple_if_init $h1
+ simple_if_init $h2
+ simple_if_init $h3
+
+ ip link add name br1 type bridge vlan_filtering 1
+ ip link set dev br1 up
+
+ ip link set dev $swp1 master br1
+ ip link set dev $swp1 up
+ tc qdisc add dev $swp1 clsact
+
+ ip link set dev $swp2 master br1
+ ip link set dev $swp2 up
+
+ ip link set dev $swp3 up
+}
+
+mirror_gre_cleanup()
+{
+ mirror_gre_tunnels_destroy $mirror_gre_tunnels
+
+ ip link set dev $swp3 down
+
+ ip link set dev $swp2 down
+
+ tc qdisc del dev $swp1 clsact
+ ip link set dev $swp1 down
+
+ ip link del dev br1
+
+ simple_if_fini $h3
+ simple_if_fini $h2
+ simple_if_fini $h1
+
+ vrf_cleanup
+}
--
2.4.11
^ permalink raw reply related
* [PATCH net-next 12/13] selftests: mlxsw: Add target for mirror-to-gretap test on spectrum
From: Petr Machata @ 2018-06-30 0:53 UTC (permalink / raw)
To: netdev, linux-kselftest; +Cc: jiri, idosch, shuah, davem
In-Reply-To: <cover.1530319109.git.petrm@mellanox.com>
Add a wrapper around mlxsw/mirror_gre_scale.sh that parameterized number
of offloadable mirrors on Spectrum machines.
Signed-off-by: Petr Machata <petrm@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
.../drivers/net/mlxsw/spectrum/mirror_gre_scale.sh | 13 +++++++++++++
1 file changed, 13 insertions(+)
create mode 100644 tools/testing/selftests/drivers/net/mlxsw/spectrum/mirror_gre_scale.sh
diff --git a/tools/testing/selftests/drivers/net/mlxsw/spectrum/mirror_gre_scale.sh b/tools/testing/selftests/drivers/net/mlxsw/spectrum/mirror_gre_scale.sh
new file mode 100644
index 000000000000..8d2186c7c62b
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/mlxsw/spectrum/mirror_gre_scale.sh
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0
+source ../mirror_gre_scale.sh
+
+mirror_gre_get_target()
+{
+ local should_fail=$1; shift
+
+ if ((! should_fail)); then
+ echo 3
+ else
+ echo 4
+ fi
+}
--
2.4.11
^ permalink raw reply related
* [PATCH net-next 13/13] selftests: mlxsw: Add scale test for resources
From: Petr Machata @ 2018-06-30 0:53 UTC (permalink / raw)
To: netdev, linux-kselftest; +Cc: jiri, idosch, shuah, davem
In-Reply-To: <cover.1530319109.git.petrm@mellanox.com>
From: Yuval Mintz <yuvalm@mellanox.com>
Add a scale test capable of validating that offloaded network
functionality is indeed functional at scale when configured to
the different KVD profiles available.
Start by testing offloaded routes are functional at scale by
passing traffic on each one of them in turn.
Signed-off-by: Yuval Mintz <yuvalm@mellanox.com>
Signed-off-by: Petr Machata <petrm@mellanox.com>
---
.../drivers/net/mlxsw/spectrum/resource_scale.sh | 55 ++++++++++++++++++++++
1 file changed, 55 insertions(+)
create mode 100755 tools/testing/selftests/drivers/net/mlxsw/spectrum/resource_scale.sh
diff --git a/tools/testing/selftests/drivers/net/mlxsw/spectrum/resource_scale.sh b/tools/testing/selftests/drivers/net/mlxsw/spectrum/resource_scale.sh
new file mode 100755
index 000000000000..a0a80e1a69e8
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/mlxsw/spectrum/resource_scale.sh
@@ -0,0 +1,55 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+NUM_NETIFS=6
+source ../../../../net/forwarding/lib.sh
+source ../../../../net/forwarding/tc_common.sh
+source devlink_lib_spectrum.sh
+
+current_test=""
+
+cleanup()
+{
+ pre_cleanup
+ if [ ! -z $current_test ]; then
+ ${current_test}_cleanup
+ fi
+ devlink_sp_size_kvd_to_default
+}
+
+devlink_sp_read_kvd_defaults
+trap cleanup EXIT
+
+ALL_TESTS="router tc_flower mirror_gre"
+for current_test in ${TESTS:-$ALL_TESTS}; do
+ source ${current_test}_scale.sh
+
+ num_netifs_var=${current_test^^}_NUM_NETIFS
+ num_netifs=${!num_netifs_var:-$NUM_NETIFS}
+
+ for profile in $KVD_PROFILES; do
+ RET=0
+ devlink_sp_resource_kvd_profile_set $profile
+ if [[ $RET -gt 0 ]]; then
+ log_test "'$current_test' [$profile] setting"
+ continue
+ fi
+
+ for should_fail in 0 1; do
+ RET=0
+ target=$(${current_test}_get_target "$should_fail")
+ ${current_test}_setup_prepare
+ setup_wait $num_netifs
+ ${current_test}_test "$target" "$should_fail"
+ ${current_test}_cleanup
+ if [[ "$should_fail" -eq 0 ]]; then
+ log_test "'$current_test' [$profile] $target"
+ else
+ log_test "'$current_test' [$profile] overflow $target"
+ fi
+ done
+ done
+done
+current_test=""
+
+exit "$RET"
--
2.4.11
^ permalink raw reply related
* Re: [RFC v2 PATCH 1/4] eBPF: Add new eBPF prog type BPF_PROG_TYPE_SOCKET_SG_FILTER
From: Tushar Dave @ 2018-06-30 0:46 UTC (permalink / raw)
To: Daniel Borkmann, ast, davem, jakub.kicinski, quentin.monnet,
jiong.wang, guro, sandipan, john.fastabend, kafai, rdna, brakmo,
netdev, acme, sowmini.varadhan
In-Reply-To: <9795dd0e-c5d8-2970-bef3-9cca6000d68a@iogearbox.net>
On 06/29/2018 01:27 AM, Daniel Borkmann wrote:
> On 06/19/2018 08:00 PM, Tushar Dave wrote:
>> Add new eBPF prog type BPF_PROG_TYPE_SOCKET_SG_FILTER which uses the
>> existing socket filter infrastructure for bpf program attach and load.
>> SOCKET_SG_FILTER eBPF program receives struct scatterlist as bpf context
>> contrast to SOCKET_FILTER which deals with struct skb. This is useful
>> for kernel entities that don't have skb to represent packet data but
>> want to run eBPF socket filter on packet data that is in form of struct
>> scatterlist e.g. IB/RDMA
>>
>> Signed-off-by: Tushar Dave <tushar.n.dave@oracle.com>
>> Acked-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
>> ---
>> include/linux/bpf_types.h | 1 +
>> include/linux/filter.h | 8 +++++
>> include/uapi/linux/bpf.h | 7 ++++
>> kernel/bpf/syscall.c | 1 +
>> kernel/bpf/verifier.c | 1 +
>> net/core/filter.c | 77 ++++++++++++++++++++++++++++++++++++++++--
>> samples/bpf/bpf_load.c | 11 ++++--
>> tools/bpf/bpftool/prog.c | 1 +
>> tools/include/uapi/linux/bpf.h | 7 ++++
>> tools/lib/bpf/libbpf.c | 3 ++
>> tools/lib/bpf/libbpf.h | 2 ++
>> 11 files changed, 114 insertions(+), 5 deletions(-)
>>
> [...]
>>
>> +static bool socksg_filter_is_valid_access(int off, int size,
>> + enum bpf_access_type type,
>> + const struct bpf_prog *prog,
>> + struct bpf_insn_access_aux *info)
>> +{
>> + switch (off) {
>> + case offsetof(struct sg_filter_md, data):
>> + info->reg_type = PTR_TO_PACKET;
>> + break;
>> + case offsetof(struct sg_filter_md, data_end):
>> + info->reg_type = PTR_TO_PACKET_END;
>> + break;
>> + }
>> +
>> + if (off < 0 || off >= sizeof(struct sg_filter_md))
>> + return false;
>> + if (off % size != 0)
>> + return false;
>> + if (size != sizeof(__u64))
>> + return false;
>> +
>> + return true;
>> +}
>
> Just a note, don't know much about rds, but when you make this writeable for
> rds/tcp you definitely must make sure that it can be handled properly in there,
> meaning when program rewrites packet data that this data is private to the BPF
> prog (to avoid races/corruption) and that the rewritten data is correctly handled
> from there.
Sure thing. When I add something like bpf_sg_store_bytes(), I will make
sure to take care of rewrites.
Thanks.
-Tushar
>
^ permalink raw reply
* [PATCH net-next 1/2] tcp: notify when a delayed ack is sent
From: Lawrence Brakmo @ 2018-06-30 1:48 UTC (permalink / raw)
To: netdev
Cc: Kernel Team, Blake Matheny, Alexei Starovoitov, Neal Cardwell,
Yuchung Cheng, Steve Ibanez, Eric Dumazet
In-Reply-To: <20180630014815.2881895-1-brakmo@fb.com>
DCTCP depends on the CA_EVENT_NON_DELAYED_ACK and CA_EVENT_DELAYED_ACK
notifications to keep track if it needs to send an ACK for packets that
were received with a particular ECN state but whose ACK was delayed.
Under some circumstances, for example when a delayed ACK is sent with a
data packet, DCTCP state was not being updated due to a lack of
notification that the previously delayed ACK was sent. As a result, it
would sometimes send a duplicate ACK when a new data packet arrived.
This patch insures that DCTCP's state is correctly updated so it will
not send the duplicate ACK.
Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
---
net/ipv4/tcp_output.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index f8f6129160dd..41f6ad7a21e4 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -172,6 +172,8 @@ static inline void tcp_event_ack_sent(struct sock *sk, unsigned int pkts)
__sock_put(sk);
}
tcp_dec_quickack_mode(sk, pkts);
+ if (inet_csk_ack_scheduled(sk))
+ tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK);
inet_csk_clear_xmit_timer(sk, ICSK_TIME_DACK);
}
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 2/2] tcp: ack immediately when a cwr packet arrives
From: Lawrence Brakmo @ 2018-06-30 1:48 UTC (permalink / raw)
To: netdev
Cc: Kernel Team, Blake Matheny, Alexei Starovoitov, Neal Cardwell,
Yuchung Cheng, Steve Ibanez, Eric Dumazet
In-Reply-To: <20180630014815.2881895-1-brakmo@fb.com>
We observed high 99 and 99.9% latencies when doing RPCs with DCTCP. The
problem is triggered when the last packet of a request arrives CE
marked. The reply will carry the ECE mark causing TCP to shrink its cwnd
to 1 (because there are no packets in flight). When the 1st packet of
the next request arrives it was sometimes delayed adding up to 40ms to
the latency.
This patch insures that CWR makred data packets arriving will be acked
immediately.
Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
---
net/ipv4/tcp_input.c | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 76ca88f63b70..b024d36f0d56 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -98,6 +98,8 @@ int sysctl_tcp_max_orphans __read_mostly = NR_FILE;
#define FLAG_UPDATE_TS_RECENT 0x4000 /* tcp_replace_ts_recent() */
#define FLAG_NO_CHALLENGE_ACK 0x8000 /* do not call tcp_send_challenge_ack() */
#define FLAG_ACK_MAYBE_DELAYED 0x10000 /* Likely a delayed ACK */
+#define FLAG_OFO_POSSIBLE 0x20000 /* Possible OFO */
+#define FLAG_CWR 0x40000 /* CWR in this ACK */
#define FLAG_ACKED (FLAG_DATA_ACKED|FLAG_SYN_ACKED)
#define FLAG_NOT_DUP (FLAG_DATA|FLAG_WIN_UPDATE|FLAG_ACKED)
@@ -5087,7 +5089,7 @@ static inline void tcp_data_snd_check(struct sock *sk)
/*
* Check if sending an ack is needed.
*/
-static void __tcp_ack_snd_check(struct sock *sk, int ofo_possible)
+static void __tcp_ack_snd_check(struct sock *sk, int flags)
{
struct tcp_sock *tp = tcp_sk(sk);
unsigned long rtt, delay;
@@ -5102,13 +5104,16 @@ static void __tcp_ack_snd_check(struct sock *sk, int ofo_possible)
(tp->rcv_nxt - tp->copied_seq < sk->sk_rcvlowat ||
__tcp_select_window(sk) >= tp->rcv_wnd)) ||
/* We ACK each frame or... */
- tcp_in_quickack_mode(sk)) {
+ tcp_in_quickack_mode(sk) ||
+ /* We received CWR */
+ flags & FLAG_CWR) {
send_now:
tcp_send_ack(sk);
return;
}
- if (!ofo_possible || RB_EMPTY_ROOT(&tp->out_of_order_queue)) {
+ if (!(flags & FLAG_OFO_POSSIBLE) ||
+ RB_EMPTY_ROOT(&tp->out_of_order_queue)) {
tcp_send_delayed_ack(sk);
return;
}
@@ -5134,13 +5139,13 @@ static void __tcp_ack_snd_check(struct sock *sk, int ofo_possible)
HRTIMER_MODE_REL_PINNED_SOFT);
}
-static inline void tcp_ack_snd_check(struct sock *sk)
+static inline void tcp_ack_snd_check(struct sock *sk, int flags)
{
if (!inet_csk_ack_scheduled(sk)) {
/* We sent a data segment already. */
return;
}
- __tcp_ack_snd_check(sk, 1);
+ __tcp_ack_snd_check(sk, flags | FLAG_OFO_POSSIBLE);
}
/*
@@ -5489,6 +5494,7 @@ void tcp_rcv_established(struct sock *sk, struct sk_buff *skb)
goto discard;
}
} else {
+ int flags;
int eaten = 0;
bool fragstolen = false;
@@ -5525,7 +5531,9 @@ void tcp_rcv_established(struct sock *sk, struct sk_buff *skb)
goto no_ack;
}
- __tcp_ack_snd_check(sk, 0);
+ flags = (tcp_flag_word(th) & TCP_FLAG_CWR) ?
+ FLAG_CWR : 0;
+ __tcp_ack_snd_check(sk, flags);
no_ack:
if (eaten)
kfree_skb_partial(skb, fragstolen);
@@ -5561,7 +5569,8 @@ void tcp_rcv_established(struct sock *sk, struct sk_buff *skb)
tcp_data_queue(sk, skb);
tcp_data_snd_check(sk);
- tcp_ack_snd_check(sk);
+ tcp_ack_snd_check(sk, (tcp_flag_word(th) & TCP_FLAG_CWR) ?
+ FLAG_CWR : 0);
return;
csum_error:
@@ -6150,7 +6159,7 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
/* tcp_data could move socket to TIME-WAIT */
if (sk->sk_state != TCP_CLOSE) {
tcp_data_snd_check(sk);
- tcp_ack_snd_check(sk);
+ tcp_ack_snd_check(sk, 0);
}
if (!queued) {
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 0/2] tcp: fix high tail latencies in DCTCP
From: Lawrence Brakmo @ 2018-06-30 1:48 UTC (permalink / raw)
To: netdev
Cc: Kernel Team, Blake Matheny, Alexei Starovoitov, Neal Cardwell,
Yuchung Cheng, Steve Ibanez, Eric Dumazet
When have observed high tail latencies when using DCTCP for RPCs as
compared to using Cubic. For example, in one setup there are 2 hosts
sending to a 3rd one, with each sender having 3 flows (1 stream,
1 1MB back-to-back RPCs and 1 10KB back-to-back RPCs). The following
table shows the 99% and 99.9% latencies for both Cubic and dctcp:
Cubic 99% Cubic 99.9% dctcp 99% dctcp 99.9%
1MB RPCs 2.6ms 5.5ms 43ms 208ms
10KB RPCs 1.1ms 1.3ms 53ms 212ms
Looking at tcpdump traces showed that there are two causes for the
latency.
1) RTOs caused by the receiver sending a dup ACK and not ACKing
the last (and only) packet sent.
2) Delaying ACKs when the sender has a cwnd of 1, so everything
pauses for the duration of the delayed ACK.
The first patch fixes the cause of the dup ACKs, not updating DCTCP
state when an ACK that was initially delayed has been sent with a
data packet.
The second patch insures that an ACK is sent immediately when a
CWR marked packet arrives.
With the patches the latencies for DCTCP now look like:
dctcp 99% dctcp 99.9%
1MB RPCs 4.8ms 6.5ms
10KB RPCs 143us 184us
Note that while the 1MB RPCs tail latencies are higher than Cubic's,
the 10KB latencies are much smaller than Cubic's. These patches fix
issues on the receiver, but tcpdump traces indicate there is an
opportunity to also fix an issue at the sender that adds about 3ms
to the tail latencies.
The following trace shows the issue that tiggers an RTO (fixed by these patches):
Host A sends the last packets of the request
Host B receives them, and the last packet is marked with congestion (CE)
Host B sends ACKs for packets not marked with congestion
Host B sends data packet with reply and ACK for packet marked with
congestion (TCP flag ECE)
Host A receives ACKs with no ECE flag
Host A receives data packet with ACK for the last packet of request
and which has TCP ECE bit set
Host A sends 1st data packet of the next request with TCP flag CWR
Host B receives the packet (as seen in tcpdump at B), no CE flag
Host B sends a dup ACK that also has the TCP ECE flag
Host A RTO timer fires!
Host A to send the next packet
Host A receives an ACK for everything it has sent (i.e. Host B
did receive 1st packet of request)
Host A send more packets…
[PATCH net-next 1/2] tcp: notify when a delayed ack is sent
[PATCH net-next 2/2] tcp: ack immediately when a cwr packet arrives
net/ipv4/tcp_input.c | 25 +++++++++++++++++--------
net/ipv4/tcp_output.c | 2 ++
2 files changed, 19 insertions(+), 8 deletions(-)
^ permalink raw reply
* Re: [PATCH] atmel: using strlcpy() to avoid possible buffer overflows
From: YueHaibing @ 2018-06-30 6:30 UTC (permalink / raw)
To: Andy Shevchenko
Cc: simon, Kalle Valo, Linux Kernel Mailing List, netdev,
open list:TI WILINK WIRELES..., David S. Miller
In-Reply-To: <CAHp75VcTVtzLuwH=Ff6DBKJyQE4t=HT7hB2O=gvgN+V-38LH5g@mail.gmail.com>
On 2018/6/30 5:51, Andy Shevchenko wrote:
> On Sat, Jun 30, 2018 at 12:47 AM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Fri, Jun 29, 2018 at 5:51 AM, YueHaibing <yuehaibing@huawei.com> wrote:
>>> 'firmware' is a module param which may been longer than firmware_id,
>>> so using strlcpy() to guard against overflows
>>
>> strncat() is against overflow, this does a bit more.
>>
>>> priv->firmware_id[0] = '\0';
>> ...
>>> if (firmware) /* module parameter */
>>> - strcpy(priv->firmware_id, firmware);
>>> + strlcpy(priv->firmware_id, firmware, sizeof(priv->firmware_id));
>>
>> In either case the above '\0' is not needed.
>> But it looks like the intention was to use strncat() / strlcat().
>
> Ah, this is under condition, yes. If no parameter supplied, this needs
> to be clean, but
> priv is allocated with zeroed memory
> https://elixir.bootlin.com/linux/latest/source/net/core/dev.c#L8369
so just remove this line:
priv->firmware_id[0] = '\0';
and using strlcpy while 'firmware' NOT null is enough.
Will send v2.
>
^ permalink raw reply
* [PATCH v2] atmel: using strlcpy() to avoid possible buffer overflows
From: YueHaibing @ 2018-06-30 6:33 UTC (permalink / raw)
To: simon, kvalo
Cc: linux-kernel, netdev, linux-wireless, davem, andy.shevchenko,
YueHaibing
'firmware' is a module param which may been longer than firmware_id,
so using strlcpy() to guard against overflows. Also priv is allocated
with zeroed memory,no need to set firmware_id[0] to '\0'.
v1 -> v2: remove priv->firmware_id[0] = '\0';
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/net/wireless/atmel/atmel.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/wireless/atmel/atmel.c b/drivers/net/wireless/atmel/atmel.c
index b01dc34..cec715b 100644
--- a/drivers/net/wireless/atmel/atmel.c
+++ b/drivers/net/wireless/atmel/atmel.c
@@ -1516,10 +1516,9 @@ struct net_device *init_atmel_card(unsigned short irq, unsigned long port,
priv->present_callback = card_present;
priv->card = card;
priv->firmware = NULL;
- priv->firmware_id[0] = '\0';
priv->firmware_type = fw_type;
if (firmware) /* module parameter */
- strcpy(priv->firmware_id, firmware);
+ strlcpy(priv->firmware_id, firmware, sizeof(priv->firmware_id));
priv->bus_type = card_present ? BUS_TYPE_PCCARD : BUS_TYPE_PCI;
priv->station_state = STATION_STATE_DOWN;
priv->do_rx_crc = 0;
--
2.7.0
^ permalink raw reply related
* [PATCH net-next v3 0/4] net: vhost: improve performance when enable busyloop
From: xiangxia.m.yue @ 2018-06-30 6:33 UTC (permalink / raw)
To: jasowang; +Cc: mst, makita.toshiaki, virtualization, netdev, Tonghao Zhang
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
This patches improve the guest receive and transmit performance.
On the handle_tx side, we poll the sock receive queue at the same time.
handle_rx do that in the same way.
This patches are splited from previous big patch:
http://patchwork.ozlabs.org/patch/934673/
For more performance report, see patch 4.
Tonghao Zhang (4):
net: vhost: lock the vqs one by one
net: vhost: replace magic number of lock annotation
net: vhost: factor out busy polling logic to vhost_net_busy_poll()
net: vhost: add rx busy polling in tx path
drivers/vhost/net.c | 106 +++++++++++++++++++++++++++-----------------------
drivers/vhost/vhost.c | 24 ++++--------
2 files changed, 65 insertions(+), 65 deletions(-)
--
1.8.3.1
^ permalink raw reply
* [PATCH net-next v3 1/4] net: vhost: lock the vqs one by one
From: xiangxia.m.yue @ 2018-06-30 6:33 UTC (permalink / raw)
To: jasowang
Cc: mst, makita.toshiaki, virtualization, netdev, Tonghao Zhang,
Tonghao Zhang
In-Reply-To: <1530340438-3039-1-git-send-email-xiangxia.m.yue@gmail.com>
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
This patch changes the way that lock all vqs
at the same, to lock them one by one. It will
be used for next patch to avoid the deadlock.
Signed-off-by: Tonghao Zhang <zhangtonghao@didichuxing.com>
---
drivers/vhost/vhost.c | 24 +++++++-----------------
1 file changed, 7 insertions(+), 17 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 895eaa2..4ca9383 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -294,8 +294,11 @@ static void vhost_vq_meta_reset(struct vhost_dev *d)
{
int i;
- for (i = 0; i < d->nvqs; ++i)
+ for (i = 0; i < d->nvqs; ++i) {
+ mutex_lock(&d->vqs[i]->mutex);
__vhost_vq_meta_reset(d->vqs[i]);
+ mutex_unlock(&d->vqs[i]->mutex);
+ }
}
static void vhost_vq_reset(struct vhost_dev *dev,
@@ -887,20 +890,6 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
#define vhost_get_used(vq, x, ptr) \
vhost_get_user(vq, x, ptr, VHOST_ADDR_USED)
-static void vhost_dev_lock_vqs(struct vhost_dev *d)
-{
- int i = 0;
- for (i = 0; i < d->nvqs; ++i)
- mutex_lock_nested(&d->vqs[i]->mutex, i);
-}
-
-static void vhost_dev_unlock_vqs(struct vhost_dev *d)
-{
- int i = 0;
- for (i = 0; i < d->nvqs; ++i)
- mutex_unlock(&d->vqs[i]->mutex);
-}
-
static int vhost_new_umem_range(struct vhost_umem *umem,
u64 start, u64 size, u64 end,
u64 userspace_addr, int perm)
@@ -950,7 +939,10 @@ static void vhost_iotlb_notify_vq(struct vhost_dev *d,
if (msg->iova <= vq_msg->iova &&
msg->iova + msg->size - 1 > vq_msg->iova &&
vq_msg->type == VHOST_IOTLB_MISS) {
+ mutex_lock(&node->vq->mutex);
vhost_poll_queue(&node->vq->poll);
+ mutex_unlock(&node->vq->mutex);
+
list_del(&node->node);
kfree(node);
}
@@ -982,7 +974,6 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
int ret = 0;
mutex_lock(&dev->mutex);
- vhost_dev_lock_vqs(dev);
switch (msg->type) {
case VHOST_IOTLB_UPDATE:
if (!dev->iotlb) {
@@ -1016,7 +1007,6 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
break;
}
- vhost_dev_unlock_vqs(dev);
mutex_unlock(&dev->mutex);
return ret;
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next v3 2/4] net: vhost: replace magic number of lock annotation
From: xiangxia.m.yue @ 2018-06-30 6:33 UTC (permalink / raw)
To: jasowang
Cc: mst, makita.toshiaki, virtualization, netdev, Tonghao Zhang,
Tonghao Zhang
In-Reply-To: <1530340438-3039-1-git-send-email-xiangxia.m.yue@gmail.com>
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Use the VHOST_NET_VQ_XXX as a subclass for mutex_lock_nested.
Signed-off-by: Tonghao Zhang <zhangtonghao@didichuxing.com>
---
drivers/vhost/net.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index e7cf7d2..62bb8e8 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -484,7 +484,7 @@ static void handle_tx(struct vhost_net *net)
bool zcopy, zcopy_used;
int sent_pkts = 0;
- mutex_lock(&vq->mutex);
+ mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_TX);
sock = vq->private_data;
if (!sock)
goto out;
@@ -655,7 +655,7 @@ static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk)
/* Flush batched heads first */
vhost_rx_signal_used(rvq);
/* Both tx vq and rx socket were polled here */
- mutex_lock_nested(&vq->mutex, 1);
+ mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_TX);
vhost_disable_notify(&net->dev, vq);
preempt_disable();
@@ -789,7 +789,7 @@ static void handle_rx(struct vhost_net *net)
__virtio16 num_buffers;
int recv_pkts = 0;
- mutex_lock_nested(&vq->mutex, 0);
+ mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_RX);
sock = vq->private_data;
if (!sock)
goto out;
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next v3 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
From: xiangxia.m.yue @ 2018-06-30 6:33 UTC (permalink / raw)
To: jasowang
Cc: mst, makita.toshiaki, virtualization, netdev, Tonghao Zhang,
Tonghao Zhang
In-Reply-To: <1530340438-3039-1-git-send-email-xiangxia.m.yue@gmail.com>
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Factor out generic busy polling logic and will be
used for tx path in the next patch. And with the patch,
qemu can set differently the busyloop_timeout for rx queue.
Signed-off-by: Tonghao Zhang <zhangtonghao@didichuxing.com>
---
drivers/vhost/net.c | 92 ++++++++++++++++++++++++++++++-----------------------
1 file changed, 53 insertions(+), 39 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 62bb8e8..458f81d 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -429,6 +429,50 @@ static int vhost_net_enable_vq(struct vhost_net *n,
return vhost_poll_start(poll, sock->file);
}
+static int sk_has_rx_data(struct sock *sk)
+{
+ struct socket *sock = sk->sk_socket;
+
+ if (sock->ops->peek_len)
+ return sock->ops->peek_len(sock);
+
+ return skb_queue_empty(&sk->sk_receive_queue);
+}
+
+static void vhost_net_busy_poll(struct vhost_net *net,
+ struct vhost_virtqueue *rvq,
+ struct vhost_virtqueue *tvq,
+ bool rx)
+{
+ unsigned long uninitialized_var(endtime);
+ struct socket *sock = rvq->private_data;
+ struct vhost_virtqueue *vq = rx ? tvq : rvq;
+ unsigned long busyloop_timeout = rx ? rvq->busyloop_timeout :
+ tvq->busyloop_timeout;
+
+ mutex_lock_nested(&vq->mutex, rx ? VHOST_NET_VQ_TX: VHOST_NET_VQ_RX);
+ vhost_disable_notify(&net->dev, vq);
+
+ preempt_disable();
+ endtime = busy_clock() + busyloop_timeout;
+ while (vhost_can_busy_poll(tvq->dev, endtime) &&
+ !(sock && sk_has_rx_data(sock->sk)) &&
+ vhost_vq_avail_empty(tvq->dev, tvq))
+ cpu_relax();
+ preempt_enable();
+
+ if ((rx && !vhost_vq_avail_empty(&net->dev, vq)) ||
+ (!rx && (sock && sk_has_rx_data(sock->sk)))) {
+ vhost_poll_queue(&vq->poll);
+ } else if (unlikely(vhost_enable_notify(&net->dev, vq))) {
+ vhost_disable_notify(&net->dev, vq);
+ vhost_poll_queue(&vq->poll);
+ }
+
+ mutex_unlock(&vq->mutex);
+}
+
+
static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
struct vhost_virtqueue *vq,
struct iovec iov[], unsigned int iov_size,
@@ -621,16 +665,6 @@ static int peek_head_len(struct vhost_net_virtqueue *rvq, struct sock *sk)
return len;
}
-static int sk_has_rx_data(struct sock *sk)
-{
- struct socket *sock = sk->sk_socket;
-
- if (sock->ops->peek_len)
- return sock->ops->peek_len(sock);
-
- return skb_queue_empty(&sk->sk_receive_queue);
-}
-
static void vhost_rx_signal_used(struct vhost_net_virtqueue *nvq)
{
struct vhost_virtqueue *vq = &nvq->vq;
@@ -645,39 +679,19 @@ static void vhost_rx_signal_used(struct vhost_net_virtqueue *nvq)
static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk)
{
- struct vhost_net_virtqueue *rvq = &net->vqs[VHOST_NET_VQ_RX];
- struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
- struct vhost_virtqueue *vq = &nvq->vq;
- unsigned long uninitialized_var(endtime);
- int len = peek_head_len(rvq, sk);
+ struct vhost_net_virtqueue *nvq_rx = &net->vqs[VHOST_NET_VQ_RX];
+ struct vhost_net_virtqueue *nvq_tx = &net->vqs[VHOST_NET_VQ_TX];
- if (!len && vq->busyloop_timeout) {
- /* Flush batched heads first */
- vhost_rx_signal_used(rvq);
- /* Both tx vq and rx socket were polled here */
- mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_TX);
- vhost_disable_notify(&net->dev, vq);
+ int len = peek_head_len(nvq_rx, sk);
- preempt_disable();
- endtime = busy_clock() + vq->busyloop_timeout;
-
- while (vhost_can_busy_poll(&net->dev, endtime) &&
- !sk_has_rx_data(sk) &&
- vhost_vq_avail_empty(&net->dev, vq))
- cpu_relax();
-
- preempt_enable();
-
- if (!vhost_vq_avail_empty(&net->dev, vq))
- vhost_poll_queue(&vq->poll);
- else if (unlikely(vhost_enable_notify(&net->dev, vq))) {
- vhost_disable_notify(&net->dev, vq);
- vhost_poll_queue(&vq->poll);
- }
+ if (!len && nvq_rx->vq.busyloop_timeout) {
+ /* Flush batched heads first */
+ vhost_rx_signal_used(nvq_rx);
- mutex_unlock(&vq->mutex);
+ /* Both tx vq and rx socket were polled here */
+ vhost_net_busy_poll(net, &nvq_rx->vq, &nvq_tx->vq, true);
- len = peek_head_len(rvq, sk);
+ len = peek_head_len(nvq_rx, sk);
}
return len;
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next v3 4/4] net: vhost: add rx busy polling in tx path
From: xiangxia.m.yue @ 2018-06-30 6:33 UTC (permalink / raw)
To: jasowang
Cc: mst, makita.toshiaki, virtualization, netdev, Tonghao Zhang,
Tonghao Zhang
In-Reply-To: <1530340438-3039-1-git-send-email-xiangxia.m.yue@gmail.com>
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
This patch improves the guest receive and transmit performance.
On the handle_tx side, we poll the sock receive queue at the
same time. handle_rx do that in the same way.
We set the poll-us=100us and use the iperf3 to test
its bandwidth, use the netperf to test throughput and mean
latency. When running the tests, the vhost-net kthread of
that VM, is alway 100% CPU. The commands are shown as below.
iperf3 -s -D
iperf3 -c IP -i 1 -P 1 -t 20 -M 1400
or
netserver
netperf -H IP -t TCP_RR -l 20 -- -O "THROUGHPUT,MEAN_LATENCY"
host -> guest:
iperf3:
* With the patch: 27.0 Gbits/sec
* Without the patch: 14.4 Gbits/sec
netperf (TCP_RR):
* With the patch: 48039.56 trans/s, 20.64us mean latency
* Without the patch: 46027.07 trans/s, 21.58us mean latency
This patch also improves the guest transmit performance.
guest -> host:
iperf3:
* With the patch: 27.2 Gbits/sec
* Without the patch: 24.4 Gbits/sec
netperf (TCP_RR):
* With the patch: 47963.25 trans/s, 20.71us mean latency
* Without the patch: 45796.70 trans/s, 21.68us mean latency
Signed-off-by: Tonghao Zhang <zhangtonghao@didichuxing.com>
---
drivers/vhost/net.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 458f81d..fb43d82 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -478,17 +478,13 @@ static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
struct iovec iov[], unsigned int iov_size,
unsigned int *out_num, unsigned int *in_num)
{
- unsigned long uninitialized_var(endtime);
+ struct vhost_net_virtqueue *nvq_rx = &net->vqs[VHOST_NET_VQ_RX];
int r = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
out_num, in_num, NULL, NULL);
if (r == vq->num && vq->busyloop_timeout) {
- preempt_disable();
- endtime = busy_clock() + vq->busyloop_timeout;
- while (vhost_can_busy_poll(vq->dev, endtime) &&
- vhost_vq_avail_empty(vq->dev, vq))
- cpu_relax();
- preempt_enable();
+ vhost_net_busy_poll(net, &nvq_rx->vq, vq, false);
+
r = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
out_num, in_num, NULL, NULL);
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH net-next v3 4/4] net: vhost: add rx busy polling in tx path
From: Jesper Dangaard Brouer @ 2018-06-30 7:03 UTC (permalink / raw)
To: xiangxia.m.yue; +Cc: mst, netdev, brouer, virtualization, Tonghao Zhang
In-Reply-To: <1530340438-3039-5-git-send-email-xiangxia.m.yue@gmail.com>
On Fri, 29 Jun 2018 23:33:58 -0700
xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> This patch improves the guest receive and transmit performance.
> On the handle_tx side, we poll the sock receive queue at the
> same time. handle_rx do that in the same way.
>
> We set the poll-us=100us and use the iperf3 to test
Where/how do you configure poll-us=100us ?
Are you talking about /proc/sys/net/core/busy_poll ?
p.s. Nice performance boost! :-)
> its bandwidth, use the netperf to test throughput and mean
> latency. When running the tests, the vhost-net kthread of
> that VM, is alway 100% CPU. The commands are shown as below.
>
> iperf3 -s -D
> iperf3 -c IP -i 1 -P 1 -t 20 -M 1400
>
> or
> netserver
> netperf -H IP -t TCP_RR -l 20 -- -O "THROUGHPUT,MEAN_LATENCY"
>
> host -> guest:
> iperf3:
> * With the patch: 27.0 Gbits/sec
> * Without the patch: 14.4 Gbits/sec
>
> netperf (TCP_RR):
> * With the patch: 48039.56 trans/s, 20.64us mean latency
> * Without the patch: 46027.07 trans/s, 21.58us mean latency
>
> This patch also improves the guest transmit performance.
>
> guest -> host:
> iperf3:
> * With the patch: 27.2 Gbits/sec
> * Without the patch: 24.4 Gbits/sec
>
> netperf (TCP_RR):
> * With the patch: 47963.25 trans/s, 20.71us mean latency
> * Without the patch: 45796.70 trans/s, 21.68us mean latency
>
> Signed-off-by: Tonghao Zhang <zhangtonghao@didichuxing.com>
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [net-next PATCH v5 1/7] net: Refactor XPS for CPUs and Rx queues
From: Nambiar, Amritha @ 2018-06-30 7:48 UTC (permalink / raw)
To: David Miller
Cc: netdev, alexander.h.duyck, willemdebruijn.kernel,
sridhar.samudrala, alexander.duyck, edumazet, hannes, tom, tom
In-Reply-To: <20180629.215938.2008552830198656487.davem@davemloft.net>
On 6/29/2018 5:59 AM, David Miller wrote:
> From: Amritha Nambiar <amritha.nambiar@intel.com>
> Date: Wed, 27 Jun 2018 15:31:18 -0700
>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index c6b377a..3790ac9 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
> ...
>> +static inline bool attr_test_online(unsigned long j,
>> + const unsigned long *online_mask,
>> + unsigned int nr_bits)
>
> This is a networking header file, so for names you are putting into
> the global namespace please give them some kind of prefix that
> indicates it is about networking.
>
Sure, will prefix these with 'netif_'.
^ permalink raw reply
* Re: [net-next PATCH v5 3/7] net: sock: Change tx_queue_mapping in sock_common to unsigned short
From: Nambiar, Amritha @ 2018-06-30 7:49 UTC (permalink / raw)
To: David Miller
Cc: netdev, alexander.h.duyck, willemdebruijn.kernel,
sridhar.samudrala, alexander.duyck, edumazet, hannes, tom, tom
In-Reply-To: <20180629.220511.870587764860032381.davem@davemloft.net>
On 6/29/2018 6:05 AM, David Miller wrote:
> From: Amritha Nambiar <amritha.nambiar@intel.com>
> Date: Wed, 27 Jun 2018 15:31:28 -0700
>
>> @@ -1681,17 +1681,25 @@ static inline int sk_receive_skb(struct sock *sk, struct sk_buff *skb,
>>
>> static inline void sk_tx_queue_set(struct sock *sk, int tx_queue)
>> {
>> + /* sk_tx_queue_mapping accept only upto a 16-bit value */
>> + if (WARN_ON_ONCE((unsigned short)tx_queue > USHRT_MAX))
>> + return;
>> sk->sk_tx_queue_mapping = tx_queue;
>> }
>>
>> +#define NO_QUEUE_MAPPING USHRT_MAX
>
> I think you need to check ">= USHRT_MAX" since USHRT_MAX is how you
> indicate no queue.
>
Agree, I'll fix this.
^ permalink raw reply
* Re: [net-next PATCH v5 4/7] net: Record receive queue number for a connection
From: Nambiar, Amritha @ 2018-06-30 7:50 UTC (permalink / raw)
To: David Miller
Cc: netdev, alexander.h.duyck, willemdebruijn.kernel,
sridhar.samudrala, alexander.duyck, edumazet, hannes, tom, tom
In-Reply-To: <20180629.220639.1360385545962536071.davem@davemloft.net>
On 6/29/2018 6:06 AM, David Miller wrote:
> From: Amritha Nambiar <amritha.nambiar@intel.com>
> Date: Wed, 27 Jun 2018 15:31:34 -0700
>
>> @@ -1702,6 +1709,13 @@ static inline int sk_tx_queue_get(const struct sock *sk)
>> return -1;
>> }
>>
>> +static inline void sk_rx_queue_set(struct sock *sk, const struct sk_buff *skb)
>> +{
>> +#ifdef CONFIG_XPS
>> + sk->sk_rx_queue_mapping = skb_get_rx_queue(skb);
>> +#endif
>> +}
>
> Maybe add a >= NO_QUEUE_MAPPING check like you did for
> skc_tx_queue_mapping.
>
Will fix.
^ permalink raw reply
* Re: [PATCH net-next 3/5] sctp: add spp_ipv6_flowlabel and spp_dscp for sctp_paddrparams
From: 吉藤英明 @ 2018-06-30 8:15 UTC (permalink / raw)
To: Xin Long
Cc: Marcelo Ricardo Leitner, Neil Horman, David Miller, network dev,
linux-sctp, yoshfuji
In-Reply-To: <CADvbK_cHRA1aN+-tdzURqkZUf29w-0VwGHC_oKDbfLPjYQ1+qg@mail.gmail.com>
Hi,
2018-06-28 15:40 GMT+09:00 Xin Long <lucien.xin@gmail.com>:
> On Tue, Jun 26, 2018 at 8:02 PM, 吉藤英明
> <hideaki.yoshifuji@miraclelinux.com> wrote:
>> 2018-06-26 13:33 GMT+09:00 Xin Long <lucien.xin@gmail.com>:
>>> On Tue, Jun 26, 2018 at 12:31 AM, Marcelo Ricardo Leitner
>>> <marcelo.leitner@gmail.com> wrote:
>>>> Hi,
>>>>
>>>> On Tue, Jun 26, 2018 at 01:12:00AM +0900, 吉藤英明 wrote:
>>>>> Hi,
>>>>>
>>>>> 2018-06-25 22:03 GMT+09:00 Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>:
>>>>> > On Mon, Jun 25, 2018 at 07:28:47AM -0400, Neil Horman wrote:
>>>>> >> On Mon, Jun 25, 2018 at 04:31:26PM +0900, David Miller wrote:
>>>>> >> > From: Xin Long <lucien.xin@gmail.com>
>>>>> >> > Date: Mon, 25 Jun 2018 10:14:35 +0800
>>>>> >> >
>>>>> >> > > struct sctp_paddrparams {
>>>>> >> > > @@ -773,6 +775,8 @@ struct sctp_paddrparams {
>>>>> >> > > __u32 spp_pathmtu;
>>>>> >> > > __u32 spp_sackdelay;
>>>>> >> > > __u32 spp_flags;
>>>>> >> > > + __u32 spp_ipv6_flowlabel;
>>>>> >> > > + __u8 spp_dscp;
>>>>> >> > > } __attribute__((packed, aligned(4)));
>>>>> >> >
>>>>> >> > I don't think you can change the size of this structure like this.
>>>>> >> >
>>>>> >> > This check in sctp_setsockopt_peer_addr_params():
>>>>> >> >
>>>>> >> > if (optlen != sizeof(struct sctp_paddrparams))
>>>>> >> > return -EINVAL;
>>>>> >> >
>>>>> >> > is going to trigger in old kernels when executing programs
>>>>> >> > built against the new struct definition.
>>>>> >
>>>>> > That will happen, yes, but do we really care about being future-proof
>>>>> > here? I mean: if we also update such check(s) to support dealing with
>>>>> > smaller-than-supported structs, newer kernels will be able to run
>>>>> > programs built against the old struct, and the new one; while building
>>>>> > using newer headers and running on older kernel may fool the
>>>>> > application in other ways too (like enabling support for something
>>>>> > that is available on newer kernel and that is not present in the older
>>>>> > one).
>>>>>
>>>>> We should not break existing apps.
>>>>> We still accept apps of pre-2.4 era without sin6_scope_id
>>>>> (e.g., net/ipv6/af_inet6.c:inet6_bind()).
>>>>
>>>> Yes. That's what I tried to say. That is supporting an old app built
>>>> with old kernel headers and running on a newer kernel, and not the
>>>> other way around (an app built with fresh headers and running on an
>>>> old kernel).
>>> To make it, I will update the check like:
>>>
>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>> index 1df5d07..c949d8c 100644
>>> --- a/net/sctp/socket.c
>>> +++ b/net/sctp/socket.c
>>> @@ -2715,13 +2715,18 @@ static int
>>> sctp_setsockopt_peer_addr_params(struct sock *sk,
>>> struct sctp_sock *sp = sctp_sk(sk);
>>> int error;
>>> int hb_change, pmtud_change, sackdelay_change;
>>> + int plen = sizeof(params);
>>> + int old_plen = plen - sizeof(u32) * 2;
>>
>> if (optlen < offsetof(struct sctp_paddrparams, spp_ipv6_flowlabel))
>> maybe?
> Hi, yoshfuji,
> offsetof() is better. thank you.
>
>>
>>>
>>> - if (optlen != sizeof(struct sctp_paddrparams))
>>> + if (optlen != plen && optlen != old_plen)
>>> return -EINVAL;
>>>
>>> if (copy_from_user(¶ms, optval, optlen))
>>> return -EFAULT;
>>>
>>> + if (optlen == old_plen)
>>> + params.spp_flags &= ~(SPP_DSCP | SPP_IPV6_FLOWLABEL);
>>
>> I think we should return -EINVAL if size is not new one.
> Sorry, if we returned -EINVAL when size is the old one,
> how can we guarantee an old app built with old kernel
> headers and running on a newer kernel works well?
> or you meant?
> if ((params.spp_flags & (SPP_DSCP | SPP_IPV6_FLOWLABEL)) &&
> optlen != plen)
> return EINVAL;
Yes, I meant this (it should be -EINVAL though).
>
>>
>> --yoshfuji
>>
>>> +
>>> /* Validate flags and value parameters. */
>>> hb_change = params.spp_flags & SPP_HB;
>>> pmtud_change = params.spp_flags & SPP_PMTUD;
>>> @@ -5591,10 +5596,13 @@ static int
>>> sctp_getsockopt_peer_addr_params(struct sock *sk, int len,
>>> struct sctp_transport *trans = NULL;
>>> struct sctp_association *asoc = NULL;
>>> struct sctp_sock *sp = sctp_sk(sk);
>>> + int plen = sizeof(params);
>>> + int old_plen = plen - sizeof(u32) * 2;
>>>
>>> - if (len < sizeof(struct sctp_paddrparams))
>>> + if (len < old_plen)
>>> return -EINVAL;
>>> - len = sizeof(struct sctp_paddrparams);
>>> +
>>> + len = len >= plen ? plen : old_plen;
>>> if (copy_from_user(¶ms, optval, len))
>>> return -EFAULT;
>>>
>>> does it look ok to you?
^ permalink raw reply
* Re: [PATCH net-next v2 3/4] net: check tunnel option type in tunnel flags
From: Daniel Borkmann @ 2018-06-30 8:47 UTC (permalink / raw)
To: Jakub Kicinski, Daniel Borkmann
Cc: Jiri Benc, davem, Roopa Prabhu, jiri, jhs, xiyou.wangcong,
oss-drivers, netdev, Pieter Jansen van Vuuren
In-Reply-To: <20180629100133.16286f24@cakuba.netronome.com>
On 06/29/2018 07:01 PM, Jakub Kicinski wrote:
> On Fri, 29 Jun 2018 09:04:15 +0200, Daniel Borkmann wrote:
>> On 06/28/2018 06:54 PM, Jakub Kicinski wrote:
>>> On Thu, 28 Jun 2018 09:42:06 +0200, Jiri Benc wrote:
>>>> On Wed, 27 Jun 2018 11:49:49 +0200, Daniel Borkmann wrote:
>>>>> Looks good to me, and yes in BPF case a mask like TUNNEL_OPTIONS_PRESENT is
>>>>> right approach since this is opaque info and solely defined by the BPF prog
>>>>> that is using the generic helper.
>>>>
>>>> Wouldn't it make sense to introduce some safeguards here (in a backward
>>>> compatible way, of course)? It's easy to mistakenly set data for a
>>>> different tunnel type in a BPF program and then be surprised by the
>>>> result. It might help users if such usage was detected by the kernel,
>>>> one way or another.
>>>
>>> Well, that's how it works today ;)
>>
>> Well, it was designed like that on purpose, to be i) agnostic of the underlying
>> device, ii) to not clutter BPF API with tens of different APIs effectively doing
>> the same thing, and at the same time to avoid adding protocol specifics. E.g. at
>> least core bits of bpf_skb_{set,get}_tunnel_key() will work whether I use vxlan
>> or geneve underneath (we are actually using it this way) and I could use things
>> like tun_id to encode custom meta data from BPF for either of them depending on flavor
>> picked by orchestration system. For the tunnel options in bpf_skb_{set,get}_tunnel_opt()
>> it's similar although here there needs to be awareness of the underlying dev depending
>> on whether you encode data into e.g. gbp or tlvs, etc. However, downside right now I
>> can see with a patch like below is that:
>>
>> i) People might still just keep using 'TUNNEL_OPTIONS_PRESENT path' since available
>> and backwards compatible with current/older kernels, ii) we cut bits away from
>> size over time for each new tunnel proto added in future that would support tunnel
>> options, iii) that extension is one-sided (at least below) and same would be needed
>> in getter part, and iv) there needs to be a way for the case when folks add new
>> tunnel options where we don't need to worry that we forget updating BPF_F_TUN_*
>> each time otherwise this will easily slip through and again people will just rely
>> on using TUNNEL_OPTIONS_PRESENT catchall. Given latter and in particular point i)
>> I wouldn't think it's worth the pain, the APIs were added to BPF in v4.6 so this
>> would buy them 2 more years wrt kernel compatibility with same functionality level.
>> And point v), I just noticed the patch is actually buggy: size is ARG_CONST_SIZE and
>> verifier will attempt to check the value whether the buffer passed in argument 2 is
>> valid or not, so using flags here in upper bits would let verification fail, you'd
>> really have to make a new helper just for this.
>
> Ah, indeed. I'd rather avoid a new helper, if we reuse an old one
> people can always write a program like:
>
> err = helper(all_flags);
> if (err == -EINVAL)
> err = helper(fallback_flags);
>
> With a new helper the program will not even load on old kernels :(
>
> Could we add the flags new to bpf_skb_set_tunnel_key(), maybe? It is a
> bit ugly because only options care about the flags and in theory
> bpf_skb_set_tunnel_key() doesn't have to be called before
> bpf_skb_set_tunnel_opt() ...
Right, though it's also ugly splitting things this way over two helpers
when this is _specifically_ only for one of them (unless you do need it for
hw offload where you need to signal for tunnel key _and_ options in BPF
itself but that's not from what I see is the case here). For sake of
'safeguard'-only I bet users won't do such fallback for reasons above but
again use currently TUNNEL_OPTIONS_PRESENT route, as is since it's simpler
and available since long time already. Thus I would prefer to leave it as
is in such case since ship has sailed long ago on this, and usage of this
extra check is not really obvious either, so I don't think it's worth it
and 'value-add' is marginal.
Thanks,
Daniel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox