Linux RDMA and InfiniBand development
 help / color / mirror / Atom feed
* [PATCH V2 mlx5-next 10/13] net/mlx5: E-Switch, Pass metadata from FDB to eswitch manager
From: Saeed Mahameed @ 2019-06-25 17:48 UTC (permalink / raw)
  To: Saeed Mahameed, Leon Romanovsky
  Cc: netdev@vger.kernel.org, linux-rdma@vger.kernel.org, Jianbo Liu,
	Roi Dayan, Mark Bloch
In-Reply-To: <20190625174727.20309-1-saeedm@mellanox.com>

From: Jianbo Liu <jianbol@mellanox.com>

In order to do matching on metadata in slow path when demuxing traffic
to representors, explicitly enable the feature that allows HW to pass
metadata REG_C_0 from FDB to eswitch manager NIC_RX table.

Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 .../mellanox/mlx5/core/eswitch_offloads.c     | 64 +++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index a3cf787382ee..178ff9b05258 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -574,6 +574,59 @@ void mlx5_eswitch_del_send_to_vport_rule(struct mlx5_flow_handle *rule)
 	mlx5_del_flow_rules(rule);
 }
 
+static int mlx5_eswitch_enable_passing_vport_metadata(struct mlx5_eswitch *esw)
+{
+	u32 out[MLX5_ST_SZ_DW(query_esw_vport_context_out)] = {};
+	u32 in[MLX5_ST_SZ_DW(modify_esw_vport_context_in)] = {};
+	u8 fdb_to_vport_reg_c_id;
+	int err;
+
+	err = mlx5_eswitch_query_esw_vport_context(esw, esw->manager_vport,
+						   out, sizeof(out));
+	if (err)
+		return err;
+
+	fdb_to_vport_reg_c_id = MLX5_GET(query_esw_vport_context_out, out,
+					 esw_vport_context.fdb_to_vport_reg_c_id);
+
+	fdb_to_vport_reg_c_id |= MLX5_FDB_TO_VPORT_REG_C_0;
+	MLX5_SET(modify_esw_vport_context_in, in,
+		 esw_vport_context.fdb_to_vport_reg_c_id, fdb_to_vport_reg_c_id);
+
+	MLX5_SET(modify_esw_vport_context_in, in,
+		 field_select.fdb_to_vport_reg_c_id, 1);
+
+	return mlx5_eswitch_modify_esw_vport_context(esw, esw->manager_vport,
+						     in, sizeof(in));
+}
+
+static int mlx5_eswitch_disable_passing_vport_metadata(struct mlx5_eswitch *esw)
+{
+	u32 out[MLX5_ST_SZ_DW(query_esw_vport_context_out)] = {};
+	u32 in[MLX5_ST_SZ_DW(modify_esw_vport_context_in)] = {};
+	u8 fdb_to_vport_reg_c_id;
+	int err;
+
+	err = mlx5_eswitch_query_esw_vport_context(esw, esw->manager_vport,
+						   out, sizeof(out));
+	if (err)
+		return err;
+
+	fdb_to_vport_reg_c_id = MLX5_GET(query_esw_vport_context_out, out,
+					 esw_vport_context.fdb_to_vport_reg_c_id);
+
+	fdb_to_vport_reg_c_id &= ~MLX5_FDB_TO_VPORT_REG_C_0;
+
+	MLX5_SET(modify_esw_vport_context_in, in,
+		 esw_vport_context.fdb_to_vport_reg_c_id, fdb_to_vport_reg_c_id);
+
+	MLX5_SET(modify_esw_vport_context_in, in,
+		 field_select.fdb_to_vport_reg_c_id, 1);
+
+	return mlx5_eswitch_modify_esw_vport_context(esw, esw->manager_vport,
+						     in, sizeof(in));
+}
+
 static void peer_miss_rules_setup(struct mlx5_core_dev *peer_dev,
 				  struct mlx5_flow_spec *spec,
 				  struct mlx5_flow_destination *dest)
@@ -1977,6 +2030,12 @@ int esw_offloads_init(struct mlx5_eswitch *esw, int vf_nvports,
 	if (err)
 		return err;
 
+	if (mlx5_eswitch_vport_match_metadata_enabled(esw)) {
+		err = mlx5_eswitch_enable_passing_vport_metadata(esw);
+		if (err)
+			goto err_vport_metadata;
+	}
+
 	/* Only load special vports reps. VF reps will be loaded in
 	 * context of functions_changed event handler through real
 	 * or emulated event.
@@ -2004,6 +2063,9 @@ int esw_offloads_init(struct mlx5_eswitch *esw, int vf_nvports,
 	return 0;
 
 err_reps:
+	if (mlx5_eswitch_vport_match_metadata_enabled(esw))
+		mlx5_eswitch_disable_passing_vport_metadata(esw);
+err_vport_metadata:
 	esw_offloads_steering_cleanup(esw);
 	return err;
 }
@@ -2033,6 +2095,8 @@ void esw_offloads_cleanup(struct mlx5_eswitch *esw)
 	mlx5_rdma_disable_roce(esw->dev);
 	esw_offloads_devcom_cleanup(esw);
 	esw_offloads_unload_all_reps(esw, esw->esw_funcs.num_vfs);
+	if (mlx5_eswitch_vport_match_metadata_enabled(esw))
+		mlx5_eswitch_disable_passing_vport_metadata(esw);
 	esw_offloads_steering_cleanup(esw);
 }
 
-- 
2.21.0


^ permalink raw reply related

* [PATCH V2 mlx5-next 09/13] net/mlx5: E-Switch, Add query and modify esw vport context functions
From: Saeed Mahameed @ 2019-06-25 17:48 UTC (permalink / raw)
  To: Saeed Mahameed, Leon Romanovsky
  Cc: netdev@vger.kernel.org, linux-rdma@vger.kernel.org, Jianbo Liu,
	Roi Dayan, Mark Bloch
In-Reply-To: <20190625174727.20309-1-saeedm@mellanox.com>

From: Jianbo Liu <jianbol@mellanox.com>

Add esw vport query and modify functions, and exposing them is needed for
enabling or disabling registers passed as metatdata to vport NIC_RX table
in slow path.

Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 .../net/ethernet/mellanox/mlx5/core/eswitch.c | 24 +++++++++++++++++++
 .../net/ethernet/mellanox/mlx5/core/eswitch.h |  5 ++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
index 1235fd84ae3a..67598272d4a9 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
@@ -134,6 +134,30 @@ static int modify_esw_vport_context_cmd(struct mlx5_core_dev *dev, u16 vport,
 	return mlx5_cmd_exec(dev, in, inlen, out, sizeof(out));
 }
 
+int mlx5_eswitch_modify_esw_vport_context(struct mlx5_eswitch *esw, u16 vport,
+					  void *in, int inlen)
+{
+	return modify_esw_vport_context_cmd(esw->dev, vport, in, inlen);
+}
+
+static int query_esw_vport_context_cmd(struct mlx5_core_dev *dev, u16 vport,
+				       void *out, int outlen)
+{
+	u32 in[MLX5_ST_SZ_DW(query_esw_vport_context_in)] = {};
+
+	MLX5_SET(query_esw_vport_context_in, in, opcode,
+		 MLX5_CMD_OP_QUERY_ESW_VPORT_CONTEXT);
+	MLX5_SET(modify_esw_vport_context_in, in, vport_number, vport);
+	MLX5_SET(modify_esw_vport_context_in, in, other_vport, 1);
+	return mlx5_cmd_exec(dev, in, sizeof(in), out, outlen);
+}
+
+int mlx5_eswitch_query_esw_vport_context(struct mlx5_eswitch *esw, u16 vport,
+					 void *out, int outlen)
+{
+	return query_esw_vport_context_cmd(esw->dev, vport, out, outlen);
+}
+
 static int modify_esw_vport_cvlan(struct mlx5_core_dev *dev, u16 vport,
 				  u16 vlan, u8 qos, u8 set_flags)
 {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index 51e71b824abf..335cbeee1b9e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -276,6 +276,11 @@ int mlx5_eswitch_get_vport_stats(struct mlx5_eswitch *esw,
 				 struct ifla_vf_stats *vf_stats);
 void mlx5_eswitch_del_send_to_vport_rule(struct mlx5_flow_handle *rule);
 
+int mlx5_eswitch_modify_esw_vport_context(struct mlx5_eswitch *esw, u16 vport,
+					  void *in, int inlen);
+int mlx5_eswitch_query_esw_vport_context(struct mlx5_eswitch *esw, u16 vport,
+					 void *out, int outlen);
+
 struct mlx5_flow_spec;
 struct mlx5_esw_flow_attr;
 
-- 
2.21.0


^ permalink raw reply related

* [PATCH V2 mlx5-next 08/13] net/mlx5: E-Switch, Add match on vport metadata for rule in fast path
From: Saeed Mahameed @ 2019-06-25 17:48 UTC (permalink / raw)
  To: Saeed Mahameed, Leon Romanovsky
  Cc: netdev@vger.kernel.org, linux-rdma@vger.kernel.org, Jianbo Liu,
	Eli Britstein, Roi Dayan, Mark Bloch
In-Reply-To: <20190625174727.20309-1-saeedm@mellanox.com>

From: Jianbo Liu <jianbol@mellanox.com>

If FW's capabilities and configurations meet the requirement of vport
metadata matching, this feature will be used. As the information
about vport number and vhca_id related to packet is already stored to
its metadata register, which is used as an indicator for perticular
vport, now we can change to match on this metadata for all the
offloading rules in fast path.

Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
Reviewed-by: Eli Britstein <elibr@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 .../mellanox/mlx5/core/eswitch_offloads.c     | 85 +++++++++++--------
 1 file changed, 51 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 4bcbc872cd08..a3cf787382ee 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -88,6 +88,53 @@ u16 mlx5_eswitch_get_prio_range(struct mlx5_eswitch *esw)
 	return 1;
 }
 
+static void
+mlx5_eswitch_set_rule_source_port(struct mlx5_eswitch *esw,
+				  struct mlx5_flow_spec *spec,
+				  struct mlx5_esw_flow_attr *attr)
+{
+	void *misc2;
+	void *misc;
+
+	/* Use metadata matching because vport is not represented by single
+	 * VHCA in dual-port RoCE mode, and matching on source vport may fail.
+	 */
+	if (mlx5_eswitch_vport_match_metadata_enabled(esw)) {
+		misc2 = MLX5_ADDR_OF(fte_match_param, spec->match_value, misc_parameters_2);
+		MLX5_SET(fte_match_set_misc2, misc2, metadata_reg_c_0,
+			 mlx5_eswitch_get_vport_metadata_for_match(attr->in_mdev->priv.eswitch,
+								   attr->in_rep->vport));
+
+		misc2 = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters_2);
+		MLX5_SET_TO_ONES(fte_match_set_misc2, misc2, metadata_reg_c_0);
+
+		spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS_2;
+		misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters);
+		if (memchr_inv(misc, 0, MLX5_ST_SZ_BYTES(fte_match_set_misc)))
+			spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS;
+	} else {
+		misc = MLX5_ADDR_OF(fte_match_param, spec->match_value, misc_parameters);
+		MLX5_SET(fte_match_set_misc, misc, source_port, attr->in_rep->vport);
+
+		if (MLX5_CAP_ESW(esw->dev, merged_eswitch))
+			MLX5_SET(fte_match_set_misc, misc,
+				 source_eswitch_owner_vhca_id,
+				 MLX5_CAP_GEN(attr->in_mdev, vhca_id));
+
+		misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters);
+		MLX5_SET_TO_ONES(fte_match_set_misc, misc, source_port);
+		if (MLX5_CAP_ESW(esw->dev, merged_eswitch))
+			MLX5_SET_TO_ONES(fte_match_set_misc, misc,
+					 source_eswitch_owner_vhca_id);
+
+		spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS;
+	}
+
+	if (MLX5_CAP_ESW_FLOWTABLE(esw->dev, flow_source) &&
+	    attr->in_rep->vport == MLX5_VPORT_UPLINK)
+		spec->flow_context.flow_source = MLX5_FLOW_CONTEXT_FLOW_SOURCE_UPLINK;
+}
+
 struct mlx5_flow_handle *
 mlx5_eswitch_add_offloaded_rule(struct mlx5_eswitch *esw,
 				struct mlx5_flow_spec *spec,
@@ -99,7 +146,6 @@ mlx5_eswitch_add_offloaded_rule(struct mlx5_eswitch *esw,
 	struct mlx5_flow_handle *rule;
 	struct mlx5_flow_table *fdb;
 	int j, i = 0;
-	void *misc;
 
 	if (esw->mode != SRIOV_OFFLOADS)
 		return ERR_PTR(-EOPNOTSUPP);
@@ -159,21 +205,8 @@ mlx5_eswitch_add_offloaded_rule(struct mlx5_eswitch *esw,
 		i++;
 	}
 
-	misc = MLX5_ADDR_OF(fte_match_param, spec->match_value, misc_parameters);
-	MLX5_SET(fte_match_set_misc, misc, source_port, attr->in_rep->vport);
-
-	if (MLX5_CAP_ESW(esw->dev, merged_eswitch))
-		MLX5_SET(fte_match_set_misc, misc,
-			 source_eswitch_owner_vhca_id,
-			 MLX5_CAP_GEN(attr->in_mdev, vhca_id));
+	mlx5_eswitch_set_rule_source_port(esw, spec, attr);
 
-	misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters);
-	MLX5_SET_TO_ONES(fte_match_set_misc, misc, source_port);
-	if (MLX5_CAP_ESW(esw->dev, merged_eswitch))
-		MLX5_SET_TO_ONES(fte_match_set_misc, misc,
-				 source_eswitch_owner_vhca_id);
-
-	spec->match_criteria_enable = MLX5_MATCH_MISC_PARAMETERS;
 	if (flow_act.action & MLX5_FLOW_CONTEXT_ACTION_DECAP) {
 		if (attr->tunnel_match_level != MLX5_MATCH_NONE)
 			spec->match_criteria_enable |= MLX5_MATCH_OUTER_HEADERS;
@@ -219,7 +252,6 @@ mlx5_eswitch_add_fwd_rule(struct mlx5_eswitch *esw,
 	struct mlx5_flow_table *fast_fdb;
 	struct mlx5_flow_table *fwd_fdb;
 	struct mlx5_flow_handle *rule;
-	void *misc;
 	int i;
 
 	fast_fdb = esw_get_prio_table(esw, attr->chain, attr->prio, 0);
@@ -251,25 +283,10 @@ mlx5_eswitch_add_fwd_rule(struct mlx5_eswitch *esw,
 	dest[i].ft = fwd_fdb,
 	i++;
 
-	misc = MLX5_ADDR_OF(fte_match_param, spec->match_value, misc_parameters);
-	MLX5_SET(fte_match_set_misc, misc, source_port, attr->in_rep->vport);
-
-	if (MLX5_CAP_ESW(esw->dev, merged_eswitch))
-		MLX5_SET(fte_match_set_misc, misc,
-			 source_eswitch_owner_vhca_id,
-			 MLX5_CAP_GEN(attr->in_mdev, vhca_id));
-
-	misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters);
-	MLX5_SET_TO_ONES(fte_match_set_misc, misc, source_port);
-	if (MLX5_CAP_ESW(esw->dev, merged_eswitch))
-		MLX5_SET_TO_ONES(fte_match_set_misc, misc,
-				 source_eswitch_owner_vhca_id);
+	mlx5_eswitch_set_rule_source_port(esw, spec, attr);
 
-	if (attr->match_level == MLX5_MATCH_NONE)
-		spec->match_criteria_enable = MLX5_MATCH_MISC_PARAMETERS;
-	else
-		spec->match_criteria_enable = MLX5_MATCH_OUTER_HEADERS |
-					      MLX5_MATCH_MISC_PARAMETERS;
+	if (attr->match_level != MLX5_MATCH_NONE)
+		spec->match_criteria_enable |= MLX5_MATCH_OUTER_HEADERS;
 
 	rule = mlx5_add_flow_rules(fast_fdb, spec, &flow_act, dest, i);
 
-- 
2.21.0


^ permalink raw reply related

* [PATCH V2 mlx5-next 07/13] net/mlx5e: Specifying known origin of packets matching the flow
From: Saeed Mahameed @ 2019-06-25 17:48 UTC (permalink / raw)
  To: Saeed Mahameed, Leon Romanovsky
  Cc: netdev@vger.kernel.org, linux-rdma@vger.kernel.org, Jianbo Liu,
	Mark Bloch
In-Reply-To: <20190625174727.20309-1-saeedm@mellanox.com>

From: Jianbo Liu <jianbol@mellanox.com>

In vport metadata matching, source port number is replaced by metadata.
While FW has no idea about what it is in the metadata, a syndrome will
happen. Specify a known origin to avoid the syndrome.
However, there is no functional change because ANY_VPORT (0) is filled
in flow_source, the same default value as before, as a pre-step towards
metadata matching for fast path.
There are two other values can be filled in flow_source. When setting
0x1, packet matching this rule is from uplink, while 0x2 is for packet
from other local vports.

Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/diag/fs_tracepoint.h | 2 ++
 drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c             | 3 +++
 include/linux/mlx5/fs.h                                      | 1 +
 3 files changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/diag/fs_tracepoint.h b/drivers/net/ethernet/mellanox/mlx5/core/diag/fs_tracepoint.h
index 9ec46edf22a6..ddf1b87f1bc0 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/diag/fs_tracepoint.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/diag/fs_tracepoint.h
@@ -187,6 +187,7 @@ TRACE_EVENT(mlx5_fs_set_fte,
 		__field(u32, index)
 		__field(u32, action)
 		__field(u32, flow_tag)
+		__field(u32, flow_source)
 		__field(u8,  mask_enable)
 		__field(int, new_fte)
 		__array(u32, mask_outer, MLX5_ST_SZ_DW(fte_match_set_lyr_2_4))
@@ -205,6 +206,7 @@ TRACE_EVENT(mlx5_fs_set_fte,
 			   __entry->action = fte->action.action;
 			   __entry->mask_enable = __entry->fg->mask.match_criteria_enable;
 			   __entry->flow_tag = fte->flow_context.flow_tag;
+			   __entry->flow_source = fte->flow_context.flow_source;
 			   memcpy(__entry->mask_outer,
 				  MLX5_ADDR_OF(fte_match_param,
 					       &__entry->fg->mask.match_criteria,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c
index fb1335a433ae..7ac1249eadc3 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c
@@ -398,6 +398,9 @@ static int mlx5_cmd_set_fte(struct mlx5_core_dev *dev,
 
 	MLX5_SET(flow_context, in_flow_context, flow_tag,
 		 fte->flow_context.flow_tag);
+	MLX5_SET(flow_context, in_flow_context, flow_source,
+		 fte->flow_context.flow_source);
+
 	MLX5_SET(flow_context, in_flow_context, extended_destination,
 		 extended_dest);
 	if (extended_dest) {
diff --git a/include/linux/mlx5/fs.h b/include/linux/mlx5/fs.h
index 9bf49ce218fa..dc7e7aa53a13 100644
--- a/include/linux/mlx5/fs.h
+++ b/include/linux/mlx5/fs.h
@@ -95,6 +95,7 @@ enum {
 struct mlx5_flow_context {
 	u32 flags;
 	u32 flow_tag;
+	u32 flow_source;
 };
 
 struct mlx5_flow_spec {
-- 
2.21.0


^ permalink raw reply related

* [PATCH V2 mlx5-next 06/13] net/mlx5: E-Switch, Tag packet with vport number in VF vports and uplink ingress ACLs
From: Saeed Mahameed @ 2019-06-25 17:48 UTC (permalink / raw)
  To: Saeed Mahameed, Leon Romanovsky
  Cc: netdev@vger.kernel.org, linux-rdma@vger.kernel.org, Jianbo Liu,
	Eli Britstein, Roi Dayan, Mark Bloch
In-Reply-To: <20190625174727.20309-1-saeedm@mellanox.com>

From: Jianbo Liu <jianbol@mellanox.com>

When a dual-port VHCA sends a RoCE packet on its non-native port, and the
packet arrives to its affiliated vport FDB, a mismatch might occur on the
rules that match the packet source vport as it is not represented by single
VHCA only in this case. So we change to match on metadata instead of source
vport.
To do that, a rule is created in all vports and uplink ingress ACLs, to
save the source vport number and vhca id in the packet's metadata in order
to match on it later.
The metadata register used is the first of the 32-bit type C registers. It
can be used for matching and header modify operations. The higher 16 bits
of this register are for vhca id, and the lower 16 ones is for vport
number.
This change is not for dual-port RoCE only. If HW and FW allow, the vport
metadata matching is enabled by default.

Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
Reviewed-by: Eli Britstein <elibr@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 .../net/ethernet/mellanox/mlx5/core/eswitch.c |   2 +
 .../net/ethernet/mellanox/mlx5/core/eswitch.h |   9 +
 .../mellanox/mlx5/core/eswitch_offloads.c     | 180 ++++++++++++++----
 include/linux/mlx5/eswitch.h                  |  17 ++
 4 files changed, 172 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
index a42a23e505df..1235fd84ae3a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
@@ -1168,6 +1168,8 @@ void esw_vport_cleanup_ingress_rules(struct mlx5_eswitch *esw,
 
 	vport->ingress.drop_rule = NULL;
 	vport->ingress.allow_rule = NULL;
+
+	esw_vport_del_ingress_acl_modify_metadata(esw, vport);
 }
 
 void esw_vport_disable_ingress_acl(struct mlx5_eswitch *esw,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index 99dc25630629..51e71b824abf 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -68,6 +68,8 @@ struct vport_ingress {
 	struct mlx5_flow_group *allow_spoofchk_only_grp;
 	struct mlx5_flow_group *allow_untagged_only_grp;
 	struct mlx5_flow_group *drop_grp;
+	int modify_metadata_id;
+	struct mlx5_flow_handle  *modify_metadata_rule;
 	struct mlx5_flow_handle  *allow_rule;
 	struct mlx5_flow_handle  *drop_rule;
 	struct mlx5_fc           *drop_counter;
@@ -196,6 +198,10 @@ struct mlx5_esw_functions {
 	u16			num_vfs;
 };
 
+enum {
+	MLX5_ESWITCH_VPORT_MATCH_METADATA = BIT(0),
+};
+
 struct mlx5_eswitch {
 	struct mlx5_core_dev    *dev;
 	struct mlx5_nb          nb;
@@ -203,6 +209,7 @@ struct mlx5_eswitch {
 	struct hlist_head       mc_table[MLX5_L2_ADDR_HASH_SIZE];
 	struct workqueue_struct *work_queue;
 	struct mlx5_vport       *vports;
+	u32 flags;
 	int                     total_vports;
 	int                     enabled_vports;
 	/* Synchronize between vport change events
@@ -240,6 +247,8 @@ void esw_vport_disable_egress_acl(struct mlx5_eswitch *esw,
 				  struct mlx5_vport *vport);
 void esw_vport_disable_ingress_acl(struct mlx5_eswitch *esw,
 				   struct mlx5_vport *vport);
+void esw_vport_del_ingress_acl_modify_metadata(struct mlx5_eswitch *esw,
+					       struct mlx5_vport *vport);
 
 /* E-Switch API */
 int mlx5_eswitch_init(struct mlx5_core_dev *dev);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index c1c42c1370b8..4bcbc872cd08 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -1555,32 +1555,16 @@ static void esw_offloads_devcom_cleanup(struct mlx5_eswitch *esw)
 static int esw_vport_ingress_prio_tag_config(struct mlx5_eswitch *esw,
 					     struct mlx5_vport *vport)
 {
-	struct mlx5_core_dev *dev = esw->dev;
 	struct mlx5_flow_act flow_act = {0};
 	struct mlx5_flow_spec *spec;
 	int err = 0;
 
 	/* For prio tag mode, there is only 1 FTEs:
-	 * 1) Untagged packets - push prio tag VLAN, allow
+	 * 1) Untagged packets - push prio tag VLAN and modify metadata if
+	 * required, allow
 	 * Unmatched traffic is allowed by default
 	 */
 
-	if (!MLX5_CAP_ESW_INGRESS_ACL(dev, ft_support))
-		return -EOPNOTSUPP;
-
-	esw_vport_cleanup_ingress_rules(esw, vport);
-
-	err = esw_vport_enable_ingress_acl(esw, vport);
-	if (err) {
-		mlx5_core_warn(esw->dev,
-			       "failed to enable prio tag ingress acl (%d) on vport[%d]\n",
-			       err, vport->vport);
-		return err;
-	}
-
-	esw_debug(esw->dev,
-		  "vport[%d] configure ingress rules\n", vport->vport);
-
 	spec = kvzalloc(sizeof(*spec), GFP_KERNEL);
 	if (!spec) {
 		err = -ENOMEM;
@@ -1596,6 +1580,12 @@ static int esw_vport_ingress_prio_tag_config(struct mlx5_eswitch *esw,
 	flow_act.vlan[0].ethtype = ETH_P_8021Q;
 	flow_act.vlan[0].vid = 0;
 	flow_act.vlan[0].prio = 0;
+
+	if (vport->ingress.modify_metadata_rule) {
+		flow_act.action |= MLX5_FLOW_CONTEXT_ACTION_MOD_HDR;
+		flow_act.modify_id = vport->ingress.modify_metadata_id;
+	}
+
 	vport->ingress.allow_rule =
 		mlx5_add_flow_rules(vport->ingress.acl, spec,
 				    &flow_act, NULL, 0);
@@ -1616,6 +1606,58 @@ static int esw_vport_ingress_prio_tag_config(struct mlx5_eswitch *esw,
 	return err;
 }
 
+static int esw_vport_add_ingress_acl_modify_metadata(struct mlx5_eswitch *esw,
+						     struct mlx5_vport *vport)
+{
+	u8 action[MLX5_UN_SZ_BYTES(set_action_in_add_action_in_auto)] = {};
+	struct mlx5_flow_act flow_act = {};
+	struct mlx5_flow_spec spec = {};
+	int err = 0;
+
+	MLX5_SET(set_action_in, action, action_type, MLX5_ACTION_TYPE_SET);
+	MLX5_SET(set_action_in, action, field, MLX5_ACTION_IN_FIELD_METADATA_REG_C_0);
+	MLX5_SET(set_action_in, action, data,
+		 mlx5_eswitch_get_vport_metadata_for_match(esw, vport->vport));
+
+	err = mlx5_modify_header_alloc(esw->dev, MLX5_FLOW_NAMESPACE_ESW_INGRESS,
+				       1, action, &vport->ingress.modify_metadata_id);
+	if (err) {
+		esw_warn(esw->dev,
+			 "failed to alloc modify header for vport %d ingress acl (%d)\n",
+			 vport->vport, err);
+		return err;
+	}
+
+	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_MOD_HDR | MLX5_FLOW_CONTEXT_ACTION_ALLOW;
+	flow_act.modify_id = vport->ingress.modify_metadata_id;
+	vport->ingress.modify_metadata_rule = mlx5_add_flow_rules(vport->ingress.acl,
+								  &spec, &flow_act, NULL, 0);
+	if (IS_ERR(vport->ingress.modify_metadata_rule)) {
+		err = PTR_ERR(vport->ingress.modify_metadata_rule);
+		esw_warn(esw->dev,
+			 "failed to add setting metadata rule for vport %d ingress acl, err(%d)\n",
+			 vport->vport, err);
+		vport->ingress.modify_metadata_rule = NULL;
+		goto out;
+	}
+
+out:
+	if (err)
+		mlx5_modify_header_dealloc(esw->dev, vport->ingress.modify_metadata_id);
+	return err;
+}
+
+void esw_vport_del_ingress_acl_modify_metadata(struct mlx5_eswitch *esw,
+					       struct mlx5_vport *vport)
+{
+	if (vport->ingress.modify_metadata_rule) {
+		mlx5_del_flow_rules(vport->ingress.modify_metadata_rule);
+		mlx5_modify_header_dealloc(esw->dev, vport->ingress.modify_metadata_id);
+
+		vport->ingress.modify_metadata_rule = NULL;
+	}
+}
+
 static int esw_vport_egress_prio_tag_config(struct mlx5_eswitch *esw,
 					    struct mlx5_vport *vport)
 {
@@ -1623,6 +1665,9 @@ static int esw_vport_egress_prio_tag_config(struct mlx5_eswitch *esw,
 	struct mlx5_flow_spec *spec;
 	int err = 0;
 
+	if (!MLX5_CAP_GEN(esw->dev, prio_tag_required))
+		return 0;
+
 	/* For prio tag mode, there is only 1 FTEs:
 	 * 1) prio tag packets - pop the prio tag VLAN, allow
 	 * Unmatched traffic is allowed by default
@@ -1676,27 +1721,75 @@ static int esw_vport_egress_prio_tag_config(struct mlx5_eswitch *esw,
 	return err;
 }
 
-static int esw_prio_tag_acls_config(struct mlx5_eswitch *esw, int nvports)
+static int esw_vport_ingress_common_config(struct mlx5_eswitch *esw,
+					   struct mlx5_vport *vport)
 {
-	struct mlx5_vport *vport = NULL;
-	int i, j;
 	int err;
 
-	mlx5_esw_for_each_vf_vport(esw, i, vport, nvports) {
+	if (!mlx5_eswitch_vport_match_metadata_enabled(esw) &&
+	    !MLX5_CAP_GEN(esw->dev, prio_tag_required))
+		return 0;
+
+	esw_vport_cleanup_ingress_rules(esw, vport);
+
+	err = esw_vport_enable_ingress_acl(esw, vport);
+	if (err) {
+		esw_warn(esw->dev,
+			 "failed to enable ingress acl (%d) on vport[%d]\n",
+			 err, vport->vport);
+		return err;
+	}
+
+	esw_debug(esw->dev,
+		  "vport[%d] configure ingress rules\n", vport->vport);
+
+	if (mlx5_eswitch_vport_match_metadata_enabled(esw)) {
+		err = esw_vport_add_ingress_acl_modify_metadata(esw, vport);
+		if (err)
+			goto out;
+	}
+
+	if (MLX5_CAP_GEN(esw->dev, prio_tag_required) &&
+	    mlx5_eswitch_is_vf_vport(esw, vport->vport)) {
 		err = esw_vport_ingress_prio_tag_config(esw, vport);
 		if (err)
-			goto err_ingress;
-		err = esw_vport_egress_prio_tag_config(esw, vport);
+			goto out;
+	}
+
+out:
+	if (err)
+		esw_vport_disable_ingress_acl(esw, vport);
+	return err;
+}
+
+static int esw_create_offloads_acl_tables(struct mlx5_eswitch *esw)
+{
+	struct mlx5_vport *vport;
+	int i, j;
+	int err;
+
+	mlx5_esw_for_all_vports(esw, i, vport) {
+		err = esw_vport_ingress_common_config(esw, vport);
 		if (err)
-			goto err_egress;
+			goto err_ingress;
+
+		if (mlx5_eswitch_is_vf_vport(esw, vport->vport)) {
+			err = esw_vport_egress_prio_tag_config(esw, vport);
+			if (err)
+				goto err_egress;
+		}
 	}
 
+	if (mlx5_eswitch_vport_match_metadata_enabled(esw))
+		esw_info(esw->dev, "Use metadata reg_c as source vport to match\n");
+
 	return 0;
 
 err_egress:
 	esw_vport_disable_ingress_acl(esw, vport);
 err_ingress:
-	mlx5_esw_for_each_vf_vport_reverse(esw, j, vport, i - 1) {
+	for (j = MLX5_VPORT_PF; j < i; j++) {
+		vport = &esw->vports[j];
 		esw_vport_disable_egress_acl(esw, vport);
 		esw_vport_disable_ingress_acl(esw, vport);
 	}
@@ -1704,15 +1797,17 @@ static int esw_prio_tag_acls_config(struct mlx5_eswitch *esw, int nvports)
 	return err;
 }
 
-static void esw_prio_tag_acls_cleanup(struct mlx5_eswitch *esw)
+static void esw_destroy_offloads_acl_tables(struct mlx5_eswitch *esw)
 {
 	struct mlx5_vport *vport;
 	int i;
 
-	mlx5_esw_for_each_vf_vport(esw, i, vport, esw->nvports) {
+	mlx5_esw_for_all_vports(esw, i, vport) {
 		esw_vport_disable_egress_acl(esw, vport);
 		esw_vport_disable_ingress_acl(esw, vport);
 	}
+
+	esw->flags &= ~MLX5_ESWITCH_VPORT_MATCH_METADATA;
 }
 
 static int esw_offloads_steering_init(struct mlx5_eswitch *esw, int nvports)
@@ -1722,15 +1817,13 @@ static int esw_offloads_steering_init(struct mlx5_eswitch *esw, int nvports)
 	memset(&esw->fdb_table.offloads, 0, sizeof(struct offloads_fdb));
 	mutex_init(&esw->fdb_table.offloads.fdb_prio_lock);
 
-	if (MLX5_CAP_GEN(esw->dev, prio_tag_required)) {
-		err = esw_prio_tag_acls_config(esw, nvports);
-		if (err)
-			return err;
-	}
+	err = esw_create_offloads_acl_tables(esw);
+	if (err)
+		return err;
 
 	err = esw_create_offloads_fdb_tables(esw, nvports);
 	if (err)
-		return err;
+		goto create_fdb_err;
 
 	err = esw_create_offloads_table(esw, nvports);
 	if (err)
@@ -1748,6 +1841,9 @@ static int esw_offloads_steering_init(struct mlx5_eswitch *esw, int nvports)
 create_ft_err:
 	esw_destroy_offloads_fdb_tables(esw);
 
+create_fdb_err:
+	esw_destroy_offloads_acl_tables(esw);
+
 	return err;
 }
 
@@ -1756,8 +1852,7 @@ static void esw_offloads_steering_cleanup(struct mlx5_eswitch *esw)
 	esw_destroy_vport_rx_group(esw);
 	esw_destroy_offloads_table(esw);
 	esw_destroy_offloads_fdb_tables(esw);
-	if (MLX5_CAP_GEN(esw->dev, prio_tag_required))
-		esw_prio_tag_acls_cleanup(esw);
+	esw_destroy_offloads_acl_tables(esw);
 }
 
 static void esw_functions_changed_event_handler(struct work_struct *work)
@@ -2296,3 +2391,16 @@ bool mlx5_eswitch_is_vf_vport(const struct mlx5_eswitch *esw, u16 vport_num)
 	return vport_num >= MLX5_VPORT_FIRST_VF &&
 	       vport_num <= esw->dev->priv.sriov.max_vfs;
 }
+
+bool mlx5_eswitch_vport_match_metadata_enabled(const struct mlx5_eswitch *esw)
+{
+	return !!(esw->flags & MLX5_ESWITCH_VPORT_MATCH_METADATA);
+}
+EXPORT_SYMBOL(mlx5_eswitch_vport_match_metadata_enabled);
+
+u32 mlx5_eswitch_get_vport_metadata_for_match(const struct mlx5_eswitch *esw,
+					      u16 vport_num)
+{
+	return ((MLX5_CAP_GEN(esw->dev, vhca_id) & 0xffff) << 16) | vport_num;
+}
+EXPORT_SYMBOL(mlx5_eswitch_get_vport_metadata_for_match);
diff --git a/include/linux/mlx5/eswitch.h b/include/linux/mlx5/eswitch.h
index 174eec0871d9..aece3ae1902d 100644
--- a/include/linux/mlx5/eswitch.h
+++ b/include/linux/mlx5/eswitch.h
@@ -67,11 +67,28 @@ mlx5_eswitch_add_send_to_vport_rule(struct mlx5_eswitch *esw,
 #ifdef CONFIG_MLX5_ESWITCH
 enum devlink_eswitch_encap_mode
 mlx5_eswitch_get_encap_mode(const struct mlx5_core_dev *dev);
+
+bool mlx5_eswitch_vport_match_metadata_enabled(const struct mlx5_eswitch *esw);
+u32 mlx5_eswitch_get_vport_metadata_for_match(const struct mlx5_eswitch *esw,
+					      u16 vport_num);
 #else  /* CONFIG_MLX5_ESWITCH */
 static inline enum devlink_eswitch_encap_mode
 mlx5_eswitch_get_encap_mode(const struct mlx5_core_dev *dev)
 {
 	return DEVLINK_ESWITCH_ENCAP_MODE_NONE;
 }
+
+static inline bool
+mlx5_eswitch_vport_match_metadata_enabled(const struct mlx5_eswitch *esw)
+{
+	return false;
+};
+
+static inline u32
+mlx5_eswitch_get_vport_metadata_for_match(const struct mlx5_eswitch *esw,
+					  int vport_num)
+{
+	return 0;
+};
 #endif /* CONFIG_MLX5_ESWITCH */
 #endif
-- 
2.21.0


^ permalink raw reply related

* [PATCH V2 mlx5-next 05/13] net/mlx5: Add flow context for flow tag
From: Saeed Mahameed @ 2019-06-25 17:47 UTC (permalink / raw)
  To: Saeed Mahameed, Leon Romanovsky
  Cc: netdev@vger.kernel.org, linux-rdma@vger.kernel.org, Jianbo Liu,
	Mark Bloch
In-Reply-To: <20190625174727.20309-1-saeedm@mellanox.com>

From: Jianbo Liu <jianbol@mellanox.com>

Refactor the flow data structures, add new flow_context and move
flow_tag into it, as flow_tag doesn't belong to the rule action.

Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/infiniband/hw/mlx5/flow.c             | 13 ++++---
 drivers/infiniband/hw/mlx5/main.c             | 30 ++++++++++------
 drivers/infiniband/hw/mlx5/mlx5_ib.h          |  1 +
 .../mellanox/mlx5/core/diag/fs_tracepoint.h   |  2 +-
 .../mellanox/mlx5/core/en_fs_ethtool.c        |  2 +-
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   |  7 ++--
 .../ethernet/mellanox/mlx5/core/fpga/ipsec.c  |  8 +++--
 .../net/ethernet/mellanox/mlx5/core/fs_cmd.c  |  3 +-
 .../net/ethernet/mellanox/mlx5/core/fs_core.c | 34 +++++++++----------
 .../net/ethernet/mellanox/mlx5/core/fs_core.h |  1 +
 include/linux/mlx5/fs.h                       | 15 +++++---
 11 files changed, 71 insertions(+), 45 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/flow.c b/drivers/infiniband/hw/mlx5/flow.c
index 1fc302d41a53..b8841355fcd5 100644
--- a/drivers/infiniband/hw/mlx5/flow.c
+++ b/drivers/infiniband/hw/mlx5/flow.c
@@ -65,11 +65,12 @@ static const struct uverbs_attr_spec mlx5_ib_flow_type[] = {
 static int UVERBS_HANDLER(MLX5_IB_METHOD_CREATE_FLOW)(
 	struct uverbs_attr_bundle *attrs)
 {
-	struct mlx5_flow_act flow_act = {.flow_tag = MLX5_FS_DEFAULT_FLOW_TAG};
+	struct mlx5_flow_context flow_context = {.flow_tag = MLX5_FS_DEFAULT_FLOW_TAG};
 	struct mlx5_ib_flow_handler *flow_handler;
 	struct mlx5_ib_flow_matcher *fs_matcher;
 	struct ib_uobject **arr_flow_actions;
 	struct ib_uflow_resources *uflow_res;
+	struct mlx5_flow_act flow_act = {};
 	void *devx_obj;
 	int dest_id, dest_type;
 	void *cmd_in;
@@ -172,17 +173,19 @@ static int UVERBS_HANDLER(MLX5_IB_METHOD_CREATE_FLOW)(
 				   arr_flow_actions[i]->object);
 	}
 
-	ret = uverbs_copy_from(&flow_act.flow_tag, attrs,
+	ret = uverbs_copy_from(&flow_context.flow_tag, attrs,
 			       MLX5_IB_ATTR_CREATE_FLOW_TAG);
 	if (!ret) {
-		if (flow_act.flow_tag >= BIT(24)) {
+		if (flow_context.flow_tag >= BIT(24)) {
 			ret = -EINVAL;
 			goto err_out;
 		}
-		flow_act.flags |= FLOW_ACT_HAS_TAG;
+		flow_context.flags |= FLOW_CONTEXT_HAS_TAG;
 	}
 
-	flow_handler = mlx5_ib_raw_fs_rule_add(dev, fs_matcher, &flow_act,
+	flow_handler = mlx5_ib_raw_fs_rule_add(dev, fs_matcher,
+					       &flow_context,
+					       &flow_act,
 					       counter_id,
 					       cmd_in, inlen,
 					       dest_id, dest_type);
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index abac70ad5c7c..be4c9a687df7 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -2666,11 +2666,15 @@ int parse_flow_flow_action(struct mlx5_ib_flow_action *maction,
 	}
 }
 
-static int parse_flow_attr(struct mlx5_core_dev *mdev, u32 *match_c,
-			   u32 *match_v, const union ib_flow_spec *ib_spec,
+static int parse_flow_attr(struct mlx5_core_dev *mdev,
+			   struct mlx5_flow_spec *spec,
+			   const union ib_flow_spec *ib_spec,
 			   const struct ib_flow_attr *flow_attr,
 			   struct mlx5_flow_act *action, u32 prev_type)
 {
+	struct mlx5_flow_context *flow_context = &spec->flow_context;
+	u32 *match_c = spec->match_criteria;
+	u32 *match_v = spec->match_value;
 	void *misc_params_c = MLX5_ADDR_OF(fte_match_param, match_c,
 					   misc_parameters);
 	void *misc_params_v = MLX5_ADDR_OF(fte_match_param, match_v,
@@ -2989,8 +2993,8 @@ static int parse_flow_attr(struct mlx5_core_dev *mdev, u32 *match_c,
 		if (ib_spec->flow_tag.tag_id >= BIT(24))
 			return -EINVAL;
 
-		action->flow_tag = ib_spec->flow_tag.tag_id;
-		action->flags |= FLOW_ACT_HAS_TAG;
+		flow_context->flow_tag = ib_spec->flow_tag.tag_id;
+		flow_context->flags |= FLOW_CONTEXT_HAS_TAG;
 		break;
 	case IB_FLOW_SPEC_ACTION_DROP:
 		if (FIELDS_NOT_SUPPORTED(ib_spec->drop,
@@ -3084,7 +3088,8 @@ is_valid_esp_aes_gcm(struct mlx5_core_dev *mdev,
 		return VALID_SPEC_NA;
 
 	return is_crypto && is_ipsec &&
-		(!egress || (!is_drop && !(flow_act->flags & FLOW_ACT_HAS_TAG))) ?
+		(!egress || (!is_drop &&
+			     !(spec->flow_context.flags & FLOW_CONTEXT_HAS_TAG))) ?
 		VALID_SPEC_VALID : VALID_SPEC_INVALID;
 }
 
@@ -3473,7 +3478,7 @@ static struct mlx5_ib_flow_handler *_create_flow_rule(struct mlx5_ib_dev *dev,
 {
 	struct mlx5_flow_table	*ft = ft_prio->flow_table;
 	struct mlx5_ib_flow_handler *handler;
-	struct mlx5_flow_act flow_act = {.flow_tag = MLX5_FS_DEFAULT_FLOW_TAG};
+	struct mlx5_flow_act flow_act = {};
 	struct mlx5_flow_spec *spec;
 	struct mlx5_flow_destination dest_arr[2] = {};
 	struct mlx5_flow_destination *rule_dst = dest_arr;
@@ -3504,8 +3509,7 @@ static struct mlx5_ib_flow_handler *_create_flow_rule(struct mlx5_ib_dev *dev,
 	}
 
 	for (spec_index = 0; spec_index < flow_attr->num_of_specs; spec_index++) {
-		err = parse_flow_attr(dev->mdev, spec->match_criteria,
-				      spec->match_value,
+		err = parse_flow_attr(dev->mdev, spec,
 				      ib_flow, flow_attr, &flow_act,
 				      prev_type);
 		if (err < 0)
@@ -3572,11 +3576,11 @@ static struct mlx5_ib_flow_handler *_create_flow_rule(struct mlx5_ib_dev *dev,
 					MLX5_FLOW_CONTEXT_ACTION_FWD_NEXT_PRIO;
 	}
 
-	if ((flow_act.flags & FLOW_ACT_HAS_TAG)  &&
+	if ((spec->flow_context.flags & FLOW_CONTEXT_HAS_TAG)  &&
 	    (flow_attr->type == IB_FLOW_ATTR_ALL_DEFAULT ||
 	     flow_attr->type == IB_FLOW_ATTR_MC_DEFAULT)) {
 		mlx5_ib_warn(dev, "Flow tag %u and attribute type %x isn't allowed in leftovers\n",
-			     flow_act.flow_tag, flow_attr->type);
+			     spec->flow_context.flow_tag, flow_attr->type);
 		err = -EINVAL;
 		goto free;
 	}
@@ -3947,6 +3951,7 @@ _create_raw_flow_rule(struct mlx5_ib_dev *dev,
 		      struct mlx5_ib_flow_prio *ft_prio,
 		      struct mlx5_flow_destination *dst,
 		      struct mlx5_ib_flow_matcher  *fs_matcher,
+		      struct mlx5_flow_context *flow_context,
 		      struct mlx5_flow_act *flow_act,
 		      void *cmd_in, int inlen,
 		      int dst_num)
@@ -3969,6 +3974,7 @@ _create_raw_flow_rule(struct mlx5_ib_dev *dev,
 	memcpy(spec->match_criteria, fs_matcher->matcher_mask.match_params,
 	       fs_matcher->mask_len);
 	spec->match_criteria_enable = fs_matcher->match_criteria_enable;
+	spec->flow_context = *flow_context;
 
 	handler->rule = mlx5_add_flow_rules(ft, spec,
 					    flow_act, dst, dst_num);
@@ -4033,6 +4039,7 @@ static bool raw_fs_is_multicast(struct mlx5_ib_flow_matcher *fs_matcher,
 struct mlx5_ib_flow_handler *
 mlx5_ib_raw_fs_rule_add(struct mlx5_ib_dev *dev,
 			struct mlx5_ib_flow_matcher *fs_matcher,
+			struct mlx5_flow_context *flow_context,
 			struct mlx5_flow_act *flow_act,
 			u32 counter_id,
 			void *cmd_in, int inlen, int dest_id,
@@ -4085,7 +4092,8 @@ mlx5_ib_raw_fs_rule_add(struct mlx5_ib_dev *dev,
 		dst_num++;
 	}
 
-	handler = _create_raw_flow_rule(dev, ft_prio, dst, fs_matcher, flow_act,
+	handler = _create_raw_flow_rule(dev, ft_prio, dst, fs_matcher,
+					flow_context, flow_act,
 					cmd_in, inlen, dst_num);
 
 	if (IS_ERR(handler)) {
diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
index a043af7ee366..1c205c2bd486 100644
--- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
+++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
@@ -1317,6 +1317,7 @@ extern const struct uapi_definition mlx5_ib_devx_defs[];
 extern const struct uapi_definition mlx5_ib_flow_defs[];
 struct mlx5_ib_flow_handler *mlx5_ib_raw_fs_rule_add(
 	struct mlx5_ib_dev *dev, struct mlx5_ib_flow_matcher *fs_matcher,
+	struct mlx5_flow_context *flow_context,
 	struct mlx5_flow_act *flow_act, u32 counter_id,
 	void *cmd_in, int inlen, int dest_id, int dest_type);
 bool mlx5_ib_devx_is_flow_dest(void *obj, int *dest_id, int *dest_type);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/diag/fs_tracepoint.h b/drivers/net/ethernet/mellanox/mlx5/core/diag/fs_tracepoint.h
index a4cf123e3f17..9ec46edf22a6 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/diag/fs_tracepoint.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/diag/fs_tracepoint.h
@@ -204,7 +204,7 @@ TRACE_EVENT(mlx5_fs_set_fte,
 			   __entry->index = fte->index;
 			   __entry->action = fte->action.action;
 			   __entry->mask_enable = __entry->fg->mask.match_criteria_enable;
-			   __entry->flow_tag = fte->action.flow_tag;
+			   __entry->flow_tag = fte->flow_context.flow_tag;
 			   memcpy(__entry->mask_outer,
 				  MLX5_ADDR_OF(fte_match_param,
 					       &__entry->fg->mask.match_criteria,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c
index 4421c10f58ae..839662644ed3 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c
@@ -426,7 +426,7 @@ add_ethtool_flow_rule(struct mlx5e_priv *priv,
 	}
 
 	spec->match_criteria_enable = (!outer_header_zero(spec->match_criteria));
-	flow_act.flow_tag = MLX5_FS_DEFAULT_FLOW_TAG;
+	spec->flow_context.flow_tag = MLX5_FS_DEFAULT_FLOW_TAG;
 	rule = mlx5_add_flow_rules(ft, spec, &flow_act, dst, dst ? 1 : 0);
 	if (IS_ERR(rule)) {
 		err = PTR_ERR(rule);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 122f457091a2..8ff1ca46d8d3 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -716,19 +716,22 @@ mlx5e_tc_add_nic_flow(struct mlx5e_priv *priv,
 		      struct mlx5e_tc_flow *flow,
 		      struct netlink_ext_ack *extack)
 {
+	struct mlx5_flow_context *flow_context = &parse_attr->spec.flow_context;
 	struct mlx5_nic_flow_attr *attr = flow->nic_attr;
 	struct mlx5_core_dev *dev = priv->mdev;
 	struct mlx5_flow_destination dest[2] = {};
 	struct mlx5_flow_act flow_act = {
 		.action = attr->action,
-		.flow_tag = attr->flow_tag,
 		.reformat_id = 0,
-		.flags    = FLOW_ACT_HAS_TAG | FLOW_ACT_NO_APPEND,
+		.flags    = FLOW_ACT_NO_APPEND,
 	};
 	struct mlx5_fc *counter = NULL;
 	bool table_created = false;
 	int err, dest_ix = 0;
 
+	flow_context->flags |= FLOW_CONTEXT_HAS_TAG;
+	flow_context->flow_tag = attr->flow_tag;
+
 	if (flow->flags & MLX5E_TC_FLOW_HAIRPIN) {
 		err = mlx5e_hairpin_flow_add(priv, flow, parse_attr, extack);
 		if (err) {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c b/drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c
index 52c47d3dd5a5..c76da309506b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c
@@ -636,7 +636,8 @@ static bool mlx5_is_fpga_egress_ipsec_rule(struct mlx5_core_dev *dev,
 					   u8 match_criteria_enable,
 					   const u32 *match_c,
 					   const u32 *match_v,
-					   struct mlx5_flow_act *flow_act)
+					   struct mlx5_flow_act *flow_act,
+					   struct mlx5_flow_context *flow_context)
 {
 	const void *outer_c = MLX5_ADDR_OF(fte_match_param, match_c,
 					   outer_headers);
@@ -655,7 +656,7 @@ static bool mlx5_is_fpga_egress_ipsec_rule(struct mlx5_core_dev *dev,
 	    (match_criteria_enable &
 	     ~(MLX5_MATCH_OUTER_HEADERS | MLX5_MATCH_MISC_PARAMETERS)) ||
 	    (flow_act->action & ~(MLX5_FLOW_CONTEXT_ACTION_ENCRYPT | MLX5_FLOW_CONTEXT_ACTION_ALLOW)) ||
-	     (flow_act->flags & FLOW_ACT_HAS_TAG))
+	     (flow_context->flags & FLOW_CONTEXT_HAS_TAG))
 		return false;
 
 	return true;
@@ -767,7 +768,8 @@ mlx5_fpga_ipsec_fs_create_sa_ctx(struct mlx5_core_dev *mdev,
 					    fg->mask.match_criteria_enable,
 					    fg->mask.match_criteria,
 					    fte->val,
-					    &fte->action))
+					    &fte->action,
+					    &fte->flow_context))
 		return ERR_PTR(-EINVAL);
 	else if (!mlx5_is_fpga_ipsec_rule(mdev,
 					  fg->mask.match_criteria_enable,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c
index 4f1d402926f1..fb1335a433ae 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c
@@ -396,7 +396,8 @@ static int mlx5_cmd_set_fte(struct mlx5_core_dev *dev,
 	in_flow_context = MLX5_ADDR_OF(set_fte_in, in, flow_context);
 	MLX5_SET(flow_context, in_flow_context, group_id, group_id);
 
-	MLX5_SET(flow_context, in_flow_context, flow_tag, fte->action.flow_tag);
+	MLX5_SET(flow_context, in_flow_context, flow_tag,
+		 fte->flow_context.flow_tag);
 	MLX5_SET(flow_context, in_flow_context, extended_destination,
 		 extended_dest);
 	if (extended_dest) {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
index fb5b61727ee7..9f5544ac6b8a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
@@ -584,7 +584,7 @@ static int insert_fte(struct mlx5_flow_group *fg, struct fs_fte *fte)
 }
 
 static struct fs_fte *alloc_fte(struct mlx5_flow_table *ft,
-				u32 *match_value,
+				struct mlx5_flow_spec *spec,
 				struct mlx5_flow_act *flow_act)
 {
 	struct mlx5_flow_steering *steering = get_steering(&ft->node);
@@ -594,9 +594,10 @@ static struct fs_fte *alloc_fte(struct mlx5_flow_table *ft,
 	if (!fte)
 		return ERR_PTR(-ENOMEM);
 
-	memcpy(fte->val, match_value, sizeof(fte->val));
+	memcpy(fte->val, &spec->match_value, sizeof(fte->val));
 	fte->node.type =  FS_TYPE_FLOW_ENTRY;
 	fte->action = *flow_act;
+	fte->flow_context = spec->flow_context;
 
 	tree_init_node(&fte->node, NULL, del_sw_fte);
 
@@ -1428,7 +1429,9 @@ static bool check_conflicting_actions(u32 action1, u32 action2)
 	return false;
 }
 
-static int check_conflicting_ftes(struct fs_fte *fte, const struct mlx5_flow_act *flow_act)
+static int check_conflicting_ftes(struct fs_fte *fte,
+				  const struct mlx5_flow_context *flow_context,
+				  const struct mlx5_flow_act *flow_act)
 {
 	if (check_conflicting_actions(flow_act->action, fte->action.action)) {
 		mlx5_core_warn(get_dev(&fte->node),
@@ -1436,12 +1439,12 @@ static int check_conflicting_ftes(struct fs_fte *fte, const struct mlx5_flow_act
 		return -EEXIST;
 	}
 
-	if ((flow_act->flags & FLOW_ACT_HAS_TAG) &&
-	    fte->action.flow_tag != flow_act->flow_tag) {
+	if ((flow_context->flags & FLOW_CONTEXT_HAS_TAG) &&
+	    fte->flow_context.flow_tag != flow_context->flow_tag) {
 		mlx5_core_warn(get_dev(&fte->node),
 			       "FTE flow tag %u already exists with different flow tag %u\n",
-			       fte->action.flow_tag,
-			       flow_act->flow_tag);
+			       fte->flow_context.flow_tag,
+			       flow_context->flow_tag);
 		return -EEXIST;
 	}
 
@@ -1449,7 +1452,7 @@ static int check_conflicting_ftes(struct fs_fte *fte, const struct mlx5_flow_act
 }
 
 static struct mlx5_flow_handle *add_rule_fg(struct mlx5_flow_group *fg,
-					    u32 *match_value,
+					    struct mlx5_flow_spec *spec,
 					    struct mlx5_flow_act *flow_act,
 					    struct mlx5_flow_destination *dest,
 					    int dest_num,
@@ -1460,7 +1463,7 @@ static struct mlx5_flow_handle *add_rule_fg(struct mlx5_flow_group *fg,
 	int i;
 	int ret;
 
-	ret = check_conflicting_ftes(fte, flow_act);
+	ret = check_conflicting_ftes(fte, &spec->flow_context, flow_act);
 	if (ret)
 		return ERR_PTR(ret);
 
@@ -1635,7 +1638,7 @@ try_add_to_existing_fg(struct mlx5_flow_table *ft,
 	u64  version;
 	int err;
 
-	fte = alloc_fte(ft, spec->match_value, flow_act);
+	fte = alloc_fte(ft, spec, flow_act);
 	if (IS_ERR(fte))
 		return  ERR_PTR(-ENOMEM);
 
@@ -1651,8 +1654,7 @@ try_add_to_existing_fg(struct mlx5_flow_table *ft,
 		fte_tmp = lookup_fte_locked(g, spec->match_value, take_write);
 		if (!fte_tmp)
 			continue;
-		rule = add_rule_fg(g, spec->match_value,
-				   flow_act, dest, dest_num, fte_tmp);
+		rule = add_rule_fg(g, spec, flow_act, dest, dest_num, fte_tmp);
 		up_write_ref_node(&fte_tmp->node, false);
 		tree_put_node(&fte_tmp->node, false);
 		kmem_cache_free(steering->ftes_cache, fte);
@@ -1699,8 +1701,7 @@ try_add_to_existing_fg(struct mlx5_flow_table *ft,
 
 		nested_down_write_ref_node(&fte->node, FS_LOCK_CHILD);
 		up_write_ref_node(&g->node, false);
-		rule = add_rule_fg(g, spec->match_value,
-				   flow_act, dest, dest_num, fte);
+		rule = add_rule_fg(g, spec, flow_act, dest, dest_num, fte);
 		up_write_ref_node(&fte->node, false);
 		tree_put_node(&fte->node, false);
 		return rule;
@@ -1786,7 +1787,7 @@ _mlx5_add_flow_rules(struct mlx5_flow_table *ft,
 	if (err)
 		goto err_release_fg;
 
-	fte = alloc_fte(ft, spec->match_value, flow_act);
+	fte = alloc_fte(ft, spec, flow_act);
 	if (IS_ERR(fte)) {
 		err = PTR_ERR(fte);
 		goto err_release_fg;
@@ -1800,8 +1801,7 @@ _mlx5_add_flow_rules(struct mlx5_flow_table *ft,
 
 	nested_down_write_ref_node(&fte->node, FS_LOCK_CHILD);
 	up_write_ref_node(&g->node, false);
-	rule = add_rule_fg(g, spec->match_value, flow_act, dest,
-			   dest_num, fte);
+	rule = add_rule_fg(g, spec, flow_act, dest, dest_num, fte);
 	up_write_ref_node(&fte->node, false);
 	tree_put_node(&fte->node, false);
 	tree_put_node(&g->node, false);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.h b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.h
index a08c3d09a50f..c48c382f926f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.h
@@ -170,6 +170,7 @@ struct fs_fte {
 	u32				val[MLX5_ST_SZ_DW_MATCH_PARAM];
 	u32				dests_size;
 	u32				index;
+	struct mlx5_flow_context	flow_context;
 	struct mlx5_flow_act		action;
 	enum fs_fte_status		status;
 	struct mlx5_fc			*counter;
diff --git a/include/linux/mlx5/fs.h b/include/linux/mlx5/fs.h
index 2ddaa97f2179..9bf49ce218fa 100644
--- a/include/linux/mlx5/fs.h
+++ b/include/linux/mlx5/fs.h
@@ -88,10 +88,20 @@ struct mlx5_flow_group;
 struct mlx5_flow_namespace;
 struct mlx5_flow_handle;
 
+enum {
+	FLOW_CONTEXT_HAS_TAG = BIT(0),
+};
+
+struct mlx5_flow_context {
+	u32 flags;
+	u32 flow_tag;
+};
+
 struct mlx5_flow_spec {
 	u8   match_criteria_enable;
 	u32  match_criteria[MLX5_ST_SZ_DW(fte_match_param)];
 	u32  match_value[MLX5_ST_SZ_DW(fte_match_param)];
+	struct mlx5_flow_context flow_context;
 };
 
 enum {
@@ -173,13 +183,11 @@ struct mlx5_fs_vlan {
 #define MLX5_FS_VLAN_DEPTH	2
 
 enum {
-	FLOW_ACT_HAS_TAG   = BIT(0),
-	FLOW_ACT_NO_APPEND = BIT(1),
+	FLOW_ACT_NO_APPEND = BIT(0),
 };
 
 struct mlx5_flow_act {
 	u32 action;
-	u32 flow_tag;
 	u32 reformat_id;
 	u32 modify_id;
 	uintptr_t esp_id;
@@ -190,7 +198,6 @@ struct mlx5_flow_act {
 
 #define MLX5_DECLARE_FLOW_ACT(name) \
 	struct mlx5_flow_act name = { .action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST,\
-				      .flow_tag = MLX5_FS_DEFAULT_FLOW_TAG, \
 				      .reformat_id = 0, \
 				      .modify_id = 0, \
 				      .flags =  0, }
-- 
2.21.0


^ permalink raw reply related

* [PATCH V2 mlx5-next 04/13] net/mlx5: Introduce a helper API to check VF vport
From: Saeed Mahameed @ 2019-06-25 17:47 UTC (permalink / raw)
  To: Saeed Mahameed, Leon Romanovsky
  Cc: netdev@vger.kernel.org, linux-rdma@vger.kernel.org, Parav Pandit,
	Jianbo Liu
In-Reply-To: <20190625174727.20309-1-saeedm@mellanox.com>

From: Parav Pandit <parav@mellanox.com>

Introduce a helper API mlx5_eswitch_is_vf_vport() to check
if a given vport_num belongs to VF or not.

Signed-off-by: Parav Pandit <parav@mellanox.com>
Reviewed-by: Jianbo Liu <jianbol@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.h          | 2 ++
 drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index 8b9f2cf58e91..99dc25630629 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -508,6 +508,8 @@ void mlx5e_tc_clean_fdb_peer_flows(struct mlx5_eswitch *esw);
 struct mlx5_vport *__must_check
 mlx5_eswitch_get_vport(struct mlx5_eswitch *esw, u16 vport_num);
 
+bool mlx5_eswitch_is_vf_vport(const struct mlx5_eswitch *esw, u16 vport_num);
+
 #else  /* CONFIG_MLX5_ESWITCH */
 /* eswitch API stubs */
 static inline int  mlx5_eswitch_init(struct mlx5_core_dev *dev) { return 0; }
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 17abb98b48af..c1c42c1370b8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -2290,3 +2290,9 @@ struct mlx5_eswitch_rep *mlx5_eswitch_vport_rep(struct mlx5_eswitch *esw,
 	return mlx5_eswitch_get_rep(esw, vport);
 }
 EXPORT_SYMBOL(mlx5_eswitch_vport_rep);
+
+bool mlx5_eswitch_is_vf_vport(const struct mlx5_eswitch *esw, u16 vport_num)
+{
+	return vport_num >= MLX5_VPORT_FIRST_VF &&
+	       vport_num <= esw->dev->priv.sriov.max_vfs;
+}
-- 
2.21.0


^ permalink raw reply related

* [PATCH V2 mlx5-next 03/13] net/mlx5: Support allocating modify header context from ingress ACL
From: Saeed Mahameed @ 2019-06-25 17:47 UTC (permalink / raw)
  To: Saeed Mahameed, Leon Romanovsky
  Cc: netdev@vger.kernel.org, linux-rdma@vger.kernel.org, Jianbo Liu,
	Eli Britstein, Roi Dayan, Mark Bloch
In-Reply-To: <20190625174727.20309-1-saeedm@mellanox.com>

From: Jianbo Liu <jianbol@mellanox.com>

That modify header action can be then attached to a steering rule in
the ingress ACL.

Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
Reviewed-by: Eli Britstein <elibr@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c
index bb24c3797218..4f1d402926f1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_cmd.c
@@ -771,6 +771,10 @@ int mlx5_modify_header_alloc(struct mlx5_core_dev *dev,
 		max_actions = MLX5_CAP_FLOWTABLE_NIC_TX(dev, max_modify_header_actions);
 		table_type = FS_FT_NIC_TX;
 		break;
+	case MLX5_FLOW_NAMESPACE_ESW_INGRESS:
+		max_actions = MLX5_CAP_ESW_INGRESS_ACL(dev, max_modify_header_actions);
+		table_type = FS_FT_ESW_INGRESS_ACL;
+		break;
 	default:
 		return -EOPNOTSUPP;
 	}
-- 
2.21.0


^ permalink raw reply related

* [PATCH V2 mlx5-next 02/13] net/mlx5: Get vport ACL namespace by vport index
From: Saeed Mahameed @ 2019-06-25 17:47 UTC (permalink / raw)
  To: Saeed Mahameed, Leon Romanovsky
  Cc: netdev@vger.kernel.org, linux-rdma@vger.kernel.org, Jianbo Liu,
	Oz Shlomo, Eli Britstein, Roi Dayan, Mark Bloch
In-Reply-To: <20190625174727.20309-1-saeedm@mellanox.com>

From: Jianbo Liu <jianbol@mellanox.com>

The ingress and egress ACL root namespaces are created per vport and
stored into arrays. However, the vport number is not the same as the
index. Passing the array index, instead of vport number, to get the
correct ingress and egress acl namespace.

Fixes: 9b93ab981e3b ("net/mlx5: Separate ingress/egress namespaces for each vport")
Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
Reviewed-by: Oz Shlomo <ozsh@mellanox.com>
Reviewed-by: Eli Britstein <elibr@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
index 12010f85fa35..a42a23e505df 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
@@ -939,7 +939,7 @@ int esw_vport_enable_egress_acl(struct mlx5_eswitch *esw,
 		  vport->vport, MLX5_CAP_ESW_EGRESS_ACL(dev, log_max_ft_size));
 
 	root_ns = mlx5_get_flow_vport_acl_namespace(dev, MLX5_FLOW_NAMESPACE_ESW_EGRESS,
-						    vport->vport);
+			mlx5_eswitch_vport_num_to_index(esw, vport->vport));
 	if (!root_ns) {
 		esw_warn(dev, "Failed to get E-Switch egress flow namespace for vport (%d)\n", vport->vport);
 		return -EOPNOTSUPP;
@@ -1057,7 +1057,7 @@ int esw_vport_enable_ingress_acl(struct mlx5_eswitch *esw,
 		  vport->vport, MLX5_CAP_ESW_INGRESS_ACL(dev, log_max_ft_size));
 
 	root_ns = mlx5_get_flow_vport_acl_namespace(dev, MLX5_FLOW_NAMESPACE_ESW_INGRESS,
-						    vport->vport);
+			mlx5_eswitch_vport_num_to_index(esw, vport->vport));
 	if (!root_ns) {
 		esw_warn(dev, "Failed to get E-Switch ingress flow namespace for vport (%d)\n", vport->vport);
 		return -EOPNOTSUPP;
-- 
2.21.0


^ permalink raw reply related

* [PATCH V2 mlx5-next 01/13] net/mlx5: Introduce vport metadata matching bits and enum constants
From: Saeed Mahameed @ 2019-06-25 17:47 UTC (permalink / raw)
  To: Saeed Mahameed, Leon Romanovsky
  Cc: netdev@vger.kernel.org, linux-rdma@vger.kernel.org, Jianbo Liu,
	Eli Britstein, Roi Dayan, Mark Bloch
In-Reply-To: <20190625174727.20309-1-saeedm@mellanox.com>

From: Jianbo Liu <jianbol@mellanox.com>

When a dual-port VHCA sends a RoCE packet on its non-native port, and
the packet arrives to its affiliated vport FDB, a mismatch might occur
on the rules that match the packet source vport. So we replace the
match on source port with the match on metadata that was configured in
ingress ACL, and that metadata will be passed further also to the NIC
RX table of the eswitch manager.

Introduce vport metadata matching bits and enum constants as a pre-step
towards metadata matching.
    o metadata type C registers in the misc parameters 2 fields.
    o esw_uplink_ingress_acl bit in esw cap. If it set, the device supports
      ingress ACL for the uplink vport.
    o fdb_to_vport_reg_* bits in flow table cap and esw vport context, to
      support propagating the metadata to the nic rx through the loopback
      path.
    o flow_source in flow context, to indicate the known origin of packets.
    o enum constants, to support the above bits.

Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
Reviewed-by: Eli Britstein <elibr@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Mark Bloch <markb@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 include/linux/mlx5/mlx5_ifc.h | 56 ++++++++++++++++++++++++++++++-----
 1 file changed, 49 insertions(+), 7 deletions(-)

diff --git a/include/linux/mlx5/mlx5_ifc.h b/include/linux/mlx5/mlx5_ifc.h
index e3c154b573a2..d4409654f760 100644
--- a/include/linux/mlx5/mlx5_ifc.h
+++ b/include/linux/mlx5/mlx5_ifc.h
@@ -528,7 +528,21 @@ struct mlx5_ifc_fte_match_set_misc2_bits {
 
 	struct mlx5_ifc_fte_match_mpls_bits outer_first_mpls_over_udp;
 
-	u8         reserved_at_80[0x100];
+	u8         metadata_reg_c_7[0x20];
+
+	u8         metadata_reg_c_6[0x20];
+
+	u8         metadata_reg_c_5[0x20];
+
+	u8         metadata_reg_c_4[0x20];
+
+	u8         metadata_reg_c_3[0x20];
+
+	u8         metadata_reg_c_2[0x20];
+
+	u8         metadata_reg_c_1[0x20];
+
+	u8         metadata_reg_c_0[0x20];
 
 	u8         metadata_reg_a[0x20];
 
@@ -636,8 +650,22 @@ struct mlx5_ifc_flow_table_nic_cap_bits {
 	u8         reserved_at_e00[0x7200];
 };
 
+enum {
+	MLX5_FDB_TO_VPORT_REG_C_0 = 0x01,
+	MLX5_FDB_TO_VPORT_REG_C_1 = 0x02,
+	MLX5_FDB_TO_VPORT_REG_C_2 = 0x04,
+	MLX5_FDB_TO_VPORT_REG_C_3 = 0x08,
+	MLX5_FDB_TO_VPORT_REG_C_4 = 0x10,
+	MLX5_FDB_TO_VPORT_REG_C_5 = 0x20,
+	MLX5_FDB_TO_VPORT_REG_C_6 = 0x40,
+	MLX5_FDB_TO_VPORT_REG_C_7 = 0x80,
+};
+
 struct mlx5_ifc_flow_table_eswitch_cap_bits {
-	u8      reserved_at_0[0x1a];
+	u8      fdb_to_vport_reg_c_id[0x8];
+	u8      reserved_at_8[0xf];
+	u8      flow_source[0x1];
+	u8      reserved_at_18[0x2];
 	u8      multi_fdb_encap[0x1];
 	u8      reserved_at_1b[0x1];
 	u8      fdb_multi_path_to_table[0x1];
@@ -665,7 +693,9 @@ struct mlx5_ifc_e_switch_cap_bits {
 	u8         vport_svlan_insert[0x1];
 	u8         vport_cvlan_insert_if_not_exist[0x1];
 	u8         vport_cvlan_insert_overwrite[0x1];
-	u8         reserved_at_5[0x14];
+	u8         reserved_at_5[0x3];
+	u8         esw_uplink_ingress_acl[0x1];
+	u8         reserved_at_9[0x10];
 	u8         esw_functions_changed[0x1];
 	u8         reserved_at_1a[0x1];
 	u8         ecpf_vport_exists[0x1];
@@ -2555,6 +2585,12 @@ enum {
 	MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH_2 = 0x800,
 };
 
+enum {
+	MLX5_FLOW_CONTEXT_FLOW_SOURCE_ANY_VPORT         = 0x0,
+	MLX5_FLOW_CONTEXT_FLOW_SOURCE_UPLINK            = 0x1,
+	MLX5_FLOW_CONTEXT_FLOW_SOURCE_LOCAL_VPORT       = 0x2,
+};
+
 struct mlx5_ifc_vlan_bits {
 	u8         ethtype[0x10];
 	u8         prio[0x3];
@@ -2574,7 +2610,9 @@ struct mlx5_ifc_flow_context_bits {
 	u8         action[0x10];
 
 	u8         extended_destination[0x1];
-	u8         reserved_at_80[0x7];
+	u8         reserved_at_81[0x1];
+	u8         flow_source[0x2];
+	u8         reserved_at_84[0x4];
 	u8         destination_list_size[0x18];
 
 	u8         reserved_at_a0[0x8];
@@ -3099,12 +3137,14 @@ struct mlx5_ifc_hca_vport_context_bits {
 };
 
 struct mlx5_ifc_esw_vport_context_bits {
-	u8         reserved_at_0[0x3];
+	u8         fdb_to_vport_reg_c[0x1];
+	u8         reserved_at_1[0x2];
 	u8         vport_svlan_strip[0x1];
 	u8         vport_cvlan_strip[0x1];
 	u8         vport_svlan_insert[0x1];
 	u8         vport_cvlan_insert[0x2];
-	u8         reserved_at_8[0x18];
+	u8         fdb_to_vport_reg_c_id[0x8];
+	u8         reserved_at_10[0x10];
 
 	u8         reserved_at_20[0x20];
 
@@ -4985,7 +5025,8 @@ struct mlx5_ifc_modify_esw_vport_context_out_bits {
 };
 
 struct mlx5_ifc_esw_vport_context_fields_select_bits {
-	u8         reserved_at_0[0x1c];
+	u8         reserved_at_0[0x1b];
+	u8         fdb_to_vport_reg_c_id[0x1];
 	u8         vport_cvlan_insert[0x1];
 	u8         vport_svlan_insert[0x1];
 	u8         vport_cvlan_strip[0x1];
@@ -5182,6 +5223,7 @@ enum {
 	MLX5_ACTION_IN_FIELD_OUT_DIPV4         = 0x16,
 	MLX5_ACTION_IN_FIELD_OUT_FIRST_VID     = 0x17,
 	MLX5_ACTION_IN_FIELD_OUT_IPV6_HOPLIMIT = 0x47,
+	MLX5_ACTION_IN_FIELD_METADATA_REG_C_0  = 0x51,
 };
 
 struct mlx5_ifc_alloc_modify_header_context_out_bits {
-- 
2.21.0


^ permalink raw reply related

* [PATCH V2 mlx5-next 00/13] Mellanox, mlx5 vport metadata matching
From: Saeed Mahameed @ 2019-06-25 17:47 UTC (permalink / raw)
  To: Saeed Mahameed, Leon Romanovsky
  Cc: netdev@vger.kernel.org, linux-rdma@vger.kernel.org

This series includes mlx5 updates for both rdma and net-next trees.
In case of no objection it will be applied to mlx5-next branch and later
on will be sent as pull request to rdma and net-next.

From Jianbo, Vport meta data matching:

Hardware steering has no notion of vport number, and vport is an
abstract concept, so firmware need to translate the source vport
matching to match on the VHCA ID (Virtual HCA ID).

In dual-port RoCE, the dual-port VHCA is able to send also on the
second port on behalf of the affiliated vport, so now we can’t assume
anymore that vport is represented by single VHCA only.

To resolve this issue, we use metadata register as source port
indicator instead.

When a packet enters the eswitch, eswitch ingress traffic passes the
ingress ACL flow tables, where we tag the packets (via the metadata
value, in this case REG_C at index 0) with a unique value which will
act as an alias of the vport. In order to guarantee uniqueness, we use
the eswitch owner vhca id and the vport number as that value.

Usually, the vports are numbered in each eswitch as followed:
    - Physical Function (PF) vport, the number is 0.
    - Virtual Function (VF) vport, starting from 1.
    - Uplink vport, the reserved vport number for it is 0xFFFF.

With the metadata in each packet, we can then do matching on it, in
both fast path and slow path.

For slow path, there is a representor for each vport. Packet that
misses all offloaded rules in FDB, will be forwarded to the eswitch
manager vport. In its NIC RX, it then will be steered to the right
representor. The rules, which decide the destination representor,
previously were matching on source port, will now match metadata
instead.

V2:
- Remove eswitch cleanup patches from bodong, will submit later.
- Remove IB specified APIs (mlx5_ib_eswitch_*) added in V1.
- Add mlx5_eswitch_is_vf_vport() to check if the vport is VF vport.
- Other small changes.

Thanks,
Saeed.

---

Jianbo Liu (12):
  net/mlx5: Introduce vport metadata matching bits and enum constants
  net/mlx5: Get vport ACL namespace by vport index
  net/mlx5: Support allocating modify header context from ingress ACL
  net/mlx5: Add flow context for flow tag
  net/mlx5: E-Switch, Tag packet with vport number in VF vports and
    uplink ingress ACLs
  net/mlx5e: Specifying known origin of packets matching the flow
  net/mlx5: E-Switch, Add match on vport metadata for rule in fast path
  net/mlx5: E-Switch, Add query and modify esw vport context functions
  net/mlx5: E-Switch, Pass metadata from FDB to eswitch manager
  net/mlx5: E-Switch, Add match on vport metadata for rule in slow path
  RDMA/mlx5: Add vport metadata matching for IB representors
  net/mlx5: E-Switch, Enable vport metadata matching if firmware
    supports it

Parav Pandit (1):
  net/mlx5: Introduce a helper API to check VF vport

 drivers/infiniband/hw/mlx5/flow.c             |  13 +-
 drivers/infiniband/hw/mlx5/main.c             |  75 ++-
 drivers/infiniband/hw/mlx5/mlx5_ib.h          |   1 +
 .../mellanox/mlx5/core/diag/fs_tracepoint.h   |   4 +-
 .../mellanox/mlx5/core/en_fs_ethtool.c        |   2 +-
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   |   7 +-
 .../net/ethernet/mellanox/mlx5/core/eswitch.c |  30 +-
 .../net/ethernet/mellanox/mlx5/core/eswitch.h |  16 +
 .../mellanox/mlx5/core/eswitch_offloads.c     | 500 ++++++++++++++----
 .../ethernet/mellanox/mlx5/core/fpga/ipsec.c  |   8 +-
 .../net/ethernet/mellanox/mlx5/core/fs_cmd.c  |  10 +-
 .../net/ethernet/mellanox/mlx5/core/fs_core.c |  34 +-
 .../net/ethernet/mellanox/mlx5/core/fs_core.h |   1 +
 include/linux/mlx5/eswitch.h                  |  17 +
 include/linux/mlx5/fs.h                       |  16 +-
 include/linux/mlx5/mlx5_ifc.h                 |  56 +-
 16 files changed, 622 insertions(+), 168 deletions(-)

-- 
2.21.0


^ permalink raw reply

* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Christoph Hellwig @ 2019-06-25 17:01 UTC (permalink / raw)
  To: Logan Gunthorpe
  Cc: Christoph Hellwig, linux-kernel, linux-block, linux-nvme,
	linux-pci, linux-rdma, Jens Axboe, Bjorn Helgaas, Dan Williams,
	Sagi Grimberg, Keith Busch, Jason Gunthorpe, Stephen Bates
In-Reply-To: <f0f002bf-2b94-cd18-d18f-5d0b08311495@deltatee.com>

On Tue, Jun 25, 2019 at 09:57:52AM -0600, Logan Gunthorpe wrote:
> > You assume all addressing is done by the PCI bus address.  If a device
> > is addressing its own BAR there is no reason to use the PCI bus address,
> > as it might have much more intelligent schemes (usually bar + offset).
> 
> Yes, that will be a bit tricky regardless of what we do.

At least right now it isn't at all.  I've implemented support for
a draft NVMe proposal for that, and it basically boils down to this
in the p2p path:

	addr = sg_phys(sg);

	if (page->pgmap->dev == ctrl->dev && HAS_RELATIVE_ADDRESSING)
		addr -= ctrl->cmb_start_addr;

		// set magic flag in the SGL
	} else {
		addr -= pgmap->pci_p2pdma_bus_offset;
	}

without the pagemap it would require a range compare instead, which
isn't all that hard either.

> >>> Also duplicating the whole block I/O stack, including hooks all over
> >>> the fast path is pretty much a no-go.
> >>
> >> There was very little duplicate code in the patch set. (Really just the
> >> mapping code). There are a few hooks, but in practice not that many if
> >> we ignore the WARN_ONs. We might be able to work to reduce this further.
> >> The main hooks are: when we skip bouncing, when we skip integrity prep,
> >> when we split, and when we map. And the patchset drops the PCI_P2PDMA
> >> hook when we map. So we're talking about maybe three or four extra ifs
> >> that would likely normally be fast due to the branch predictor.
> > 
> > And all of those add code to the block layer fast path.
> 
> If we can't add any ifs to the block layer, there's really nothing we
> can do.

That is not what I said.  Of course we can.  But we rather have a
really good reason.  And adding a parallel I/O path violating the
highlevel model is not one.

> So then we're committed to using struct page for P2P?

Only until we have a significantly better soltution.  And I think
using physical address in some form instead of pages is that,
adding a parallel path with dma_addr_t is not, it actually is worse
than the current code in many respects.

^ permalink raw reply

* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Logan Gunthorpe @ 2019-06-25 15:57 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-kernel, linux-block, linux-nvme, linux-pci, linux-rdma,
	Jens Axboe, Bjorn Helgaas, Dan Williams, Sagi Grimberg,
	Keith Busch, Jason Gunthorpe, Stephen Bates
In-Reply-To: <20190625072008.GB30350@lst.de>



On 2019-06-25 1:20 a.m., Christoph Hellwig wrote:
> On Mon, Jun 24, 2019 at 10:07:56AM -0600, Logan Gunthorpe wrote:
>>> For one passing a dma_addr_t through the block layer is a layering
>>> violation, and one that I think will also bite us in practice.
>>> The host physical to PCIe bus address mapping can have offsets, and
>>> those offsets absolutely can be different for differnet root ports.
>>> So with your caller generated dma_addr_t everything works fine with
>>> a switched setup as the one you are probably testing on, but on a
>>> sufficiently complicated setup with multiple root ports it can break.
>>
>> I don't follow this argument. Yes, I understand PCI Bus offsets and yes
>> I understand that they only apply beyond the bus they're working with.
>> But this isn't *that* complicated and it should be the responsibility of
>> the P2PDMA code to sort out and provide a dma_addr_t for. The dma_addr_t
>> that's passed through the block layer could be a bus address or it could
>> be the result of a dma_map_* request (if the transaction is found to go
>> through an RC) depending on the requirements of the devices being used.
> 
> You assume all addressing is done by the PCI bus address.  If a device
> is addressing its own BAR there is no reason to use the PCI bus address,
> as it might have much more intelligent schemes (usually bar + offset).

Yes, that will be a bit tricky regardless of what we do.

>>> Also duplicating the whole block I/O stack, including hooks all over
>>> the fast path is pretty much a no-go.
>>
>> There was very little duplicate code in the patch set. (Really just the
>> mapping code). There are a few hooks, but in practice not that many if
>> we ignore the WARN_ONs. We might be able to work to reduce this further.
>> The main hooks are: when we skip bouncing, when we skip integrity prep,
>> when we split, and when we map. And the patchset drops the PCI_P2PDMA
>> hook when we map. So we're talking about maybe three or four extra ifs
>> that would likely normally be fast due to the branch predictor.
> 
> And all of those add code to the block layer fast path.

If we can't add any ifs to the block layer, there's really nothing we
can do.

So then we're committed to using struct page for P2P?

Logan

^ permalink raw reply

* Re: [PATCH rdma-next v1 11/12] IB/mlx5: Implement DEVX dispatching event
From: Yishai Hadas @ 2019-06-25 14:41 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Leon Romanovsky, Doug Ledford, Leon Romanovsky, RDMA mailing list,
	Yishai Hadas, Saeed Mahameed, linux-netdev
In-Reply-To: <20190624180558.GL7418@mellanox.com>

On 6/24/2019 9:06 PM, Jason Gunthorpe wrote:
> On Mon, Jun 24, 2019 at 07:55:32PM +0300, Yishai Hadas wrote:
> 
>>>> +	/* Explicit filtering to kernel events which may occur frequently */
>>>> +	if (event_type == MLX5_EVENT_TYPE_CMD ||
>>>> +	    event_type == MLX5_EVENT_TYPE_PAGE_REQUEST)
>>>> +		return NOTIFY_OK;
>>>> +
>>>> +	table = container_of(nb, struct mlx5_devx_event_table, devx_nb.nb);
>>>> +	dev = container_of(table, struct mlx5_ib_dev, devx_event_table);
>>>> +	is_unaffiliated = is_unaffiliated_event(dev->mdev, event_type);
>>>> +
>>>> +	if (!is_unaffiliated)
>>>> +		obj_type = get_event_obj_type(event_type, data);
>>>> +	event = xa_load(&table->event_xa, event_type | (obj_type << 16));
>>>> +	if (!event)
>>>> +		return NOTIFY_DONE;
>>>
>>> event should be in the rcu as well
>>
>> Do we really need this ? I didn't see a flow that really requires
>> that.
> 
> I think there are no frees left? Even so it makes much more sense to
> include the event in the rcu as if we ever did need to kfree it would
> have to be via rcu
> 

OK

>>>> +	while (list_empty(&ev_queue->event_list)) {
>>>> +		spin_unlock_irq(&ev_queue->lock);
>>>> +
>>>> +		if (filp->f_flags & O_NONBLOCK)
>>>> +			return -EAGAIN;
>>>> +
>>>> +		if (wait_event_interruptible(ev_queue->poll_wait,
>>>> +			    (!list_empty(&ev_queue->event_list) ||
>>>> +			     ev_queue->is_destroyed))) {
>>>> +			return -ERESTARTSYS;
>>>> +		}
>>>> +
>>>> +		if (list_empty(&ev_queue->event_list) &&
>>>> +		    ev_queue->is_destroyed)
>>>> +			return -EIO;
>>>
>>> All these tests should be under the lock.
>>
>> We can't call wait_event_interruptible() above which may sleep under the
>> lock, correct ? are you referring to the list_empty() and
>> is_destroyed ?
> 
> yes
> 
>> By the way looking in uverb code [1], similar code which is not done under
>> the lock as of here..
>>
>> [1] https://elixir.bootlin.com/linux/latest/source/drivers/infiniband/core/uverbs_main.c#L244
> 
> Also not a good idea
> 
>>> Why don't we return EIO as soon as is-destroyed happens? What is the
>>> point of flushing out the accumulated events?
>>
>> It follows the above uverb code/logic that returns existing events even in
>> that case, also the async command events in this file follows that logic, I
>> suggest to stay consistent.
> 
> Don't follow broken uverbs stuff...

May it be that there is some event that we still want to deliver post 
unbind/hot-unplug ? for example IB_EVENT_DEVICE_FATAL in uverbs and 
others from the driver code.

Not sure that we want to change this logic.
What do you think ?

> 
>>> Maybe the event should be re-added on error? Tricky.
>>
>> What will happen if another copy_to_user may then fail again (loop ?) ...
>> not sure that we want to get into this tricky handling ...
>>
>> As of above, It follows the logic from uverbs at that area.
>> https://elixir.bootlin.com/linux/latest/source/drivers/infiniband/core/uverbs_main.c#L267
> 
> again it is wrong...
> 
> There is no loop if you just stick the item back on the head of the
> list and exit, which is probably the right thing to do..
> 

What if copy_to_user() will fail again just later on ? we might end-up 
with loop of read(s) that always find an event as it was put back.
I suggest to leave this flow as it's now, at least for this series 
submission.

Agree ?


>>>> @@ -2374,6 +2705,17 @@ static int devx_hot_unplug_async_cmd_event_file(struct ib_uobject *uobj,
>>>>    static int devx_hot_unplug_async_event_file(struct ib_uobject *uobj,
>>>>    					    enum rdma_remove_reason why)
>>>>    {
>>>> +	struct devx_async_event_file *ev_file =
>>>> +		container_of(uobj, struct devx_async_event_file,
>>>> +			     uobj);
>>>> +	struct devx_async_event_queue *ev_queue = &ev_file->ev_queue;
>>>> +
>>>> +	spin_lock_irq(&ev_queue->lock);
>>>> +	ev_queue->is_destroyed = 1;
>>>> +	spin_unlock_irq(&ev_queue->lock);
>>>> +
>>>> +	if (why == RDMA_REMOVE_DRIVER_REMOVE)
>>>> +		wake_up_interruptible(&ev_queue->poll_wait);
>>>
>>> Why isn't this wakeup always done?
>>
>> Maybe you are right and this can be always done to wake up any readers as
>> the 'is_destroyed' was set.
>>
>> By the way, any idea why it was done as such in uverbs [1] for similar flow
>> ? also the command events follows that.
> 
> I don't know, it is probably pointless too.
> 
> If we don't need it here then we shouldn't have it.
> 
> These random pointless ifs bother me as we have to spend time trying
> to figure out that they are pointless down the road.
> 

OK, will drop this if.

^ permalink raw reply

* Re: [RFC][PATCH] wake_up_var() memory ordering
From: Peter Zijlstra @ 2019-06-25 13:27 UTC (permalink / raw)
  To: Andreas Gruenbacher
  Cc: Martin Brandenburg, linux-cachefs-H+wXaHxf7aLQT0dZR+AlfA,
	Mike Snitzer, linux-aio-Bw31MaZKKs3YtjvyW6yDsg, David Airlie,
	samba-technical, Joonas Lahtinen, Will Deacon,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, David Howells,
	Chris Mason, dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
	keyrings-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar,
	linux-afs-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Alasdair Kergon,
	Mike Marshall, linux-cifs-u79uwXL29TY76Z2rM5mHXA,
	rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA, James Morris, cluster-devel,
	Antti Palosaari, Matthias Brugger, Paul McKenney,
	intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <CAHc6FU6zUCdQZ1AfN2KYcPYVKc5bwvc0bD7=-KZpFXws+F9QZQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

On Tue, Jun 25, 2019 at 02:12:22PM +0200, Andreas Gruenbacher wrote:

> > Only if we do as David suggested and make clean_and_wake_up_bit()
> > provide the RELEASE barrier.
> 
> (It's clear_and_wake_up_bit, not clean_and_wake_up_bit.)

Yes, typing hard.

> > That is, currently clear_and_wake_up_bit() is
> >
> >         clear_bit()
> >         smp_mb__after_atomic();
> >         wake_up_bit();
> >

> Now I'm confused because clear_and_wake_up_bit() in mainline does use
> clear_bit_unlock(), so it's the exact opposite of what you just said.

Argh; clearly I couldn't read. And then yes, you're right.

^ permalink raw reply

* Re: [PATCH v3 13/33] docs: infiniband: convert docs to ReST and rename to *.rst
From: Jason Gunthorpe @ 2019-06-25 13:24 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Linux Doc Mailing List, Mauro Carvalho Chehab,
	linux-kernel@vger.kernel.org, Jonathan Corbet, Doug Ledford,
	linux-rdma@vger.kernel.org
In-Reply-To: <09036fdb89c4bec94cb92d25398c026afdb134e7.1560045490.git.mchehab+samsung@kernel.org>

On Sat, Jun 08, 2019 at 11:27:03PM -0300, Mauro Carvalho Chehab wrote:
> The InfiniBand docs are plain text with no markups.
> So, all we needed to do were to add the title markups and
> some markup sequences in order to properly parse tables,
> lists and literal blocks.
> 
> At its new index.rst, let's add a :orphan: while this is not linked to
> the main index.rst file, in order to avoid build warnings.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
>  .../{core_locking.txt => core_locking.rst}    |  64 ++++++-----
>  Documentation/infiniband/index.rst            |  23 ++++
>  .../infiniband/{ipoib.txt => ipoib.rst}       |  24 ++--
>  .../infiniband/{opa_vnic.txt => opa_vnic.rst} | 108 +++++++++---------
>  .../infiniband/{sysfs.txt => sysfs.rst}       |   4 +-
>  .../{tag_matching.txt => tag_matching.rst}    |   5 +
>  .../infiniband/{user_mad.txt => user_mad.rst} |  33 ++++--
>  .../{user_verbs.txt => user_verbs.rst}        |  12 +-
>  drivers/infiniband/core/user_mad.c            |   2 +-
>  drivers/infiniband/ulp/ipoib/Kconfig          |   2 +-
>  10 files changed, 174 insertions(+), 103 deletions(-)
>  rename Documentation/infiniband/{core_locking.txt => core_locking.rst} (78%)
>  create mode 100644 Documentation/infiniband/index.rst
>  rename Documentation/infiniband/{ipoib.txt => ipoib.rst} (90%)
>  rename Documentation/infiniband/{opa_vnic.txt => opa_vnic.rst} (63%)
>  rename Documentation/infiniband/{sysfs.txt => sysfs.rst} (69%)
>  rename Documentation/infiniband/{tag_matching.txt => tag_matching.rst} (98%)
>  rename Documentation/infiniband/{user_mad.txt => user_mad.rst} (90%)
>  rename Documentation/infiniband/{user_verbs.txt => user_verbs.rst} (93%)

Applied to for-next, thanks

Jason

^ permalink raw reply

* Re: [PATCH][next] RDMA/hns: fix potential integer overflow on left shift
From: Jason Gunthorpe @ 2019-06-25 13:19 UTC (permalink / raw)
  To: Colin King
  Cc: Lijun Ou, Wei Hu, Doug Ledford, linux-rdma@vger.kernel.org,
	kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20190624214608.11765-1-colin.king@canonical.com>

On Mon, Jun 24, 2019 at 10:46:08PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> There is a potential integer overflow when int i is left shifted
> as this is evaluated using 32 bit arithmetic but is being used in
> a context that expects an expression of type dma_addr_t.  Fix this
> by casting integer i to dma_addr_t before shifting to avoid the
> overflow.
> 
> Addresses-Coverity: ("Unintentional integer overflow")
> Fixes: 2ac0bc5e725e ("RDMA/hns: Add a group interfaces for optimizing buffers getting flow")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/infiniband/hw/hns/hns_roce_alloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to for-next, thanks

Jason

^ permalink raw reply

* Re: [RFC][PATCH] wake_up_var() memory ordering
From: Andreas Gruenbacher @ 2019-06-25 12:12 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Martin Brandenburg, linux-cachefs, Mike Snitzer, linux-aio,
	David Airlie, samba-technical, Will Deacon, dri-devel,
	David Howells, Chris Mason, dm-devel, keyrings, Ingo Molnar,
	linux-afs, Alasdair Kergon, Mike Marshall, linux-cifs, rds-devel,
	linux-rdma, James Morris, cluster-devel, Antti Palosaari,
	Matthias Brugger, Paul McKenney, intel-gfx, devel,
	Serge E. Hallyn
In-Reply-To: <20190625103430.GW3402@hirez.programming.kicks-ass.net>

On Tue, 25 Jun 2019 at 12:36, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Jun 25, 2019 at 11:19:35AM +0200, Andreas Gruenbacher wrote:
> > > diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
> > > index cf4c767005b1..666629ea5da7 100644
> > > --- a/fs/gfs2/glops.c
> > > +++ b/fs/gfs2/glops.c
> > > @@ -227,6 +227,7 @@ static void gfs2_clear_glop_pending(struct gfs2_inode *ip)
> > >                 return;
> > >
> > >         clear_bit_unlock(GIF_GLOP_PENDING, &ip->i_flags);
> > > +       smp_mb__after_atomic();
> > >         wake_up_bit(&ip->i_flags, GIF_GLOP_PENDING);
> >
> > This should become clear_and_wake_up_bit as well, right? There are
> > several more instances of the same pattern.
>
> Only if we do as David suggested and make clean_and_wake_up_bit()
> provide the RELEASE barrier.

(It's clear_and_wake_up_bit, not clean_and_wake_up_bit.)

> That is, currently clear_and_wake_up_bit() is
>
>         clear_bit()
>         smp_mb__after_atomic();
>         wake_up_bit();
>
> But the above is:
>
>         clear_bit_unlock();
>         smp_mb__after_atomic();
>         wake_up_bit()
>
> the difference is that _unlock() uses RELEASE semantics, where
> clear_bit() does not.
>
> The difference is illustrated with something like:
>
>         cond = true;
>         clear_bit()
>         smp_mb__after_atomic();
>         wake_up_bit();
>
> In this case, a remote CPU can first observe the clear_bit() and then
> the 'cond = true' store. When we use clear_bit_unlock() this is not
> possible, because the RELEASE barrier ensures that everything before,
> stays before.

Now I'm confused because clear_and_wake_up_bit() in mainline does use
clear_bit_unlock(), so it's the exact opposite of what you just said.

Thanks,
Andreas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply

* Re: [RFC][PATCH] wake_up_var() memory ordering
From: Peter Zijlstra @ 2019-06-25 10:34 UTC (permalink / raw)
  To: Andreas Gruenbacher
  Cc: Martin Brandenburg, linux-cachefs-H+wXaHxf7aLQT0dZR+AlfA,
	Mike Snitzer, linux-aio-Bw31MaZKKs3YtjvyW6yDsg, David Airlie,
	samba-technical, Joonas Lahtinen, Will Deacon,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, David Howells,
	Chris Mason, dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
	keyrings-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar,
	linux-afs-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Alasdair Kergon,
	Mike Marshall, linux-cifs-u79uwXL29TY76Z2rM5mHXA,
	rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA, James Morris, cluster-devel,
	Antti Palosaari, Matthias Brugger, Paul McKenney,
	intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <CAHc6FU7j5iW7WQoxN_OSfvK4zxv_MxTWJpiNsqFW8TEDMX1rjw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

On Tue, Jun 25, 2019 at 11:19:35AM +0200, Andreas Gruenbacher wrote:
> > diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
> > index cf4c767005b1..666629ea5da7 100644
> > --- a/fs/gfs2/glops.c
> > +++ b/fs/gfs2/glops.c
> > @@ -227,6 +227,7 @@ static void gfs2_clear_glop_pending(struct gfs2_inode *ip)
> >                 return;
> >
> >         clear_bit_unlock(GIF_GLOP_PENDING, &ip->i_flags);
> > +       smp_mb__after_atomic();
> >         wake_up_bit(&ip->i_flags, GIF_GLOP_PENDING);
> 
> This should become clear_and_wake_up_bit as well, right? There are
> several more instances of the same pattern.

Only if we do as David suggested and make clean_and_wake_up_bit()
provide the RELEASE barrier.

That is, currently clear_and_wake_up_bit() is

	clear_bit()
	smp_mb__after_atomic();
	wake_up_bit();

But the above is:

	clear_bit_unlock();
	smp_mb__after_atomic();
	wake_up_bit()

the difference is that _unlock() uses RELEASE semantics, where
clear_bit() does not.

The difference is illustrated with something like:

	cond = true;
	clear_bit()
	smp_mb__after_atomic();
	wake_up_bit();

In this case, a remote CPU can first observe the clear_bit() and then
the 'cond = true' store. When we use clear_bit_unlock() this is not
possible, because the RELEASE barrier ensures that everything before,
stays before.

> >  }
> >

^ permalink raw reply

* Re: [RFC][PATCH] wake_up_var() memory ordering
From: Andreas Gruenbacher @ 2019-06-25  9:19 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: David Howells, Paul McKenney, Will Deacon, Marcel Holtmann,
	Johan Hedberg, Matthias Brugger, Sean Wang, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, David Airlie, Daniel Vetter,
	Alasdair Kergon, Mike Snitzer, dm-devel, Antti Palosaari,
	MauroCarvalho Chehab, Alexander Viro, Benjamin LaHaise,
	Chris Mason, Josef Bacik, David Sterba, Steve French,
	Bob Peterson
In-Reply-To: <20190624165012.GH3436@hirez.programming.kicks-ass.net>

Peter,

thanks for the fixes.

On Mon, 24 Jun 2019 at 18:53, Peter Zijlstra <peterz@infradead.org> wrote:
> Hi all,
>
> I tried using wake_up_var() today and accidentally noticed that it
> didn't imply an smp_mb() and specifically requires it through
> wake_up_bit() / waitqueue_active().
>
> Now, wake_up_bit() doesn't imply the barrier because it is assumed to be
> used with the atomic bitops API which either implies (test_and_clear) or
> only needs smp_mb__after_atomic(), which is 'much' cheaper than an
> unconditional smp_mb().
>
> Still, while auditing all that, I found a whole bunch of things that
> could be improved. There were missing barriers, superfluous barriers and
> a whole bunch of sites that could use clear_and_wake_up_bit().
>
> So this fixes all wake_up_bit() usage without actually changing
> semantics of it (which are unfortunate but understandable).

thanks for the fixes.

> This does
> however change the semantics of wake_up_var(); even though wake_up_var()
> is most often used with atomics and then the additional smp_mb() is most
> often superfluous :/
>
> There isn't really a good option here, comments (other than I need to
> split this up)?
>
>
> ---
>  drivers/bluetooth/btmtksdio.c               |  5 +----
>  drivers/bluetooth/btmtkuart.c               |  5 +----
>  drivers/bluetooth/hci_mrvl.c                |  8 ++------
>  drivers/gpu/drm/i915/i915_reset.c           |  6 ++----
>  drivers/md/dm-bufio.c                       | 10 ++--------
>  drivers/media/usb/dvb-usb-v2/dvb_usb_core.c | 15 ++++-----------
>  fs/afs/fs_probe.c                           |  1 +
>  fs/afs/server.c                             |  1 +
>  fs/afs/vl_probe.c                           |  1 +
>  fs/afs/volume.c                             |  1 +
>  fs/aio.c                                    |  4 +---
>  fs/block_dev.c                              |  1 +
>  fs/btrfs/extent_io.c                        |  4 +---
>  fs/cachefiles/namei.c                       |  1 +
>  fs/cifs/connect.c                           |  3 +--
>  fs/cifs/misc.c                              | 15 +++++----------
>  fs/fscache/cookie.c                         |  2 ++
>  fs/fscache/object.c                         |  2 ++
>  fs/fscache/page.c                           |  3 +++
>  fs/gfs2/glock.c                             |  8 ++------
>  fs/gfs2/glops.c                             |  1 +
>  fs/gfs2/lock_dlm.c                          |  8 ++------
>  fs/gfs2/recovery.c                          |  4 +---
>  fs/gfs2/super.c                             |  1 +
>  fs/gfs2/sys.c                               |  4 +---
>  fs/nfs/nfs4state.c                          |  4 +---
>  fs/nfs/pnfs_nfs.c                           |  4 +---
>  fs/nfsd/nfs4recover.c                       |  4 +---
>  fs/orangefs/file.c                          |  2 +-
>  kernel/sched/wait_bit.c                     |  1 +
>  net/bluetooth/hci_event.c                   |  5 +----
>  net/rds/ib_recv.c                           |  1 +
>  security/keys/gc.c                          |  5 ++---
>  33 files changed, 50 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
> index 813338288453..27523cfeac9a 100644
> --- a/drivers/bluetooth/btmtksdio.c
> +++ b/drivers/bluetooth/btmtksdio.c
> @@ -356,11 +356,8 @@ static int btmtksdio_recv_event(struct hci_dev *hdev, struct sk_buff *skb)
>
>         if (hdr->evt == HCI_EV_VENDOR) {
>                 if (test_and_clear_bit(BTMTKSDIO_TX_WAIT_VND_EVT,
> -                                      &bdev->tx_state)) {
> -                       /* Barrier to sync with other CPUs */
> -                       smp_mb__after_atomic();
> +                                      &bdev->tx_state))
>                         wake_up_bit(&bdev->tx_state, BTMTKSDIO_TX_WAIT_VND_EVT);
> -               }
>         }
>
>         return 0;
> diff --git a/drivers/bluetooth/btmtkuart.c b/drivers/bluetooth/btmtkuart.c
> index f5dbeec8e274..7fe324df3799 100644
> --- a/drivers/bluetooth/btmtkuart.c
> +++ b/drivers/bluetooth/btmtkuart.c
> @@ -340,11 +340,8 @@ static int btmtkuart_recv_event(struct hci_dev *hdev, struct sk_buff *skb)
>
>         if (hdr->evt == HCI_EV_VENDOR) {
>                 if (test_and_clear_bit(BTMTKUART_TX_WAIT_VND_EVT,
> -                                      &bdev->tx_state)) {
> -                       /* Barrier to sync with other CPUs */
> -                       smp_mb__after_atomic();
> +                                      &bdev->tx_state))
>                         wake_up_bit(&bdev->tx_state, BTMTKUART_TX_WAIT_VND_EVT);
> -               }
>         }
>
>         return 0;
> diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c
> index 50212ac629e3..f03294d39d08 100644
> --- a/drivers/bluetooth/hci_mrvl.c
> +++ b/drivers/bluetooth/hci_mrvl.c
> @@ -157,9 +157,7 @@ static int mrvl_recv_fw_req(struct hci_dev *hdev, struct sk_buff *skb)
>
>         mrvl->tx_len = le16_to_cpu(pkt->lhs);
>
> -       clear_bit(STATE_FW_REQ_PENDING, &mrvl->flags);
> -       smp_mb__after_atomic();
> -       wake_up_bit(&mrvl->flags, STATE_FW_REQ_PENDING);
> +       clear_and_wake_up_bit(STATE_FW_REQ_PENDING, &mrvl->flags);
>
>  done:
>         kfree_skb(skb);
> @@ -192,9 +190,7 @@ static int mrvl_recv_chip_ver(struct hci_dev *hdev, struct sk_buff *skb)
>
>         bt_dev_info(hdev, "Controller id = %x, rev = %x", mrvl->id, mrvl->rev);
>
> -       clear_bit(STATE_CHIP_VER_PENDING, &mrvl->flags);
> -       smp_mb__after_atomic();
> -       wake_up_bit(&mrvl->flags, STATE_CHIP_VER_PENDING);
> +       clear_and_wake_up_bit(STATE_CHIP_VER_PENDING, &mrvl->flags);
>
>  done:
>         kfree_skb(skb);
> diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
> index 677d59304e78..6809367dbfa9 100644
> --- a/drivers/gpu/drm/i915/i915_reset.c
> +++ b/drivers/gpu/drm/i915/i915_reset.c
> @@ -1324,10 +1324,8 @@ void i915_handle_error(struct drm_i915_private *i915,
>                         if (i915_reset_engine(engine, msg) == 0)
>                                 engine_mask &= ~engine->mask;
>
> -                       clear_bit(I915_RESET_ENGINE + engine->id,
> -                                 &error->flags);
> -                       wake_up_bit(&error->flags,
> -                                   I915_RESET_ENGINE + engine->id);
> +                       clear_and_wake_up_bit(I915_RESET_ENGINE + engine->id,
> +                                             &error->flags);
>                 }
>         }
>
> diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
> index 2a48ea3f1b30..3114836cc717 100644
> --- a/drivers/md/dm-bufio.c
> +++ b/drivers/md/dm-bufio.c
> @@ -667,10 +667,7 @@ static void write_endio(struct dm_buffer *b, blk_status_t status)
>         BUG_ON(!test_bit(B_WRITING, &b->state));
>
>         smp_mb__before_atomic();
> -       clear_bit(B_WRITING, &b->state);
> -       smp_mb__after_atomic();
> -
> -       wake_up_bit(&b->state, B_WRITING);
> +       clear_and_wake_up_bit(B_WRITING, &b->state);
>  }
>
>  /*
> @@ -1045,10 +1042,7 @@ static void read_endio(struct dm_buffer *b, blk_status_t status)
>         BUG_ON(!test_bit(B_READING, &b->state));
>
>         smp_mb__before_atomic();
> -       clear_bit(B_READING, &b->state);
> -       smp_mb__after_atomic();
> -
> -       wake_up_bit(&b->state, B_READING);
> +       clear_and_wake_up_bit(B_READING, &b->state);
>  }
>
>  /*
> diff --git a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
> index e5e056bf9dfa..fa46bc930704 100644
> --- a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
> +++ b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
> @@ -374,9 +374,7 @@ static int dvb_usb_stop_feed(struct dvb_demux_feed *dvbdmxfeed)
>         usb_urb_killv2(&adap->stream);
>
>         /* clear 'streaming' status bit */
> -       clear_bit(ADAP_STREAMING, &adap->state_bits);
> -       smp_mb__after_atomic();
> -       wake_up_bit(&adap->state_bits, ADAP_STREAMING);
> +       clear_and_wake_up_bit(ADAP_STREAMING, &adap->state_bits);
>  skip_feed_stop:
>
>         if (ret)
> @@ -578,11 +576,8 @@ static int dvb_usb_fe_init(struct dvb_frontend *fe)
>                         goto err;
>         }
>  err:
> -       if (!adap->suspend_resume_active) {
> -               clear_bit(ADAP_INIT, &adap->state_bits);
> -               smp_mb__after_atomic();
> -               wake_up_bit(&adap->state_bits, ADAP_INIT);
> -       }
> +       if (!adap->suspend_resume_active)
> +               clear_and_wake_up_bit(ADAP_INIT, &adap->state_bits);
>
>         dev_dbg(&d->udev->dev, "%s: ret=%d\n", __func__, ret);
>         return ret;
> @@ -619,9 +614,7 @@ static int dvb_usb_fe_sleep(struct dvb_frontend *fe)
>  err:
>         if (!adap->suspend_resume_active) {
>                 adap->active_fe = -1;
> -               clear_bit(ADAP_SLEEP, &adap->state_bits);
> -               smp_mb__after_atomic();
> -               wake_up_bit(&adap->state_bits, ADAP_SLEEP);
> +               clear_and_wake_up_bit(ADAP_SLEEP, &adap->state_bits);
>         }
>
>         dev_dbg(&d->udev->dev, "%s: ret=%d\n", __func__, ret);
> diff --git a/fs/afs/fs_probe.c b/fs/afs/fs_probe.c
> index cfe62b154f68..377ee07d5f76 100644
> --- a/fs/afs/fs_probe.c
> +++ b/fs/afs/fs_probe.c
> @@ -18,6 +18,7 @@ static bool afs_fs_probe_done(struct afs_server *server)
>
>         wake_up_var(&server->probe_outstanding);
>         clear_bit_unlock(AFS_SERVER_FL_PROBING, &server->flags);
> +       smp_mb__after_atomic();
>         wake_up_bit(&server->flags, AFS_SERVER_FL_PROBING);
>         return true;
>  }
> diff --git a/fs/afs/server.c b/fs/afs/server.c
> index e900cd74361b..c9b74a397cc1 100644
> --- a/fs/afs/server.c
> +++ b/fs/afs/server.c
> @@ -569,6 +569,7 @@ bool afs_check_server_record(struct afs_fs_cursor *fc, struct afs_server *server
>         if (!test_and_set_bit_lock(AFS_SERVER_FL_UPDATING, &server->flags)) {
>                 success = afs_update_server_record(fc, server);
>                 clear_bit_unlock(AFS_SERVER_FL_UPDATING, &server->flags);
> +               smp_mb__after_atomic();
>                 wake_up_bit(&server->flags, AFS_SERVER_FL_UPDATING);
>                 _leave(" = %d", success);
>                 return success;
> diff --git a/fs/afs/vl_probe.c b/fs/afs/vl_probe.c
> index 858498cc1b05..1939624aa147 100644
> --- a/fs/afs/vl_probe.c
> +++ b/fs/afs/vl_probe.c
> @@ -18,6 +18,7 @@ static bool afs_vl_probe_done(struct afs_vlserver *server)
>
>         wake_up_var(&server->probe_outstanding);
>         clear_bit_unlock(AFS_VLSERVER_FL_PROBING, &server->flags);
> +       smp_mb__after_atomic();
>         wake_up_bit(&server->flags, AFS_VLSERVER_FL_PROBING);
>         return true;
>  }
> diff --git a/fs/afs/volume.c b/fs/afs/volume.c
> index 08fdb3951c49..b968e4e96f6b 100644
> --- a/fs/afs/volume.c
> +++ b/fs/afs/volume.c
> @@ -303,6 +303,7 @@ int afs_check_volume_status(struct afs_volume *volume, struct key *key)
>                 ret = afs_update_volume_status(volume, key);
>                 clear_bit_unlock(AFS_VOLUME_WAIT, &volume->flags);
>                 clear_bit_unlock(AFS_VOLUME_UPDATING, &volume->flags);
> +               smp_mb__after_atomic();
>                 wake_up_bit(&volume->flags, AFS_VOLUME_WAIT);
>                 _leave(" = %d", ret);
>                 return ret;
> diff --git a/fs/aio.c b/fs/aio.c
> index 3490d1fa0e16..a21acb7ee2a5 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1154,9 +1154,7 @@ static void aio_complete(struct aio_kiocb *iocb)
>          * like in wake_up_bit() where clearing a bit has to be
>          * ordered with the unlocked test.
>          */
> -       smp_mb();
> -
> -       if (waitqueue_active(&ctx->wait))
> +       if (wq_has_sleeper(&ctx->wait))
>                 wake_up(&ctx->wait);
>  }
>
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 749f5984425d..80e35cbfc7cc 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1687,6 +1687,7 @@ int blkdev_get(struct block_device *bdev, fmode_t mode, void *holder)
>                 /* tell others that we're done */
>                 BUG_ON(whole->bd_claiming != holder);
>                 whole->bd_claiming = NULL;
> +               smp_mb();
>                 wake_up_bit(&whole->bd_claiming, 0);
>
>                 spin_unlock(&bdev_lock);
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index db337e53aab3..ee1136ae46fd 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3683,9 +3683,7 @@ static noinline_for_stack int lock_extent_buffer_for_io(struct extent_buffer *eb
>
>  static void end_extent_buffer_writeback(struct extent_buffer *eb)
>  {
> -       clear_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags);
> -       smp_mb__after_atomic();
> -       wake_up_bit(&eb->bflags, EXTENT_BUFFER_WRITEBACK);
> +       clear_and_wake_up_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags);
>  }
>
>  static void set_btree_ioerr(struct page *page)
> diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> index ecc8ecbbfa5a..ecebba72ec7d 100644
> --- a/fs/cachefiles/namei.c
> +++ b/fs/cachefiles/namei.c
> @@ -267,6 +267,7 @@ void cachefiles_mark_object_inactive(struct cachefiles_cache *cache,
>         clear_bit(CACHEFILES_OBJECT_ACTIVE, &object->flags);
>         write_unlock(&cache->active_lock);
>
> +       smp_mb__after_atomic();
>         wake_up_bit(&object->flags, CACHEFILES_OBJECT_ACTIVE);
>
>         /* This object can now be culled, so we need to let the daemon know
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 8c4121da624e..8c1115ed1c28 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -5235,8 +5235,7 @@ cifs_sb_tlink(struct cifs_sb_info *cifs_sb)
>         }
>
>         tlink->tl_tcon = cifs_construct_tcon(cifs_sb, fsuid);
> -       clear_bit(TCON_LINK_PENDING, &tlink->tl_flags);
> -       wake_up_bit(&tlink->tl_flags, TCON_LINK_PENDING);
> +       clear_and_wake_up_bit(TCON_LINK_PENDING, &tlink->tl_flags);
>
>         if (IS_ERR(tlink->tl_tcon)) {
>                 cifs_put_tlink(tlink);
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> index b1a696a73f7c..961751d89113 100644
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -584,10 +584,8 @@ int cifs_get_writer(struct cifsInodeInfo *cinode)
>         /* Check to see if we have started servicing an oplock break */
>         if (test_bit(CIFS_INODE_PENDING_OPLOCK_BREAK, &cinode->flags)) {
>                 cinode->writers--;
> -               if (cinode->writers == 0) {
> -                       clear_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
> -                       wake_up_bit(&cinode->flags, CIFS_INODE_PENDING_WRITERS);
> -               }
> +               if (cinode->writers == 0)
> +                       clear_and_wake_up_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
>                 spin_unlock(&cinode->writers_lock);
>                 goto start;
>         }
> @@ -599,10 +597,8 @@ void cifs_put_writer(struct cifsInodeInfo *cinode)
>  {
>         spin_lock(&cinode->writers_lock);
>         cinode->writers--;
> -       if (cinode->writers == 0) {
> -               clear_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
> -               wake_up_bit(&cinode->flags, CIFS_INODE_PENDING_WRITERS);
> -       }
> +       if (cinode->writers == 0)
> +               clear_and_wake_up_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
>         spin_unlock(&cinode->writers_lock);
>  }
>
> @@ -630,8 +626,7 @@ void cifs_queue_oplock_break(struct cifsFileInfo *cfile)
>
>  void cifs_done_oplock_break(struct cifsInodeInfo *cinode)
>  {
> -       clear_bit(CIFS_INODE_PENDING_OPLOCK_BREAK, &cinode->flags);
> -       wake_up_bit(&cinode->flags, CIFS_INODE_PENDING_OPLOCK_BREAK);
> +       clear_and_wake_up_bit(CIFS_INODE_PENDING_OPLOCK_BREAK, &cinode->flags);
>  }
>
>  bool
> diff --git a/fs/fscache/cookie.c b/fs/fscache/cookie.c
> index 0ce39658a620..40ba8957ebf0 100644
> --- a/fs/fscache/cookie.c
> +++ b/fs/fscache/cookie.c
> @@ -381,6 +381,7 @@ void __fscache_enable_cookie(struct fscache_cookie *cookie,
>
>  out_unlock:
>         clear_bit_unlock(FSCACHE_COOKIE_ENABLEMENT_LOCK, &cookie->flags);
> +       smp_mb__after_atomic();
>         wake_up_bit(&cookie->flags, FSCACHE_COOKIE_ENABLEMENT_LOCK);
>  }
>  EXPORT_SYMBOL(__fscache_enable_cookie);
> @@ -778,6 +779,7 @@ void __fscache_disable_cookie(struct fscache_cookie *cookie,
>
>  out_unlock_enable:
>         clear_bit_unlock(FSCACHE_COOKIE_ENABLEMENT_LOCK, &cookie->flags);
> +       smp_mb__after_atomic();
>         wake_up_bit(&cookie->flags, FSCACHE_COOKIE_ENABLEMENT_LOCK);
>         _leave("");
>  }
> diff --git a/fs/fscache/object.c b/fs/fscache/object.c
> index cfeba839a0f2..08b2edec6596 100644
> --- a/fs/fscache/object.c
> +++ b/fs/fscache/object.c
> @@ -524,6 +524,7 @@ void fscache_object_lookup_negative(struct fscache_object *object)
>
>                 _debug("wake up lookup %p", &cookie->flags);
>                 clear_bit_unlock(FSCACHE_COOKIE_LOOKING_UP, &cookie->flags);
> +               smp_mb__after_atomic();
>                 wake_up_bit(&cookie->flags, FSCACHE_COOKIE_LOOKING_UP);
>         }
>         _leave("");
> @@ -559,6 +560,7 @@ void fscache_obtained_object(struct fscache_object *object)
>                  * to begin shovelling data.
>                  */
>                 clear_bit_unlock(FSCACHE_COOKIE_LOOKING_UP, &cookie->flags);
> +               smp_mb__after_atomic();
>                 wake_up_bit(&cookie->flags, FSCACHE_COOKIE_LOOKING_UP);
>         } else {
>                 fscache_stat(&fscache_n_object_created);
> diff --git a/fs/fscache/page.c b/fs/fscache/page.c
> index 26af6fdf1538..49ef238502a3 100644
> --- a/fs/fscache/page.c
> +++ b/fs/fscache/page.c
> @@ -113,6 +113,7 @@ bool __fscache_maybe_release_page(struct fscache_cookie *cookie,
>                 fscache_stat(&fscache_n_store_vmscan_gone);
>         }
>
> +       smp_mb();
>         wake_up_bit(&cookie->flags, 0);
>         trace_fscache_wake_cookie(cookie);
>         if (xpage)
> @@ -171,6 +172,7 @@ static void fscache_end_page_write(struct fscache_object *object,
>                         trace_fscache_page(cookie, page, fscache_page_write_end_pend);
>                 }
>                 spin_unlock(&cookie->stores_lock);
> +               smp_mb();
>                 wake_up_bit(&cookie->flags, 0);
>                 trace_fscache_wake_cookie(cookie);
>         } else {
> @@ -922,6 +924,7 @@ void fscache_invalidate_writes(struct fscache_cookie *cookie)
>                         put_page(results[i]);
>         }
>
> +       smp_mb();
>         wake_up_bit(&cookie->flags, 0);
>         trace_fscache_wake_cookie(cookie);
>
> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> index f1ebcb42cbf5..8024b6bdb6d4 100644
> --- a/fs/gfs2/glock.c
> +++ b/fs/gfs2/glock.c
> @@ -302,9 +302,7 @@ static inline int may_grant(const struct gfs2_glock *gl, const struct gfs2_holde
>
>  static void gfs2_holder_wake(struct gfs2_holder *gh)
>  {
> -       clear_bit(HIF_WAIT, &gh->gh_iflags);
> -       smp_mb__after_atomic();
> -       wake_up_bit(&gh->gh_iflags, HIF_WAIT);
> +       clear_and_wake_up_bit(HIF_WAIT, &gh->gh_iflags);
>  }
>
>  /**
> @@ -436,9 +434,7 @@ static void state_change(struct gfs2_glock *gl, unsigned int new_state)
>  static void gfs2_demote_wake(struct gfs2_glock *gl)
>  {
>         gl->gl_demote_state = LM_ST_EXCLUSIVE;
> -       clear_bit(GLF_DEMOTE, &gl->gl_flags);
> -       smp_mb__after_atomic();
> -       wake_up_bit(&gl->gl_flags, GLF_DEMOTE);
> +       clear_and_wake_up_bit(GLF_DEMOTE, &gl->gl_flags);
>  }
>
>  /**
> diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
> index cf4c767005b1..666629ea5da7 100644
> --- a/fs/gfs2/glops.c
> +++ b/fs/gfs2/glops.c
> @@ -227,6 +227,7 @@ static void gfs2_clear_glop_pending(struct gfs2_inode *ip)
>                 return;
>
>         clear_bit_unlock(GIF_GLOP_PENDING, &ip->i_flags);
> +       smp_mb__after_atomic();
>         wake_up_bit(&ip->i_flags, GIF_GLOP_PENDING);

This should become clear_and_wake_up_bit as well, right? There are
several more instances of the same pattern.

>  }
>
> diff --git a/fs/gfs2/lock_dlm.c b/fs/gfs2/lock_dlm.c
> index 4361804646d8..0fa1865dd600 100644
> --- a/fs/gfs2/lock_dlm.c
> +++ b/fs/gfs2/lock_dlm.c
> @@ -1139,9 +1139,7 @@ static void gdlm_recover_done(void *arg, struct dlm_slot *slots, int num_slots,
>         if (!test_bit(DFL_UNMOUNT, &ls->ls_recover_flags))
>                 queue_delayed_work(gfs2_control_wq, &sdp->sd_control_work, 0);
>
> -       clear_bit(DFL_DLM_RECOVERY, &ls->ls_recover_flags);
> -       smp_mb__after_atomic();
> -       wake_up_bit(&ls->ls_recover_flags, DFL_DLM_RECOVERY);
> +       clear_and_wake_up_bit(DFL_DLM_RECOVERY, &ls->ls_recover_flags);
>         spin_unlock(&ls->ls_recover_spin);
>  }
>
> @@ -1276,9 +1274,7 @@ static int gdlm_mount(struct gfs2_sbd *sdp, const char *table)
>         }
>
>         ls->ls_first = !!test_bit(DFL_FIRST_MOUNT, &ls->ls_recover_flags);
> -       clear_bit(SDF_NOJOURNALID, &sdp->sd_flags);
> -       smp_mb__after_atomic();
> -       wake_up_bit(&sdp->sd_flags, SDF_NOJOURNALID);
> +       clear_and_wake_up_bit(SDF_NOJOURNALID, &sdp->sd_flags);
>         return 0;
>
>  fail_release:
> diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
> index 2299a3fa1911..ef9a8ef6689b 100644
> --- a/fs/gfs2/recovery.c
> +++ b/fs/gfs2/recovery.c
> @@ -436,9 +436,7 @@ void gfs2_recover_func(struct work_struct *work)
>         jd->jd_recover_error = error;
>         gfs2_recovery_done(sdp, jd->jd_jid, LM_RD_GAVEUP);
>  done:
> -       clear_bit(JDF_RECOVERY, &jd->jd_flags);
> -       smp_mb__after_atomic();
> -       wake_up_bit(&jd->jd_flags, JDF_RECOVERY);
> +       clear_and_wake_up_bit(JDF_RECOVERY, &jd->jd_flags);
>  }
>
>  int gfs2_recover_journal(struct gfs2_jdesc *jd, bool wait)
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index b70cea5c8c59..28ff2e10bdd9 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -985,6 +985,7 @@ void gfs2_freeze_func(struct work_struct *work)
>         }
>         deactivate_super(sb);
>         clear_bit_unlock(SDF_FS_FROZEN, &sdp->sd_flags);
> +       smp_mb__after_atomic();
>         wake_up_bit(&sdp->sd_flags, SDF_FS_FROZEN);
>         return;
>  }
> diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
> index 159aedf63c2a..b337798d8957 100644
> --- a/fs/gfs2/sys.c
> +++ b/fs/gfs2/sys.c
> @@ -505,9 +505,7 @@ static ssize_t jid_store(struct gfs2_sbd *sdp, const char *buf, size_t len)
>         if (sdp->sd_args.ar_spectator && jid > 0)
>                 rv = jid = -EINVAL;
>         sdp->sd_lockstruct.ls_jid = jid;
> -       clear_bit(SDF_NOJOURNALID, &sdp->sd_flags);
> -       smp_mb__after_atomic();
> -       wake_up_bit(&sdp->sd_flags, SDF_NOJOURNALID);
> +       clear_and_wake_up_bit(SDF_NOJOURNALID, &sdp->sd_flags);
>  out:
>         spin_unlock(&sdp->sd_jindex_spin);
>         return rv ? rv : len;
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index e2e3c4f04d3e..a3ed413a38c5 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -1196,9 +1196,7 @@ static int nfs4_run_state_manager(void *);
>  static void nfs4_clear_state_manager_bit(struct nfs_client *clp)
>  {
>         smp_mb__before_atomic();
> -       clear_bit(NFS4CLNT_MANAGER_RUNNING, &clp->cl_state);
> -       smp_mb__after_atomic();
> -       wake_up_bit(&clp->cl_state, NFS4CLNT_MANAGER_RUNNING);
> +       clear_and_wake_up_bit(NFS4CLNT_MANAGER_RUNNING, &clp->cl_state);
>         rpc_wake_up(&clp->cl_rpcwaitq);
>  }
>
> diff --git a/fs/nfs/pnfs_nfs.c b/fs/nfs/pnfs_nfs.c
> index c0046c348910..37c4fe5a595c 100644
> --- a/fs/nfs/pnfs_nfs.c
> +++ b/fs/nfs/pnfs_nfs.c
> @@ -567,9 +567,7 @@ static void nfs4_wait_ds_connect(struct nfs4_pnfs_ds *ds)
>  static void nfs4_clear_ds_conn_bit(struct nfs4_pnfs_ds *ds)
>  {
>         smp_mb__before_atomic();
> -       clear_bit(NFS4DS_CONNECTING, &ds->ds_state);
> -       smp_mb__after_atomic();
> -       wake_up_bit(&ds->ds_state, NFS4DS_CONNECTING);
> +       clear_and_wake_up_bit(NFS4DS_CONNECTING, &ds->ds_state);
>  }
>
>  static struct nfs_client *(*get_v3_ds_connect)(
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index 87679557d0d6..0a30f9cdf66a 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -1638,9 +1638,7 @@ static void
>  nfsd4_cltrack_upcall_unlock(struct nfs4_client *clp)
>  {
>         smp_mb__before_atomic();
> -       clear_bit(NFSD4_CLIENT_UPCALL_LOCK, &clp->cl_flags);
> -       smp_mb__after_atomic();
> -       wake_up_bit(&clp->cl_flags, NFSD4_CLIENT_UPCALL_LOCK);
> +       clear_and_wake_up_bit(NFSD4_CLIENT_UPCALL_LOCK, &clp->cl_flags);
>  }
>
>  static void
> diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
> index a35c17017210..1958bb457143 100644
> --- a/fs/orangefs/file.c
> +++ b/fs/orangefs/file.c
> @@ -302,7 +302,7 @@ int orangefs_revalidate_mapping(struct inode *inode)
>         orangefs_inode->mapping_time = jiffies +
>             orangefs_cache_timeout_msecs*HZ/1000;
>
> -       clear_bit(1, bitlock);
> +       clear_bit_unlock(1, bitlock);
>         smp_mb__after_atomic();
>         wake_up_bit(bitlock, 1);
>
> diff --git a/kernel/sched/wait_bit.c b/kernel/sched/wait_bit.c
> index 45eba18a2898..2662975473e3 100644
> --- a/kernel/sched/wait_bit.c
> +++ b/kernel/sched/wait_bit.c
> @@ -189,6 +189,7 @@ EXPORT_SYMBOL(init_wait_var_entry);
>
>  void wake_up_var(void *var)
>  {
> +       smp_mb();
>         __wake_up_bit(__var_waitqueue(var), var, -1);
>  }
>  EXPORT_SYMBOL(wake_up_var);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 9e4fcf406d9c..4e57f1a1b7a2 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -50,9 +50,7 @@ static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff *skb)
>         if (status)
>                 return;
>
> -       clear_bit(HCI_INQUIRY, &hdev->flags);
> -       smp_mb__after_atomic(); /* wake_up_bit advises about this barrier */
> -       wake_up_bit(&hdev->flags, HCI_INQUIRY);
> +       clear_and_wake_up_bit(HCI_INQUIRY, &hdev->flags);
>
>         hci_dev_lock(hdev);
>         /* Set discovery state to stopped if we're not doing LE active
> @@ -2342,7 +2340,6 @@ static void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
>         if (!test_and_clear_bit(HCI_INQUIRY, &hdev->flags))
>                 return;
>
> -       smp_mb__after_atomic(); /* wake_up_bit advises about this barrier */
>         wake_up_bit(&hdev->flags, HCI_INQUIRY);
>
>         if (!hci_dev_test_flag(hdev, HCI_MGMT))
> diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
> index 3cae88cbdaa0..2c2773172964 100644
> --- a/net/rds/ib_recv.c
> +++ b/net/rds/ib_recv.c
> @@ -363,6 +363,7 @@ static int acquire_refill(struct rds_connection *conn)
>  static void release_refill(struct rds_connection *conn)
>  {
>         clear_bit(RDS_RECV_REFILL, &conn->c_flags);
> +       smp_mb__after_atomic();
>
>         /* We don't use wait_on_bit()/wake_up_bit() because our waking is in a
>          * hot path and finding waiters is very rare.  We don't want to walk
> diff --git a/security/keys/gc.c b/security/keys/gc.c
> index 44e58a3e5663..f452816bd7d0 100644
> --- a/security/keys/gc.c
> +++ b/security/keys/gc.c
> @@ -308,9 +308,8 @@ static void key_garbage_collector(struct work_struct *work)
>
>         if (unlikely(gc_state & KEY_GC_REAPING_DEAD_3)) {
>                 kdebug("dead wake");
> -               smp_mb();
> -               clear_bit(KEY_GC_REAPING_KEYTYPE, &key_gc_flags);
> -               wake_up_bit(&key_gc_flags, KEY_GC_REAPING_KEYTYPE);
> +               smp_mb__before_atomic();
> +               clear_and_wake_up_bit(KEY_GC_REAPING_KEYTYPE, &key_gc_flags);
>         }
>
>         if (gc_state & KEY_GC_REAP_AGAIN)

Thanks,
Andreas

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>

^ permalink raw reply

* Re: [RFC][PATCH] wake_up_var() memory ordering
From: Peter Zijlstra @ 2019-06-25  8:11 UTC (permalink / raw)
  To: David Howells
  Cc: Martin Brandenburg, Mike Snitzer,
	linux-aio-Bw31MaZKKs3YtjvyW6yDsg, David Airlie,
	samba-technical-w/Ol4Ecudpl8XjKLYN78aQ, Joonas Lahtinen,
	Will Deacon, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	J. Bruce Fields, Chris Mason, dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
	keyrings-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar,
	linux-afs-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Alasdair Kergon,
	Mike Marshall, linux-cifs-u79uwXL29TY76Z2rM5mHXA,
	rds-devel-N0ozoZBvEnrZJqsBc5GL+g, Andreas Gruenbacher,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA, James Morris,
	cluster-devel-H+wXaHxf7aLQT0dZR+AlfA, Antti Palosaari,
	Matthias Brugger, Paul McKenney,
	intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, devel
In-Reply-To: <32379.1561449061-S6HVgzuS8uM4Awkfq6JHfwNdhmdF6hFW@public.gmane.org>


(sorry for cross-posting to moderated lists btw, I've since
 acquired a patch to get_maintainers.pl that wil exclude them
 in the future)

On Tue, Jun 25, 2019 at 08:51:01AM +0100, David Howells wrote:
> Peter Zijlstra <peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:
> 
> > I tried using wake_up_var() today and accidentally noticed that it
> > didn't imply an smp_mb() and specifically requires it through
> > wake_up_bit() / waitqueue_active().
> 
> Thinking about it again, I'm not sure why you need to add the barrier when
> wake_up() (which this is a wrapper around) is required to impose a barrier at
> the front if there's anything to wake up (ie. the wait queue isn't empty).
> 
> If this is insufficient, does it make sense just to have wake_up*() functions
> do an unconditional release or full barrier right at the front, rather than it
> being conditional on something being woken up?

The curprit is __wake_up_bit()'s usage of waitqueue_active(); it is this
latter (see its comment) that requires the smp_mb().

wake_up_bit() and wake_up_var() are wrappers around __wake_up_bit().

Without this barrier it is possible for the waitqueue_active() load to
be hoisted over the cond=true store and the remote end can miss the
store and we can miss its enqueue and we'll all miss a wakeup and get
stuck.

Adding an smp_mb() (or use wq_has_sleeper()) in __wake_up_bit() would be
nice, but I fear some people will complain about overhead, esp. since
about half the sites don't need the barrier due to being behind
test_and_clear_bit() and the other half using smp_mb__after_atomic()
after some clear_bit*() variant.

There's a few sites that seem to open-code
wait_var_event()/wake_up_var() and those actually need the full
smp_mb(), but then maybe they should be converted to var instread of bit
anyway.

> > @@ -619,9 +614,7 @@ static int dvb_usb_fe_sleep(struct dvb_frontend *fe)
> >  err:
> >  	if (!adap->suspend_resume_active) {
> >  		adap->active_fe = -1;
> 
> I'm wondering if there's a missing barrier here.  Should the clear_bit() on
> the next line be clear_bit_unlock() or clear_bit_release()?

That looks reasonable, but I'd like to hear from the DVB folks on that.

> > -		clear_bit(ADAP_SLEEP, &adap->state_bits);
> > -		smp_mb__after_atomic();
> > -		wake_up_bit(&adap->state_bits, ADAP_SLEEP);
> > +		clear_and_wake_up_bit(ADAP_SLEEP, &adap->state_bits);
> >  	}
> >  
> >  	dev_dbg(&d->udev->dev, "%s: ret=%d\n", __func__, ret);
> > diff --git a/fs/afs/fs_probe.c b/fs/afs/fs_probe.c
> > index cfe62b154f68..377ee07d5f76 100644
> > --- a/fs/afs/fs_probe.c
> > +++ b/fs/afs/fs_probe.c
> > @@ -18,6 +18,7 @@ static bool afs_fs_probe_done(struct afs_server *server)
> >  
> >  	wake_up_var(&server->probe_outstanding);
> >  	clear_bit_unlock(AFS_SERVER_FL_PROBING, &server->flags);
> > +	smp_mb__after_atomic();
> >  	wake_up_bit(&server->flags, AFS_SERVER_FL_PROBING);
> >  	return true;
> >  }
> 
> Looking at this and the dvb one, does it make sense to stick the release
> semantics of clear_bit_unlock() into clear_and_wake_up_bit()?

I was thinking of adding another helper, maybe unlock_and_wake_up_bit()
that included that extra barrier, but maybe making it unconditional
isn't the worst idea.

> Also, should clear_bit_unlock() be renamed to clear_bit_release() (and
> similarly test_and_set_bit_lock() -> test_and_set_bit_acquire()) if we seem to
> be trying to standardise on that terminology.

That definitely makes sense to me, there's only 157 clear_bit_unlock()
and 76 test_and_set_bit_lock() users (note the asymetry of that).

^ permalink raw reply

* Re: [RFC][PATCH] wake_up_var() memory ordering
From: David Howells @ 2019-06-25  7:51 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Martin Brandenburg, linux-cachefs, Mike Snitzer, linux-aio,
	David Airlie, samba-technical, Will Deacon, dri-devel, dhowells,
	Chris Mason, dm-devel, keyrings, Ingo Molnar, linux-afs,
	Alasdair Kergon, Mike Marshall, linux-cifs, rds-devel,
	Andreas Gruenbacher, linux-rdma, James Morris, cluster-devel,
	Matthias Brugger, Paul McKenney, intel-gfx, devel,
	Serge E. Hallyn, Santosh Shilimkar <santosh.s>
In-Reply-To: <20190624165012.GH3436@hirez.programming.kicks-ass.net>

Peter Zijlstra <peterz@infradead.org> wrote:

> I tried using wake_up_var() today and accidentally noticed that it
> didn't imply an smp_mb() and specifically requires it through
> wake_up_bit() / waitqueue_active().

Thinking about it again, I'm not sure why you need to add the barrier when
wake_up() (which this is a wrapper around) is required to impose a barrier at
the front if there's anything to wake up (ie. the wait queue isn't empty).

If this is insufficient, does it make sense just to have wake_up*() functions
do an unconditional release or full barrier right at the front, rather than it
being conditional on something being woken up?

> @@ -619,9 +614,7 @@ static int dvb_usb_fe_sleep(struct dvb_frontend *fe)
>  err:
>  	if (!adap->suspend_resume_active) {
>  		adap->active_fe = -1;

I'm wondering if there's a missing barrier here.  Should the clear_bit() on
the next line be clear_bit_unlock() or clear_bit_release()?

> -		clear_bit(ADAP_SLEEP, &adap->state_bits);
> -		smp_mb__after_atomic();
> -		wake_up_bit(&adap->state_bits, ADAP_SLEEP);
> +		clear_and_wake_up_bit(ADAP_SLEEP, &adap->state_bits);
>  	}
>  
>  	dev_dbg(&d->udev->dev, "%s: ret=%d\n", __func__, ret);
> diff --git a/fs/afs/fs_probe.c b/fs/afs/fs_probe.c
> index cfe62b154f68..377ee07d5f76 100644
> --- a/fs/afs/fs_probe.c
> +++ b/fs/afs/fs_probe.c
> @@ -18,6 +18,7 @@ static bool afs_fs_probe_done(struct afs_server *server)
>  
>  	wake_up_var(&server->probe_outstanding);
>  	clear_bit_unlock(AFS_SERVER_FL_PROBING, &server->flags);
> +	smp_mb__after_atomic();
>  	wake_up_bit(&server->flags, AFS_SERVER_FL_PROBING);
>  	return true;
>  }

Looking at this and the dvb one, does it make sense to stick the release
semantics of clear_bit_unlock() into clear_and_wake_up_bit()?

Also, should clear_bit_unlock() be renamed to clear_bit_release() (and
similarly test_and_set_bit_lock() -> test_and_set_bit_acquire()) if we seem to
be trying to standardise on that terminology.

David
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply

* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Christoph Hellwig @ 2019-06-25  7:20 UTC (permalink / raw)
  To: Logan Gunthorpe
  Cc: Christoph Hellwig, linux-kernel, linux-block, linux-nvme,
	linux-pci, linux-rdma, Jens Axboe, Bjorn Helgaas, Dan Williams,
	Sagi Grimberg, Keith Busch, Jason Gunthorpe, Stephen Bates
In-Reply-To: <558a27ba-e7c9-9d94-cad0-377b8ee374a6@deltatee.com>

On Mon, Jun 24, 2019 at 10:07:56AM -0600, Logan Gunthorpe wrote:
> > For one passing a dma_addr_t through the block layer is a layering
> > violation, and one that I think will also bite us in practice.
> > The host physical to PCIe bus address mapping can have offsets, and
> > those offsets absolutely can be different for differnet root ports.
> > So with your caller generated dma_addr_t everything works fine with
> > a switched setup as the one you are probably testing on, but on a
> > sufficiently complicated setup with multiple root ports it can break.
> 
> I don't follow this argument. Yes, I understand PCI Bus offsets and yes
> I understand that they only apply beyond the bus they're working with.
> But this isn't *that* complicated and it should be the responsibility of
> the P2PDMA code to sort out and provide a dma_addr_t for. The dma_addr_t
> that's passed through the block layer could be a bus address or it could
> be the result of a dma_map_* request (if the transaction is found to go
> through an RC) depending on the requirements of the devices being used.

You assume all addressing is done by the PCI bus address.  If a device
is addressing its own BAR there is no reason to use the PCI bus address,
as it might have much more intelligent schemes (usually bar + offset).
> 
> > Also duplicating the whole block I/O stack, including hooks all over
> > the fast path is pretty much a no-go.
> 
> There was very little duplicate code in the patch set. (Really just the
> mapping code). There are a few hooks, but in practice not that many if
> we ignore the WARN_ONs. We might be able to work to reduce this further.
> The main hooks are: when we skip bouncing, when we skip integrity prep,
> when we split, and when we map. And the patchset drops the PCI_P2PDMA
> hook when we map. So we're talking about maybe three or four extra ifs
> that would likely normally be fast due to the branch predictor.

And all of those add code to the block layer fast path.

^ permalink raw reply

* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Christoph Hellwig @ 2019-06-25  7:18 UTC (permalink / raw)
  To: Logan Gunthorpe
  Cc: Jason Gunthorpe, Christoph Hellwig, Dan Williams,
	Linux Kernel Mailing List, linux-block, linux-nvme, linux-pci,
	linux-rdma, Jens Axboe, Bjorn Helgaas, Sagi Grimberg, Keith Busch,
	Stephen Bates
In-Reply-To: <1041d2c6-f22c-81f2-c141-fb821b35c0c1@deltatee.com>

On Mon, Jun 24, 2019 at 10:10:16AM -0600, Logan Gunthorpe wrote:
> Yes, that's correct. The intent was to invert it so the dma_map could
> happen at the start of the process so that P2PDMA code could be called
> with all the information it needs to make it's decision on how to map;
> without having to hook into the mapping process of every driver that
> wants to participate.

And that just isn't how things work in layering.  We need to keep
generating the dma addresses in the driver in the receiving end, as
there are all kinds of interesting ideas how we do that.  E.g. for the
Mellanox NICs addressing their own bars is not done by PCIe bus
addresses but by relative offsets.  And while NVMe has refused to go
down that route in the current band aid fix for CMB addressing I suspect
it will sooner or later have to do the same to deal with the addressing
problems in a multiple PASID world.

^ permalink raw reply

* Re: [PATCH v4 hmm 11/12] mm/hmm: Remove confusing comment and logic from hmm_release
From: Christoph Hellwig @ 2019-06-25  7:03 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Andrea Arcangeli, Philip Yang, Ralph Campbell,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA, John Hubbard,
	Felix.Kuehling-5C7GfCeVMHo,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jerome Glisse, Jason Gunthorpe,
	amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Ira Weiny,
	Christoph Hellwig, Ben Skeggs
In-Reply-To: <20190624210110.5098-12-jgg-uk2M96/98Pc@public.gmane.org>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox