From: Tom Herbert <tom@herbertland.com> To: Davide Caratti <dcaratti@redhat.com> Cc: Alexander Duyck <alexander.duyck@gmail.com>, David Laight <David.Laight@aculab.com>, "David S . Miller" <davem@davemloft.net>, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>, Linux Kernel Network Developers <netdev@vger.kernel.org>, "linux-sctp @ vger . kernel . org" <linux-sctp@vger.kernel.org> Subject: Re: [PATCH RFC net-next v3 4/7] net: use skb->csum_algo to identify packets needing crc32c Date: Fri, 7 Apr 2017 11:11:11 -0700 [thread overview] Message-ID: <CALx6S36rem=OuN_At6qYA=se5cpuYM1N2R8efoaszvo8b8Tz5A@mail.gmail.com> (raw) In-Reply-To: <1491586160.2505.87.camel@redhat.com> On Fri, Apr 7, 2017 at 10:29 AM, Davide Caratti <dcaratti@redhat.com> wrote: > hello Tom, > > On Fri, 2017-04-07 at 08:43 -0700, Tom Herbert wrote: >> On Fri, Apr 7, 2017 at 7:16 AM, Davide Caratti <dcaratti@redhat.com> wrote: >> > @@ -742,8 +744,10 @@ struct sk_buff { >> > __u8 csum_valid:1; >> > __u8 csum_complete_sw:1; >> > __u8 csum_level:2; >> > - __u8 __unused:1; /* one bit hole */ >> > - >> > + enum { >> > + INTERNET_CHECKSUM = 0, >> > + CRC32C_CHECKSUM, >> > + } csum_algo:1; >> >> I am worried this opens the door to a new open ended functionality >> that will be rarely used in practice. Checksum offload is pervasive, >> CRC offload is still a very narrow use case. > > thank you for the prompt response. I thought there was a silent > agreement on that - Alexander proposed usage of an enum bitfield to be > ready for FCoE (and I'm not against it, unless I have to find a second > free bit in struct sk_buff :-) ). But maybe I'm misunderstanding your > concern: is it the name of the variable, (csum_algo instead of > crc32c_csum), or the usage of enum bitfield (or both?) ? > > On Fri, 2017-04-07 at 08:43 -0700, Tom Herbert wrote: >> Adding yet more >> CRC/checksum variants will need more bits. It may be sufficient for >> now just to make this a single bit which indicates "ones' checksum" or >> indicates "other". In this case of "other" we need some analysis so >> determine which checksum it is, this might be something that flow >> dissector could support. > > ... which is my intent: by the way, from my perspective, we don't need more than 1 bit > to extend the functionality. While reviewing my code, I was also considering > extending the witdth of skb->ip_summed from 2 to 3 bit, so that it was possible > to > Maybe just call it csum_not_ip then. Then just do "if (unlikely(skb->csum_not_ip)) ..." > #define > > CRC32C_PARTIAL <- for SCTP > CRC_PARTIAL <- for FCoE > CHECKSUM_PARTIAL <- for everything else > > It's conceptually the same thing, and the free bit is used more > efficiently. But then I would need to check all places where > CHECKSUM_PARTIAL is used in assignments and test: so, I told myself it's > not worth doing it until somebody requests to extend this functionality to > FCoE. > I've thought about extending ip_summed before with something like csum_invalid. I think it opens up a can of worms since ip_summed is being used in so many places already and the semantics of each value have to be extremely well defined for the whole system (this is one place where we can't tolerate any ambiguity at all and it everything needs to be clearly documented). >> > @@ -3129,6 +3133,14 @@ struct skb_checksum_ops { >> > >> > extern const struct skb_checksum_ops *crc32c_csum_stub __read_mostly; >> > >> > +static inline void skb_set_crc32c_ipsummed(struct sk_buff *skb, >> > + const u8 ip_summed) >> > +{ >> > + skb->csum_algo = ip_summed == CHECKSUM_PARTIAL ? CRC32C_CHECKSUM : >> > + INTERNET_CHECKSUM; >> > + skb->ip_summed = ip_summed; >> >> This seems odd to me. skb->csum_algo and skb->ip_summed always end up >> having the same value. > > this is accidentally true for CHEKSUM_NONE and CHECKSUM_PARTIAL, and only > if skb carries a SCTP packet. This was my intent: > > ip_summed (2 bit) | csum_algo (1 bit) > ---------------------------------------+------------------- > CHEKSUM_NONE = 0 | INTERNET_CHECKSUM = 0 > CHECKSUM_PARTIAL = 1 | CRC32C_CHECKSUM = 1 > CHECKSUM_COMPLETE = 2 (not applicable) | INTERNET_CHECKSUM = 0 (don't care) > CHECKSUM_UNNECESSARY = 3 | INTERNET_CHECKSUM = 0 > > I can do this in a more explicit way, changing the prototype to > > static inline void skb_set_crc32c_ipsummed(struct sk_buff *skb, > const u8 ip_summed, > const u8 csum_algo) > > (with the advantage of saving a test on the value of ip_summed). > Find in the comment below the reason why I'm clearing csum_algo every time > the SCTP CRC32c is computed. > >> > --- a/net/netfilter/ipvs/ip_vs_proto_sctp.c >> > +++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c >> > @@ -81,7 +81,7 @@ static void sctp_nat_csum(struct sk_buff *skb, sctp_sctphdr_t *sctph, >> > unsigned int sctphoff) >> > { >> > sctph->checksum = sctp_compute_cksum(skb, sctphoff); >> > - skb->ip_summed = CHECKSUM_UNNECESSARY; >> > + skb_set_crc32c_ipsummed(skb, CHECKSUM_UNNECESSARY); >> >> The old code is better. CHECKSUM_UNNECESSARY already applies to non IP >> checksums. There is nothing special about crc32 in this regard and >> skb->csum_algo should only be valid when skb->ip_summed == >> CHECKSUM_PARTIAL so no need to set it here. This point should also be >> in documentation. > > In my understanding, csum_algo needs to be set to INTERNET_CHECKSUM after the > CRC32c is computed. Otherwise, after subsequent operation on the skb (e.g. it > is encapsulated in a UDP frame), there is the possibility for skb->ip_summed > to become CHECKSUM_PARTIAL again. So, to ensure that skb_checksum_help() and > not skb_crc32c_help() will be called, csum_algo must be 0. > ip_summed should no longer be CHECKSUM_PARTIAL with CRC32c is computed. > To minimize the impact of the patch, I substituted all assignments of skb->ip_summed, > done by SCTP-related code, with calls to skb_set_crc32c_ipsummed(). The alternative is > to explicitly set csum_algo to 0 (INTERNET_CHECKSUM) in SCTP-related code. Do you agree? > No, like I said the only case where this new bit is relevant is when CHECKSUM_PARTIAL for a CRC is being done. When it's set for offloading sctp crc it must be set. When CRC is resolved, in the helper for instance, it must be cleared. If these rules are properly followed then the bit will be zero in all other cases without needing any additional work or conditionals. Tom > thank you in advance, > regards > -- > davide >
WARNING: multiple messages have this Message-ID (diff)
From: Tom Herbert <tom@herbertland.com> To: Davide Caratti <dcaratti@redhat.com> Cc: Alexander Duyck <alexander.duyck@gmail.com>, David Laight <David.Laight@aculab.com>, "David S . Miller" <davem@davemloft.net>, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>, Linux Kernel Network Developers <netdev@vger.kernel.org>, "linux-sctp @ vger . kernel . org" <linux-sctp@vger.kernel.org> Subject: Re: [PATCH RFC net-next v3 4/7] net: use skb->csum_algo to identify packets needing crc32c Date: Fri, 07 Apr 2017 18:11:11 +0000 [thread overview] Message-ID: <CALx6S36rem=OuN_At6qYA=se5cpuYM1N2R8efoaszvo8b8Tz5A@mail.gmail.com> (raw) In-Reply-To: <1491586160.2505.87.camel@redhat.com> On Fri, Apr 7, 2017 at 10:29 AM, Davide Caratti <dcaratti@redhat.com> wrote: > hello Tom, > > On Fri, 2017-04-07 at 08:43 -0700, Tom Herbert wrote: >> On Fri, Apr 7, 2017 at 7:16 AM, Davide Caratti <dcaratti@redhat.com> wrote: >> > @@ -742,8 +744,10 @@ struct sk_buff { >> > __u8 csum_valid:1; >> > __u8 csum_complete_sw:1; >> > __u8 csum_level:2; >> > - __u8 __unused:1; /* one bit hole */ >> > - >> > + enum { >> > + INTERNET_CHECKSUM = 0, >> > + CRC32C_CHECKSUM, >> > + } csum_algo:1; >> >> I am worried this opens the door to a new open ended functionality >> that will be rarely used in practice. Checksum offload is pervasive, >> CRC offload is still a very narrow use case. > > thank you for the prompt response. I thought there was a silent > agreement on that - Alexander proposed usage of an enum bitfield to be > ready for FCoE (and I'm not against it, unless I have to find a second > free bit in struct sk_buff :-) ). But maybe I'm misunderstanding your > concern: is it the name of the variable, (csum_algo instead of > crc32c_csum), or the usage of enum bitfield (or both?) ? > > On Fri, 2017-04-07 at 08:43 -0700, Tom Herbert wrote: >> Adding yet more >> CRC/checksum variants will need more bits. It may be sufficient for >> now just to make this a single bit which indicates "ones' checksum" or >> indicates "other". In this case of "other" we need some analysis so >> determine which checksum it is, this might be something that flow >> dissector could support. > > ... which is my intent: by the way, from my perspective, we don't need more than 1 bit > to extend the functionality. While reviewing my code, I was also considering > extending the witdth of skb->ip_summed from 2 to 3 bit, so that it was possible > to > Maybe just call it csum_not_ip then. Then just do "if (unlikely(skb->csum_not_ip)) ..." > #define > > CRC32C_PARTIAL <- for SCTP > CRC_PARTIAL <- for FCoE > CHECKSUM_PARTIAL <- for everything else > > It's conceptually the same thing, and the free bit is used more > efficiently. But then I would need to check all places where > CHECKSUM_PARTIAL is used in assignments and test: so, I told myself it's > not worth doing it until somebody requests to extend this functionality to > FCoE. > I've thought about extending ip_summed before with something like csum_invalid. I think it opens up a can of worms since ip_summed is being used in so many places already and the semantics of each value have to be extremely well defined for the whole system (this is one place where we can't tolerate any ambiguity at all and it everything needs to be clearly documented). >> > @@ -3129,6 +3133,14 @@ struct skb_checksum_ops { >> > >> > extern const struct skb_checksum_ops *crc32c_csum_stub __read_mostly; >> > >> > +static inline void skb_set_crc32c_ipsummed(struct sk_buff *skb, >> > + const u8 ip_summed) >> > +{ >> > + skb->csum_algo = ip_summed = CHECKSUM_PARTIAL ? CRC32C_CHECKSUM : >> > + INTERNET_CHECKSUM; >> > + skb->ip_summed = ip_summed; >> >> This seems odd to me. skb->csum_algo and skb->ip_summed always end up >> having the same value. > > this is accidentally true for CHEKSUM_NONE and CHECKSUM_PARTIAL, and only > if skb carries a SCTP packet. This was my intent: > > ip_summed (2 bit) | csum_algo (1 bit) > ---------------------------------------+------------------- > CHEKSUM_NONE = 0 | INTERNET_CHECKSUM = 0 > CHECKSUM_PARTIAL = 1 | CRC32C_CHECKSUM = 1 > CHECKSUM_COMPLETE = 2 (not applicable) | INTERNET_CHECKSUM = 0 (don't care) > CHECKSUM_UNNECESSARY = 3 | INTERNET_CHECKSUM = 0 > > I can do this in a more explicit way, changing the prototype to > > static inline void skb_set_crc32c_ipsummed(struct sk_buff *skb, > const u8 ip_summed, > const u8 csum_algo) > > (with the advantage of saving a test on the value of ip_summed). > Find in the comment below the reason why I'm clearing csum_algo every time > the SCTP CRC32c is computed. > >> > --- a/net/netfilter/ipvs/ip_vs_proto_sctp.c >> > +++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c >> > @@ -81,7 +81,7 @@ static void sctp_nat_csum(struct sk_buff *skb, sctp_sctphdr_t *sctph, >> > unsigned int sctphoff) >> > { >> > sctph->checksum = sctp_compute_cksum(skb, sctphoff); >> > - skb->ip_summed = CHECKSUM_UNNECESSARY; >> > + skb_set_crc32c_ipsummed(skb, CHECKSUM_UNNECESSARY); >> >> The old code is better. CHECKSUM_UNNECESSARY already applies to non IP >> checksums. There is nothing special about crc32 in this regard and >> skb->csum_algo should only be valid when skb->ip_summed = >> CHECKSUM_PARTIAL so no need to set it here. This point should also be >> in documentation. > > In my understanding, csum_algo needs to be set to INTERNET_CHECKSUM after the > CRC32c is computed. Otherwise, after subsequent operation on the skb (e.g. it > is encapsulated in a UDP frame), there is the possibility for skb->ip_summed > to become CHECKSUM_PARTIAL again. So, to ensure that skb_checksum_help() and > not skb_crc32c_help() will be called, csum_algo must be 0. > ip_summed should no longer be CHECKSUM_PARTIAL with CRC32c is computed. > To minimize the impact of the patch, I substituted all assignments of skb->ip_summed, > done by SCTP-related code, with calls to skb_set_crc32c_ipsummed(). The alternative is > to explicitly set csum_algo to 0 (INTERNET_CHECKSUM) in SCTP-related code. Do you agree? > No, like I said the only case where this new bit is relevant is when CHECKSUM_PARTIAL for a CRC is being done. When it's set for offloading sctp crc it must be set. When CRC is resolved, in the helper for instance, it must be cleared. If these rules are properly followed then the bit will be zero in all other cases without needing any additional work or conditionals. Tom > thank you in advance, > regards > -- > davide >
next prev parent reply other threads:[~2017-04-07 18:11 UTC|newest] Thread overview: 104+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-01-23 16:52 [RFC PATCH net-next 0/5] net: improve support for SCTP checksums Davide Caratti 2017-01-23 16:52 ` Davide Caratti 2017-01-23 16:52 ` [RFC PATCH net-next 1/5] skbuff: add stub to help computing crc32c on SCTP packets Davide Caratti 2017-01-23 16:52 ` Davide Caratti 2017-01-23 16:52 ` [RFC PATCH net-next 2/5] net: split skb_checksum_help Davide Caratti 2017-01-23 16:52 ` Davide Caratti 2017-01-23 20:59 ` Tom Herbert 2017-01-23 20:59 ` Tom Herbert 2017-01-24 16:35 ` David Laight 2017-01-24 16:35 ` David Laight 2017-02-02 15:07 ` Davide Caratti 2017-02-02 15:07 ` Davide Caratti 2017-02-02 16:55 ` David Laight 2017-02-02 16:55 ` David Laight 2017-02-02 18:08 ` Tom Herbert 2017-02-02 18:08 ` Tom Herbert 2017-02-27 13:39 ` Davide Caratti 2017-02-27 13:39 ` Davide Caratti 2017-02-27 15:11 ` Tom Herbert 2017-02-27 15:11 ` Tom Herbert 2017-02-28 10:31 ` Davide Caratti 2017-02-28 10:31 ` Davide Caratti 2017-02-28 10:32 ` [PATCH RFC net-next v2 1/4] skbuff: add stub to help computing crc32c on SCTP packets Davide Caratti 2017-02-28 10:32 ` Davide Caratti 2017-02-28 10:32 ` [PATCH RFC net-next v2 2/4] net: introduce skb_sctp_csum_help Davide Caratti 2017-02-28 10:32 ` Davide Caratti 2017-02-28 10:32 ` [PATCH RFC net-next v2 3/4] net: more accurate checksumming in validate_xmit_skb Davide Caratti 2017-02-28 10:32 ` Davide Caratti 2017-02-28 19:50 ` Tom Herbert 2017-02-28 19:50 ` Tom Herbert 2017-02-28 10:32 ` [PATCH RFC net-next v2 4/4] Documentation: update notes on checksum offloading Davide Caratti 2017-02-28 10:32 ` Davide Caratti 2017-02-28 22:46 ` [PATCH RFC net-next v2 1/4] skbuff: add stub to help computing crc32c on SCTP packets Alexander Duyck 2017-02-28 22:46 ` Alexander Duyck 2017-03-01 3:17 ` Tom Herbert 2017-03-01 3:17 ` Tom Herbert 2017-03-01 10:53 ` David Laight 2017-03-01 10:53 ` David Laight 2017-03-06 21:51 ` Davide Caratti 2017-03-06 21:51 ` Davide Caratti 2017-03-07 18:06 ` Alexander Duyck 2017-03-07 18:06 ` Alexander Duyck 2017-03-18 13:17 ` Davide Caratti 2017-03-18 13:17 ` Davide Caratti 2017-03-18 22:35 ` Tom Herbert 2017-03-18 22:35 ` Tom Herbert 2017-04-07 14:16 ` [PATCH RFC net-next v3 0/7] improve CRC32c in the forwarding path Davide Caratti 2017-04-07 14:16 ` Davide Caratti 2017-04-07 14:16 ` [PATCH RFC net-next v3 1/7] skbuff: add stub to help computing crc32c on SCTP packets Davide Caratti 2017-04-07 14:16 ` Davide Caratti 2017-04-07 14:16 ` [PATCH RFC net-next v3 2/7] net: introduce skb_crc32c_csum_help Davide Caratti 2017-04-07 14:16 ` Davide Caratti 2017-04-07 14:16 ` [PATCH RFC net-next v3 3/7] sk_buff: remove support for csum_bad in sk_buff Davide Caratti 2017-04-07 14:16 ` Davide Caratti 2017-04-07 14:16 ` [PATCH RFC net-next v3 4/7] net: use skb->csum_algo to identify packets needing crc32c Davide Caratti 2017-04-07 14:16 ` Davide Caratti 2017-04-07 15:43 ` Tom Herbert 2017-04-07 15:43 ` Tom Herbert 2017-04-07 17:29 ` Davide Caratti 2017-04-07 17:29 ` Davide Caratti 2017-04-07 18:11 ` Tom Herbert [this message] 2017-04-07 18:11 ` Tom Herbert 2017-04-13 10:36 ` Davide Caratti 2017-04-13 10:36 ` Davide Caratti 2017-04-20 13:38 ` [PATCH RFC net-next v4 0/7] net: improve support for SCTP checksums Davide Caratti 2017-04-20 13:38 ` Davide Caratti 2017-04-20 13:38 ` [PATCH RFC net-next v4 1/7] skbuff: add stub to help computing crc32c on SCTP packets Davide Caratti 2017-04-20 13:38 ` Davide Caratti 2017-04-20 13:38 ` [PATCH RFC net-next v4 2/7] net: introduce skb_crc32c_csum_help Davide Caratti 2017-04-20 13:38 ` Davide Caratti 2017-04-27 12:29 ` Marcelo Ricardo Leitner 2017-04-27 12:29 ` Marcelo Ricardo Leitner 2017-04-20 13:38 ` [PATCH RFC net-next v4 3/7] sk_buff: remove support for csum_bad in sk_buff Davide Caratti 2017-04-20 13:38 ` Davide Caratti 2017-04-27 1:34 ` [sk_buff] 95510aef27: BUG:Bad_page_state_in_process kernel test robot 2017-04-27 1:34 ` kernel test robot 2017-04-29 20:21 ` [PATCH RFC net-next v4 3/7] sk_buff: remove support for csum_bad in sk_buff Tom Herbert 2017-04-29 20:21 ` Tom Herbert 2017-04-20 13:38 ` [PATCH RFC net-next v4 4/7] net: use skb->csum_not_inet to identify packets needing crc32c Davide Caratti 2017-04-20 13:38 ` Davide Caratti 2017-04-29 20:18 ` Tom Herbert 2017-04-29 20:18 ` Tom Herbert 2017-04-20 13:38 ` [PATCH RFC net-next v4 5/7] net: more accurate checksumming in validate_xmit_skb() Davide Caratti 2017-04-20 13:38 ` Davide Caratti 2017-04-20 13:38 ` [PATCH RFC net-next v4 6/7] openvswitch: more accurate checksumming in queue_userspace_packet() Davide Caratti 2017-04-20 13:38 ` Davide Caratti 2017-04-20 13:38 ` [PATCH RFC net-next v4 7/7] sk_buff.h: improve description of CHECKSUM_{COMPLETE,UNNECESSARY} Davide Caratti 2017-04-20 13:38 ` Davide Caratti 2017-04-29 20:20 ` Tom Herbert 2017-04-29 20:20 ` Tom Herbert 2017-04-27 12:41 ` [PATCH RFC net-next v4 0/7] net: improve support for SCTP checksums Marcelo Ricardo Leitner 2017-04-27 12:41 ` Marcelo Ricardo Leitner 2017-04-07 14:16 ` [PATCH RFC net-next v3 5/7] net: more accurate checksumming in validate_xmit_skb() Davide Caratti 2017-04-07 14:16 ` Davide Caratti 2017-04-07 14:16 ` [PATCH RFC net-next v3 6/7] openvswitch: more accurate checksumming in queue_userspace_packet() Davide Caratti 2017-04-07 14:16 ` Davide Caratti 2017-04-07 14:16 ` [PATCH RFC net-next v3 7/7] sk_buff.h: improve description of CHECKSUM_{COMPLETE,UNNECESSARY} Davide Caratti 2017-04-07 14:16 ` Davide Caratti 2017-01-23 16:52 ` [RFC PATCH net-next 3/5] net: introduce skb_sctp_csum_help Davide Caratti 2017-01-23 16:52 ` Davide Caratti 2017-01-23 16:52 ` [RFC PATCH net-next 4/5] net: more accurate checksumming in validate_xmit_skb Davide Caratti 2017-01-23 16:52 ` Davide Caratti 2017-01-23 16:52 ` [RFC PATCH net-next 5/5] Documentation: add description of skb_sctp_csum_help Davide Caratti 2017-01-23 16:52 ` Davide Caratti
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CALx6S36rem=OuN_At6qYA=se5cpuYM1N2R8efoaszvo8b8Tz5A@mail.gmail.com' \ --to=tom@herbertland.com \ --cc=David.Laight@aculab.com \ --cc=alexander.duyck@gmail.com \ --cc=davem@davemloft.net \ --cc=dcaratti@redhat.com \ --cc=linux-sctp@vger.kernel.org \ --cc=marcelo.leitner@gmail.com \ --cc=netdev@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.