Question related to GSO6 checksum magic

Question related to GSO6 checksum magic

[Heiner Kallweit]

Few network drivers like Intel e1000e or r8169 have the following in the
GSO6 tx path:

ipv6_hdr(skb)->payload_len = 0;
tcp_hdr(skb)->check = ~csum_ipv6_magic(&ipv6_hdr(skb)->saddr,
				       &ipv6_hdr(skb)->daddr,
				       0, IPPROTO_TCP, 0);
(partially also w/o the payload_len assignment)

This sounds like we should factor it out to a helper.
The code however leaves few questions to me, but I'm not familiar enough
with the net core low-level details to answer them:

- This code is used in a number of drivers, so is it something that
  should be moved to the core? If yes, where would it belong to?

- Is clearing payload_len needed? IOW, can it be a problem if drivers
  miss this?

Thanks, Heiner

Re: Question related to GSO6 checksum magic

[Alexander Duyck]

On Tue, 2020-02-11 at 20:48 +0100, Heiner Kallweit wrote:
> Few network drivers like Intel e1000e or r8169 have the following in the
> GSO6 tx path:
> 
> ipv6_hdr(skb)->payload_len = 0;
> tcp_hdr(skb)->check = ~csum_ipv6_magic(&ipv6_hdr(skb)->saddr,
> 				       &ipv6_hdr(skb)->daddr,
> 				       0, IPPROTO_TCP, 0);
> (partially also w/o the payload_len assignment)
> 
> This sounds like we should factor it out to a helper.
> The code however leaves few questions to me, but I'm not familiar enough
> with the net core low-level details to answer them:
> 
> - This code is used in a number of drivers, so is it something that
>   should be moved to the core? If yes, where would it belong to?
> 
> - Is clearing payload_len needed? IOW, can it be a problem if drivers
>   miss this?
> 
> Thanks, Heiner

The hardware is expecting the TCP header to contain the partial checksum
minus the length. It does this because it reuses the value when it
computes the checksum for the header of outgoing TCP frames and it will
add the payload length as it is segmenting the frames.

An alternative approach would be to pull the original checksum value out
and simply do the checksum math to subtract the length from it. If I am
not mistaken there are some drivers that take that approach for some of
the headers.

- Alex

Re: Question related to GSO6 checksum magic

[Heiner Kallweit]

On 11.02.2020 22:01, Alexander Duyck wrote:
> On Tue, 2020-02-11 at 20:48 +0100, Heiner Kallweit wrote:
>> Few network drivers like Intel e1000e or r8169 have the following in the
>> GSO6 tx path:
>>
>> ipv6_hdr(skb)->payload_len = 0;
>> tcp_hdr(skb)->check = ~csum_ipv6_magic(&ipv6_hdr(skb)->saddr,
>> 				       &ipv6_hdr(skb)->daddr,
>> 				       0, IPPROTO_TCP, 0);
>> (partially also w/o the payload_len assignment)
>>
>> This sounds like we should factor it out to a helper.
>> The code however leaves few questions to me, but I'm not familiar enough
>> with the net core low-level details to answer them:
>>
>> - This code is used in a number of drivers, so is it something that
>>   should be moved to the core? If yes, where would it belong to?
>>
>> - Is clearing payload_len needed? IOW, can it be a problem if drivers
>>   miss this?
>>
>> Thanks, Heiner
> 
> The hardware is expecting the TCP header to contain the partial checksum
> minus the length. It does this because it reuses the value when it
> computes the checksum for the header of outgoing TCP frames and it will
> add the payload length as it is segmenting the frames.
> 
Thanks, that helped a lot!

> An alternative approach would be to pull the original checksum value out
> and simply do the checksum math to subtract the length from it. If I am
> not mistaken there are some drivers that take that approach for some of
> the headers.
> 
> - Alex
> 
Heiner

Re: Question related to GSO6 checksum magic

[Heiner Kallweit]

On 11.02.2020 22:01, Alexander Duyck wrote:
> On Tue, 2020-02-11 at 20:48 +0100, Heiner Kallweit wrote:
>> Few network drivers like Intel e1000e or r8169 have the following in the
>> GSO6 tx path:
>>
>> ipv6_hdr(skb)->payload_len = 0;
>> tcp_hdr(skb)->check = ~csum_ipv6_magic(&ipv6_hdr(skb)->saddr,
>> 				       &ipv6_hdr(skb)->daddr,
>> 				       0, IPPROTO_TCP, 0);
>> (partially also w/o the payload_len assignment)
>>
>> This sounds like we should factor it out to a helper.
>> The code however leaves few questions to me, but I'm not familiar enough
>> with the net core low-level details to answer them:
>>
>> - This code is used in a number of drivers, so is it something that
>>   should be moved to the core? If yes, where would it belong to?
>>
>> - Is clearing payload_len needed? IOW, can it be a problem if drivers
>>   miss this?
>>
>> Thanks, Heiner
> 
> The hardware is expecting the TCP header to contain the partial checksum
> minus the length. It does this because it reuses the value when it
> computes the checksum for the header of outgoing TCP frames and it will
> add the payload length as it is segmenting the frames.
> 
> An alternative approach would be to pull the original checksum value out
> and simply do the checksum math to subtract the length from it. If I am
> not mistaken there are some drivers that take that approach for some of
> the headers.
> 
> - Alex
> 
I submitted a series that adds a helper for what we talked about, see
https://marc.info/?l=linux-usb&m=158197558425809
Status in patchwork is "needs review / ack". Would be great if you could
have a look at it.

Thanks, Heiner