| Message ID | 1586071063-51656-1-git-send-email-decui@microsoft.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] skbuff.h: Improve the checksum related comments | expand |
On Sun, Apr 05, 2020 at 12:17:43AM -0700, Dexuan Cui wrote: > * CHECKSUM_COMPLETE: > * > - * This is the most generic way. The device supplied checksum of the _whole_ > - * packet as seen by netif_rx() and fills out in skb->csum. Meaning, the > + * This is the most generic way. The device supplies checksum of the _whole_ > + * packet as seen by netif_rx() and fills out in skb->csum. This means the I think both 'supplies' and 'supplied' are correct in this sentence. The nuances are slightly different, but the meaning is the same in this instance. You missed a mistake in the second line though, it should be either 'fills out' or 'fills in'. I think we tend to prefer 'fills in'. > * CHECKSUM_COMPLETE: > * Not used in checksum output. If a driver observes a packet with this value > - * set in skbuff, if should treat as CHECKSUM_NONE being set. > + * set in skbuff, the driver should treat it as CHECKSUM_NONE being set. I would go with "it should treat the packet as if CHECKSUM_NONE were set." > @@ -211,7 +211,7 @@ > * is implied by the SKB_GSO_* flags in gso_type. Most obviously, if the > * gso_type is SKB_GSO_TCPV4 or SKB_GSO_TCPV6, TCP checksum offload as > * part of the GSO operation is implied. If a checksum is being offloaded > - * with GSO then ip_summed is CHECKSUM_PARTIAL, csum_start and csum_offset > + * with GSO then ip_summed is CHECKSUM_PARTIAL AND csum_start and csum_offset > * are set to refer to the outermost checksum being offload (two offloaded > * checksums are possible with UDP encapsulation). Why the capitalisation of 'AND'? Thanks for the improvements, Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> From: Matthew Wilcox <willy@infradead.org> > Sent: Sunday, April 5, 2020 3:36 AM > To: Dexuan Cui <decui@microsoft.com> > > On Sun, Apr 05, 2020 at 12:17:43AM -0700, Dexuan Cui wrote: > > * CHECKSUM_COMPLETE: > > * > > - * This is the most generic way. The device supplied checksum of the > _whole_ > > - * packet as seen by netif_rx() and fills out in skb->csum. Meaning, the > > + * This is the most generic way. The device supplies checksum of the > _whole_ > > + * packet as seen by netif_rx() and fills out in skb->csum. This means the > > I think both 'supplies' and 'supplied' are correct in this sentence. The > nuances are slightly different, but the meaning is the same in this instance. I see. So let me rever back to "supplied". > You missed a mistake in the second line though, it should be either 'fills > out' or 'fills in'. I think we tend to prefer 'fills in'. Thanks! Will use "fills in" in v2. > > * CHECKSUM_COMPLETE: > > * Not used in checksum output. If a driver observes a packet with this > value > > - * set in skbuff, if should treat as CHECKSUM_NONE being set. > > + * set in skbuff, the driver should treat it as CHECKSUM_NONE being set. > > I would go with "it should treat the packet as if CHECKSUM_NONE were set." Thanks. Will use this version. > > @@ -211,7 +211,7 @@ > > * is implied by the SKB_GSO_* flags in gso_type. Most obviously, if the > > * gso_type is SKB_GSO_TCPV4 or SKB_GSO_TCPV6, TCP checksum offload > as > > * part of the GSO operation is implied. If a checksum is being offloaded > > - * with GSO then ip_summed is CHECKSUM_PARTIAL, csum_start and > csum_offset > > + * with GSO then ip_summed is CHECKSUM_PARTIAL AND csum_start and > csum_offset > > * are set to refer to the outermost checksum being offload (two offloaded > > * checksums are possible with UDP encapsulation). > > Why the capitalisation of 'AND'? The current text without the patch is: * part of the GSO operation is implied. If a checksum is being offloaded * with GSO then ip_summed is CHECKSUM_PARTIAL, csum_start and csum_offset * are set to refer to the outermost checksum being offload (two offloaded * checksums are possible with UDP encapsulation). The comma after the "CHECKSUM_PARTIAL" seems suspicious to me. I feel we should add an "and" after the comma, or replace the comma with "and", but either way we'll have "... and csum_start and csum_offset...", which seems a little unnatural to me since we have 2 'and's here... So I tried to make it a little natural by replacing the first 'and' with 'AND', which obviously causes confusion to you. Please suggest the best change here. Thanks! > Thanks for the improvements, > > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org> Thanks for the comments! I'll wait for your suggestion on the 'AND' and post a v2. Thanks, -- Dexuan
On 4/5/20 9:33 AM, Dexuan Cui wrote: >> From: Matthew Wilcox <willy@infradead.org> >> Sent: Sunday, April 5, 2020 3:36 AM >> To: Dexuan Cui <decui@microsoft.com> >> >> On Sun, Apr 05, 2020 at 12:17:43AM -0700, Dexuan Cui wrote: >>> * CHECKSUM_COMPLETE: >>> * >>> - * This is the most generic way. The device supplied checksum of the >> _whole_ >>> - * packet as seen by netif_rx() and fills out in skb->csum. Meaning, the >>> + * This is the most generic way. The device supplies checksum of the >> _whole_ >>> + * packet as seen by netif_rx() and fills out in skb->csum. This means the >> >> I think both 'supplies' and 'supplied' are correct in this sentence. The >> nuances are slightly different, but the meaning is the same in this instance. > > I see. So let me rever back to "supplied". > >> You missed a mistake in the second line though, it should be either 'fills >> out' or 'fills in'. I think we tend to prefer 'fills in'. > > Thanks! Will use "fills in" in v2. > >>> * CHECKSUM_COMPLETE: >>> * Not used in checksum output. If a driver observes a packet with this >> value >>> - * set in skbuff, if should treat as CHECKSUM_NONE being set. >>> + * set in skbuff, the driver should treat it as CHECKSUM_NONE being set. >> >> I would go with "it should treat the packet as if CHECKSUM_NONE were set." > > Thanks. Will use this version. > >>> @@ -211,7 +211,7 @@ >>> * is implied by the SKB_GSO_* flags in gso_type. Most obviously, if the >>> * gso_type is SKB_GSO_TCPV4 or SKB_GSO_TCPV6, TCP checksum offload >> as >>> * part of the GSO operation is implied. If a checksum is being offloaded >>> - * with GSO then ip_summed is CHECKSUM_PARTIAL, csum_start and >> csum_offset >>> + * with GSO then ip_summed is CHECKSUM_PARTIAL AND csum_start and >> csum_offset >>> * are set to refer to the outermost checksum being offload (two offloaded >>> * checksums are possible with UDP encapsulation). >> >> Why the capitalisation of 'AND'? > > The current text without the patch is: > * part of the GSO operation is implied. If a checksum is being offloaded > * with GSO then ip_summed is CHECKSUM_PARTIAL, csum_start and csum_offset > * are set to refer to the outermost checksum being offload (two offloaded > * checksums are possible with UDP encapsulation). > > The comma after the "CHECKSUM_PARTIAL" seems suspicious to me. I feel we > should add an "and" after the comma, or replace the comma with "and", but > either way we'll have "... and csum_start and csum_offset...", which seems a little > unnatural to me since we have 2 'and's here... So I tried to make it a little natural > by replacing the first 'and' with 'AND', which obviously causes confusion to you. maybe "both csum_start and csum_offset are set to refer to". > Please suggest the best change here. Thanks! > >> Thanks for the improvements, >> >> Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > Thanks for the comments! I'll wait for your suggestion on the 'AND' and post > a v2.
> From: Randy Dunlap <rdunlap@infradead.org> > Sent: Sunday, April 5, 2020 9:41 AM > To: Dexuan Cui <decui@microsoft.com>; Matthew Wilcox > >> Why the capitalisation of 'AND'? > > ... > > The comma after the "CHECKSUM_PARTIAL" seems suspicious to me. I feel > > we should add an "and" after the comma, or replace the comma with "and", > > but either way we'll have "... and csum_start and csum_offset...", which > > seems a little unnatural to me since we have 2 'and's here... So I tried to > > make it a little natural by replacing the first 'and' with 'AND', which > > obviously causes confusion to you. > > maybe "both csum_start and csum_offset are set to refer to". > ~Randy Looks good. I'll post a v2 shortly. Thank you both! Thanks, -- Dexuan
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 28b1a2b..746049c 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -47,8 +47,8 @@ * A. IP checksum related features * * Drivers advertise checksum offload capabilities in the features of a device. - * From the stack's point of view these are capabilities offered by the driver, - * a driver typically only advertises features that it is capable of offloading + * From the stack's point of view these are capabilities offered by the driver. + * A driver typically only advertises features that it is capable of offloading * to its device. * * The checksum related features are: @@ -63,7 +63,7 @@ * TCP or UDP packets over IPv4. These are specifically * unencapsulated packets of the form IPv4|TCP or * IPv4|UDP where the Protocol field in the IPv4 header - * is TCP or UDP. The IPv4 header may contain IP options + * is TCP or UDP. The IPv4 header may contain IP options. * This feature cannot be set in features for a device * with NETIF_F_HW_CSUM also set. This feature is being * DEPRECATED (see below). @@ -79,13 +79,13 @@ * DEPRECATED (see below). * * NETIF_F_RXCSUM - Driver (device) performs receive checksum offload. - * This flag is used only used to disable the RX checksum + * This flag is only used to disable the RX checksum * feature for a device. The stack will accept receive * checksum indication in packets received on a device * regardless of whether NETIF_F_RXCSUM is set. * * B. Checksumming of received packets by device. Indication of checksum - * verification is in set skb->ip_summed. Possible values are: + * verification is set in skb->ip_summed. Possible values are: * * CHECKSUM_NONE: * @@ -115,16 +115,16 @@ * the packet minus one that have been verified as CHECKSUM_UNNECESSARY. * For instance if a device receives an IPv6->UDP->GRE->IPv4->TCP packet * and a device is able to verify the checksums for UDP (possibly zero), - * GRE (checksum flag is set), and TCP-- skb->csum_level would be set to + * GRE (checksum flag is set) and TCP, skb->csum_level would be set to * two. If the device were only able to verify the UDP checksum and not - * GRE, either because it doesn't support GRE checksum of because GRE + * GRE, either because it doesn't support GRE checksum or because GRE * checksum is bad, skb->csum_level would be set to zero (TCP checksum is * not considered in this case). * * CHECKSUM_COMPLETE: * - * This is the most generic way. The device supplied checksum of the _whole_ - * packet as seen by netif_rx() and fills out in skb->csum. Meaning, the + * This is the most generic way. The device supplies checksum of the _whole_ + * packet as seen by netif_rx() and fills out in skb->csum. This means the * hardware doesn't need to parse L3/L4 headers to implement this. * * Notes: @@ -153,8 +153,8 @@ * from skb->csum_start up to the end, and to record/write the checksum at * offset skb->csum_start + skb->csum_offset. A driver may verify that the * csum_start and csum_offset values are valid values given the length and - * offset of the packet, however they should not attempt to validate that the - * checksum refers to a legitimate transport layer checksum-- it is the + * offset of the packet, but it should not attempt to validate that the + * checksum refers to a legitimate transport layer checksum -- it is the * purview of the stack to validate that csum_start and csum_offset are set * correctly. * @@ -178,18 +178,18 @@ * * CHECKSUM_UNNECESSARY: * - * This has the same meaning on as CHECKSUM_NONE for checksum offload on + * This has the same meaning as CHECKSUM_NONE for checksum offload on * output. * * CHECKSUM_COMPLETE: * Not used in checksum output. If a driver observes a packet with this value - * set in skbuff, if should treat as CHECKSUM_NONE being set. + * set in skbuff, the driver should treat it as CHECKSUM_NONE being set. * * D. Non-IP checksum (CRC) offloads * * NETIF_F_SCTP_CRC - This feature indicates that a device is capable of * offloading the SCTP CRC in a packet. To perform this offload the stack - * will set set csum_start and csum_offset accordingly, set ip_summed to + * will set csum_start and csum_offset accordingly, set ip_summed to * CHECKSUM_PARTIAL and set csum_not_inet to 1, to provide an indication in * the skbuff that the CHECKSUM_PARTIAL refers to CRC32c. * A driver that supports both IP checksum offload and SCTP CRC32c offload @@ -200,10 +200,10 @@ * NETIF_F_FCOE_CRC - This feature indicates that a device is capable of * offloading the FCOE CRC in a packet. To perform this offload the stack * will set ip_summed to CHECKSUM_PARTIAL and set csum_start and csum_offset - * accordingly. Note the there is no indication in the skbuff that the - * CHECKSUM_PARTIAL refers to an FCOE checksum, a driver that supports + * accordingly. Note that there is no indication in the skbuff that the + * CHECKSUM_PARTIAL refers to an FCOE checksum, so a driver that supports * both IP checksum offload and FCOE CRC offload must verify which offload - * is configured for a packet presumably by inspecting packet headers. + * is configured for a packet, presumably by inspecting packet headers. * * E. Checksumming on output with GSO. * @@ -211,7 +211,7 @@ * is implied by the SKB_GSO_* flags in gso_type. Most obviously, if the * gso_type is SKB_GSO_TCPV4 or SKB_GSO_TCPV6, TCP checksum offload as * part of the GSO operation is implied. If a checksum is being offloaded - * with GSO then ip_summed is CHECKSUM_PARTIAL, csum_start and csum_offset + * with GSO then ip_summed is CHECKSUM_PARTIAL AND csum_start and csum_offset * are set to refer to the outermost checksum being offload (two offloaded * checksums are possible with UDP encapsulation). */
Fixed the punctuation and some typos. Improved a few sentences with minor changes. No change to the semantics or the code. Signed-off-by: Dexuan Cui <decui@microsoft.com> --- English is not my mother tongue, so I may not be making the best changes here. I'm happy to post a v2 if necessary. Looking forward to your comments! include/linux/skbuff.h | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-)