[PATCH net v4 1/2] net/mlx5e: Update hw flows when encap source mac changed

[PATCH net v4 1/2] net/mlx5e: Update hw flows when encap source mac changed

[xiangxia.m.yue]

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

When we offload tc filters to hardware, hardware flows can
be updated when mac of encap destination ip is changed.
But we ignore one case, that the mac of local encap ip can
be changed too, so we should also update them.

To fix it, add route_dev in mlx5e_encap_entry struct to save
the local encap netdevice, and when mac changed, kernel will
flush all the neighbour on the netdevice and send NETEVENT_NEIGH_UPDATE
event. The mlx5 driver will delete the flows and add them when neighbour
available again.

Fixes: 232c001398ae ("net/mlx5e: Add support to neighbour update flow")
Cc: Hadar Hen Zion <hadarh@mellanox.com>
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Reviewed-by: Or Gerlitz <ogerlitz@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c | 2 ++
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c    | 4 ++++
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.h    | 1 +
 3 files changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
index 046948e..114e0a2 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
@@ -256,6 +256,7 @@ int mlx5e_tc_tun_create_header_ipv4(struct mlx5e_priv *priv,
 	e->m_neigh.family = n->ops->family;
 	memcpy(&e->m_neigh.dst_ip, n->primary_key, n->tbl->key_len);
 	e->out_dev = out_dev;
+	e->route_dev = route_dev;
 
 	/* It's important to add the neigh to the hash table before checking
 	 * the neigh validity state. So if we'll get a notification, in case the
@@ -369,6 +370,7 @@ int mlx5e_tc_tun_create_header_ipv6(struct mlx5e_priv *priv,
 	e->m_neigh.family = n->ops->family;
 	memcpy(&e->m_neigh.dst_ip, n->primary_key, n->tbl->key_len);
 	e->out_dev = out_dev;
+	e->route_dev = route_dev;
 
 	/* It's importent to add the neigh to the hash table before checking
 	 * the neigh validity state. So if we'll get a notification, in case the
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
index 96cc0c6..c4b9073 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
@@ -594,6 +594,10 @@ static void mlx5e_rep_update_flows(struct mlx5e_priv *priv,
 	if (neigh_connected && !(e->flags & MLX5_ENCAP_ENTRY_VALID)) {
 		ether_addr_copy(e->h_dest, ha);
 		ether_addr_copy(eth->h_dest, ha);
+		/* Update the encap source mac, in case that we delete
+		 * the flows when encap source mac changed.
+		 */
+		ether_addr_copy(eth->h_source, e->route_dev->dev_addr);
 
 		mlx5e_tc_encap_flows_add(priv, e);
 	}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
index edd7228..36eafc8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
@@ -148,6 +148,7 @@ struct mlx5e_encap_entry {
 	unsigned char h_dest[ETH_ALEN];	/* destination eth addr	*/
 
 	struct net_device *out_dev;
+	struct net_device *route_dev;
 	int tunnel_type;
 	int tunnel_hlen;
 	int reformat_type;
-- 
1.8.3.1

[PATCH net v4 2/2] net/mlx5e: Don't overwrite pedit action when multiple pedit used

[xiangxia.m.yue]

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

In some case, we may use multiple pedit actions to modify packets.
The command shown as below: the last pedit action is effective.

$ tc filter add dev netdev_rep parent ffff: protocol ip prio 1	\
	flower skip_sw ip_proto icmp dst_ip 3.3.3.3		\
	action pedit ex munge ip dst set 192.168.1.100 pipe	\
	action pedit ex munge eth src set 00:00:00:00:00:01 pipe	\
	action pedit ex munge eth dst set 00:00:00:00:00:02 pipe	\
	action csum ip pipe	\
	action tunnel_key set src_ip 1.1.1.100 dst_ip 1.1.1.200 dst_port 4789 id 100 \
	action mirred egress redirect dev vxlan0

To fix it, we add max_mod_hdr_actions to mlx5e_tc_flow_parse_attr struction,
max_mod_hdr_actions will store the max pedit action number we support and
num_mod_hdr_actions indicates how many pedit action we used, and store all
pedit action to mod_hdr_actions.

Fixes: d79b6df6b10a ("net/mlx5e: Add parsing of TC pedit actions to HW format")
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
v2: Fix comment message and change tag from net-next to net.
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 26 +++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index cae6c6d..833a29a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -128,6 +128,7 @@ struct mlx5e_tc_flow_parse_attr {
 	struct net_device *filter_dev;
 	struct mlx5_flow_spec spec;
 	int num_mod_hdr_actions;
+	int max_mod_hdr_actions;
 	void *mod_hdr_actions;
 	int mirred_ifindex[MLX5_MAX_FLOW_FWD_VPORTS];
 };
@@ -1934,9 +1935,9 @@ struct mlx5_fields {
 	OFFLOAD(UDP_DPORT, 2, udp.dest,   0),
 };
 
-/* On input attr->num_mod_hdr_actions tells how many HW actions can be parsed at
- * max from the SW pedit action. On success, it says how many HW actions were
- * actually parsed.
+/* On input attr->max_mod_hdr_actions tells how many HW actions can be parsed at
+ * max from the SW pedit action. On success, attr->num_mod_hdr_actions
+ * says how many HW actions were actually parsed.
  */
 static int offload_pedit_fields(struct pedit_headers *masks,
 				struct pedit_headers *vals,
@@ -1960,9 +1961,11 @@ static int offload_pedit_fields(struct pedit_headers *masks,
 	add_vals = &vals[TCA_PEDIT_KEY_EX_CMD_ADD];
 
 	action_size = MLX5_UN_SZ_BYTES(set_action_in_add_action_in_auto);
-	action = parse_attr->mod_hdr_actions;
-	max_actions = parse_attr->num_mod_hdr_actions;
-	nactions = 0;
+	action = parse_attr->mod_hdr_actions +
+		 parse_attr->num_mod_hdr_actions * action_size;
+
+	max_actions = parse_attr->max_mod_hdr_actions;
+	nactions = parse_attr->num_mod_hdr_actions;
 
 	for (i = 0; i < ARRAY_SIZE(fields); i++) {
 		f = &fields[i];
@@ -2073,7 +2076,8 @@ static int alloc_mod_hdr_actions(struct mlx5e_priv *priv,
 	if (!parse_attr->mod_hdr_actions)
 		return -ENOMEM;
 
-	parse_attr->num_mod_hdr_actions = max_actions;
+	parse_attr->max_mod_hdr_actions = max_actions;
+	parse_attr->num_mod_hdr_actions = 0;
 	return 0;
 }
 
@@ -2119,9 +2123,11 @@ static int parse_tc_pedit_action(struct mlx5e_priv *priv,
 			goto out_err;
 	}
 
-	err = alloc_mod_hdr_actions(priv, a, namespace, parse_attr);
-	if (err)
-		goto out_err;
+	if (!parse_attr->mod_hdr_actions) {
+		err = alloc_mod_hdr_actions(priv, a, namespace, parse_attr);
+		if (err)
+			goto out_err;
+	}
 
 	err = offload_pedit_fields(masks, vals, parse_attr, extack);
 	if (err < 0)
-- 
1.8.3.1

Re: [PATCH net v4 1/2] net/mlx5e: Update hw flows when encap source mac changed

[Or Gerlitz]

On Sun, Jan 27, 2019 at 1:06 PM <xiangxia.m.yue@gmail.com> wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> When we offload tc filters to hardware, hardware flows can
> be updated when mac of encap destination ip is changed.
> But we ignore one case, that the mac of local encap ip can
> be changed too, so we should also update them.
>
> To fix it, add route_dev in mlx5e_encap_entry struct to save
> the local encap netdevice, and when mac changed, kernel will
> flush all the neighbour on the netdevice and send NETEVENT_NEIGH_UPDATE
> event. The mlx5 driver will delete the flows and add them when neighbour
> available again.
>
> Fixes: 232c001398ae ("net/mlx5e: Add support to neighbour update flow")
> Cc: Hadar Hen Zion <hadarh@mellanox.com>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> Reviewed-by: Or Gerlitz <ogerlitz@mellanox.com>

+1 again

this one is good to go upstream

Re: [PATCH net v4 2/2] net/mlx5e: Don't overwrite pedit action when multiple pedit used

[Or Gerlitz]

On Sun, Jan 27, 2019 at 1:06 PM <xiangxia.m.yue@gmail.com> wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> In some case, we may use multiple pedit actions to modify packets.
> The command shown as below: the last pedit action is effective.

> @@ -2073,7 +2076,8 @@ static int alloc_mod_hdr_actions(struct mlx5e_priv *priv,
>         if (!parse_attr->mod_hdr_actions)
>                 return -ENOMEM;
>
> -       parse_attr->num_mod_hdr_actions = max_actions;
> +       parse_attr->max_mod_hdr_actions = max_actions;
> +       parse_attr->num_mod_hdr_actions = 0;

why would we want to do this zeroing? what purpose does it serve?

On a probably related note, I suspect that the patch broke the caching
we do for modify header contexts, see mlx5e_attach_mod_hdr where we
look if a given set of modify header operations already has hw modify header
context and we use it.

To test that, put two tc rules with different matching but same set of
modify header
(pedit) actions and see that only one modify header context is used.

Re: [PATCH net v4 2/2] net/mlx5e: Don't overwrite pedit action when multiple pedit used

[Tonghao Zhang]

On Mon, Jan 28, 2019 at 12:40 AM Or Gerlitz <gerlitz.or@gmail.com> wrote:
>
> On Sun, Jan 27, 2019 at 1:06 PM <xiangxia.m.yue@gmail.com> wrote:
> > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >
> > In some case, we may use multiple pedit actions to modify packets.
> > The command shown as below: the last pedit action is effective.
>
> > @@ -2073,7 +2076,8 @@ static int alloc_mod_hdr_actions(struct mlx5e_priv *priv,
> >         if (!parse_attr->mod_hdr_actions)
> >                 return -ENOMEM;
> >
> > -       parse_attr->num_mod_hdr_actions = max_actions;
> > +       parse_attr->max_mod_hdr_actions = max_actions;
> > +       parse_attr->num_mod_hdr_actions = 0;
>
> why would we want to do this zeroing? what purpose does it serve?
Because we use the num_mod_hdr_actions to store the number of actions
we have parsed,
and when we alloc it, we init it 0 as default.

> On a probably related note, I suspect that the patch broke the caching
> we do for modify header contexts, see mlx5e_attach_mod_hdr where we
> look if a given set of modify header operations already has hw modify header
> context and we use it.
>
> To test that, put two tc rules with different matching but same set of
> modify header
> (pedit) actions and see that only one modify header context is used.
The patch does't break the cache, I think that different matching may
share the same set of
pedit actions.

I use the tc filters to test it: only one same pedit actions in hw,
and after deleting one tc filter, other filter work fine.

tc filter add dev eth4_0 parent ffff: protocol ip prio 1  \
        flower skip_sw ip_proto icmp dst_ip 3.3.3.3                     \
        action pedit ex munge ip dst set 192.168.1.100 pipe             \
        action pedit ex munge eth src set 00:00:00:00:00:01 pipe        \
        action pedit ex munge eth dst set 00:00:00:00:00:02 pipe        \
        action csum ip pipe                                             \
        action tunnel_key set src_ip 192.168.6.2 dst_ip 192.168.10.2
dst_port 4789 id 100 \
        action mirred egress redirect dev vxlan0

note: match 3.3.3.3, vxlan id 100

tc filter add dev eth4_0 parent ffff: protocol ip prio 1  \
        flower skip_sw ip_proto icmp dst_ip 3.3.3.4                     \
        action pedit ex munge ip dst set 192.168.1.100 pipe             \
        action pedit ex munge eth src set 00:00:00:00:00:01 pipe        \
        action pedit ex munge eth dst set 00:00:00:00:00:02 pipe        \
        action csum ip pipe                                             \
        action tunnel_key set src_ip 192.168.6.2 dst_ip 192.168.10.2
dst_port 4789 id 200 \
        action mirred egress redirect dev vxlan0

note: match 3.3.3.4, vxlan id 200

Re: [PATCH net v4 2/2] net/mlx5e: Don't overwrite pedit action when multiple pedit used

[Or Gerlitz]

On Mon, Jan 28, 2019 at 2:10 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> On Mon, Jan 28, 2019 at 12:40 AM Or Gerlitz <gerlitz.or@gmail.com> wrote:
> >
> > On Sun, Jan 27, 2019 at 1:06 PM <xiangxia.m.yue@gmail.com> wrote:
> > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > >
> > > In some case, we may use multiple pedit actions to modify packets.
> > > The command shown as below: the last pedit action is effective.
> >
> > > @@ -2073,7 +2076,8 @@ static int alloc_mod_hdr_actions(struct mlx5e_priv *priv,
> > >         if (!parse_attr->mod_hdr_actions)
> > >                 return -ENOMEM;
> > >
> > > -       parse_attr->num_mod_hdr_actions = max_actions;
> > > +       parse_attr->max_mod_hdr_actions = max_actions;
> > > +       parse_attr->num_mod_hdr_actions = 0;
> >
> > why would we want to do this zeroing? what purpose does it serve?
> Because we use the num_mod_hdr_actions to store the number of actions
> we have parsed,
> and when we alloc it, we init it 0 as default.
>
> > On a probably related note, I suspect that the patch broke the caching
> > we do for modify header contexts, see mlx5e_attach_mod_hdr where we
> > look if a given set of modify header operations already has hw modify header
> > context and we use it.
> >
> > To test that, put two tc rules with different matching but same set of
> > modify header
> > (pedit) actions and see that only one modify header context is used.

> The patch does't break the cache, I think that different matching may
> share the same set of pedit actions.

I suspect it does break it.. at [1]  we have this code for the cache lookup:

num_actions  = parse_attr->num_mod_hdr_actions;
[..]
key.actions = parse_attr->mod_hdr_actions;
key.num_actions = num_actions;

hash_key = hash_mod_hdr_info(&key);

so we are doing the cached insertion and lookup with
parse_attr->num_mod_hdr_actions
which was zeroed along the way and not accounting for the full set of
pedit actions, agree?

[1] https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c#L179

Re: [PATCH net v4 2/2] net/mlx5e: Don't overwrite pedit action when multiple pedit used

[Tonghao Zhang]

On Mon, Jan 28, 2019 at 11:34 PM Or Gerlitz <gerlitz.or@gmail.com> wrote:
>
> On Mon, Jan 28, 2019 at 2:10 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > On Mon, Jan 28, 2019 at 12:40 AM Or Gerlitz <gerlitz.or@gmail.com> wrote:
> > >
> > > On Sun, Jan 27, 2019 at 1:06 PM <xiangxia.m.yue@gmail.com> wrote:
> > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > > >
> > > > In some case, we may use multiple pedit actions to modify packets.
> > > > The command shown as below: the last pedit action is effective.
> > >
> > > > @@ -2073,7 +2076,8 @@ static int alloc_mod_hdr_actions(struct mlx5e_priv *priv,
> > > >         if (!parse_attr->mod_hdr_actions)
> > > >                 return -ENOMEM;
> > > >
> > > > -       parse_attr->num_mod_hdr_actions = max_actions;
> > > > +       parse_attr->max_mod_hdr_actions = max_actions;
> > > > +       parse_attr->num_mod_hdr_actions = 0;
> > >
> > > why would we want to do this zeroing? what purpose does it serve?
> > Because we use the num_mod_hdr_actions to store the number of actions
> > we have parsed,
> > and when we alloc it, we init it 0 as default.
> >
> > > On a probably related note, I suspect that the patch broke the caching
> > > we do for modify header contexts, see mlx5e_attach_mod_hdr where we
> > > look if a given set of modify header operations already has hw modify header
> > > context and we use it.
> > >
> > > To test that, put two tc rules with different matching but same set of
> > > modify header
> > > (pedit) actions and see that only one modify header context is used.
>
> > The patch does't break the cache, I think that different matching may
> > share the same set of pedit actions.
>
> I suspect it does break it.. at [1]  we have this code for the cache lookup:
>
> num_actions  = parse_attr->num_mod_hdr_actions;
> [..]
> key.actions = parse_attr->mod_hdr_actions;
> key.num_actions = num_actions;
>
> hash_key = hash_mod_hdr_info(&key);
>
> so we are doing the cached insertion and lookup with
> parse_attr->num_mod_hdr_actions
> which was zeroed along the way and not accounting for the full set of
> pedit actions, agree?
not really, before calling the  mlx5e_attach_mod_hdr,  we have call
the offload_pedit_fields that will
update the attr->num_mod_hdr_actions that indicate  how many pedit
action we parsed.

> [1] https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c#L179

Re: [PATCH net v4 2/2] net/mlx5e: Don't overwrite pedit action when multiple pedit used

[Or Gerlitz]

On Mon, Jan 28, 2019 at 6:18 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> On Mon, Jan 28, 2019 at 11:34 PM Or Gerlitz <gerlitz.or@gmail.com> wrote:
> > On Mon, Jan 28, 2019 at 2:10 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > > On Mon, Jan 28, 2019 at 12:40 AM Or Gerlitz <gerlitz.or@gmail.com> wrote:
> > > >
> > > > On Sun, Jan 27, 2019 at 1:06 PM <xiangxia.m.yue@gmail.com> wrote:
> > > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > > > >
> > > > > In some case, we may use multiple pedit actions to modify packets.
> > > > > The command shown as below: the last pedit action is effective.
> > > >
> > > > > @@ -2073,7 +2076,8 @@ static int alloc_mod_hdr_actions(struct mlx5e_priv *priv,
> > > > >         if (!parse_attr->mod_hdr_actions)
> > > > >                 return -ENOMEM;
> > > > >
> > > > > -       parse_attr->num_mod_hdr_actions = max_actions;
> > > > > +       parse_attr->max_mod_hdr_actions = max_actions;
> > > > > +       parse_attr->num_mod_hdr_actions = 0;
> > > >
> > > > why would we want to do this zeroing? what purpose does it serve?
> > > Because we use the num_mod_hdr_actions to store the number of actions
> > > we have parsed,
> > > and when we alloc it, we init it 0 as default.
> > >
> > > > On a probably related note, I suspect that the patch broke the caching
> > > > we do for modify header contexts, see mlx5e_attach_mod_hdr where we
> > > > look if a given set of modify header operations already has hw modify header
> > > > context and we use it.
> > > >
> > > > To test that, put two tc rules with different matching but same set of
> > > > modify header
> > > > (pedit) actions and see that only one modify header context is used.
> >
> > > The patch does't break the cache, I think that different matching may
> > > share the same set of pedit actions.
> >
> > I suspect it does break it.. at [1]  we have this code for the cache lookup:
> >
> > num_actions  = parse_attr->num_mod_hdr_actions;
> > [..]
> > key.actions = parse_attr->mod_hdr_actions;
> > key.num_actions = num_actions;
> >
> > hash_key = hash_mod_hdr_info(&key);
> >
> > so we are doing the cached insertion and lookup with
> > parse_attr->num_mod_hdr_actions
> > which was zeroed along the way and not accounting for the full set of
> > pedit actions, agree?

> not really, before calling the  mlx5e_attach_mod_hdr,  we have call
> the offload_pedit_fields that will
> update the attr->num_mod_hdr_actions that indicate  how many pedit
> action we parsed.

ok, got you, so why do we need this line

 parse_attr->num_mod_hdr_actions = 0;

in alloc_mod_hdr_actions()? this should be zero
by kzalloc somewhere, it got to confuse me..

I suggest to remove this zeroing, otherwise the patch LGTM, once you fix it

Reviewed-by: Or Gerlitz <ogerlitz@mellanox.com>

Re: [PATCH net v4 2/2] net/mlx5e: Don't overwrite pedit action when multiple pedit used

[Tonghao Zhang]

On Tue, Jan 29, 2019 at 11:44 PM Or Gerlitz <gerlitz.or@gmail.com> wrote:
>
> On Mon, Jan 28, 2019 at 6:18 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > On Mon, Jan 28, 2019 at 11:34 PM Or Gerlitz <gerlitz.or@gmail.com> wrote:
> > > On Mon, Jan 28, 2019 at 2:10 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > > > On Mon, Jan 28, 2019 at 12:40 AM Or Gerlitz <gerlitz.or@gmail.com> wrote:
> > > > >
> > > > > On Sun, Jan 27, 2019 at 1:06 PM <xiangxia.m.yue@gmail.com> wrote:
> > > > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > > > > >
> > > > > > In some case, we may use multiple pedit actions to modify packets.
> > > > > > The command shown as below: the last pedit action is effective.
> > > > >
> > > > > > @@ -2073,7 +2076,8 @@ static int alloc_mod_hdr_actions(struct mlx5e_priv *priv,
> > > > > >         if (!parse_attr->mod_hdr_actions)
> > > > > >                 return -ENOMEM;
> > > > > >
> > > > > > -       parse_attr->num_mod_hdr_actions = max_actions;
> > > > > > +       parse_attr->max_mod_hdr_actions = max_actions;
> > > > > > +       parse_attr->num_mod_hdr_actions = 0;
> > > > >
> > > > > why would we want to do this zeroing? what purpose does it serve?
> > > > Because we use the num_mod_hdr_actions to store the number of actions
> > > > we have parsed,
> > > > and when we alloc it, we init it 0 as default.
> > > >
> > > > > On a probably related note, I suspect that the patch broke the caching
> > > > > we do for modify header contexts, see mlx5e_attach_mod_hdr where we
> > > > > look if a given set of modify header operations already has hw modify header
> > > > > context and we use it.
> > > > >
> > > > > To test that, put two tc rules with different matching but same set of
> > > > > modify header
> > > > > (pedit) actions and see that only one modify header context is used.
> > >
> > > > The patch does't break the cache, I think that different matching may
> > > > share the same set of pedit actions.
> > >
> > > I suspect it does break it.. at [1]  we have this code for the cache lookup:
> > >
> > > num_actions  = parse_attr->num_mod_hdr_actions;
> > > [..]
> > > key.actions = parse_attr->mod_hdr_actions;
> > > key.num_actions = num_actions;
> > >
> > > hash_key = hash_mod_hdr_info(&key);
> > >
> > > so we are doing the cached insertion and lookup with
> > > parse_attr->num_mod_hdr_actions
> > > which was zeroed along the way and not accounting for the full set of
> > > pedit actions, agree?
>
> > not really, before calling the  mlx5e_attach_mod_hdr,  we have call
> > the offload_pedit_fields that will
> > update the attr->num_mod_hdr_actions that indicate  how many pedit
> > action we parsed.
>
> ok, got you, so why do we need this line
>
>  parse_attr->num_mod_hdr_actions = 0;
>
> in alloc_mod_hdr_actions()? this should be zero
> by kzalloc somewhere, it got to confuse me..
yes, should be removed
> I suggest to remove this zeroing, otherwise the patch LGTM, once you fix it
>
> Reviewed-by: Or Gerlitz <ogerlitz@mellanox.com>