[UDP6]: Restore sk_filter optimisation

[UDP6]: Restore sk_filter optimisation

[Herbert Xu]

Hi Dave:

[UDP6]: Restore sk_filter optimisation

This reverts the changeset

    [IPV6]: UDPv6 checksum.

    We always need to check UDPv6 checksum because it is mandatory.

The sk_filter optimisation has nothing to do whether we verify the
checksum.  It simply postpones it to the point when the user calls
recv or poll.

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

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
--
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 0ad4719..4474480 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -279,8 +279,10 @@ int udpv6_queue_rcv_skb(struct sock * sk, struct sk_buff *skb)
 		}
 	}
 
-	if (udp_lib_checksum_complete(skb))
-		goto drop;
+	if (sk->sk_filter) {
+		if (udp_lib_checksum_complete(skb))
+			goto drop;
+	}
 
 	if ((rc = sock_queue_rcv_skb(sk,skb)) < 0) {
 		/* Note that an ENOMEM error is charged twice */

Re: [UDP6]: Restore sk_filter optimisation

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 6 Mar 2007 12:20:10 +1100

> Hi Dave:
> 
> [UDP6]: Restore sk_filter optimisation
> 
> This reverts the changeset
> 
>     [IPV6]: UDPv6 checksum.
> 
>     We always need to check UDPv6 checksum because it is mandatory.
> 
> The sk_filter optimisation has nothing to do whether we verify the
> checksum.  It simply postpones it to the point when the user calls
> recv or poll.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, thanks.

Re: [UDP6]: Restore sk_filter optimisation

[Mitsuru Chinen]

Hello Herbert,

Let me ask a question about this patch.
After this patch was applied, 2 of the protocol stack behaviors were
changed when it receives a UDP datagram with broken checksum:

 1. udp6InDatagrams is incremented instead of udpInErrors
 2. In userland, recvfrom() replies an error with EAGAIN.
    recvfrom() wasn't aware of such a packet before.

Are these changes intentional?

Best Regards,
----
Mitsuru Chinen <mitch@linux.vnet.ibm.com>


On Tue, 6 Mar 2007 12:20:10 +1100
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> Hi Dave:
> 
> [UDP6]: Restore sk_filter optimisation
> 
> This reverts the changeset
> 
>     [IPV6]: UDPv6 checksum.
> 
>     We always need to check UDPv6 checksum because it is mandatory.
> 
> The sk_filter optimisation has nothing to do whether we verify the
> checksum.  It simply postpones it to the point when the user calls
> recv or poll.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> 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
> --
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 0ad4719..4474480 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -279,8 +279,10 @@ int udpv6_queue_rcv_skb(struct sock * sk, struct sk_buff *skb)
>  		}
>  	}
> 
> -	if (udp_lib_checksum_complete(skb))
> -		goto drop;
> +	if (sk->sk_filter) {
> +		if (udp_lib_checksum_complete(skb))
> +			goto drop;
> +	}
> 
>  	if ((rc = sock_queue_rcv_skb(sk,skb)) < 0) {
>  		/* Note that an ENOMEM error is charged twice */

Re: [UDP6]: Restore sk_filter optimisation

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20071029153320.d2c00f62.mitch@linux.vnet.ibm.com> (at Mon, 29 Oct 2007 15:33:20 +0900), Mitsuru Chinen <mitch@linux.vnet.ibm.com> says:

> Hello Herbert,
> 
> Let me ask a question about this patch.
> After this patch was applied, 2 of the protocol stack behaviors were
> changed when it receives a UDP datagram with broken checksum:
> 
>  1. udp6InDatagrams is incremented instead of udpInErrors
>  2. In userland, recvfrom() replies an error with EAGAIN.
>     recvfrom() wasn't aware of such a packet before.
> 
> Are these changes intentional?

And, we're not sure how much the "optimization"'s benefit is.
It is even worse when we are handling multicast packets.

--yoshfuji

Re: [UDP6]: Restore sk_filter optimisation

[Herbert Xu]

On Mon, Oct 29, 2007 at 03:33:20PM +0900, Mitsuru Chinen wrote:
> Hello Herbert,
> 
> Let me ask a question about this patch.
> After this patch was applied, 2 of the protocol stack behaviors were
> changed when it receives a UDP datagram with broken checksum:
> 
>  1. udp6InDatagrams is incremented instead of udpInErrors
>  2. In userland, recvfrom() replies an error with EAGAIN.
>     recvfrom() wasn't aware of such a packet before.
> 
> Are these changes intentional?

It wasn't my intention if that's what you mean :)

However, this would've happened with the old code anyway if
someone had a filter attached so this isn't new.

If it's a problem then we should just get it fixed.

Thanks,
-- 
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: [UDP6]: Restore sk_filter optimisation

[Mitsuru Chinen]

On Mon, 29 Oct 2007 20:53:28 +0800
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Mon, Oct 29, 2007 at 03:33:20PM +0900, Mitsuru Chinen wrote:
> > Hello Herbert,
> > 
> > Let me ask a question about this patch.
> > After this patch was applied, 2 of the protocol stack behaviors were
> > changed when it receives a UDP datagram with broken checksum:
> > 
> >  1. udp6InDatagrams is incremented instead of udpInErrors
> >  2. In userland, recvfrom() replies an error with EAGAIN.
> >     recvfrom() wasn't aware of such a packet before.
> > 
> > Are these changes intentional?
> 
> It wasn't my intention if that's what you mean :)
> 
> However, this would've happened with the old code anyway if
> someone had a filter attached so this isn't new.
>
> If it's a problem then we should just get it fixed.

As far as I tested, this doesn't happen with the old code even if
a filter is attached. However, this happen with the new code
without a filter and I don't see this rather when a filter is
attached. So, I'm afraid it's new.

By the way, could you answer the Yoshifuji-san's question?
I think the code where we should fix depends on this. 

On Mon, 29 Oct 2007 15:41:50 +0900 (JST)
YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@linux-ipv6.org> wrote:

> And, we're not sure how much the "optimization"'s benefit is.
> It is even worse when we are handling multicast packets.

Thank you,
----
Mitsuru Chinen <mitch@linux.vnet.ibm.com>

Re: [UDP6]: Restore sk_filter optimisation

[Herbert Xu]

On Wed, Oct 31, 2007 at 11:05:45PM +0900, Mitsuru Chinen wrote:
>
> > >  1. udp6InDatagrams is incremented instead of udpInErrors
> > >  2. In userland, recvfrom() replies an error with EAGAIN.
> > >     recvfrom() wasn't aware of such a packet before.
> > > 
> > > Are these changes intentional?
>
> As far as I tested, this doesn't happen with the old code even if
> a filter is attached. However, this happen with the new code
> without a filter and I don't see this rather when a filter is
> attached. So, I'm afraid it's new.

Sorry, I read the patch the wrong way around :)

1) is just an accounting issue.  It shouldn't be too difficult
to fix it up.  In fact, I think udpInErrors will still be
incremented once we detect the error.

2) shouldn't be an issue because we've already solved the
problem by making poll/select do the checksum verification
before indiciating that the socket is readable.

> > And, we're not sure how much the "optimization"'s benefit is.
> > It is even worse when we are hand

The checksum verification is costly because we have to bring
the payload into cache.  Since filters are very rare it's
worthwhile to postpone the checksum verification for the common
case.

Also as a general rule, we want to avoid divergent behaviour
between IPv4 and IPv6.  So for changes like this we should
really modify both stacks in future rather than have each
stack do its own thing.

Thanks,
-- 
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: [UDP6]: Restore sk_filter optimisation

[Mitsuru Chinen]

On Wed, 31 Oct 2007 22:42:57 +0800
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Wed, Oct 31, 2007 at 11:05:45PM +0900, Mitsuru Chinen wrote:
> >
> > > >  1. udp6InDatagrams is incremented instead of udpInErrors
> > > >  2. In userland, recvfrom() replies an error with EAGAIN.
> > > >     recvfrom() wasn't aware of such a packet before.
> > > > 
> > > > Are these changes intentional?
> >
> > As far as I tested, this doesn't happen with the old code even if
> > a filter is attached. However, this happen with the new code
> > without a filter and I don't see this rather when a filter is
> > attached. So, I'm afraid it's new.
> 
> Sorry, I read the patch the wrong way around :)
> 
> 1) is just an accounting issue.  It shouldn't be too difficult
> to fix it up.  In fact, I think udpInErrors will still be
> incremented once we detect the error.
> 
> 2) shouldn't be an issue because we've already solved the
> problem by making poll/select do the checksum verification
> before indiciating that the socket is readable.
> 
> > > And, we're not sure how much the "optimization"'s benefit is.
> > > It is even worse when we are hand
> 
> The checksum verification is costly because we have to bring
> the payload into cache.  Since filters are very rare it's
> worthwhile to postpone the checksum verification for the common
> case.
> 
> Also as a general rule, we want to avoid divergent behaviour
> between IPv4 and IPv6.  So for changes like this we should
> really modify both stacks in future rather than have each
> stack do its own thing.

I got it. OK. I will submit a patch to postpone the udpInError
counter incrementation, either.

Thanks for your detailed explanation!

Best Regards,
----
Mitsuru Chinen <mitch@linux.vnet.ibm.com>