| Message ID | 1547646606-16687-1-git-send-email-xiangxia.m.yue@gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: mlx5: allow default ip_proto to offload | expand |
On Thu, Jan 17, 2019 at 11:28 AM <xiangxia.m.yue@gmail.com> wrote: > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> with the current code, if a modification of the ip header is required and the ip protocol is not one of tcp, udp or icmp - we err This is done in purpose, and we don't want to allow offloading this header re-write for unknown ip protocol > Allow default ip_proto to offload, so icmp, tcp, and udp > will match the flow as show below, otherwise we must type the > ip_proto for icmp, tcp and udp respectively. > > $ tc filter add dev netdev01_rep parent ffff: protocol ip prio 1 \ > flower skip_sw dst_ip 3.3.3.3 \ > action pedit ex munge ip dst set 192.168.1.100 pipe \ > action csum ip pipe \ > action mirred egress redirect dev netdev02_rep this flow specify the ip protocol (1 which is icmp) > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > --- > drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > index 2ee377a..2a29428 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > @@ -2219,7 +2219,7 @@ static bool modify_header_match_supported(struct mlx5_flow_spec *spec, > } > > ip_proto = MLX5_GET(fte_match_set_lyr_2_4, headers_v, ip_protocol); > - if (modify_ip_header && ip_proto != IPPROTO_TCP && > + if (modify_ip_header && ip_proto != 0 && ip_proto != IPPROTO_TCP && > ip_proto != IPPROTO_UDP && ip_proto != IPPROTO_ICMP) { > NL_SET_ERR_MSG_MOD(extack, > "can't offload re-write of non TCP/UDP"); but under your patch we will not err for unknown ip protocol I have lost you BTW - I plan to change the polarity of the check here with the below patch - please see if it helps your use-case: commit 09b972083266fa8cfe2f24e1c264905d5cd021ed Author: Or Gerlitz <ogerlitz@mellanox.com> Date: Wed Oct 31 18:42:21 2018 +0200 net/mlx5e: Allow TC offload of IP header re-write for more protocols So far we allowed re-writing of IP header only for three protocols (tcp, udp and icmpv4). This is too limiting, e.g for cases where users want to apply offloading of NAT. Take a complimentary approach and allow this for wider set of IP protocols -- all of them except for three (icmpv6, sctp and udp-lite). For these protos the current HW isn't capable to properly adjust the l4 checksum while doing the modification <--- UPDATE - we probably can do icmpv6 Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c index 608025ca5c04..affb523e0e35 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c @@ -2167,11 +2167,11 @@ static bool modify_header_match_supported(struct mlx5_flow_spec *spec, } ip_proto = MLX5_GET(fte_match_set_lyr_2_4, headers_v, ip_protocol); - if (modify_ip_header && ip_proto != IPPROTO_TCP && - ip_proto != IPPROTO_UDP && ip_proto != IPPROTO_ICMP) { + if (modify_ip_header && (ip_proto == IPPROTO_ICMPV6 || + ip_proto == IPPROTO_SCTP || ip_proto == IPPROTO_UDPLITE)) { NL_SET_ERR_MSG_MOD(extack, - "can't offload re-write of non TCP/UDP"); - pr_info("can't offload re-write of ip proto %d\n", ip_proto); + "can't offload this re-write of IP addresses"); + pr_info("can't offload re-write of IP addrs for ip proto %d\n", ip_proto); return false; }
On Thu, Jan 17, 2019 at 8:58 PM Or Gerlitz <gerlitz.or@gmail.com> wrote: > > On Thu, Jan 17, 2019 at 11:28 AM <xiangxia.m.yue@gmail.com> wrote: > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > with the current code, if a modification of the ip header is required > and the ip protocol is not one of tcp, udp or icmp - we err > > This is done in purpose, and we don't want to allow offloading > this header re-write for unknown ip protocol > > > Allow default ip_proto to offload, so icmp, tcp, and udp > > will match the flow as show below, otherwise we must type the > > ip_proto for icmp, tcp and udp respectively. > > > > $ tc filter add dev netdev01_rep parent ffff: protocol ip prio 1 \ > > flower skip_sw dst_ip 3.3.3.3 \ > > action pedit ex munge ip dst set 192.168.1.100 pipe \ > > action csum ip pipe \ > > action mirred egress redirect dev netdev02_rep > > this flow specify the ip protocol (1 which is icmp) Without this patch, we should type ip_proto for flower, but not for tc protocol， for example [1] tc filter add dev eth5_0 parent ffff: protocol ip prio 1 \ flower skip_sw ip_proto icmp dst_ip 3.3.3.3 \ action xxx if we run tc without flower ip_proto, we get the err: [2] tc filter add dev eth5_0 parent ffff: protocol ip prio 1 \ flower skip_sw dst_ip 3.3.3.3 \ action xxx err log: "can't offload re-write of ip proto 0" with this patch, run the command [2], we will not get err log, and the filter work in hw. > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > --- > > drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > > index 2ee377a..2a29428 100644 > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > > @@ -2219,7 +2219,7 @@ static bool modify_header_match_supported(struct mlx5_flow_spec *spec, > > } > > > > ip_proto = MLX5_GET(fte_match_set_lyr_2_4, headers_v, ip_protocol); > > - if (modify_ip_header && ip_proto != IPPROTO_TCP && > > + if (modify_ip_header && ip_proto != 0 && ip_proto != IPPROTO_TCP && > > ip_proto != IPPROTO_UDP && ip_proto != IPPROTO_ICMP) { > > NL_SET_ERR_MSG_MOD(extack, > > "can't offload re-write of non TCP/UDP"); > > but under your patch we will not err for unknown ip protocol > > I have lost you > > BTW - I plan to change the polarity of the check here with the below > patch - please see if it helps your use-case: > > commit 09b972083266fa8cfe2f24e1c264905d5cd021ed > Author: Or Gerlitz <ogerlitz@mellanox.com> > Date: Wed Oct 31 18:42:21 2018 +0200 > > net/mlx5e: Allow TC offload of IP header re-write for more protocols > > So far we allowed re-writing of IP header only for three protocols > (tcp, udp and icmpv4). This is too limiting, e.g for cases where > users want to apply offloading of NAT. > > Take a complimentary approach and allow this for wider set of IP > protocols -- all of them except for three (icmpv6, sctp and udp-lite). > For these protos the current HW isn't capable to properly adjust the > l4 checksum while doing the modification <--- UPDATE - we probably > can do icmpv6 > > Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com> > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > index 608025ca5c04..affb523e0e35 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > @@ -2167,11 +2167,11 @@ static bool > modify_header_match_supported(struct mlx5_flow_spec *spec, > } > > ip_proto = MLX5_GET(fte_match_set_lyr_2_4, headers_v, ip_protocol); > - if (modify_ip_header && ip_proto != IPPROTO_TCP && > - ip_proto != IPPROTO_UDP && ip_proto != IPPROTO_ICMP) { > + if (modify_ip_header && (ip_proto == IPPROTO_ICMPV6 || > + ip_proto == IPPROTO_SCTP || ip_proto == IPPROTO_UDPLITE)) { > NL_SET_ERR_MSG_MOD(extack, > - "can't offload re-write of non TCP/UDP"); > - pr_info("can't offload re-write of ip proto %d\n", ip_proto); > + "can't offload this re-write of IP > addresses"); > + pr_info("can't offload re-write of IP addrs for ip > proto %d\n", ip_proto); > return false; > }
On Thu, Jan 17, 2019 at 8:58 PM Or Gerlitz <gerlitz.or@gmail.com> wrote: > > On Thu, Jan 17, 2019 at 11:28 AM <xiangxia.m.yue@gmail.com> wrote: > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > with the current code, if a modification of the ip header is required > and the ip protocol is not one of tcp, udp or icmp - we err > > This is done in purpose, and we don't want to allow offloading > this header re-write for unknown ip protocol > > > Allow default ip_proto to offload, so icmp, tcp, and udp > > will match the flow as show below, otherwise we must type the > > ip_proto for icmp, tcp and udp respectively. > > > > $ tc filter add dev netdev01_rep parent ffff: protocol ip prio 1 \ > > flower skip_sw dst_ip 3.3.3.3 \ > > action pedit ex munge ip dst set 192.168.1.100 pipe \ > > action csum ip pipe \ > > action mirred egress redirect dev netdev02_rep > > this flow specify the ip protocol (1 which is icmp) > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > --- > > drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > > index 2ee377a..2a29428 100644 > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > > @@ -2219,7 +2219,7 @@ static bool modify_header_match_supported(struct mlx5_flow_spec *spec, > > } > > > > ip_proto = MLX5_GET(fte_match_set_lyr_2_4, headers_v, ip_protocol); > > - if (modify_ip_header && ip_proto != IPPROTO_TCP && > > + if (modify_ip_header && ip_proto != 0 && ip_proto != IPPROTO_TCP && > > ip_proto != IPPROTO_UDP && ip_proto != IPPROTO_ICMP) { > > NL_SET_ERR_MSG_MOD(extack, > > "can't offload re-write of non TCP/UDP"); > > but under your patch we will not err for unknown ip protocol > > I have lost you > > BTW - I plan to change the polarity of the check here with the below > patch - please see if it helps your use-case: > > commit 09b972083266fa8cfe2f24e1c264905d5cd021ed > Author: Or Gerlitz <ogerlitz@mellanox.com> > Date: Wed Oct 31 18:42:21 2018 +0200 > > net/mlx5e: Allow TC offload of IP header re-write for more protocols > > So far we allowed re-writing of IP header only for three protocols > (tcp, udp and icmpv4). This is too limiting, e.g for cases where > users want to apply offloading of NAT. > > Take a complimentary approach and allow this for wider set of IP > protocols -- all of them except for three (icmpv6, sctp and udp-lite). > For these protos the current HW isn't capable to properly adjust the > l4 checksum while doing the modification <--- UPDATE - we probably > can do icmpv6 > > Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com> > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > index 608025ca5c04..affb523e0e35 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > @@ -2167,11 +2167,11 @@ static bool > modify_header_match_supported(struct mlx5_flow_spec *spec, > } > > ip_proto = MLX5_GET(fte_match_set_lyr_2_4, headers_v, ip_protocol); > - if (modify_ip_header && ip_proto != IPPROTO_TCP && > - ip_proto != IPPROTO_UDP && ip_proto != IPPROTO_ICMP) { > + if (modify_ip_header && (ip_proto == IPPROTO_ICMPV6 || > + ip_proto == IPPROTO_SCTP || ip_proto == IPPROTO_UDPLITE)) { > NL_SET_ERR_MSG_MOD(extack, > - "can't offload re-write of non TCP/UDP"); > - pr_info("can't offload re-write of ip proto %d\n", ip_proto); > + "can't offload this re-write of IP > addresses"); > + pr_info("can't offload re-write of IP addrs for ip > proto %d\n", ip_proto); > return false; > } We should consider ip_proto == 0, in some case, we only modify dest ip or src ip.
On Thu, Jan 17, 2019 at 3:34 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > On Thu, Jan 17, 2019 at 8:58 PM Or Gerlitz <gerlitz.or@gmail.com> wrote: > > On Thu, Jan 17, 2019 at 11:28 AM <xiangxia.m.yue@gmail.com> wrote: > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > with this patch, run the command [2], we will not get err log, > and the filter work in hw. This whole thing is done for a reason which is the inability of the current HW to adjust checksum/crc for few L3 protocols. Such adjustment is needed if you modify some fields of L3 headers, e.g re-write src/dst IP address. > We should consider ip_proto == 0, in some case, we only > modify dest ip or src ip. we can't let it go without clear matching on the ip protocol, as I explained above. With my proposed patch you will be able to NAT much more protocols (all of them expect for three, and we're working to reduce that), but you still need a tc rule per ip proto
On Fri, Jan 18, 2019 at 10:19 PM Or Gerlitz <gerlitz.or@gmail.com> wrote: > > On Thu, Jan 17, 2019 at 3:34 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > On Thu, Jan 17, 2019 at 8:58 PM Or Gerlitz <gerlitz.or@gmail.com> wrote: > > > On Thu, Jan 17, 2019 at 11:28 AM <xiangxia.m.yue@gmail.com> wrote: > > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > with this patch, run the command [2], we will not get err log, > > and the filter work in hw. > > This whole thing is done for a reason which is the inability of the current HW > to adjust checksum/crc for few L3 protocols. Such adjustment is needed if > you modify some fields of L3 headers, e.g re-write src/dst IP address. I got it, thanks > > We should consider ip_proto == 0, in some case, we only > > modify dest ip or src ip. > > we can't let it go without clear matching on the ip protocol, as I explained > above. With my proposed patch you will be able to NAT much more protocols > (all of them expect for three, and we're working to reduce that), but > you still need > a tc rule per ip proto > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > index 608025ca5c04..affb523e0e35 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > @@ -2167,11 +2167,11 @@ static bool > modify_header_match_supported(struct mlx5_flow_spec *spec, > } > > ip_proto = MLX5_GET(fte_match_set_lyr_2_4, headers_v, ip_protocol); > - if (modify_ip_header && ip_proto != IPPROTO_TCP && > - ip_proto != IPPROTO_UDP && ip_proto != IPPROTO_ICMP) { > + if (modify_ip_header && (ip_proto == IPPROTO_ICMPV6 || > + ip_proto == IPPROTO_SCTP || ip_proto == IPPROTO_UDPLITE)) { > NL_SET_ERR_MSG_MOD(extack, > - "can't offload re-write of non TCP/UDP"); > - pr_info("can't offload re-write of ip proto %d\n", ip_proto); > + "can't offload this re-write of IP > addresses"); > + pr_info("can't offload re-write of IP addrs for ip > proto %d\n", ip_proto); > return false; > } This patch work for me too, because ip_proto == 0 will not return err( and my patch allow ip_proto == 0 and not return err) and will you send it to net-next ? because i can't find it in net-next. and one question, In your patch, should we check ip_proto is valid ? for example, ip_proto == 18, is not valid protocol. flower ip_proto 18
On Sat, Jan 19, 2019 at 10:50 AM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > On Fri, Jan 18, 2019 at 10:19 PM Or Gerlitz <gerlitz.or@gmail.com> wrote: > > On Thu, Jan 17, 2019 at 3:34 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > > On Thu, Jan 17, 2019 at 8:58 PM Or Gerlitz <gerlitz.or@gmail.com> wrote: > > > > On Thu, Jan 17, 2019 at 11:28 AM <xiangxia.m.yue@gmail.com> wrote: > > > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > > with this patch, run the command [2], we will not get err log, > > > and the filter work in hw. > > > > This whole thing is done for a reason which is the inability of the current HW > > to adjust checksum/crc for few L3 protocols. Such adjustment is needed if > > you modify some fields of L3 headers, e.g re-write src/dst IP address. > I got it, thanks > > > We should consider ip_proto == 0, in some case, we only > > > modify dest ip or src ip. > > > > we can't let it go without clear matching on the ip protocol, as I explained > > above. With my proposed patch you will be able to NAT much more protocols > > (all of them expect for three, and we're working to reduce that), but > > you still need > > a tc rule per ip proto > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > > b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > > index 608025ca5c04..affb523e0e35 100644 > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c > > @@ -2167,11 +2167,11 @@ static bool > > modify_header_match_supported(struct mlx5_flow_spec *spec, > > } > > > > ip_proto = MLX5_GET(fte_match_set_lyr_2_4, headers_v, ip_protocol); > > - if (modify_ip_header && ip_proto != IPPROTO_TCP && > > - ip_proto != IPPROTO_UDP && ip_proto != IPPROTO_ICMP) { > > + if (modify_ip_header && (ip_proto == IPPROTO_ICMPV6 || > > + ip_proto == IPPROTO_SCTP || ip_proto == IPPROTO_UDPLITE)) { > > NL_SET_ERR_MSG_MOD(extack, > > - "can't offload re-write of non TCP/UDP"); > > - pr_info("can't offload re-write of ip proto %d\n", ip_proto); > > + "can't offload this re-write of IP > > addresses"); > > + pr_info("can't offload re-write of IP addrs for ip > > proto %d\n", ip_proto); > > return false; > > } > This patch work for me too, because ip_proto == 0 will not return err( > and my patch allow ip_proto == 0 and not return err) and will you send > it to net-next ? because i can't find it in net-next. basically, I was planning to upstream it on this cycle, but your comment below is something I need to look at > and one question, In your patch, should we check ip_proto is valid ? > for example, ip_proto == 18, is not valid protocol. yeah, this becomes a bit ugly, I need to see how to address that > flower ip_proto 18
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c index 2ee377a..2a29428 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c @@ -2219,7 +2219,7 @@ static bool modify_header_match_supported(struct mlx5_flow_spec *spec, } ip_proto = MLX5_GET(fte_match_set_lyr_2_4, headers_v, ip_protocol); - if (modify_ip_header && ip_proto != IPPROTO_TCP && + if (modify_ip_header && ip_proto != 0 && ip_proto != IPPROTO_TCP && ip_proto != IPPROTO_UDP && ip_proto != IPPROTO_ICMP) { NL_SET_ERR_MSG_MOD(extack, "can't offload re-write of non TCP/UDP");