| Message ID | 100e0b32b0322e70127f415ea5b26afd26ac0fed.1611477858.git.lucien.xin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: add support for ip generic checksum offload for gre | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 7 maintainers not CCed: andriin@fb.com daniel@iogearbox.net edumazet@google.com ap420073@gmail.com jiri@mellanox.com ast@kernel.org xiyou.wangcong@gmail.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 10 this patch: 10 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 19 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 10 this patch: 10 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Sun, Jan 24, 2021 at 3:47 AM Xin Long <lucien.xin@gmail.com> wrote: > > NETIF_F_IP|IPV6_CSUM feature flag indicates UDP and TCP csum offload > while NETIF_F_HW_CSUM feature flag indicates ip generic csum offload > for HW, which includes not only for TCP/UDP csum, but also for other > protocols' csum like GRE's. > > However, in skb_csum_hwoffload_help() it only checks features against > NETIF_F_CSUM_MASK(NETIF_F_HW|IP|IPV6_CSUM). So if it's a non TCP/UDP > packet and the features doesn't support NETIF_F_HW_CSUM, but supports > NETIF_F_IP|IPV6_CSUM only, it would still return 0 and leave the HW > to do csum. > > This patch is to support ip generic csum processing by checking > NETIF_F_HW_CSUM for all protocols, and check (NETIF_F_IP_CSUM | > NETIF_F_IPV6_CSUM) only for TCP and UDP. > > v1->v2: > - not extend skb->csum_not_inet, but use skb->csum_offset to tell > if it's an UDP/TCP csum packet. > > Suggested-by: Alexander Duyck <alexander.duyck@gmail.com> > Signed-off-by: Xin Long <lucien.xin@gmail.com> > --- > net/core/dev.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index 6df3f1b..aae116d 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -3621,7 +3621,18 @@ int skb_csum_hwoffload_help(struct sk_buff *skb, > return !!(features & NETIF_F_SCTP_CRC) ? 0 : > skb_crc32c_csum_help(skb); > > - return !!(features & NETIF_F_CSUM_MASK) ? 0 : skb_checksum_help(skb); > + if (features & NETIF_F_HW_CSUM) > + return 0; > + > + if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) { > + switch (skb->csum_offset) { > + case offsetof(struct tcphdr, check): > + case offsetof(struct udphdr, check): This relies on no other protocols requesting CHECKSUM_PARTIAL with these csum_offset values. That is a fragile assumption. It may well be correct, and Alex argues that point in v1 of the patch. I think that argumentation at the least should be captured as a comment or in the commit message. Or perhaps limit this optimization over s/w checksumming to skb->sk && (skb->sk->sk_family == AF_INET || .. ) && (skb->sk->sk_type == SOCK_STREAM || ..) ?
On Tue, Jan 26, 2021 at 9:59 AM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Sun, Jan 24, 2021 at 3:47 AM Xin Long <lucien.xin@gmail.com> wrote: > > > > NETIF_F_IP|IPV6_CSUM feature flag indicates UDP and TCP csum offload > > while NETIF_F_HW_CSUM feature flag indicates ip generic csum offload > > for HW, which includes not only for TCP/UDP csum, but also for other > > protocols' csum like GRE's. > > > > However, in skb_csum_hwoffload_help() it only checks features against > > NETIF_F_CSUM_MASK(NETIF_F_HW|IP|IPV6_CSUM). So if it's a non TCP/UDP > > packet and the features doesn't support NETIF_F_HW_CSUM, but supports > > NETIF_F_IP|IPV6_CSUM only, it would still return 0 and leave the HW > > to do csum. > > > > This patch is to support ip generic csum processing by checking > > NETIF_F_HW_CSUM for all protocols, and check (NETIF_F_IP_CSUM | > > NETIF_F_IPV6_CSUM) only for TCP and UDP. > > > > v1->v2: > > - not extend skb->csum_not_inet, but use skb->csum_offset to tell > > if it's an UDP/TCP csum packet. > > > > Suggested-by: Alexander Duyck <alexander.duyck@gmail.com> > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > --- > > net/core/dev.c | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/net/core/dev.c b/net/core/dev.c > > index 6df3f1b..aae116d 100644 > > --- a/net/core/dev.c > > +++ b/net/core/dev.c > > @@ -3621,7 +3621,18 @@ int skb_csum_hwoffload_help(struct sk_buff *skb, > > return !!(features & NETIF_F_SCTP_CRC) ? 0 : > > skb_crc32c_csum_help(skb); > > > > - return !!(features & NETIF_F_CSUM_MASK) ? 0 : skb_checksum_help(skb); > > + if (features & NETIF_F_HW_CSUM) > > + return 0; > > + > > + if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) { > > + switch (skb->csum_offset) { > > + case offsetof(struct tcphdr, check): > > + case offsetof(struct udphdr, check): > > This relies on no other protocols requesting CHECKSUM_PARTIAL > with these csum_offset values. > > That is a fragile assumption. It may well be correct, and Alex argues > that point in v1 of the patch. I think that argumentation at the least > should be captured as a comment or in the commit message. will add a note in changelog and repost, thanks! > > Or perhaps limit this optimization over s/w checksumming to > > skb->sk && > (skb->sk->sk_family == AF_INET || .. ) && > (skb->sk->sk_type == SOCK_STREAM || ..) > > ?
diff --git a/net/core/dev.c b/net/core/dev.c index 6df3f1b..aae116d 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -3621,7 +3621,18 @@ int skb_csum_hwoffload_help(struct sk_buff *skb, return !!(features & NETIF_F_SCTP_CRC) ? 0 : skb_crc32c_csum_help(skb); - return !!(features & NETIF_F_CSUM_MASK) ? 0 : skb_checksum_help(skb); + if (features & NETIF_F_HW_CSUM) + return 0; + + if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) { + switch (skb->csum_offset) { + case offsetof(struct tcphdr, check): + case offsetof(struct udphdr, check): + return 0; + } + } + + return skb_checksum_help(skb); } EXPORT_SYMBOL(skb_csum_hwoffload_help);
NETIF_F_IP|IPV6_CSUM feature flag indicates UDP and TCP csum offload while NETIF_F_HW_CSUM feature flag indicates ip generic csum offload for HW, which includes not only for TCP/UDP csum, but also for other protocols' csum like GRE's. However, in skb_csum_hwoffload_help() it only checks features against NETIF_F_CSUM_MASK(NETIF_F_HW|IP|IPV6_CSUM). So if it's a non TCP/UDP packet and the features doesn't support NETIF_F_HW_CSUM, but supports NETIF_F_IP|IPV6_CSUM only, it would still return 0 and leave the HW to do csum. This patch is to support ip generic csum processing by checking NETIF_F_HW_CSUM for all protocols, and check (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM) only for TCP and UDP. v1->v2: - not extend skb->csum_not_inet, but use skb->csum_offset to tell if it's an UDP/TCP csum packet. Suggested-by: Alexander Duyck <alexander.duyck@gmail.com> Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/core/dev.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)