From cb56e214679ffd1e30e980c56349831c5a00cfdf Mon Sep 17 00:00:00 2001 From: Jiri Pirko Date: Wed, 30 Jan 2019 08:58:33 +0000 Subject: [PATCH 1/5] mlxsw: spectrum_acl: Include delta bits into hashtable key Currently only ERP mask masked bits in key are considered for the hashtable key. That leads to false negative collisions and fallbacks to C-TCAM in case two keys differ only in delta bits. Fix this by taking full encoded key as a hashtable key, including delta bits. Reported-by: Nir Dotan Signed-off-by: Jiri Pirko Signed-off-by: Ido Schimmel Signed-off-by: David S. Miller --- .../mellanox/mlxsw/spectrum_acl_atcam.c | 21 +++++++++---------- .../mlxsw/spectrum_acl_bloom_filter.c | 2 +- .../mellanox/mlxsw/spectrum_acl_tcam.h | 10 +++++---- 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_atcam.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_atcam.c index 40dc76a5c4128..cda0a7170c340 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_atcam.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_atcam.c @@ -390,8 +390,7 @@ mlxsw_sp_acl_atcam_region_entry_insert(struct mlxsw_sp *mlxsw_sp, if (err) return err; - lkey_id = aregion->ops->lkey_id_get(aregion, aentry->ht_key.enc_key, - erp_id); + lkey_id = aregion->ops->lkey_id_get(aregion, aentry->enc_key, erp_id); if (IS_ERR(lkey_id)) return PTR_ERR(lkey_id); aentry->lkey_id = lkey_id; @@ -399,7 +398,7 @@ mlxsw_sp_acl_atcam_region_entry_insert(struct mlxsw_sp *mlxsw_sp, kvdl_index = mlxsw_afa_block_first_kvdl_index(rulei->act_block); mlxsw_reg_ptce3_pack(ptce3_pl, true, MLXSW_REG_PTCE3_OP_WRITE_WRITE, priority, region->tcam_region_info, - aentry->ht_key.enc_key, erp_id, + aentry->enc_key, erp_id, aentry->delta_info.start, aentry->delta_info.mask, aentry->delta_info.value, @@ -424,12 +423,11 @@ mlxsw_sp_acl_atcam_region_entry_remove(struct mlxsw_sp *mlxsw_sp, struct mlxsw_sp_acl_atcam_lkey_id *lkey_id = aentry->lkey_id; struct mlxsw_sp_acl_tcam_region *region = aregion->region; u8 erp_id = mlxsw_sp_acl_erp_mask_erp_id(aentry->erp_mask); - char *enc_key = aentry->ht_key.enc_key; char ptce3_pl[MLXSW_REG_PTCE3_LEN]; mlxsw_reg_ptce3_pack(ptce3_pl, false, MLXSW_REG_PTCE3_OP_WRITE_WRITE, 0, region->tcam_region_info, - enc_key, erp_id, + aentry->enc_key, erp_id, aentry->delta_info.start, aentry->delta_info.mask, aentry->delta_info.value, @@ -458,7 +456,7 @@ mlxsw_sp_acl_atcam_region_entry_action_replace(struct mlxsw_sp *mlxsw_sp, kvdl_index = mlxsw_afa_block_first_kvdl_index(rulei->act_block); mlxsw_reg_ptce3_pack(ptce3_pl, true, MLXSW_REG_PTCE3_OP_WRITE_UPDATE, priority, region->tcam_region_info, - aentry->ht_key.enc_key, erp_id, + aentry->enc_key, erp_id, aentry->delta_info.start, aentry->delta_info.mask, aentry->delta_info.value, @@ -481,15 +479,15 @@ __mlxsw_sp_acl_atcam_entry_add(struct mlxsw_sp *mlxsw_sp, int err; mlxsw_afk_encode(afk, region->key_info, &rulei->values, - aentry->full_enc_key, mask); + aentry->ht_key.full_enc_key, mask); erp_mask = mlxsw_sp_acl_erp_mask_get(aregion, mask, false); if (IS_ERR(erp_mask)) return PTR_ERR(erp_mask); aentry->erp_mask = erp_mask; aentry->ht_key.erp_id = mlxsw_sp_acl_erp_mask_erp_id(erp_mask); - memcpy(aentry->ht_key.enc_key, aentry->full_enc_key, - sizeof(aentry->ht_key.enc_key)); + memcpy(aentry->enc_key, aentry->ht_key.full_enc_key, + sizeof(aentry->enc_key)); /* Compute all needed delta information and clear the delta bits * from the encrypted key. @@ -498,8 +496,9 @@ __mlxsw_sp_acl_atcam_entry_add(struct mlxsw_sp *mlxsw_sp, aentry->delta_info.start = mlxsw_sp_acl_erp_delta_start(delta); aentry->delta_info.mask = mlxsw_sp_acl_erp_delta_mask(delta); aentry->delta_info.value = - mlxsw_sp_acl_erp_delta_value(delta, aentry->full_enc_key); - mlxsw_sp_acl_erp_delta_clear(delta, aentry->ht_key.enc_key); + mlxsw_sp_acl_erp_delta_value(delta, + aentry->ht_key.full_enc_key); + mlxsw_sp_acl_erp_delta_clear(delta, aentry->enc_key); /* Add rule to the list of A-TCAM rules, assuming this * rule is intended to A-TCAM. In case this rule does diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_bloom_filter.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_bloom_filter.c index f5c381dcb015a..9545b572747ee 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_bloom_filter.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_bloom_filter.c @@ -133,7 +133,7 @@ mlxsw_sp_acl_bf_key_encode(struct mlxsw_sp_acl_atcam_region *aregion, memcpy(chunk + MLXSW_BLOOM_CHUNK_PAD_BYTES, &erp_region_id, sizeof(erp_region_id)); memcpy(chunk + MLXSW_BLOOM_CHUNK_KEY_OFFSET, - &aentry->ht_key.enc_key[chunk_key_offsets[chunk_index]], + &aentry->enc_key[chunk_key_offsets[chunk_index]], MLXSW_BLOOM_CHUNK_KEY_BYTES); chunk += MLXSW_BLOOM_KEY_CHUNK_BYTES; } diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_tcam.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_tcam.h index 10512b7c6d508..0858d5b063536 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_tcam.h +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_tcam.h @@ -161,9 +161,9 @@ struct mlxsw_sp_acl_atcam_region { }; struct mlxsw_sp_acl_atcam_entry_ht_key { - char enc_key[MLXSW_REG_PTCEX_FLEX_KEY_BLOCKS_LEN]; /* Encoded key, - * minus delta bits. - */ + char full_enc_key[MLXSW_REG_PTCEX_FLEX_KEY_BLOCKS_LEN]; /* Encoded + * key. + */ u8 erp_id; }; @@ -175,7 +175,9 @@ struct mlxsw_sp_acl_atcam_entry { struct rhash_head ht_node; struct list_head list; /* Member in entries_list */ struct mlxsw_sp_acl_atcam_entry_ht_key ht_key; - char full_enc_key[MLXSW_REG_PTCEX_FLEX_KEY_BLOCKS_LEN]; /* Encoded key */ + char enc_key[MLXSW_REG_PTCEX_FLEX_KEY_BLOCKS_LEN]; /* Encoded key, + * minus delta bits. + */ struct { u16 start; u8 mask; From a97cfe4de1bee89db3286ad0a4201dc6d2b72456 Mon Sep 17 00:00:00 2001 From: Jiri Pirko Date: Wed, 30 Jan 2019 08:58:34 +0000 Subject: [PATCH 2/5] mlxsw: spectrum_acl: Add C-TCAM spill tracepoint Add some visibility to the rule addition process and trace whenever rule spilled into C-TCAM. Signed-off-by: Jiri Pirko Signed-off-by: Ido Schimmel Signed-off-by: David S. Miller --- .../mellanox/mlxsw/spectrum_acl_atcam.c | 3 ++ include/trace/events/mlxsw.h | 38 +++++++++++++++++++ 2 files changed, 41 insertions(+) create mode 100644 include/trace/events/mlxsw.h diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_atcam.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_atcam.c index cda0a7170c340..a74a390901ac8 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_atcam.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_atcam.c @@ -7,6 +7,8 @@ #include #include #include +#define CREATE_TRACE_POINTS +#include #include "reg.h" #include "core.h" @@ -578,6 +580,7 @@ int mlxsw_sp_acl_atcam_entry_add(struct mlxsw_sp *mlxsw_sp, /* It is possible we failed to add the rule to the A-TCAM due to * exceeded number of masks. Try to spill into C-TCAM. */ + trace_mlxsw_sp_acl_atcam_entry_add_ctcam_spill(mlxsw_sp, aregion); err = mlxsw_sp_acl_ctcam_entry_add(mlxsw_sp, &aregion->cregion, &achunk->cchunk, &aentry->centry, rulei, true); diff --git a/include/trace/events/mlxsw.h b/include/trace/events/mlxsw.h new file mode 100644 index 0000000000000..6c2bafcade180 --- /dev/null +++ b/include/trace/events/mlxsw.h @@ -0,0 +1,38 @@ +/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0 */ +/* Copyright (c) 2019 Mellanox Technologies. All rights reserved */ + +#undef TRACE_SYSTEM +#define TRACE_SYSTEM mlxsw + +#if !defined(_MLXSW_TRACEPOINT_H) || defined(TRACE_HEADER_MULTI_READ) +#define _MLXSW_TRACEPOINT_H + +#include + +struct mlxsw_sp; +struct mlxsw_sp_acl_atcam_region; + +TRACE_EVENT(mlxsw_sp_acl_atcam_entry_add_ctcam_spill, + TP_PROTO(const struct mlxsw_sp *mlxsw_sp, + const struct mlxsw_sp_acl_atcam_region *aregion), + + TP_ARGS(mlxsw_sp, aregion), + + TP_STRUCT__entry( + __field(const void *, mlxsw_sp) + __field(const void *, aregion) + ), + + TP_fast_assign( + __entry->mlxsw_sp = mlxsw_sp; + __entry->aregion = aregion; + ), + + TP_printk("mlxsw_sp %p, aregion %p", + __entry->mlxsw_sp, __entry->aregion) +); + +#endif /* _MLXSW_TRACEPOINT_H */ + +/* This part must be outside protection */ +#include From 1eadbd3ab9c356abc353920cfcd2481c62976f25 Mon Sep 17 00:00:00 2001 From: Jiri Pirko Date: Wed, 30 Jan 2019 08:58:35 +0000 Subject: [PATCH 3/5] selftests: spectrum-2: Extend and move trace helpers Allow to specify number of trace hits and move helpers to the beginning of the file. Signed-off-by: Jiri Pirko Signed-off-by: Ido Schimmel Signed-off-by: David S. Miller --- .../drivers/net/mlxsw/spectrum-2/tc_flower.sh | 71 +++++++++++++------ 1 file changed, 49 insertions(+), 22 deletions(-) diff --git a/tools/testing/selftests/drivers/net/mlxsw/spectrum-2/tc_flower.sh b/tools/testing/selftests/drivers/net/mlxsw/spectrum-2/tc_flower.sh index b41d6256b2d04..ed1ae902f2af8 100755 --- a/tools/testing/selftests/drivers/net/mlxsw/spectrum-2/tc_flower.sh +++ b/tools/testing/selftests/drivers/net/mlxsw/spectrum-2/tc_flower.sh @@ -38,6 +38,55 @@ h2_destroy() simple_if_fini $h2 192.0.2.2/24 198.51.100.2/24 } +tp_record() +{ + local tracepoint=$1 + local cmd=$2 + + perf record -q -e $tracepoint $cmd + return $? +} + +tp_record_all() +{ + local tracepoint=$1 + local seconds=$2 + + perf record -a -q -e $tracepoint sleep $seconds + return $? +} + +__tp_hit_count() +{ + local tracepoint=$1 + + local perf_output=`perf script -F trace:event,trace` + return `echo $perf_output | grep "$tracepoint:" | wc -l` +} + +tp_check_hits() +{ + local tracepoint=$1 + local count=$2 + + __tp_hit_count $tracepoint + if [[ "$?" -ne "$count" ]]; then + return 1 + fi + return 0 +} + +tp_check_hits_any() +{ + local tracepoint=$1 + + __tp_hit_count $tracepoint + if [[ "$?" -eq "0" ]]; then + return 1 + fi + return 0 +} + single_mask_test() { # When only a single mask is required, the device uses the master @@ -325,28 +374,6 @@ ctcam_edge_cases_test() ctcam_no_atcam_masks_test } -tp_record() -{ - local tracepoint=$1 - local cmd=$2 - - perf record -q -e $tracepoint $cmd - return $? -} - -tp_check_hits() -{ - local tracepoint=$1 - local count=$2 - - perf_output=`perf script -F trace:event,trace` - hits=`echo $perf_output | grep "$tracepoint:" | wc -l` - if [[ "$count" -ne "$hits" ]]; then - return 1 - fi - return 0 -} - delta_simple_test() { # The first filter will create eRP, the second filter will fit into From 0d0f20fb2fa08369fb776ef4cb80408517ae3a39 Mon Sep 17 00:00:00 2001 From: Jiri Pirko Date: Wed, 30 Jan 2019 08:58:36 +0000 Subject: [PATCH 4/5] selftests: spectrum-2: Fix multiple_masks_test With recent fix in C-TCAM spillage for delta masks, the test stops to be falsely positive. So fix it not to use delta by adding src_ip bits to the masks. Alongside with that, use C-TCAM spill trace to see when the spillage actually happens. Signed-off-by: Jiri Pirko Signed-off-by: Ido Schimmel Signed-off-by: David S. Miller --- .../drivers/net/mlxsw/spectrum-2/tc_flower.sh | 26 ++++++++++++++++--- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/tools/testing/selftests/drivers/net/mlxsw/spectrum-2/tc_flower.sh b/tools/testing/selftests/drivers/net/mlxsw/spectrum-2/tc_flower.sh index ed1ae902f2af8..73a35ca827ac3 100755 --- a/tools/testing/selftests/drivers/net/mlxsw/spectrum-2/tc_flower.sh +++ b/tools/testing/selftests/drivers/net/mlxsw/spectrum-2/tc_flower.sh @@ -231,20 +231,38 @@ multiple_masks_test() # spillage is performed correctly and that the right filter is # matched + if [[ "$tcflags" != "skip_sw" ]]; then + return 0; + fi + local index RET=0 NUM_MASKS=32 + NUM_ERPS=16 BASE_INDEX=100 for i in $(eval echo {1..$NUM_MASKS}); do index=$((BASE_INDEX - i)) - tc filter add dev $h2 ingress protocol ip pref $index \ - handle $index \ - flower $tcflags dst_ip 192.0.2.2/${i} src_ip 192.0.2.1 \ - action drop + if ((i > NUM_ERPS)); then + exp_hits=1 + err_msg="$i filters - C-TCAM spill did not happen when it was expected" + else + exp_hits=0 + err_msg="$i filters - C-TCAM spill happened when it should not" + fi + + tp_record "mlxsw:mlxsw_sp_acl_atcam_entry_add_ctcam_spill" \ + "tc filter add dev $h2 ingress protocol ip pref $index \ + handle $index \ + flower $tcflags \ + dst_ip 192.0.2.2/${i} src_ip 192.0.2.1/${i} \ + action drop" + tp_check_hits "mlxsw:mlxsw_sp_acl_atcam_entry_add_ctcam_spill" \ + $exp_hits + check_err $? "$err_msg" $MZ $h1 -c 1 -p 64 -a $h1mac -b $h2mac -A 192.0.2.1 \ -B 192.0.2.2 -t ip -q From 1f0ac761bcaaef5fafd220761981da7b53500dcc Mon Sep 17 00:00:00 2001 From: Jiri Pirko Date: Wed, 30 Jan 2019 08:58:37 +0000 Subject: [PATCH 5/5] selftests: spectrum-2: Add delta two masks one key test Ensure that the bug is fixed and we no longer have C-TCAM spill for two keys that differ only in delta. Signed-off-by: Jiri Pirko Signed-off-by: Ido Schimmel Signed-off-by: David S. Miller --- .../drivers/net/mlxsw/spectrum-2/tc_flower.sh | 46 ++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/tools/testing/selftests/drivers/net/mlxsw/spectrum-2/tc_flower.sh b/tools/testing/selftests/drivers/net/mlxsw/spectrum-2/tc_flower.sh index 73a35ca827ac3..f1922bf597b09 100755 --- a/tools/testing/selftests/drivers/net/mlxsw/spectrum-2/tc_flower.sh +++ b/tools/testing/selftests/drivers/net/mlxsw/spectrum-2/tc_flower.sh @@ -9,7 +9,8 @@ lib_dir=$(dirname $0)/../../../../net/forwarding ALL_TESTS="single_mask_test identical_filters_test two_masks_test \ multiple_masks_test ctcam_edge_cases_test delta_simple_test \ - bloom_simple_test bloom_complex_test bloom_delta_test" + delta_two_masks_one_key_test bloom_simple_test \ + bloom_complex_test bloom_delta_test" NUM_NETIFS=2 source $lib_dir/tc_common.sh source $lib_dir/lib.sh @@ -450,6 +451,49 @@ delta_simple_test() log_test "delta simple test ($tcflags)" } +delta_two_masks_one_key_test() +{ + # If 2 keys are the same and only differ in mask in a way that + # they belong under the same ERP (second is delta of the first), + # there should be no C-TCAM spill. + + RET=0 + + if [[ "$tcflags" != "skip_sw" ]]; then + return 0; + fi + + tp_record "mlxsw:*" "tc filter add dev $h2 ingress protocol ip \ + pref 1 handle 101 flower $tcflags dst_ip 192.0.2.0/24 \ + action drop" + tp_check_hits "mlxsw:mlxsw_sp_acl_atcam_entry_add_ctcam_spill" 0 + check_err $? "incorrect C-TCAM spill while inserting the first rule" + + tp_record "mlxsw:*" "tc filter add dev $h2 ingress protocol ip \ + pref 2 handle 102 flower $tcflags dst_ip 192.0.2.2 \ + action drop" + tp_check_hits "mlxsw:mlxsw_sp_acl_atcam_entry_add_ctcam_spill" 0 + check_err $? "incorrect C-TCAM spill while inserting the second rule" + + $MZ $h1 -c 1 -p 64 -a $h1mac -b $h2mac -A 192.0.2.1 -B 192.0.2.2 \ + -t ip -q + + tc_check_packets "dev $h2 ingress" 101 1 + check_err $? "Did not match on correct filter" + + tc filter del dev $h2 ingress protocol ip pref 1 handle 101 flower + + $MZ $h1 -c 1 -p 64 -a $h1mac -b $h2mac -A 192.0.2.1 -B 192.0.2.2 \ + -t ip -q + + tc_check_packets "dev $h2 ingress" 102 1 + check_err $? "Did not match on correct filter" + + tc filter del dev $h2 ingress protocol ip pref 2 handle 102 flower + + log_test "delta two masks one key test ($tcflags)" +} + bloom_simple_test() { # Bloom filter requires that the eRP table is used. This test