lkml.org 
[lkml]   [2020]   [Apr]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH net] skbuff.h: Improve the checksum related comments
Date
> 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

\
 
 \ /
  Last update: 2020-04-05 18:33    [W:0.067 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site