Messages in this thread | | | From | Dexuan Cui <> | Subject | RE: [PATCH net] skbuff.h: Improve the checksum related comments | Date | Sun, 5 Apr 2020 16:33:17 +0000 |
| |
> 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
| |