diff mbox

tcp: Set CHECKSUM_UNNECESSARY in tcp_init_nondata_skb

Message ID 20100408135715.GA24890@gondor.apana.org.au
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Herbert Xu April 8, 2010, 1:57 p.m. UTC 
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>


Cheers,

Comments

Herbert Xu April 8, 2010, 3:39 p.m. UTC | #1 
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,
David Miller April 8, 2010, 6:34 p.m. UTC | #2 
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?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu April 9, 2010, 8:42 a.m. UTC | #3 
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,
diff mbox

Patch

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);