[PATCH] ICMP: ICMP_MIB_OUTMSGS increment duplicated

[PATCH] ICMP: ICMP_MIB_OUTMSGS increment duplicated

[Wang Chen]

In tree net-2.6.25, commit "96793b482540f3a26e2188eaf75cb56b7829d3e3"
made a mistake.

In that patch, David L added a icmp_out_count() in ip_push_pending_frames(),
remove icmp_out_count() from icmp_reply(). But he forgot to remove 
icmp_out_count() from icmp_send() too.
Since icmp_send and icmp_reply will call icmp_push_reply, which will call
ip_push_pending_frames, a duplicated increment happened in icmp_send.

This patch remove the icmp_out_count from icmp_send too.

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
diff -Nurp linux-2.6.24.rc8.org/net/ipv4/icmp.c linux-2.6.24.rc8/net/ipv4/icmp.c
--- linux-2.6.24.rc8.org/net/ipv4/icmp.c	2008-01-16 17:45:02.000000000 +0800
+++ linux-2.6.24.rc8/net/ipv4/icmp.c	2008-01-16 17:52:13.000000000 +0800
@@ -540,7 +540,6 @@ void icmp_send(struct sk_buff *skb_in, i
 	icmp_param.data.icmph.checksum	 = 0;
 	icmp_param.skb	  = skb_in;
 	icmp_param.offset = skb_network_offset(skb_in);
-	icmp_out_count(icmp_param.data.icmph.type);
 	inet_sk(icmp_socket->sk)->tos = tos;
 	ipc.addr = iph->saddr;
 	ipc.opt = &icmp_param.replyopts;

--
WCN

Re: [PATCH] ICMP: ICMP_MIB_OUTMSGS increment duplicated

[Herbert Xu]

Wang Chen <wangchen@cn.fujitsu.com> wrote:
> In tree net-2.6.25, commit "96793b482540f3a26e2188eaf75cb56b7829d3e3"
> made a mistake.
> 
> In that patch, David L added a icmp_out_count() in ip_push_pending_frames(),
> remove icmp_out_count() from icmp_reply(). But he forgot to remove 
> icmp_out_count() from icmp_send() too.
> Since icmp_send and icmp_reply will call icmp_push_reply, which will call
> ip_push_pending_frames, a duplicated increment happened in icmp_send.

Actually having the icmp_out_count call in ip_push_pending_frames seems
inconsistent.  Having it there means that we count raw socket ICMP packets
too.  But we don't do that for any other protocol, e.g., raw UDP packets
don't get counted.

So was the inclusion of raw ICMP packets intentional?

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] ICMP: ICMP_MIB_OUTMSGS increment duplicated

[David Stevens]

Herbert Xu <herbert@gondor.apana.org.au> wrote on 01/16/2008 03:49:01 AM:

> Actually having the icmp_out_count call in ip_push_pending_frames seems
> inconsistent.  Having it there means that we count raw socket ICMP 
packets
> too.  But we don't do that for any other protocol, e.g., raw UDP packets
> don't get counted.

Herbert,
        The patch was to support the ICMPMsgStats table. Since none of 
certain
types of output ICMP messages are generated by the kernel, but are 
required
by the RFC, counting raw sockets is intentional (and the only way those 
ICMP
types can be counted at all).
        Raw UDP packets would not be counted either before or after the 
patch,
but aren't part of the ICMPMsgStats table. Adding those might be 
worthwhile,
but it isn't quite the hole that the ICMP out stats were, since there is a
cooked interface for UDP output that counts the common use, at least.

Wang,
        I think your patch is correct; did you test the same case for 
IPv6?

 +-DLS

Re: [PATCH] ICMP: ICMP_MIB_OUTMSGS increment duplicated

[Herbert Xu]

David Stevens <dlstevens@us.ibm.com> wrote:
>
>        The patch was to support the ICMPMsgStats table. Since none of 
> certain
> types of output ICMP messages are generated by the kernel, but are 
> required
> by the RFC, counting raw sockets is intentional (and the only way those 
> ICMP
> types can be counted at all).
>        Raw UDP packets would not be counted either before or after the 
> patch,
> but aren't part of the ICMPMsgStats table. Adding those might be 
> worthwhile,
> but it isn't quite the hole that the ICMP out stats were, since there is a
> cooked interface for UDP output that counts the common use, at least.

Fair enough.  How about moving this code back into icmp.c and just
add a new count call in raw.c? The push pending function is used on
the UDP fast path so the leaner it is the better.

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: [PATCH] ICMP: ICMP_MIB_OUTMSGS increment duplicated

[David Stevens]

netdev-owner@vger.kernel.org wrote on 01/16/2008 03:17:29 PM:

> Fair enough.  How about moving this code back into icmp.c and just
> add a new count call in raw.c? The push pending function is used on
> the UDP fast path so the leaner it is the better.

        I started out with it there, but it certainly wasn't
cleaner. You have to peek on the queue, which I didn't
want to do for all the SMP issues that brings in, and
then replicate all the code push_pending_frames does to
get you a protocol header pointer in one buffer, and there
are multiple ways in the raw path to get you there (so the
counter code itself would appear in multiple places).
        I don't think the 2 instructions measurably impact
anything in the fast path and that is the earliest common
point for the multiple paths to generate ICMP output where
the header and type are available without replicating code.
So, simplicity is exactly why I put it there, rather than
the "obvious" places at the higher layer. :-) It's probably
the better, single place to put the other protocol "out messages"
counters, too, since those probably also have multiple paths
that end up here, but ICMPMsgStats was all that patch was
after.


                                                +-DLS

Re: [PATCH] ICMP: ICMP_MIB_OUTMSGS increment duplicated

[Wang Chen]

David Stevens said the following on 2008-1-17 0:23:
> Wang,
>         I think your patch is correct; did you test the same case for 
> IPv6?
> 

I've tested IPv4, but IPv6 not yet.
If Davem accept this one, I will see the IPv6, or you take care of it.

--
WCN

[PATCH 1/2] IPV6: ICMP6_MIB_OUTMSGS increment duplicated

[Wang Chen]

[IPV6]: ICMP6_MIB_OUTMSGS increment duplicated

icmpv6_send() calls ip6_push_pending_frames() indirectly.
Both ip6_push_pending_frames() and icmpv6_send() increment
counter ICMP6_MIB_OUTMSGS.

This patch remove the increment from icmpv6_send.

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>

--- linux-2.6.24.rc8.org/net/ipv6/icmp.c	2008-01-16 17:45:03.000000000 +0800
+++ linux-2.6.24.rc8/net/ipv6/icmp.c	2008-01-21 15:56:06.000000000 +0800
@@ -458,8 +458,6 @@ void icmpv6_send(struct sk_buff *skb, in
 	}
 	err = icmpv6_push_pending_frames(sk, &fl, &tmp_hdr, len + sizeof(struct icmp6hdr));
 
-	ICMP6_INC_STATS_BH(idev, ICMP6_MIB_OUTMSGS);
-
 out_put:
 	if (likely(idev != NULL))
 		in6_dev_put(idev);

Re: [PATCH 1/2] IPV6: ICMP6_MIB_OUTMSGS increment duplicated

[YOSHIFUJI Hideaki / 吉藤英明]

In article <479469F8.4010203@cn.fujitsu.com> (at Mon, 21 Jan 2008 17:46:32 +0800), Wang Chen <wangchen@cn.fujitsu.com> says:

> [IPV6]: ICMP6_MIB_OUTMSGS increment duplicated
> 
> icmpv6_send() calls ip6_push_pending_frames() indirectly.
> Both ip6_push_pending_frames() and icmpv6_send() increment
> counter ICMP6_MIB_OUTMSGS.
> 
> This patch remove the increment from icmpv6_send.
> 
> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
Acked-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>

--yoshfuji

Re: [PATCH 1/2] IPV6: ICMP6_MIB_OUTMSGS increment duplicated

[David Miller]

From: Wang Chen <wangchen@cn.fujitsu.com>
Date: Mon, 21 Jan 2008 17:46:32 +0800

> [IPV6]: ICMP6_MIB_OUTMSGS increment duplicated
> 
> icmpv6_send() calls ip6_push_pending_frames() indirectly.
> Both ip6_push_pending_frames() and icmpv6_send() increment
> counter ICMP6_MIB_OUTMSGS.
> 
> This patch remove the increment from icmpv6_send.
> 
> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>

Which tree are these two changes targetted at?

Re: [PATCH 1/2] IPV6: ICMP6_MIB_OUTMSGS increment duplicated

[Wang Chen]

David Miller said the following on 2008-1-21 18:57:
>> [IPV6]: ICMP6_MIB_OUTMSGS increment duplicated
>>
>> icmpv6_send() calls ip6_push_pending_frames() indirectly.
>> Both ip6_push_pending_frames() and icmpv6_send() increment
>> counter ICMP6_MIB_OUTMSGS.
>>
>> This patch remove the increment from icmpv6_send.
>>
>> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
> 
> Which tree are these two changes targetted at?
> 
> 

Dave, because the network of my working place can't clone git
tree, I used 2.6.24-rc8 to make the patches.
Can you apply them to net-2.6 tree?

Re: [PATCH 1/2] IPV6: ICMP6_MIB_OUTMSGS increment duplicated

[David Miller]

From: Wang Chen <wangchen@cn.fujitsu.com>
Date: Mon, 21 Jan 2008 18:59:43 +0800

> Dave, because the network of my working place can't clone git
> tree, I used 2.6.24-rc8 to make the patches.

Does someone know how to setup GIT to update the HTTP
fetch information as a post-push hook?

> Can you apply them to net-2.6 tree?

Done.

Re: [PATCH 1/2] IPV6: ICMP6_MIB_OUTMSGS increment duplicated

[Wang Chen]

David Miller said the following on 2008-1-21 19:06:
> Does someone know how to setup GIT to update the HTTP
> fetch information as a post-push hook?
> 

Seems Herbert knows how to do it.
Maybe run git-update-server-info every time when you rebase
your tree.

>> Can you apply them to net-2.6 tree?
> 
> Done.
> 
Thanks.

--
WCN

[PATCH 2/2] IPV6: RFC 2011 compatibility broken

[Wang Chen]

[IPV6]: RFC 2011 compatibility broken

The snmp6 entry name was changed, and it broke compatibility
to RFC 2011.

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>

--- linux-2.6.24.rc8.org/net/ipv6/proc.c	2008-01-16 17:45:03.000000000 +0800
+++ linux-2.6.24.rc8/net/ipv6/proc.c	2008-01-21 15:48:52.000000000 +0800
@@ -88,7 +88,7 @@ static char *icmp6type2name[256] = {
 	[ICMPV6_PKT_TOOBIG] = "PktTooBigs",
 	[ICMPV6_TIME_EXCEED] = "TimeExcds",
 	[ICMPV6_PARAMPROB] = "ParmProblems",
-	[ICMPV6_ECHO_REQUEST] = "EchoRequest",
+	[ICMPV6_ECHO_REQUEST] = "Echos",
 	[ICMPV6_ECHO_REPLY] = "EchoReplies",
 	[ICMPV6_MGM_QUERY] = "GroupMembQueries",
 	[ICMPV6_MGM_REPORT] = "GroupMembResponses",
@@ -98,7 +98,7 @@ static char *icmp6type2name[256] = {
 	[NDISC_ROUTER_SOLICITATION] = "RouterSolicits",
 	[NDISC_NEIGHBOUR_ADVERTISEMENT] = "NeighborAdvertisements",
 	[NDISC_NEIGHBOUR_SOLICITATION] = "NeighborSolicits",
-	[NDISC_REDIRECT] = "NeighborRedirects",
+	[NDISC_REDIRECT] = "Redirects",
 };
 
 

Re: [PATCH 2/2] IPV6: RFC 2011 compatibility broken

[YOSHIFUJI Hideaki / 吉藤英明]

In article <47946A04.3080702@cn.fujitsu.com> (at Mon, 21 Jan 2008 17:46:44 +0800), Wang Chen <wangchen@cn.fujitsu.com> says:

> The snmp6 entry name was changed, and it broke compatibility
> to RFC 2011.
> 
> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
Acked-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>

--yoshfuji

Re: [PATCH 2/2] IPV6: RFC 2011 compatibility broken

[David Stevens]

RFC 2011 doesn't apply to IPv6, and the internal names of /proc entries
are not used by the SNMP protocol, but it is an unintentional 
incompatibility with the
previous Linux entry names, so I agree. :-)

                                        +-DLS

Acked-by: David L Stevens <dlstevens@us.ibm.com>

netdev-owner@vger.kernel.org wrote on 01/21/2008 01:46:44 AM:

> [IPV6]: RFC 2011 compatibility broken
> 
> The snmp6 entry name was changed, and it broke compatibility
> to RFC 2011.
> 
> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
>

Re: [PATCH] ICMP: ICMP_MIB_OUTMSGS increment duplicated

[Wang Chen]

Dave, how about this one.
It's like that one of IPV6.

Wang Chen said the following on 2008-1-16 17:59:
> In tree net-2.6.25, commit "96793b482540f3a26e2188eaf75cb56b7829d3e3"
> made a mistake.
> 
> In that patch, David L added a icmp_out_count() in ip_push_pending_frames(),
> remove icmp_out_count() from icmp_reply(). But he forgot to remove 
> icmp_out_count() from icmp_send() too.
> Since icmp_send and icmp_reply will call icmp_push_reply, which will call
> ip_push_pending_frames, a duplicated increment happened in icmp_send.
> 
> This patch remove the icmp_out_count from icmp_send too.
> 
> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
> ---
> diff -Nurp linux-2.6.24.rc8.org/net/ipv4/icmp.c linux-2.6.24.rc8/net/ipv4/icmp.c
> --- linux-2.6.24.rc8.org/net/ipv4/icmp.c	2008-01-16 17:45:02.000000000 +0800
> +++ linux-2.6.24.rc8/net/ipv4/icmp.c	2008-01-16 17:52:13.000000000 +0800
> @@ -540,7 +540,6 @@ void icmp_send(struct sk_buff *skb_in, i
>  	icmp_param.data.icmph.checksum	 = 0;
>  	icmp_param.skb	  = skb_in;
>  	icmp_param.offset = skb_network_offset(skb_in);
> -	icmp_out_count(icmp_param.data.icmph.type);
>  	inet_sk(icmp_socket->sk)->tos = tos;
>  	ipc.addr = iph->saddr;
>  	ipc.opt = &icmp_param.replyopts;
> 

Re: [PATCH] ICMP: ICMP_MIB_OUTMSGS increment duplicated

[David Miller]

From: Wang Chen <wangchen@cn.fujitsu.com>
Date: Mon, 21 Jan 2008 19:15:07 +0800

> Dave, how about this one.
> It's like that one of IPV6.

I am confused, this changelog for this patch mentions changes made
only in the net-2.6.25 tree.

Yet you just told me the two other patches should be applied to
net-2.6

What is the exact story here?

Thank you.

Re: [PATCH] ICMP: ICMP_MIB_OUTMSGS increment duplicated

[Wang Chen]

David Miller said the following on 2008-1-21 19:25:
> I am confused, this changelog for this patch mentions changes made
> only in the net-2.6.25 tree.
> 

Although I wrote that I find David.S's patch in net-2.6.25 was wrong,
the guilty patch also in net-2.6 tree. Here is the web link.
http://git.kernel.org/?p=linux/kernel/git/davem/net-2.6.git;a=commit;h=96793b482540f3a26e2188eaf75cb56b7829d3e3 

So, I think apply my patches to net-2.6 is ok.

Re: [PATCH] ICMP: ICMP_MIB_OUTMSGS increment duplicated

[David Miller]

From: Wang Chen <wangchen@cn.fujitsu.com>
Date: Mon, 21 Jan 2008 19:34:56 +0800

> David Miller said the following on 2008-1-21 19:25:
> > I am confused, this changelog for this patch mentions changes made
> > only in the net-2.6.25 tree.
> > 
> 
> Although I wrote that I find David.S's patch in net-2.6.25 was wrong,
> the guilty patch also in net-2.6 tree. Here is the web link.
> http://git.kernel.org/?p=linux/kernel/git/davem/net-2.6.git;a=commit;h=96793b482540f3a26e2188eaf75cb56b7829d3e3 
> 
> So, I think apply my patches to net-2.6 is ok.

Thank you for explaining this.

Patch applied to net-2.6