Merge branch 'mlxsw-spectrum_acl-Include-delta-bits-into-hashtable-key'

Ido Schimmel says:

====================
mlxsw: spectrum_acl: Include delta bits into hashtable key

The Spectrum-2 ASIC allows multiple rules to use the same mask provided
that the difference between their masks is small enough (up to 8
consecutive delta bits). A more detailed explanation is provided in
merge commit 756cd366 ("Merge branch
'mlxsw-Introduce-algorithmic-TCAM-support'").

These delta bits are part of the rule's key and therefore rules that
only differ in their delta bits can be inserted with the same A-TCAM
mask. In case two rules share the same key and only differ in their
priority, then the second will spill to the C-TCAM.

Current code does not take the delta bits into account when checking for
duplicate rules, which leads to unnecessary spillage to the C-TCAM.
This may result in reduced scale and performance.

Patch #1 includes the delta bits in the rule's key to avoid the above
mentioned problem.

Patch #2 adds a tracepoint when a rule is inserted into the C-TCAM.

Patches #3-#5 add test cases to make sure unnecessary spillage into the
C-TCAM does not occur.
====================
Signed-off-by: David S. Miller <davem@davemloft.net>

drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_atcam.c

@@ -7,6 +7,8 @@
 #include <linux/gfp.h>
 #include <linux/refcount.h>
 #include <linux/rhashtable.h>
+#define CREATE_TRACE_POINTS
+#include <trace/events/mlxsw.h>
 
 #include "reg.h"
 #include "core.h"
@@ -390,8 +392,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 +400,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 +425,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 +458,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 +481,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 +498,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
@@ -579,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);

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;
 	}

drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_tcam.h

@@ -161,8 +161,8 @@ 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;

include/trace/events/mlxsw.h

+/* 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 <linux/tracepoint.h>
+
+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 <trace/define_trace.h>

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
@@ -38,6 +39,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
@@ -182,20 +232,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 \
+		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 \
-			action drop
+				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
@@ -325,28 +393,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
@@ -405,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