| Message ID | 5fd15672173653d6904333ef197b605b0644e205.1689064922.git.leonro@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | mlx5 IPsec packet offload support in eswitch mode | expand |
On Tue, 11 Jul 2023 12:29:07 +0300 Leon Romanovsky wrote: > From: Jianbo Liu <jianbol@nvidia.com> > > The rule destination must be comprared with the old_dest passed in. > > Fixes: 74491de93712 ("net/mlx5: Add multi dest support") This says Fixes, should I quickly toss it into net so it makes tomorrow's PR? The commit message is pretty useless :(
On Wed, Jul 12, 2023 at 05:32:59PM -0700, Jakub Kicinski wrote: > On Tue, 11 Jul 2023 12:29:07 +0300 Leon Romanovsky wrote: > > From: Jianbo Liu <jianbol@nvidia.com> > > > > The rule destination must be comprared with the old_dest passed in. > > > > Fixes: 74491de93712 ("net/mlx5: Add multi dest support") > > This says Fixes, should I quickly toss it into net so it makes > tomorrow's PR? This is a fix, but it useful for this series only, which actually needs to modify flow steering rule destinations on the fly. There is no other code in mlx5 which needs this fix. Thanks
On Thu, 13 Jul 2023 09:33:45 +0300 Leon Romanovsky wrote: > > This says Fixes, should I quickly toss it into net so it makes > > tomorrow's PR? > > This is a fix, but it useful for this series only, which actually > needs to modify flow steering rule destinations on the fly. > > There is no other code in mlx5 which needs this fix. Reads like "can't be triggered with current code", in which case the right thing to do is to add "can't be triggered with current code" to the commit message, rather than the Fixes tag. I had a look thru the series yesterday, and it looks good to me (tho I'm no ipsec expert). Thanks for putting in the work! Could you add some info about how the code in the series can be exercised / example configurations? And please CC Simon, it'd be great to get him / someone at Corigine to review. And obviously Steffen, why did you not CC Steffen?! :o
On Thu, Jul 13, 2023 at 10:04:01AM -0700, Jakub Kicinski wrote: > On Thu, 13 Jul 2023 09:33:45 +0300 Leon Romanovsky wrote: > > > This says Fixes, should I quickly toss it into net so it makes > > > tomorrow's PR? > > > > This is a fix, but it useful for this series only, which actually > > needs to modify flow steering rule destinations on the fly. > > > > There is no other code in mlx5 which needs this fix. > > Reads like "can't be triggered with current code", in which case > the right thing to do is to add "can't be triggered with current > code" to the commit message, rather than the Fixes tag. The code is wrong, so comes Fixes line, but I can remove it. > > I had a look thru the series yesterday, and it looks good to me > (tho I'm no ipsec expert). Thanks for putting in the work! > > Could you add some info about how the code in the series can be > exercised / example configurations? And please CC Simon, it'd be > great to get him / someone at Corigine to review. > > And obviously Steffen, why did you not CC Steffen?! :o It works exactly like "regular" IPsec, nothing special, except now users can switch to switchdev before adding IPsec rules. devlink dev eswitch set pci/0000:06:00.0 mode switchdev Same configurations as here: https://lore.kernel.org/netdev/cover.1670005543.git.leonro@nvidia.com/ Packet offload mode: ip xfrm state offload packet dev <if-name> dir <in|out> ip xfrm policy .... offload packet dev <if-name> Crypto offload mode: ip xfrm state offload crypto dev <if-name> dir <in|out> or (backward compatibility) ip xfrm state offload dev <if-name> dir <in|out> I didn't add Steffen as it is more flow steering magic series and not IPsec :). I'll resubmit on Sunday. Thanks
On Thu, 13 Jul 2023 20:43:17 +0300 Leon Romanovsky wrote: > > Reads like "can't be triggered with current code", in which case > > the right thing to do is to add "can't be triggered with current > > code" to the commit message, rather than the Fixes tag. > > The code is wrong, so comes Fixes line, but I can remove it. Yes, perhaps after death we will inhabit a world with clear, non-conflicting rules, where law can be followed to the letter and "truth" and "good" are clearly and objectively defined. Until the sweat release, tho, let's apply common sense, and not add Fixes tags to patches which can't possibly be of interest to backporters. Please and thank you... > > I had a look thru the series yesterday, and it looks good to me > > (tho I'm no ipsec expert). Thanks for putting in the work! > > > > Could you add some info about how the code in the series can be > > exercised / example configurations? And please CC Simon, it'd be > > great to get him / someone at Corigine to review. > > > > And obviously Steffen, why did you not CC Steffen?! :o > > It works exactly like "regular" IPsec, nothing special, except > now users can switch to switchdev before adding IPsec rules. > > devlink dev eswitch set pci/0000:06:00.0 mode switchdev > > Same configurations as here: > https://lore.kernel.org/netdev/cover.1670005543.git.leonro@nvidia.com/ > Packet offload mode: > ip xfrm state offload packet dev <if-name> dir <in|out> > ip xfrm policy .... offload packet dev <if-name> > Crypto offload mode: > ip xfrm state offload crypto dev <if-name> dir <in|out> > or (backward compatibility) > ip xfrm state offload dev <if-name> dir <in|out> I see, so all policy based IPsec? Does the order of processing in the device match the kernel? TC packet rewrites or IPsec comes first? > I didn't add Steffen as it is more flow steering magic series > and not IPsec :). > > I'll resubmit on Sunday. Thanks!
On Thu, Jul 13, 2023 at 11:05:56AM -0700, Jakub Kicinski wrote: > On Thu, 13 Jul 2023 20:43:17 +0300 Leon Romanovsky wrote: > > > Reads like "can't be triggered with current code", in which case > > > the right thing to do is to add "can't be triggered with current > > > code" to the commit message, rather than the Fixes tag. > > > > The code is wrong, so comes Fixes line, but I can remove it. > > Yes, perhaps after death we will inhabit a world with clear, > non-conflicting rules, where law can be followed to the letter > and "truth" and "good" are clearly and objectively defined. > > Until the sweat release, tho, let's apply common sense, and > not add Fixes tags to patches which can't possibly be of interest > to backporters. > > Please and thank you... Sure > > > > I had a look thru the series yesterday, and it looks good to me > > > (tho I'm no ipsec expert). Thanks for putting in the work! > > > > > > Could you add some info about how the code in the series can be > > > exercised / example configurations? And please CC Simon, it'd be > > > great to get him / someone at Corigine to review. > > > > > > And obviously Steffen, why did you not CC Steffen?! :o > > > > It works exactly like "regular" IPsec, nothing special, except > > now users can switch to switchdev before adding IPsec rules. > > > > devlink dev eswitch set pci/0000:06:00.0 mode switchdev > > > > Same configurations as here: > > https://lore.kernel.org/netdev/cover.1670005543.git.leonro@nvidia.com/ > > Packet offload mode: > > ip xfrm state offload packet dev <if-name> dir <in|out> > > ip xfrm policy .... offload packet dev <if-name> > > Crypto offload mode: > > ip xfrm state offload crypto dev <if-name> dir <in|out> > > or (backward compatibility) > > ip xfrm state offload dev <if-name> dir <in|out> > > I see, so all policy based IPsec? Yes, it is. > Does the order of processing in the device match the kernel? Yes and this it why this fix was needed to make sure that we update destinations properly. > TC packet rewrites or IPsec comes first? In theory, we support any order, but in real life I don't think that TC before IPsec is really valuable. > > > I didn't add Steffen as it is more flow steering magic series > > and not IPsec :). > > > > I'll resubmit on Sunday. > > Thanks!
On Thu, 13 Jul 2023 21:58:33 +0300 Leon Romanovsky wrote: > > TC packet rewrites or IPsec comes first? > > In theory, we support any order, but in real life I don't think that TC > before IPsec is really valuable. I asked the question poorly. To clearer, you're saying that: a) host <-> TC <-> IPsec <-> "wire"/switch or b) host <-> IPsec <-> TC <-> "wire"/switch ?
On Thu, Jul 13, 2023 at 08:17:27PM -0700, Jakub Kicinski wrote: > On Thu, 13 Jul 2023 21:58:33 +0300 Leon Romanovsky wrote: > > > TC packet rewrites or IPsec comes first? > > > > In theory, we support any order, but in real life I don't think that TC > > before IPsec is really valuable. > > I asked the question poorly. To clearer, you're saying that: > > a) host <-> TC <-> IPsec <-> "wire"/switch > or > b) host <-> IPsec <-> TC <-> "wire"/switch > > ? It depends on configuration order, if user configures TC first, it will be a), if he/she configures IPsec first, it will be b). I just think that option b) is really matters. Thanks
On Fri, 14 Jul 2023 21:40:13 +0300 Leon Romanovsky wrote: > > > In theory, we support any order, but in real life I don't think that TC > > > before IPsec is really valuable. > > > > I asked the question poorly. To clearer, you're saying that: > > > > a) host <-> TC <-> IPsec <-> "wire"/switch > > or > > b) host <-> IPsec <-> TC <-> "wire"/switch > > > > ? > > It depends on configuration order, if user configures TC first, it will > be a), if he/she configures IPsec first, it will be b). > > I just think that option b) is really matters. And only b) matches what happens in the kernel with policy based IPsec, right? So can we reject a) from happening? IIUC what you're saying - the result depending on order of configuration may be a major source of surprises / hard to debug problems for the user.
On Fri, Jul 14, 2023 at 12:16:33PM -0700, Jakub Kicinski wrote: > On Fri, 14 Jul 2023 21:40:13 +0300 Leon Romanovsky wrote: > > > > In theory, we support any order, but in real life I don't think that TC > > > > before IPsec is really valuable. > > > > > > I asked the question poorly. To clearer, you're saying that: > > > > > > a) host <-> TC <-> IPsec <-> "wire"/switch > > > or > > > b) host <-> IPsec <-> TC <-> "wire"/switch > > > > > > ? > > > > It depends on configuration order, if user configures TC first, it will > > be a), if he/she configures IPsec first, it will be b). > > > > I just think that option b) is really matters. > > And only b) matches what happens in the kernel with policy based IPsec, > right? Can you please clarify what do you mean "policy based IPsec"? > So can we reject a) from happening? Technically yes. > IIUC what you're saying - > the result depending on order of configuration may be a major source > of surprises / hard to debug problems for the user. When I reviewed patches, I came exactly to an opposite conclusion :) My rationale was that users who configure IPsec and TC are advanced users who knows their data flow and if they find a) option valuable, they can do it. For example, a) allows to limit amount of data sent to IPsec engine. I believe both a) and b) should be supported. Thanks
On Fri, 14 Jul 2023 23:32:58 +0300 Leon Romanovsky wrote: > On Fri, Jul 14, 2023 at 12:16:33PM -0700, Jakub Kicinski wrote: > > On Fri, 14 Jul 2023 21:40:13 +0300 Leon Romanovsky wrote: > > > It depends on configuration order, if user configures TC first, it will > > > be a), if he/she configures IPsec first, it will be b). > > > > > > I just think that option b) is really matters. > > > > And only b) matches what happens in the kernel with policy based IPsec, > > right? > > Can you please clarify what do you mean "policy based IPsec"? I mean without a separate xfrm netdev on which you can install TC rules of its own. > > IIUC what you're saying - > > the result depending on order of configuration may be a major source > > of surprises / hard to debug problems for the user. > > When I reviewed patches, I came exactly to an opposite conclusion :) > > My rationale was that users who configure IPsec and TC are advanced > users who knows their data flow and if they find a) option valuable, > they can do it. > > For example, a) allows to limit amount of data sent to IPsec engine. > > I believe both a) and b) should be supported. What does it take to switch between the modes? Even if we want both modes we should have an explicit switch, I reckon. Or at least a way to read back what mode we ended up in.
On Fri, Jul 14, 2023 at 08:30:32PM -0700, Jakub Kicinski wrote: > On Fri, 14 Jul 2023 23:32:58 +0300 Leon Romanovsky wrote: > > On Fri, Jul 14, 2023 at 12:16:33PM -0700, Jakub Kicinski wrote: > > > On Fri, 14 Jul 2023 21:40:13 +0300 Leon Romanovsky wrote: > > > > It depends on configuration order, if user configures TC first, it will > > > > be a), if he/she configures IPsec first, it will be b). > > > > > > > > I just think that option b) is really matters. > > > > > > And only b) matches what happens in the kernel with policy based IPsec, > > > right? > > > > Can you please clarify what do you mean "policy based IPsec"? > > I mean without a separate xfrm netdev on which you can install TC > rules of its own. I call it software IPsec. > > > > IIUC what you're saying - > > > the result depending on order of configuration may be a major source > > > of surprises / hard to debug problems for the user. > > > > When I reviewed patches, I came exactly to an opposite conclusion :) > > > > My rationale was that users who configure IPsec and TC are advanced > > users who knows their data flow and if they find a) option valuable, > > they can do it. > > > > For example, a) allows to limit amount of data sent to IPsec engine. > > > > I believe both a) and b) should be supported. > > What does it take to switch between the modes? > Even if we want both modes we should have an explicit switch, I reckon. > Or at least a way to read back what mode we ended up in. I had several internal discussions about how TC and IPsec should work together, and will need some time to think about proper implementation. For now I'll add patch which makes TC and IPsec mutually exclusive. Thanks >
On Sun, Jul 16, 2023 at 01:39:47PM +0300, Leon Romanovsky wrote: > On Fri, Jul 14, 2023 at 08:30:32PM -0700, Jakub Kicinski wrote: > > On Fri, 14 Jul 2023 23:32:58 +0300 Leon Romanovsky wrote: > > > On Fri, Jul 14, 2023 at 12:16:33PM -0700, Jakub Kicinski wrote: > > > > On Fri, 14 Jul 2023 21:40:13 +0300 Leon Romanovsky wrote: > > > > > It depends on configuration order, if user configures TC first, it will > > > > > be a), if he/she configures IPsec first, it will be b). > > > > > > > > > > I just think that option b) is really matters. > > > > > > > > And only b) matches what happens in the kernel with policy based IPsec, > > > > right? > > > > > > Can you please clarify what do you mean "policy based IPsec"? > > > > I mean without a separate xfrm netdev on which you can install TC > > rules of its own. > > I call it software IPsec. > > > > > > > IIUC what you're saying - > > > > the result depending on order of configuration may be a major source > > > > of surprises / hard to debug problems for the user. > > > > > > When I reviewed patches, I came exactly to an opposite conclusion :) > > > > > > My rationale was that users who configure IPsec and TC are advanced > > > users who knows their data flow and if they find a) option valuable, > > > they can do it. > > > > > > For example, a) allows to limit amount of data sent to IPsec engine. > > > > > > I believe both a) and b) should be supported. > > > > What does it take to switch between the modes? > > Even if we want both modes we should have an explicit switch, I reckon. > > Or at least a way to read back what mode we ended up in. > > I had several internal discussions about how TC and IPsec should work > together, and will need some time to think about proper implementation. > > For now I'll add patch which makes TC and IPsec mutually exclusive. Even this so called trivial patch is not so trivial in mlx5 current implementation. Jianbo is working on it. Thanks > > Thanks > > > >
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c index 830ff8480fe1..59df6156246e 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c @@ -1066,7 +1066,7 @@ int mlx5_modify_rule_destination(struct mlx5_flow_handle *handle, } for (i = 0; i < handle->num_rules; i++) { - if (mlx5_flow_dests_cmp(new_dest, &handle->rule[i]->dest_attr)) + if (mlx5_flow_dests_cmp(old_dest, &handle->rule[i]->dest_attr)) return _mlx5_modify_rule_destination(handle->rule[i], new_dest); }