[RFC] [PATCH] ip: skip IP checksum for skbs with CHECKSUM_UNNECESSARY set

[RFC] [PATCH] ip: skip IP checksum for skbs with CHECKSUM_UNNECESSARY set

[Octavian Purdila]

We are working on a completely in hardware LRO implementation and this patch 
would simplify the hardware implementation. Is this acceptable?

--- a/net/ipv4/ip_input.c
+++ b/net/ipv4/ip_input.c
@@ -415,7 +415,8 @@ int ip_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt,
 
        iph = ip_hdr(skb);
 
-       if (unlikely(ip_fast_csum((u8 *)iph, iph->ihl)))
+       if (!skb_csum_unnecessary(skb) &&
+           unlikely(ip_fast_csum((u8 *)iph, iph->ihl)))
                goto inhdr_error;
 
        len = ntohs(iph->tot_len);

Re: [RFC] [PATCH] ip: skip IP checksum for skbs with CHECKSUM_UNNECESSARY set

[Stephen Hemminger]

On Fri, 22 Aug 2008 22:24:31 +0300
Octavian Purdila <opurdila@ixiacom.com> wrote:

> 
> We are working on a completely in hardware LRO implementation and this patch 
> would simplify the hardware implementation. Is this acceptable?
> 
> --- a/net/ipv4/ip_input.c
> +++ b/net/ipv4/ip_input.c
> @@ -415,7 +415,8 @@ int ip_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt,
>  
>         iph = ip_hdr(skb);
>  
> -       if (unlikely(ip_fast_csum((u8 *)iph, iph->ihl)))
> +       if (!skb_csum_unnecessary(skb) &&
> +           unlikely(ip_fast_csum((u8 *)iph, iph->ihl)))
>                 goto inhdr_error;
>  
>         len = ntohs(iph->tot_len);
> 
> --
> 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

The overhead of the additional conditional might outweigh any benefit.

Re: [RFC] [PATCH] ip: skip IP checksum for skbs with CHECKSUM_UNNECESSARY set

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Fri, 22 Aug 2008 18:33:20 -0400

> On Fri, 22 Aug 2008 22:24:31 +0300
> Octavian Purdila <opurdila@ixiacom.com> wrote:
> 
> > 
> > We are working on a completely in hardware LRO implementation and this patch 
> > would simplify the hardware implementation. Is this acceptable?
> > 
> > --- a/net/ipv4/ip_input.c
> > +++ b/net/ipv4/ip_input.c
> > @@ -415,7 +415,8 @@ int ip_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt,
> >  
> >         iph = ip_hdr(skb);
> >  
> > -       if (unlikely(ip_fast_csum((u8 *)iph, iph->ihl)))
> > +       if (!skb_csum_unnecessary(skb) &&
> > +           unlikely(ip_fast_csum((u8 *)iph, iph->ihl)))
> >                 goto inhdr_error;
> >  
> >         len = ntohs(iph->tot_len);
> > 
> > --
> > 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
> 
> The overhead of the additional conditional might outweigh any benefit.

I don't think they want this for performance, they want to not have to
compute the IP header checksum in their HW LRO implementation, which
is just as silly :-)

Re: [RFC] [PATCH] ip: skip IP checksum for skbs with CHECKSUM_UNNECESSARY set

[Stephen Hemminger]

On Fri, 22 Aug 2008 17:36:25 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Fri, 22 Aug 2008 18:33:20 -0400
> 
> > On Fri, 22 Aug 2008 22:24:31 +0300
> > Octavian Purdila <opurdila@ixiacom.com> wrote:
> > 
> > > 
> > > We are working on a completely in hardware LRO implementation and this patch 
> > > would simplify the hardware implementation. Is this acceptable?
> > > 
> > > --- a/net/ipv4/ip_input.c
> > > +++ b/net/ipv4/ip_input.c
> > > @@ -415,7 +415,8 @@ int ip_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt,
> > >  
> > >         iph = ip_hdr(skb);
> > >  
> > > -       if (unlikely(ip_fast_csum((u8 *)iph, iph->ihl)))
> > > +       if (!skb_csum_unnecessary(skb) &&
> > > +           unlikely(ip_fast_csum((u8 *)iph, iph->ihl)))
> > >                 goto inhdr_error;
> > >  
> > >         len = ntohs(iph->tot_len);
> > > 
> > > --
> > > 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
> > 
> > The overhead of the additional conditional might outweigh any benefit.
> 
> I don't think they want this for performance, they want to not have to
> compute the IP header checksum in their HW LRO implementation, which
> is just as silly :-)

And braindead for any other uses (like filtering or forwarding).

Re: [RFC] [PATCH] ip: skip IP checksum for skbs with CHECKSUM_UNNECESSARY set

[Herbert Xu]

David Miller <davem@davemloft.net> wrote:
>
> I don't think they want this for performance, they want to not have to
> compute the IP header checksum in their HW LRO implementation, which
> is just as silly :-)

I sure hope they're not planning on leaving out the IP header
checksum verification too :)
-- 
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: [RFC] [PATCH] ip: skip IP checksum for skbs with CHECKSUM_UNNECESSARY set

[Octavian Purdila]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 23 Aug 2008 15:15:17 +1000

> > I don't think they want this for performance, they want to not have to
> > compute the IP header checksum in their HW LRO implementation, which
> > is just as silly :-)

Yes, this was the intention, it seemed like a reasonable trade-off between FPGA 
space & complexity / software complexity at the time. Thanks for the input, 
will do it in hardware then.

> I sure hope they're not planning on leaving out the IP header
> checksum verification too :)

Of course not :) 

tavi

Re: [RFC] [PATCH] ip: skip IP checksum for skbs with CHECKSUM_UNNECESSARY set

[David Miller]

From: Octavian Purdila <opurdila@ixiacom.com>
Date: Fri, 22 Aug 2008 22:24:31 +0300

> We are working on a completely in hardware LRO implementation and this patch 
> would simplify the hardware implementation. Is this acceptable?

This is not worth the bother, otherwise we would have done it a long
time ago.

Please provide a correct IP header checksum in your hw LRO implementation.