[PATCH] tcp: Set CHECKSUM_UNNECESSARY in tcp_init_nondata_skb

[PATCH] tcp: Set CHECKSUM_UNNECESSARY in tcp_init_nondata_skb

[David Miller]

Back in commit 04a0551c87363f100b04d28d7a15a632b70e18e7
("loopback: Drop obsolete ip_summed setting") we stopped
setting CHECKSUM_UNNECESSARY in the loopback xmit.

This is because such a setting was a lie since it implies that the
checksum field of the packet is properly filled in.

Instead what happens normally is that CHECKSUM_PARTIAL is set and
skb->csum is calculated as needed.

But this was only happening for TCP data packets (via the
skb->ip_summed assignment done in tcp_sendmsg()).  It doesn't
happen for non-data packets like ACKs etc.

Fix this by setting skb->ip_summed in the common non-data packet
constructor.  It already is setting skb->csum to zero.

But this reminds us that we still have things like ip_output.c's
ip_dev_loopback_xmit() which sets skb->ip_summed to the value
CHECKSUM_UNNECESSARY, which Herbert's patch teaches us is not
valid.  So we'll have to address that at some point too.

Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index f181b78..00afbb0 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -349,6 +349,7 @@ static inline void TCP_ECN_send(struct sock *sk, struct sk_buff *skb,
  */
 static void tcp_init_nondata_skb(struct sk_buff *skb, u32 seq, u8 flags)
 {
+	skb->ip_summed = CHECKSUM_PARTIAL;
 	skb->csum = 0;
 
 	TCP_SKB_CB(skb)->flags = flags;

Re: [PATCH] tcp: Set CHECKSUM_UNNECESSARY in tcp_init_nondata_skb

[Joe Perches]

On Thu, 2010-04-08 at 01:26 -0700, David Miller wrote:
> Fix this by setting skb->ip_summed in the common non-data packet
> constructor.  It already is setting skb->csum to zero.
> Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index f181b78..00afbb0 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -349,6 +349,7 @@ static inline void TCP_ECN_send(struct sock *sk, struct sk_buff *skb,
>   */
>  static void tcp_init_nondata_skb(struct sk_buff *skb, u32 seq, u8 flags)
>  {
> +	skb->ip_summed = CHECKSUM_PARTIAL;
>  	skb->csum = 0;
>  
>  	TCP_SKB_CB(skb)->flags = flags;

There might be trivial value in using the
struct layout order for the sets avoiding
crossing cachelines.

from:

static void tcp_init_nondata_skb(struct sk_buff *skb, u32 seq, u8 flags)
{
	skb->ip_summed = CHECKSUM_PARTIAL;
	skb->csum = 0;

	TCP_SKB_CB(skb)->flags = flags;
	TCP_SKB_CB(skb)->sacked = 0;

	skb_shinfo(skb)->gso_segs = 1;
	skb_shinfo(skb)->gso_size = 0;
	skb_shinfo(skb)->gso_type = 0;

	TCP_SKB_CB(skb)->seq = seq;
	if (flags & (TCPCB_FLAG_SYN | TCPCB_FLAG_FIN))
		seq++;
	TCP_SKB_CB(skb)->end_seq = seq;
}

to:

static void tcp_init_nondata_skb(struct sk_buff *skb, u32 seq, u8 flags)
{
	skb->ip_summed = CHECKSUM_PARTIAL; 
	skb->csum = 0;

	TCP_SKB_CB(skb)->seq = seq;
	if (flags & (TCPCB_FLAG_SYN | TCPCB_FLAG_FIN))
		seq++;
	TCP_SKB_CB(skb)->end_seq = seq;
	TCP_SKB_CB(skb)->sacked = 0;
	TCP_SKB_CB(skb)->flags = flags;

	skb_shinfo(skb)->gso_size = 0;
	skb_shinfo(skb)->gso_segs = 1;
	skb_shinfo(skb)->gso_type = 0;
}

Re: [PATCH] tcp: Set CHECKSUM_UNNECESSARY in tcp_init_nondata_skb

[Herbert Xu]

On Thu, Apr 08, 2010 at 01:26:17AM -0700, David Miller wrote:
> 
> Back in commit 04a0551c87363f100b04d28d7a15a632b70e18e7
> ("loopback: Drop obsolete ip_summed setting") we stopped
> setting CHECKSUM_UNNECESSARY in the loopback xmit.
> 
> This is because such a setting was a lie since it implies that the
> checksum field of the packet is properly filled in.
> 
> Instead what happens normally is that CHECKSUM_PARTIAL is set and
> skb->csum is calculated as needed.
> 
> But this was only happening for TCP data packets (via the
> skb->ip_summed assignment done in tcp_sendmsg()).  It doesn't
> happen for non-data packets like ACKs etc.

I don't understand.  If said packet was going across the network,
how could it possibly have worked?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] tcp: Set CHECKSUM_UNNECESSARY in tcp_init_nondata_skb

[Herbert Xu]

On Thu, Apr 08, 2010 at 01:26:17AM -0700, David Miller wrote:
> 
> Back in commit 04a0551c87363f100b04d28d7a15a632b70e18e7
> ("loopback: Drop obsolete ip_summed setting") we stopped
> setting CHECKSUM_UNNECESSARY in the loopback xmit.
> 
> This is because such a setting was a lie since it implies that the
> checksum field of the packet is properly filled in.
> 
> Instead what happens normally is that CHECKSUM_PARTIAL is set and
> skb->csum is calculated as needed.
> 
> But this was only happening for TCP data packets (via the
> skb->ip_summed assignment done in tcp_sendmsg()).  It doesn't
> happen for non-data packets like ACKs etc.

Actually, the checksum is still calculated in this case as otherwise
people would've screamed murder since their TCP connections would
have stopped working :)

However, it is suboptimal for loopback because we'll end up doing
an unnecessary verification for non-data packets.

> Fix this by setting skb->ip_summed in the common non-data packet
> constructor.  It already is setting skb->csum to zero.

The problem here is that for non-data packets CHECKSUM_PARTIAL
can actually end up being worse if we wind up going out through
an interface that doesn't support checksums.

The crux of the matter is to how to distinguish between those
packets where we have computed the checksum ourselves (such as
these non-data TCP packets), vs. packets from other sources where
we don't trust the checksum to be correct.

Since using CHECKSUM_PARTIAL is problematic, another possibility
is to use CHECKSUM_UNNECESSARY to indicate this.

I've gone through the core TX paths and it would appear that
all of them can handle packets with CHECKSUM_UNNECESSARY.  They
will be treated in the same way as CHECKSUM_NONE.

In fact most drivers can handle this too, since they just do
skb->ip_summed == CHECKSUM_PARTIAL and treat everything else
as the same.  Unfortunately, some drivers do the opposite check.

So for the time being, we need to ensure that by the time the skb
hits the driver ip_summed is either CHECKSUM_PARTIAL or CHECKSUM_NONE.

net: Sanitise ip_summed before ndo_start_xmit

We would like to use CHECKSUM_UNNECESSARY on the TX path to mark
packets that are known to have a correct checksum.  However, some
drivers cannot handle this value.

This patch ensures that the value of ip_summed is one that all
drivers can handle before calling ndo_start_xmit.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/core/dev.c b/net/core/dev.c
index 1c8a0ce..2784298 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1877,6 +1877,13 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 		if (dev->priv_flags & IFF_XMIT_DST_RELEASE)
 			skb_dst_drop(skb);
 
+		/*
+		 * When drivers are ready for all possible values of
+		 * ip_summed we can remove this.
+		 */
+		if (skb->ip_summed != CHECKSUM_PARTIAL)
+			skb->ip_summed = CHECKSUM_NONE;
+
 		rc = ops->ndo_start_xmit(skb, dev);
 		if (rc == NETDEV_TX_OK)
 			txq_trans_update(txq);

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] tcp: Set CHECKSUM_UNNECESSARY in tcp_init_nondata_skb

[Herbert Xu]

On Thu, Apr 08, 2010 at 09:57:15PM +0800, Herbert Xu wrote:
> 
> The problem here is that for non-data packets CHECKSUM_PARTIAL
> can actually end up being worse if we wind up going out through
> an interface that doesn't support checksums.

I don't know what I was thinking but the above is totally wrong.
CHECKSUM_PARTIAL should be just fine on non-checksuming interfaces
as we'll checksum everything once just as the CHECKSUM_NONE case
would.

So with that in mind, we don't need my CHECKSUM_UNNECESSARY patch
at all and your CHECKSUM_PARTIAL path is the right solution after
all :)

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] tcp: Set CHECKSUM_UNNECESSARY in tcp_init_nondata_skb

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 8 Apr 2010 23:39:06 +0800

> On Thu, Apr 08, 2010 at 09:57:15PM +0800, Herbert Xu wrote:
>> 
>> The problem here is that for non-data packets CHECKSUM_PARTIAL
>> can actually end up being worse if we wind up going out through
>> an interface that doesn't support checksums.
> 
> I don't know what I was thinking but the above is totally wrong.
> CHECKSUM_PARTIAL should be just fine on non-checksuming interfaces
> as we'll checksum everything once just as the CHECKSUM_NONE case
> would.
> 
> So with that in mind, we don't need my CHECKSUM_UNNECESSARY patch
> at all and your CHECKSUM_PARTIAL path is the right solution after
> all :)

Ok, thanks for doing all of the analysis :)

That still leaves that MC loopback code in ip_dev_loopback_xmit()
which still sets CHECKSUM_UNNECESSARY unconditionally.

Should it do like the loopback driver and just leave the ip_summed
value alone?

Re: [PATCH] tcp: Set CHECKSUM_UNNECESSARY in tcp_init_nondata_skb

[Herbert Xu]

On Thu, Apr 08, 2010 at 11:34:20AM -0700, David Miller wrote:
>
> That still leaves that MC loopback code in ip_dev_loopback_xmit()
> which still sets CHECKSUM_UNNECESSARY unconditionally.
> 
> Should it do like the loopback driver and just leave the ip_summed
> value alone?

Yes I think so.

It will mean an unnecessary verification on the receiving end,
but we could always add CHECKSUM_PARTIAL to the spots that we
care about like your patch did for TCP.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt