Re: [ PATCH 2.6.17-rc6 1/1] udp.c: counting InDatagrams which are never delivered

2006-06-12 Thread Gerrit Renker 
Quoting David Miller:
|  
|   Fix:   Move the `UDP_INC_STATS_BH(UDP_MIB_INDATAGRAMS)' statement from 
|  udp_queue_rcv_skb to udp_recvmsg. Now InDatagrams only counts those
|  datagrams which were really delivered (as per RFC 2013). 
|   
|  
|  Unfortunately this breaks NFS and other in-kernel UDP socket usages,
|  which never call recvmsg() and instead take the packet via the
|  -data_ready() callback done by sock_queue_receive_skb().
|  
|  Your patch will make the counter never get incremented when such
|  a user is using the UDP socket.
|  
|  Probably a better way to handle this is to correct the
|  INDATAGRAMS value by decrementing it when we notice that
|  the checksum is incorrect in a deferred manner.
This is clearly preferable - would it look like this:

csum_copy_err:
 UDP_INC_STATS_BH(UDP_MIB_INERRORS);
 UDP_DEC_STATS_BH(UDP_MIB_INDATAGRAMS); /* requires new macro */

 skb_kill_datagram(sk, skb, flags);
 /* ... */

in udp_recvmsg? Here I must pass - there is no xxx_DEC_BH macro in 
include/net/snmp.h and I don't know whether the following guess is correct:

#define SNMP_DEC_STATS_BH(mib, field)   \
(per_cpu_ptr(mib[0], raw_smp_processor_id())-mibs[field]--)

If this is correct, then it seems done; one could use this macro or add
a corresponding UDP_DEC_STATS_BH to include/net/udp.h .



-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ PATCH 2.6.17-rc6 1/1] udp.c: counting InDatagrams which are never delivered

2006-06-12 Thread David Miller 
From: Gerrit Renker [EMAIL PROTECTED]
Date: Mon, 12 Jun 2006 07:02:45 +0100

 This is clearly preferable - would it look like this:
 
 csum_copy_err:
  UDP_INC_STATS_BH(UDP_MIB_INERRORS);
  UDP_DEC_STATS_BH(UDP_MIB_INDATAGRAMS); /* requires new macro */
 
  skb_kill_datagram(sk, skb, flags);
  /* ... */
 
 in udp_recvmsg? Here I must pass - there is no xxx_DEC_BH macro in 
 include/net/snmp.h and I don't know whether the following guess is correct:
 
 #define SNMP_DEC_STATS_BH(mib, field) \
   (per_cpu_ptr(mib[0], raw_smp_processor_id())-mibs[field]--)
 
 If this is correct, then it seems done; one could use this macro or add
 a corresponding UDP_DEC_STATS_BH to include/net/udp.h .

The index of mib[] in those macros is always !in_softirq(), the
*_BH() variants use zero for the index because they are called in
contexts where we know that !in_sortirq() evaluates to false.

So your SNMP_DEC_STATS_BH() macro is correct.

Can you cook up the patch, which adds your SNMP_DEC_STATS_BH() macro,
the UDP_DEC_STATS_BH counterpart, and the change that uses it in
net/ipv4/udp.c?

I'd appreciate this, thanks a lot.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ PATCH 2.6.17-rc6 1/1] udp.c: counting InDatagrams which are never delivered

2006-06-12 Thread Herbert Xu 
David Miller [EMAIL PROTECTED] wrote:
 
 Probably a better way to handle this is to correct the
 INDATAGRAMS value by decrementing it when we notice that
 the checksum is incorrect in a deferred manner.

I think sunrpc should instead increment the appropriate counters directly
as otherwise checksum errors won't be recorded correctly for sunrpc packets.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ PATCH 2.6.17-rc6 1/1] udp.c: counting InDatagrams which are never delivered

2006-06-12 Thread David Miller 
From: Herbert Xu [EMAIL PROTECTED]
Date: Mon, 12 Jun 2006 16:18:09 +1000

 David Miller [EMAIL PROTECTED] wrote:
  
  Probably a better way to handle this is to correct the
  INDATAGRAMS value by decrementing it when we notice that
  the checksum is incorrect in a deferred manner.
 
 I think sunrpc should instead increment the appropriate counters directly
 as otherwise checksum errors won't be recorded correctly for sunrpc packets.

Yeah.  Good point.  But how much protocol internals do we want to
slide into the -data_ready() callbacks of such layers?  That's ugly
and something we should try to avoid.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ PATCH 2.6.17-rc6 1/1] udp.c: counting InDatagrams which are never delivered

2006-06-12 Thread Gerrit Renker 
The code below implements the discussed solution of decrementing
InDatagrams if a datagram fails the checksum within udp_recvmsg().

I have given it a quick test / build and checked the outcome against
previous results: I now obtained correct counter values, i.e. the application
counted exactly InDatagrams datagrams, whereas with the same settings before
this was not the case (datagrams with checksum errors were counted both as
InErrors and as InDatagrams). 

Will add this patch to http://bugzilla.kernel.org/show_bug.cgi?id=6660
where this problem is also described. Patches under 2.6.16 with no complaints.

Signed-off-by: Gerrit Renker [EMAIL PROTECTED]
---

 include/net/snmp.h |2 ++
 include/net/udp.h  |1 +
 net/ipv4/udp.c |1 +
 3 files changed, 4 insertions(+)


diff -Nurp  a/include/net/snmp.h b/include/net/snmp.h
--- a/include/net/snmp.h2006-06-05 21:52:55.0 +0100
+++ b/include/net/snmp.h2006-06-12 07:38:11.0 +0100
@@ -137,6 +137,8 @@ struct linux_mib {
(per_cpu_ptr(mib[!in_softirq()], raw_smp_processor_id())-mibs[field]++)
 #define SNMP_DEC_STATS(mib, field) \
(per_cpu_ptr(mib[!in_softirq()], raw_smp_processor_id())-mibs[field]--)
+#define SNMP_DEC_STATS_BH(mib, field)  \
+   (per_cpu_ptr(mib[0], raw_smp_processor_id())-mibs[field]--)
 #define SNMP_ADD_STATS_BH(mib, field, addend)  \
(per_cpu_ptr(mib[0], raw_smp_processor_id())-mibs[field] += addend)
 #define SNMP_ADD_STATS_USER(mib, field, addend)\
diff -Nurp  a/include/net/udp.h b/include/net/udp.h
--- a/include/net/udp.h 2006-06-06 18:04:36.0 +0100
+++ b/include/net/udp.h 2006-06-12 07:39:29.0 +0100
@@ -78,6 +78,7 @@ DECLARE_SNMP_STAT(struct udp_mib, udp_st
 #define UDP_INC_STATS(field)   SNMP_INC_STATS(udp_statistics, field)
 #define UDP_INC_STATS_BH(field)
SNMP_INC_STATS_BH(udp_statistics, field)
 #define UDP_INC_STATS_USER(field)  SNMP_INC_STATS_USER(udp_statistics, 
field)
+#define UDP_DEC_STATS_BH(field)
SNMP_DEC_STATS_BH(udp_statistics, field)
 
 /* /proc */
 struct udp_seq_afinfo {
diff -Nurp  a/net/ipv4/udp.c b/net/ipv4/udp.c
--- a/net/ipv4/udp.c2006-06-07 20:44:13.0 +0100
+++ b/net/ipv4/udp.c2006-06-12 07:40:02.0 +0100
@@ -846,6 +846,7 @@ out:
 
 csum_copy_err:
UDP_INC_STATS_BH(UDP_MIB_INERRORS);
+   UDP_DEC_STATS_BH(UDP_MIB_INDATAGRAMS);
 
skb_kill_datagram(sk, skb, flags);
 

-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ PATCH 2.6.17-rc6 1/1] udp.c: counting InDatagrams which are never delivered

2006-06-12 Thread Herbert Xu 
On Sun, Jun 11, 2006 at 11:49:05PM -0700, David Miller wrote:
 
 Yeah.  Good point.  But how much protocol internals do we want to
 slide into the -data_ready() callbacks of such layers?  That's ugly
 and something we should try to avoid.

I agree with the objective of minimising the exposure of internals.
However, in this particular instance we're already exposing much more
than a couple of UDP SNMP counters in the sunrpc code.  This makes
adjusting the counters there the most expedient course of action.

Longer term we probably want to restructure the code a bit so that
more if it moves to udp.c.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ PATCH 2.6.17-rc6 1/1] udp.c: counting InDatagrams which are never delivered

2006-06-11 Thread David Miller 
From: Gerrit Renker [EMAIL PROTECTED]
Date: Tue, 6 Jun 2006 19:25:40 +0100

 Fix:   Move the `UDP_INC_STATS_BH(UDP_MIB_INDATAGRAMS)' statement from 
udp_queue_rcv_skb to udp_recvmsg. Now InDatagrams only counts those
datagrams which were really delivered (as per RFC 2013). 
 
 Please CC: any correspondence to [EMAIL PROTECTED]  
 
 Signed-off-by: [EMAIL PROTECTED]

Unfortunately this breaks NFS and other in-kernel UDP socket usages,
which never call recvmsg() and instead take the packet via the
-data_ready() callback done by sock_queue_receive_skb().

Your patch will make the counter never get incremented when such
a user is using the UDP socket.

Probably a better way to handle this is to correct the
INDATAGRAMS value by decrementing it when we notice that
the checksum is incorrect in a deferred manner.

-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[ PATCH 2.6.17-rc6 1/1] udp.c: counting InDatagrams which are never delivered

2006-06-06 Thread Gerrit Renker 
This problem involves MIB counter inaccuracies triggered by 
failed UDP checksums.

Problem: ip_local_deliver_finish calls udp_rcv, which calls
 udp_queue_rcv_skb. 
 Unless the sk_filter is set, the checksum of the incoming
 UDP datagram is not verified. If there are no other problems
 InDatagrams (UDP_MIB_INDATAGRAMS) is then incremented.
 Now, if udp_recvmsg is called as a handler for incoming
 UDP datagrams, the checksum is verified for the first time
 (unless sk_filter was set) and if the checksum fails, the
 `goto csum_copy_err' leads to forcibly removing the datagram
 _and_ incrementing InErrors (UDP_MIB_INERRORS). 

Issue:   When problem occurs in the manner described, the datagram
 is counted twice: once as InDatagram and once as InErrors.
 RFC 2013 defines InDatagrams as counter for delivered datagrams;
 these datagrams are counted but never delivered.

How to reproduce: Send UDP datagrams with checksums enabled, use middlebox
 which corrupts part of the traffic (e.g. bit errors / NetEm) and
 use /proc/net/snmp to watch the counters. The sum of InErrors,
 NoPorts and InDatagrams exceeds the real number of sent datagrams 
 by the number of datagrams which were counted twice and forcibly
 removed by udp_recvmsg.
Non-occurrence: The problem does not occur if the sender disabled 
 UDP checksums (zero field; allowed for IPv4, but not for IPv6), 
 since then the checksum code returns success.

Fix:   Move the `UDP_INC_STATS_BH(UDP_MIB_INDATAGRAMS)' statement from 
   udp_queue_rcv_skb to udp_recvmsg. Now InDatagrams only counts those
   datagrams which were really delivered (as per RFC 2013). 

Please CC: any correspondence to [EMAIL PROTECTED]  

Signed-off-by: [EMAIL PROTECTED]

---

diff -Nurp  a/net/ipv4/udp.c b/net/ipv4/udp.c
--- a/net/ipv4/udp.c2006-06-06 17:01:26.0 +0100
+++ b/net/ipv4/udp.c2006-06-06 23:39:45.0 +0100
@@ -823,6 +823,7 @@ try_again:
goto out_free;
 
sock_recv_timestamp(msg, sk, skb);
+   UDP_INC_STATS_BH(UDP_MIB_INDATAGRAMS);
 
/* Copy the address. */
if (sin)
@@ -1032,7 +1033,6 @@ static int udp_queue_rcv_skb(struct sock
kfree_skb(skb);
return -1;
}
-   UDP_INC_STATS_BH(UDP_MIB_INDATAGRAMS);
return 0;
 }
 


-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html