diff mbox series

[ovs-dev,v2,1/2] netdev-tc-offload: Add csum offload of protocols IGMP/UDPLITE/SCTP

Message ID AO2AFQARJAhHKNzoe0FxUKox.1.1690966775513.Hmail.mocan@ucloud.cn
State Superseded
Headers show
Series Add more protocols to offload in ip rewrite | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Faicker Mo Aug. 2, 2023, 8:59 a.m. UTC 
Add tc csum offload support of protocols IGMP/UDPLITE/SCTP

Signed-off-by: Faicker Mo <faicker.mo@ucloud.cn>
---
 lib/tc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

--
2.39.3

Comments

Simon Horman Aug. 4, 2023, 3:33 p.m. UTC | #1 
On Wed, Aug 02, 2023 at 04:59:35PM +0800, Faicker Mo via dev wrote:
> Add tc csum offload support of protocols IGMP/UDPLITE/SCTP
> 
> Signed-off-by: Faicker Mo <faicker.mo@ucloud.cn>

UDPLite will be deprecated in the Linux kernel from v6.4 [1].
Are you sure that we want to increase it's support in OvS at this time?

[1] https://lore.kernel.org/netdev/20230614194705.90673-1-kuniyu@amazon.com/

Moreover, please include some motivation - the why - in the patch description.

> ---
>  lib/tc.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/lib/tc.c b/lib/tc.c
> index f49048cda..52a74d9d0 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -2978,6 +2978,15 @@ csum_update_flag(struct tc_flower *flower,
>          } else if (flower->key.ip_proto == IPPROTO_ICMPV6) {
>              flower->needs_full_ip_proto_mask = true;
>              flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_ICMP;
> +        } else if (flower->key.ip_proto == IPPROTO_IGMP) {
> +            flower->needs_full_ip_proto_mask = true;
> +            flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_IGMP;
> +        } else if (flower->key.ip_proto == IPPROTO_UDPLITE) {
> +            flower->needs_full_ip_proto_mask = true;
> +            flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_UDPLITE;
> +        } else if (flower->key.ip_proto == IPPROTO_SCTP) {
> +            flower->needs_full_ip_proto_mask = true;
> +            flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_SCTP;
>          } else {
>              VLOG_WARN_RL(&error_rl,
>                           "can't offload rewrite of IP/IPV6 with ip_proto: %d",
> --
> 2.39.3
> 
> 
> 
> 
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Faicker Mo Aug. 9, 2023, 2:54 a.m. UTC | #2 
Actually I meet the problem with IPIP.
The IGMP/UDPLITE/SCTP are added because they are supported in tc csum action now.



From: Simon Horman <horms@ovn.org>
Date: 2023-08-04 23:33:43
To:  Faicker Mo <faicker.mo@ucloud.cn>
Cc:  dev@openvswitch.org,i.maximets@ovn.org
Subject: Re: [ovs-dev] [PATCH v2 1/2] netdev-tc-offload: Add csum offload of protocols IGMP/UDPLITE/SCTP>On Wed, Aug 02, 2023 at 04:59:35PM +0800, Faicker Mo via dev wrote:
>> Add tc csum offload support of protocols IGMP/UDPLITE/SCTP
>> 
>> Signed-off-by: Faicker Mo <faicker.mo@ucloud.cn>
>
>UDPLite will be deprecated in the Linux kernel from v6.4 [1].
>Are you sure that we want to increase it's support in OvS at this time?
>
>[1] https://lore.kernel.org/netdev/20230614194705.90673-1-kuniyu@amazon.com/
>
>Moreover, please include some motivation - the why - in the patch description.
>
>> ---
>>  lib/tc.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/lib/tc.c b/lib/tc.c
>> index f49048cda..52a74d9d0 100644
>> --- a/lib/tc.c
>> +++ b/lib/tc.c
>> @@ -2978,6 +2978,15 @@ csum_update_flag(struct tc_flower *flower,
>>          } else if (flower->key.ip_proto == IPPROTO_ICMPV6) {
>>              flower->needs_full_ip_proto_mask = true;
>>              flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_ICMP;
>> +        } else if (flower->key.ip_proto == IPPROTO_IGMP) {
>> +            flower->needs_full_ip_proto_mask = true;
>> +            flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_IGMP;
>> +        } else if (flower->key.ip_proto == IPPROTO_UDPLITE) {
>> +            flower->needs_full_ip_proto_mask = true;
>> +            flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_UDPLITE;
>> +        } else if (flower->key.ip_proto == IPPROTO_SCTP) {
>> +            flower->needs_full_ip_proto_mask = true;
>> +            flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_SCTP;
>>          } else {
>>              VLOG_WARN_RL(&error_rl,
>>                           "can't offload rewrite of IP/IPV6 with ip_proto: %d",
>> --
>> 2.39.3
>> 
>> 
>> 
>> 
>> 
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
Simon Horman Aug. 16, 2023, 1:35 p.m. UTC | #3 
On Wed, Aug 09, 2023 at 10:54:35AM +0800, Faicker Mo via dev wrote:
> Actually I meet the problem with IPIP.
> The IGMP/UDPLITE/SCTP are added because they are supported in tc csum action now.

Please don't top-post.

Regarding UDPLITE, ack, I see this is reasonable given existing
support in the tree.

Regarding morivation, please add some text to the patch
description describing why this is needed. Ideally with
instructions on how to reproduce the problem.

If possible, please also add a test case - is this handled
in the test case included in patch 2/2 ?

...
Faicker Mo Aug. 18, 2023, 7:44 a.m. UTC | #4 
No, I'll add a test case with this patch 1/2.
Aaron Conole Aug. 25, 2023, 3:25 p.m. UTC | #5 
Faicker Mo  <faicker.mo@ucloud.cn> writes:

> Add tc csum offload support of protocols IGMP/UDPLITE/SCTP
>
> Signed-off-by: Faicker Mo <faicker.mo@ucloud.cn>
> ---

Acked-by: Aaron Conole <aconole@redhat.com>

>  lib/tc.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/lib/tc.c b/lib/tc.c
> index f49048cda..52a74d9d0 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -2978,6 +2978,15 @@ csum_update_flag(struct tc_flower *flower,
>          } else if (flower->key.ip_proto == IPPROTO_ICMPV6) {
>              flower->needs_full_ip_proto_mask = true;
>              flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_ICMP;
> +        } else if (flower->key.ip_proto == IPPROTO_IGMP) {
> +            flower->needs_full_ip_proto_mask = true;
> +            flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_IGMP;
> +        } else if (flower->key.ip_proto == IPPROTO_UDPLITE) {
> +            flower->needs_full_ip_proto_mask = true;
> +            flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_UDPLITE;
> +        } else if (flower->key.ip_proto == IPPROTO_SCTP) {
> +            flower->needs_full_ip_proto_mask = true;
> +            flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_SCTP;
>          } else {
>              VLOG_WARN_RL(&error_rl,
>                           "can't offload rewrite of IP/IPV6 with ip_proto: %d",
> --
> 2.39.3
diff mbox series

Patch

diff --git a/lib/tc.c b/lib/tc.c
index f49048cda..52a74d9d0 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -2978,6 +2978,15 @@  csum_update_flag(struct tc_flower *flower,
         } else if (flower->key.ip_proto == IPPROTO_ICMPV6) {
             flower->needs_full_ip_proto_mask = true;
             flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_ICMP;
+        } else if (flower->key.ip_proto == IPPROTO_IGMP) {
+            flower->needs_full_ip_proto_mask = true;
+            flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_IGMP;
+        } else if (flower->key.ip_proto == IPPROTO_UDPLITE) {
+            flower->needs_full_ip_proto_mask = true;
+            flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_UDPLITE;
+        } else if (flower->key.ip_proto == IPPROTO_SCTP) {
+            flower->needs_full_ip_proto_mask = true;
+            flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_SCTP;
         } else {
             VLOG_WARN_RL(&error_rl,
                          "can't offload rewrite of IP/IPV6 with ip_proto: %d",