* Re: [PATCH 7/7] [TFRC] New rx history code
From: Gerrit Renker @ 2007-12-05 9:35 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, netdev, dccp
In-Reply-To: <20071204115939.GA18084@ghostprotocols.net>
Quoting Arnaldo:
| Em Tue, Dec 04, 2007 at 06:55:17AM +0000, Gerrit Renker escreveu:
| > NAK. You have changed the control flow of the algorithm and the underlying
| > data structure. Originally it had been an array of pointers, and this had
| > been a design decision right from the first submissions in March. From I
| > of http://www.erg.abdn.ac.uk/users/gerrit/dccp/notes/ccid3_packet_reception/loss_detection/loss_detection_algorithm_notes.txt
| >
| > "1. 'ring' is an array of pointers rather than a contiguous area in
| > memory. The reason is that, when having to swap adjacent entries
| > to sort the history, it is easier to swap pointers than to copy
| > (heavy-weight) history-entry data structs."
| >
| > But in your algorithm it becomes a normal linear array.
| >
| > As a consequence all subsequent code needs to be rewritten to use
| > copying instead of swapping pointers. And there is a lot of that, since
| > the loss detection code makes heavy use of swapping entries in order to
| > cope with reordered and/or delayed packets.
|
| So lets not do that, the decision was made based on the RX history patch
| alone, where, as pointed out, no swapping occurs.
|
| > This is not what I would call "entirely equivalent" as you claim. Your
| > algorithm is fundamentally different, the control flow is not the same,
| > nor are the underlying data structures.
|
| Its is equivalent up to what is in the tree, thank you for point out
| that in subsequent patches it will be used.
|
| Had this design decision been made explicit in the changeset comment
| that introduces the data structure I wouldn't have tried to reduce the
| number of allocations.
|
| And you even said that it was a good decision when first reacting to
| this change, which I saw as positive feedback for the RFC I sent.
|
That was my fault. Your solution "looked" much better to me but even I had forgotten
that the algorithm is based on an array of pointers. When I finally sat down
and looked through the patch set I found the original notes from March and
saw that using a linear array instead of an array of pointers will require quite
a few changes and means changing the algorithm. I then thought through whether there
could be any advantage in using a linear array as in this patch, but could find none.
In the end this went back to the same questions and issues that were tackled between
February and March this year. I wish we could have had this dicussion then; it would
have made this email unnecessary.
I thank you for your replay and I am sorry if you took offense at my perhaps a little strong
reaction, here is the essence what I tried to convey but what maybe did not succeed in conveying:
* I am absolutely ok with you making changes to variable names, namespaces, file organisation,
overall arrangement etc (as per previous email). You have experience and this will often be a
benefit. For instance, you combined tfrc_entry_from_skb() with tfrc_rx_hist_update() to give
a new function, tfrc_rx_hist_add_packet(). This is ok since entry_from_skb() is not otherwise
used (and I only found this out when reading your patch). (On the other hand, since this is a
4-element ring buffer, it is no real adding, the same entry will frequently be overwritten,
this is why it was initially called hist_update().)
* I am also not so much concerned about credit. As long as the (changed) end result is as
least as good as or hopefully better than the submitted input, the author name is a side
issue; so I do think that your approach is sound and fair. The only place where credits
are needed is for the SatSix project (www.ist-satsix.org) which funded this work. This is
why some of the copyrights now included "University of Aberdeen". What in any case I'd like
to add is at least the Signed-off-by: if it turns out to be a mess I want to contribute my
part of responsibility.
* But where I need to interact is when changes are made to the algorithm, even if these may
seem small. The underlying reasons that lead to the code may not be immediately clear,
but since this is a solution for a specific problem, there are also specific reasons; which
is why for algorithm changes (and underlying data structure) I'd ask you to discuss this first
before committing the patch.
* In part you are already doing this (message subject); it may be necessary to adapt the way
of discussion/presentation. The subject is complex (spent a week with the flow chart only)
and it is a lot - initially this had been 40 small patches.
| > | I modified it just to try to make it closer to the existing API, hide details from
| > | the CCIDs and fix a couple bugs found in the initial implementation.
| > What is "a couple bugs"? So far you have pointed out only one problem and that was
| > agreed, it can be fixed. But it remains a side issue, it is not a failure of the
|
| I pointed two problems one ended up being just the fact that tfrc_ewma
| checks if the average is zero for every calculation.
|
With regard to the tfrc_ewma we need to consider the discussion as I think we are having a
misunderstanding. It is necessary to
(a) deal with the case that a peer may be sending zero-sized packets:
- one solution is to simply omit the check "if (len > 0)" and declare 0-sized data
- I think you have a point in that the len check is indeed redundant when we are
already checking that this is a data packet
packets as pathological (which is probably what they are);
- a BUG_ON would be not such a good solution since 0-sized packets are legal;
- my suggestion is to remove the "len > 0" test - then tfrc_ewma() can also be
used directly, without the surrounding inline function wrapper.
(b) reconsider the control flow: in your patch set you introduced a different control flow
in the rx_packet_recv() function. I tried to figure out why this was introduced, there
are two problems, one not immediately obvious at this moment:
- it does not consider where and how loss detection is done (which is part of a later
patch). This is a key point why the flow may seem strange: loss detection must be
done on both non-data and data packets; there are several cases depending on whether
loss previously occurred; the accounting must also be done in parallel for data packets
(for instance s and bytes_recvd) and non-data packets (highest-received seqno is always
- it is not equivalent with the initial one, when comparing this to the flowchart:
http://www.erg.abdn.ac.uk/users/gerrit/dccp/notes/ccid3_packet_reception/reception-main-flowchart.png
| > I have provided documentation, written test modules, and am able to prove
| > that the algorithm as submitted works reasonably correct. In addition, the
| > behaviour has been verified using regression tests.
| >
| > I am not prepared to take your claims and expressions of "deepest
| > respect" at face value since your actions - not your words - show that
| > you are, in fact, not dealing respectfully with work which has taken
| > months to complete and verify.
| >
| > During 9 months on dccp@vger you did not provide so much as a paragraph
| > of feedback. Instead you mopped up code from the test tree, modified it
| > according to your own whims and now, in the circle of your
| > invitation-only buddies, start to talk about having discussions and
| > iterations. The only iteration I can see is in fixing up the things you
| > introduced, so it is not you who has to pay the price.
| >
| > Sorry, unless you can offer a more mature model of collaboration,
| > consider me out of this and the patches summarily withdrawn. I am not
| > prepared to throw away several months of work just because you feel
| > inspired to do as you please with the work of others.
|
| Again you want to have your patches accepted as-is. Pointed to one case
| where I gave you credit while improving on your work (TX history) and
| another where the changes were up for review. I don't consider this a
| warm welcome for me to finally dedicate time to this effort. Would you
| prefer to continue working without help? I think we should encourage
| more people to work on DCCP, you seem to think that changes to your work
| are not acceptable.
|
Yes and no. The internal algorithm I would rather have accepted as-is, unless
there are good reasons to change it. But this by no means means I want this
waved through like a diplomat's car at the border: there are reasons why
it is as it is, and I am happy to discuss these reasons; and if someone
can point out a better solution, will accept that. It is just that this
is a solution to a specific problem which so far has proven to work, and
I therefore think that we can spent the time more efficiently by not
fixing things that have already been fixed once: there are many more
problems in DCCP (just recently you mentioned robustness of memory usage,
then there is the ongoing discussion of how to integrate CCID4 as well);
I think we could divide up work time with more benefit.
| Perhaps others can comment on what is happening and help us find, as you
| say, to find a more mature model of collaboration.
|
No need to be cynical, you have replied gracefully and with substance.
For that I thank you. The topic is complicated, adapting how it is
treated may improve the discussion - for instance, if it helps, we could
switch back to smaller patch format as original, which you have already
been introducing.
Best regards
Gerrit
^ permalink raw reply
* Re: [PATCH net-2.6.25 1/11][CORE] Remove unneeded ifdefs from sysctl_net_core.c
From: David Miller @ 2007-12-05 9:36 UTC (permalink / raw)
To: xemul; +Cc: netdev, devel
In-Reply-To: <475525DA.3050002@openvz.org>
From: Pavel Emelyanov <xemul@openvz.org>
Date: Tue, 04 Dec 2007 13:03:06 +0300
> This file is already compiled out when the SYSCTL=n, so
> these ifdefs, that enclose the whole file, can be removed.
>
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
Applied.
^ permalink raw reply
* Re: [PATCH net-2.6.25 2/11][CORE] Isolate the net/core/ sysctl table
From: David Miller @ 2007-12-05 9:37 UTC (permalink / raw)
To: xemul; +Cc: netdev, devel
In-Reply-To: <47552633.8040306@openvz.org>
From: Pavel Emelyanov <xemul@openvz.org>
Date: Tue, 04 Dec 2007 13:04:35 +0300
> Using ctl paths we can put all the stuff, related to net/core/
> sysctl table, into one file and remove all the references on it.
>
> As a good side effect this hides the "core_table" name from
> the global scope :)
>
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
Applied.
^ permalink raw reply
* Re: [PATCH 2/3] [UDP]: Restore missing inDatagrams increments
From: Eric Dumazet @ 2007-12-05 9:36 UTC (permalink / raw)
To: Herbert Xu; +Cc: Wang Chen, David S. Miller, netdev
In-Reply-To: <20071205021559.GA27707@gondor.apana.org.au>
Herbert Xu a écrit :
> On Wed, Dec 05, 2007 at 09:42:45AM +0800, Wang Chen wrote:
>> I apologize for my miss in previous patch.
>>
>> Herbert, don't you think the udp6inDatagrams should be counted too
>> in xprtsock?
>
> Good point. Here's a better version.
>
> [UDP]: Restore missing inDatagrams increments
>
> The previous move of the the UDP inDatagrams counter caused the
> counting of encapsulated packets, SUNRPC data (as opposed to call)
> packets and RXRPC packets to go missing.
>
> This patch restores all of these.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> diff --git a/include/net/ipv6.h b/include/net/ipv6.h
> index e90f962..a84f3f6 100644
> --- a/include/net/ipv6.h
> +++ b/include/net/ipv6.h
> @@ -164,15 +164,6 @@ DECLARE_SNMP_STAT(struct icmpv6msg_mib, icmpv6msg_statistics);
> #define ICMP6MSGIN_INC_STATS_USER(idev, field) \
> _DEVINC(icmpv6msg, _USER, idev, field)
>
> -DECLARE_SNMP_STAT(struct udp_mib, udp_stats_in6);
> -DECLARE_SNMP_STAT(struct udp_mib, udplite_stats_in6);
> -#define UDP6_INC_STATS_BH(field, is_udplite) do { \
> - if (is_udplite) SNMP_INC_STATS_BH(udplite_stats_in6, field); \
> - else SNMP_INC_STATS_BH(udp_stats_in6, field); } while(0)
> -#define UDP6_INC_STATS_USER(field, is_udplite) do { \
> - if (is_udplite) SNMP_INC_STATS_USER(udplite_stats_in6, field); \
> - else SNMP_INC_STATS_USER(udp_stats_in6, field); } while(0)
> -
> struct ip6_ra_chain
> {
> struct ip6_ra_chain *next;
> diff --git a/include/net/udp.h b/include/net/udp.h
> index 98755eb..87170bb 100644
> --- a/include/net/udp.h
> +++ b/include/net/udp.h
> @@ -139,6 +139,12 @@ extern int udp_lib_setsockopt(struct sock *sk, int level, int optname,
> int (*push_pending_frames)(struct sock *));
>
> DECLARE_SNMP_STAT(struct udp_mib, udp_statistics);
> +DECLARE_SNMP_STAT(struct udp_mib, udp_stats_in6);
> +
> +/* UDP-Lite does not have a standardized MIB yet, so we inherit from UDP */
> +DECLARE_SNMP_STAT(struct udp_mib, udplite_statistics);
> +DECLARE_SNMP_STAT(struct udp_mib, udplite_stats_in6);
> +
> /*
> * SNMP statistics for UDP and UDP-Lite
> */
> @@ -149,6 +155,21 @@ DECLARE_SNMP_STAT(struct udp_mib, udp_statistics);
> if (is_udplite) SNMP_INC_STATS_BH(udplite_statistics, field); \
> else SNMP_INC_STATS_BH(udp_statistics, field); } while(0)
>
> +#define UDP6_INC_STATS_BH(field, is_udplite) do { \
> + if (is_udplite) SNMP_INC_STATS_BH(udplite_stats_in6, field); \
> + else SNMP_INC_STATS_BH(udp_stats_in6, field); } while(0)
> +#define UDP6_INC_STATS_USER(field, is_udplite) do { \
> + if (is_udplite) SNMP_INC_STATS_USER(udplite_stats_in6, field); \
> + else SNMP_INC_STATS_USER(udp_stats_in6, field); } while(0)
> +
> +#define UDPX_INC_STATS_BH(sk, field) \
> + do { \
> + if ((sk)->sk_family == AF_INET) \
> + UDP_INC_STATS_BH(field, 0); \
> + else \
> + UDP6_INC_STATS_BH(field, 0); \
> + } while (0);
> +
Hum, what happens if I have in my .config file :
CONFIG_IPV6=n
I suggest something like
#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
# define UDPX_INC_STATS_BH(sk, field) \
do { \
if ((sk)->sk_family == AF_INET) \
UDP_INC_STATS_BH(field, 0); \
else \
UDP6_INC_STATS_BH(field, 0); \
} while (0);
#else
# define UDPX_INC_STATS_BH(sk, field) \
UDP_INC_STATS_BH(field, 0);
#endif
^ permalink raw reply
* Re: [PATCH net-2.6.25 3/11][IPv4] Cleanup the sysctl_net_ipv4.c file
From: David Miller @ 2007-12-05 9:38 UTC (permalink / raw)
To: xemul; +Cc: netdev, devel
In-Reply-To: <475526B2.9040301@openvz.org>
From: Pavel Emelyanov <xemul@openvz.org>
Date: Tue, 04 Dec 2007 13:06:42 +0300
> This includes several cleanups:
>
> * tune Makefile to compile out this file when SYSCTL=n. Now
> it looks like net/core/sysctl_net_core.c one;
> * move the ipv4_config to af_inet.c to exist all the time;
> * remove additional sysctl_ip_nonlocal_bind declaration
> (it is already declared in net/ip.h);
> * remove no nonger needed ifdefs from this file.
>
> This is a preparation for using ctl paths for net/ipv4/
> sysctl table.
>
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
Applied.
^ permalink raw reply
* Re: [PATCH net-2.6.25 4/11][IPV4] Use ctl paths to register net/ipv4/ table
From: David Miller @ 2007-12-05 9:41 UTC (permalink / raw)
To: xemul; +Cc: netdev, devel
In-Reply-To: <475526ED.7070802@openvz.org>
From: Pavel Emelyanov <xemul@openvz.org>
Date: Tue, 04 Dec 2007 13:07:41 +0300
> This is the same as I did for the net/core/ table in the
> second patch in his series: use the paths and isolate the
> whole table in the .c file.
>
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
Applied.
^ permalink raw reply
* Re: [PATCH net-2.6.25 5/11][TR] Use ctl paths to register net/token-ring/ table
From: David Miller @ 2007-12-05 9:42 UTC (permalink / raw)
To: xemul; +Cc: netdev, devel
In-Reply-To: <47552744.50609@openvz.org>
From: Pavel Emelyanov <xemul@openvz.org>
Date: Tue, 04 Dec 2007 13:09:08 +0300
> The same thing for token-ring - use ctl paths and get
> rid of external references on the tr_table.
>
> Unfortunately, I couldn't split this patch into cleanup and
> use-the-paths parts.
>
> As a lame excuse I can say, that the cleanup is just moving
> the tr_table from one file to another - closet to a single
> variable, that this ctl table tunes. Since the source file
> becomes empty after the move, I remove it.
>
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
Applied.
^ permalink raw reply
* Re: [PATCH net-2.6.25 6/11][CORE] Remove the empty net_table
From: David Miller @ 2007-12-05 9:43 UTC (permalink / raw)
To: xemul; +Cc: netdev, devel
In-Reply-To: <47552779.40907@openvz.org>
From: Pavel Emelyanov <xemul@openvz.org>
Date: Tue, 04 Dec 2007 13:10:01 +0300
> I have removed all the entries from this table (core_table,
> ipv4_table and tr_table), so now we can safely drop it.
>
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
Applied.
Thanks Pavel.
^ permalink raw reply
* Re: [PATCH 7/7][TAKE 2][TFRC] New rx history code
From: Gerrit Renker @ 2007-12-05 9:42 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, netdev, dccp
In-Reply-To: <20071204134810.GB18084@ghostprotocols.net>
| I found a problem that I'm still investigating if was introduced by this
| patch or if was already present. When sending 1 million 256 byte packets
| with ttcp over loopback, using ccid3 it is crashing, the test machine
| I'm using doesn't have a serial port, its a notebook, will switch to
| another that has and provide the backtrace. It doesn't happens every
| time.
|
CCID3 is difficult due to the TX queue. Small packets are faster on the
wire, fill up the TX queue faster. Since there is little loss on LANs,
the slow-start algorithm will soon reach link capacity; but CCID3 can
not deal effectively with high speeds.
What is known not to work well at the moment is bidirectional data
transfer (e.g. an echo server/client). This lead to the comment in
tfrc_rx_sample_rtt(); the support for bidirectional data transfer
needs some more work, which however requires to make one-directional
transfer work well first.
| Here is tfrc_rx_hist_alloc back using ring of pointers with the fixed
| error path.
|
Thank you - I was just about to send a similar patch as update since
you clearly identified this bug. I will resubmit with your version
and upload it to the test tree.
| +int tfrc_rx_hist_alloc(struct tfrc_rx_hist *h)
| {
| + int i;
| +
| + for (i = 0; i <= TFRC_NDUPACK; i++) {
| + h->ring[i] = kmem_cache_alloc(tfrc_rx_hist_slab, GFP_ATOMIC);
| + if (h->ring[i] == NULL)
| + goto out_free;
| + }
| +
| + h->loss_count = h->loss_start = 0;
| + return 0;
| +
| +out_free:
| + while (i-- != 0) {
| + kmem_cache_free(tfrc_rx_hist_slab, h->ring[i]);
| + h->ring[i] = NULL;
| }
| + return -ENOBUFS;
| }
|
^ permalink raw reply
* Re: [PATCH net-2.6.25 7/11][IPV6] Make the ipv6/sysctl_net_ipv6.c compilation cleaner
From: David Miller @ 2007-12-05 9:43 UTC (permalink / raw)
To: xemul; +Cc: netdev, devel
In-Reply-To: <475527CB.9000003@openvz.org>
From: Pavel Emelyanov <xemul@openvz.org>
Date: Tue, 04 Dec 2007 13:11:23 +0300
> Since this file is entirely enclosed with the
> #ifdef CONFIG_SYSCTL/#endif pair, it's OK to move this
> CONFIG_ into a Makefile.
>
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
Applied.
^ permalink raw reply
* Re: [PATCH net-2.6.25 8/11][IPV6] Use sysctl paths to register ipv6 sysctl tables
From: David Miller @ 2007-12-05 9:44 UTC (permalink / raw)
To: xemul; +Cc: netdev, devel
In-Reply-To: <47552837.8080205@openvz.org>
From: Pavel Emelyanov <xemul@openvz.org>
Date: Tue, 04 Dec 2007 13:13:11 +0300
> I have already done this for core, ipv4 and tr tables, so
> repeat this for the ipv6 ones.
>
> This makes the ipv6.ko smaller and creates the ground needed
> for net namespaces support in ipv6.ko ssctls.
>
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
Applied.
^ permalink raw reply
* Re: [PATCH net-2.6.25 9/11][INET] Merge sys.net.ipv4.ip_forward and sys.net.ipv4.conf.all.forwarding
From: David Miller @ 2007-12-05 9:45 UTC (permalink / raw)
To: xemul; +Cc: netdev, devel
In-Reply-To: <475528BC.1010505@openvz.org>
From: Pavel Emelyanov <xemul@openvz.org>
Date: Tue, 04 Dec 2007 13:15:24 +0300
> AFAIS these two entries should do the same thing - change the
> forwarding state on ipv4_devconf and on all the devices.
>
> I propose to merge the handlers together using ctl paths.
>
> The inet_forward_change() is static after this and I move
> it higher to be closer to other "propagation" helpers and
> to avoid diff making patches based on { and } matching :)
> i.e. - make them easier to read.
>
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
I agree with your analysis and patch, applied.
Thanks.
^ permalink raw reply
* Re: [PATCH net-2.6.25 10/11][INET] Eliminate difference in actions of sysctl and proc handler for conf.all.forwarding
From: David Miller @ 2007-12-05 9:48 UTC (permalink / raw)
To: xemul; +Cc: netdev, devel
In-Reply-To: <4755290D.2090709@openvz.org>
From: Pavel Emelyanov <xemul@openvz.org>
Date: Tue, 04 Dec 2007 13:16:45 +0300
> AFAIS the net.ipv4.conf. <dev>, all and default sysctls should
> work like this when changed (besides changing the value itself):
>
> <dev> : optionally do smth else
> all : walk devices
> default : walk devices
>
> The proc handler for net.ipv4.conf.all works like this:
>
> <dev> : flush rt cache
> all : walk devices and flush rt cache
> default : nothing
>
> while the sysctl handler works like this:
>
> <dev> : nothing
> all : nothing
> default : walk devices but don't flush the cache
>
> All this looks strange. Am I right that regardless of whatever
> handler (proc or syscall) is called the behavior should be:
>
> <dev> : flush rt cache
> all : walk the devices and flush the cache
> default : walk the devices and flush the cache
>
> ?
>
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
Because, basically, nobody (and I really do mean nobody)
uses the sysctl() method to change these things, what
people expect is basically going to be the procfs
access behavior.
And I agree with it.
The 'default' influences future settings, it should not modify
existing devices. That's the job of 'all'.
Otherwise why have 'all' and 'default' as two different knobs
if they do exactly the same thing? That's pointless.
I've therefore dropped this patch.
^ permalink raw reply
* Re: [PATCH net-2.6.25 11/11][IPV6] Eliminate difference in actions of sysctl and proc handler for conf.all.forwarding
From: David Miller @ 2007-12-05 9:51 UTC (permalink / raw)
To: xemul; +Cc: netdev, devel
In-Reply-To: <475529A0.4010300@openvz.org>
From: Pavel Emelyanov <xemul@openvz.org>
Date: Tue, 04 Dec 2007 13:19:12 +0300
> The only difference in this case is that updating all.forwarding
> causes the update in default.forwarding when done via proc, but
> not via the system call.
>
> Besides, this consolidates a good portion of code.
>
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
This is another case where I think we should do what
the procfs case was doing, because that's what people
expect at this point.
That seems to be what your patch is doing, so I'll
apply this, thanks.
^ permalink raw reply
* Re: [PATCH 1/3] [UDP]: Avoid repeated counting of checksum errors due to peeking
From: David Miller @ 2007-12-05 9:52 UTC (permalink / raw)
To: herbert; +Cc: netdev
In-Reply-To: <20071204125844.GA2750@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 4 Dec 2007 23:58:44 +1100
> [UDP]: Avoid repeated counting of checksum errors due to peeking
>
> Currently it is possible for two processes to peek on the same socket
> and end up incrementing the error counter twice for the same packet.
>
> This patch fixes it by making skb_kill_datagram return whether it
> succeeded in unlinking the packet and only incrementing the counter
> if it did.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Applied, thanks Herbert.
^ permalink raw reply
* Re: [PATCH 2/3] [UDP]: Restore missing inDatagrams increments
From: Herbert Xu @ 2007-12-05 9:51 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Wang Chen, David S. Miller, netdev
In-Reply-To: <47567128.2060008@cosmosbay.com>
On Wed, Dec 05, 2007 at 10:36:40AM +0100, Eric Dumazet wrote:
>
> Hum, what happens if I have in my .config file :
>
> CONFIG_IPV6=n
Thanks. Obviously I shouldn't have touched this stuff at all :)
Let's hope this is the last problem.
[UDP]: Restore missing inDatagrams increments
The previous move of the the UDP inDatagrams counter caused the
counting of encapsulated packets, SUNRPC data (as opposed to call)
packets and RXRPC packets to go missing.
This patch restores all of these.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index e90f962..a84f3f6 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -164,15 +164,6 @@ DECLARE_SNMP_STAT(struct icmpv6msg_mib, icmpv6msg_statistics);
#define ICMP6MSGIN_INC_STATS_USER(idev, field) \
_DEVINC(icmpv6msg, _USER, idev, field)
-DECLARE_SNMP_STAT(struct udp_mib, udp_stats_in6);
-DECLARE_SNMP_STAT(struct udp_mib, udplite_stats_in6);
-#define UDP6_INC_STATS_BH(field, is_udplite) do { \
- if (is_udplite) SNMP_INC_STATS_BH(udplite_stats_in6, field); \
- else SNMP_INC_STATS_BH(udp_stats_in6, field); } while(0)
-#define UDP6_INC_STATS_USER(field, is_udplite) do { \
- if (is_udplite) SNMP_INC_STATS_USER(udplite_stats_in6, field); \
- else SNMP_INC_STATS_USER(udp_stats_in6, field); } while(0)
-
struct ip6_ra_chain
{
struct ip6_ra_chain *next;
diff --git a/include/net/udp.h b/include/net/udp.h
index 98755eb..98cb09c 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -139,6 +139,12 @@ extern int udp_lib_setsockopt(struct sock *sk, int level, int optname,
int (*push_pending_frames)(struct sock *));
DECLARE_SNMP_STAT(struct udp_mib, udp_statistics);
+DECLARE_SNMP_STAT(struct udp_mib, udp_stats_in6);
+
+/* UDP-Lite does not have a standardized MIB yet, so we inherit from UDP */
+DECLARE_SNMP_STAT(struct udp_mib, udplite_statistics);
+DECLARE_SNMP_STAT(struct udp_mib, udplite_stats_in6);
+
/*
* SNMP statistics for UDP and UDP-Lite
*/
@@ -149,6 +155,25 @@ DECLARE_SNMP_STAT(struct udp_mib, udp_statistics);
if (is_udplite) SNMP_INC_STATS_BH(udplite_statistics, field); \
else SNMP_INC_STATS_BH(udp_statistics, field); } while(0)
+#define UDP6_INC_STATS_BH(field, is_udplite) do { \
+ if (is_udplite) SNMP_INC_STATS_BH(udplite_stats_in6, field); \
+ else SNMP_INC_STATS_BH(udp_stats_in6, field); } while(0)
+#define UDP6_INC_STATS_USER(field, is_udplite) do { \
+ if (is_udplite) SNMP_INC_STATS_USER(udplite_stats_in6, field); \
+ else SNMP_INC_STATS_USER(udp_stats_in6, field); } while(0)
+
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+#define UDPX_INC_STATS_BH(sk, field) \
+ do { \
+ if ((sk)->sk_family == AF_INET) \
+ UDP_INC_STATS_BH(field, 0); \
+ else \
+ UDP6_INC_STATS_BH(field, 0); \
+ } while (0);
+#else
+#define UDPX_INC_STATS_BH(sk, field) UDP_INC_STATS_BH(field, 0)
+#endif
+
/* /proc */
struct udp_seq_afinfo {
struct module *owner;
diff --git a/include/net/udplite.h b/include/net/udplite.h
index 635b0ea..b76b2e3 100644
--- a/include/net/udplite.h
+++ b/include/net/udplite.h
@@ -13,9 +13,6 @@
extern struct proto udplite_prot;
extern struct hlist_head udplite_hash[UDP_HTABLE_SIZE];
-/* UDP-Lite does not have a standardized MIB yet, so we inherit from UDP */
-DECLARE_SNMP_STAT(struct udp_mib, udplite_statistics);
-
/*
* Checksum computation is all in software, hence simpler getfrag.
*/
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index f50de5d..78cfcb4 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -110,6 +110,7 @@
*/
DEFINE_SNMP_STAT(struct udp_mib, udp_statistics) __read_mostly;
+EXPORT_SYMBOL(udp_statistics);
struct hlist_head udp_hash[UDP_HTABLE_SIZE];
DEFINE_RWLOCK(udp_hash_lock);
@@ -969,8 +970,11 @@ int udp_queue_rcv_skb(struct sock * sk, struct sk_buff *skb)
int ret;
ret = (*up->encap_rcv)(sk, skb);
- if (ret <= 0)
+ if (ret <= 0) {
+ UDP_INC_STATS_BH(UDP_MIB_INDATAGRAMS,
+ is_udplite);
return -ret;
+ }
}
/* FALLTHROUGH -- it's a UDP Packet */
diff --git a/net/ipv6/proc.c b/net/ipv6/proc.c
index 8631ed7..9fc2428 100644
--- a/net/ipv6/proc.c
+++ b/net/ipv6/proc.c
@@ -27,6 +27,7 @@
#include <net/ip.h>
#include <net/sock.h>
#include <net/tcp.h>
+#include <net/udp.h>
#include <net/transp_v6.h>
#include <net/ipv6.h>
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 87bccec..36bdcd2 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -34,6 +34,7 @@
#include <linux/ipv6.h>
#include <linux/icmpv6.h>
#include <linux/init.h>
+#include <linux/module.h>
#include <linux/skbuff.h>
#include <asm/uaccess.h>
@@ -51,6 +52,7 @@
#include "udp_impl.h"
DEFINE_SNMP_STAT(struct udp_mib, udp_stats_in6) __read_mostly;
+EXPORT_SYMBOL(udp_stats_in6);
static inline int udp_v6_get_port(struct sock *sk, unsigned short snum)
{
diff --git a/net/rxrpc/ar-input.c b/net/rxrpc/ar-input.c
index 91b5bbb..f446d9b 100644
--- a/net/rxrpc/ar-input.c
+++ b/net/rxrpc/ar-input.c
@@ -20,6 +20,7 @@
#include <net/sock.h>
#include <net/af_rxrpc.h>
#include <net/ip.h>
+#include <net/udp.h>
#include "ar-internal.h"
unsigned long rxrpc_ack_timeout = 1;
@@ -707,10 +708,13 @@ void rxrpc_data_ready(struct sock *sk, int count)
if (skb_checksum_complete(skb)) {
rxrpc_free_skb(skb);
rxrpc_put_local(local);
+ UDP_INC_STATS_BH(UDP_MIB_INERRORS, 0);
_leave(" [CSUM failed]");
return;
}
+ UDP_INC_STATS_BH(UDP_MIB_INDATAGRAMS, 0);
+
/* the socket buffer we have is owned by UDP, with UDP's data all over
* it, but we really want our own */
skb_orphan(skb);
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 2f630a5..6fa52f4 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -838,8 +838,12 @@ static void xs_udp_data_ready(struct sock *sk, int len)
copied = repsize;
/* Suck it into the iovec, verify checksum if not done by hw. */
- if (csum_partial_copy_to_xdr(&rovr->rq_private_buf, skb))
+ if (csum_partial_copy_to_xdr(&rovr->rq_private_buf, skb)) {
+ UDPX_INC_STATS_BH(sk, UDP_MIB_INERRORS);
goto out_unlock;
+ }
+
+ UDPX_INC_STATS_BH(sk, UDP_MIB_INDATAGRAMS);
/* Something worked... */
dst_confirm(skb->dst);
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
^ permalink raw reply related
* Re: [PATCH 2/3] [UDP]: Restore missing inDatagrams increments
From: David Miller @ 2007-12-05 9:53 UTC (permalink / raw)
To: herbert; +Cc: wangchen, netdev
In-Reply-To: <20071205021559.GA27707@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 5 Dec 2007 13:15:59 +1100
> On Wed, Dec 05, 2007 at 09:42:45AM +0800, Wang Chen wrote:
> >
> > I apologize for my miss in previous patch.
> >
> > Herbert, don't you think the udp6inDatagrams should be counted too
> > in xprtsock?
>
> Good point. Here's a better version.
>
> [UDP]: Restore missing inDatagrams increments
>
> The previous move of the the UDP inDatagrams counter caused the
> counting of encapsulated packets, SUNRPC data (as opposed to call)
> packets and RXRPC packets to go missing.
>
> This patch restores all of these.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Applied.
^ permalink raw reply
* Re: [PATCH 3/3] [UDP]: Only increment counter on first peek/recv
From: David Miller @ 2007-12-05 9:54 UTC (permalink / raw)
To: herbert; +Cc: netdev
In-Reply-To: <20071204130310.GB3098@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 5 Dec 2007 00:03:10 +1100
> [UDP]: Only increment counter on first peek/recv
>
> The previous move of the the UDP inDatagrams counter caused each
> peek of the same packet to be counted separately. This may be
> undesirable.
>
> This patch fixes this by adding a bit to sk_buff to record whether
> this packet has already been seen through skb_recv_datagram. We
> then only increment the counter when the packet is seen for the
> first time.
>
> The only dodgy part is the fact that skb_recv_datagram doesn't have
> a good way of returning this new bit of information. So I've added
> a new function __skb_recv_datagram that does return this and made
> skb_recv_datagram a wrapper around it.
>
> The plan is to eventually replace all uses of skb_recv_datagram with
> this new function at which time it can be renamed its proper name.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Applied, thanks for fixing all of this Herbert.
^ permalink raw reply
* Re: [PATCH 2/3] [UDP]: Restore missing inDatagrams increments
From: Herbert Xu @ 2007-12-05 9:54 UTC (permalink / raw)
To: David Miller; +Cc: wangchen, netdev
In-Reply-To: <20071205.015301.87202109.davem@davemloft.net>
On Wed, Dec 05, 2007 at 01:53:01AM -0800, David Miller wrote:
>
> > [UDP]: Restore missing inDatagrams increments
> >
> > The previous move of the the UDP inDatagrams counter caused the
> > counting of encapsulated packets, SUNRPC data (as opposed to call)
> > packets and RXRPC packets to go missing.
> >
> > This patch restores all of these.
> >
> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> Applied.
Sorry, Eric spotted a build problem in my patch. So you'll need to
replace this with the fixed version that I sent literally minutes ago :)
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
^ permalink raw reply
* Re: sockets affected by IPsec always block (2.6.23)
From: David Miller @ 2007-12-05 9:55 UTC (permalink / raw)
To: herbert; +Cc: simon, linux-kernel, netdev
In-Reply-To: <20071205073927.GA12413@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 5 Dec 2007 18:39:27 +1100
> On Tue, Dec 04, 2007 at 11:34:32PM -0800, David Miller wrote:
> >
> > TCP has some built-in assumptions about characteristics of
> > interent links and what constitutes a timeout which is "too long"
> > and should thus result in a full connection failure.
> >
> > IPSEC changes this because of IPSEC route resolution via
> > ISAKMP.
> >
> > With this in mind I can definitely see people preferring
> > the "block until IPSEC resolves" behavior, especially for
> > something like, say, periodic remote backups and stuff like
> > that where you really want the thing to just sit and wait
> > for the connect() to succeed instead of failing.
>
> Hmm, but connect(2) should succeed in that case thanks to the
> blackhole route, no? The subsequent SYNs will then be dropped
> until the IPsec SAs are in place.
If it hits sysctl_tcp_syn_retries SYN attempts, the connect will hard
fail.
^ permalink raw reply
* Re: sockets affected by IPsec always block (2.6.23)
From: Herbert Xu @ 2007-12-05 9:57 UTC (permalink / raw)
To: David Miller; +Cc: simon, linux-kernel, netdev
In-Reply-To: <20071205.015558.224988608.davem@davemloft.net>
On Wed, Dec 05, 2007 at 01:55:58AM -0800, David Miller wrote:
>
> If it hits sysctl_tcp_syn_retries SYN attempts, the connect will hard
> fail.
Right. Let's just forget about this until we have a queueing system :)
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
^ permalink raw reply
* Re: [PATCH net-2.6.25 10/11][INET] Eliminate difference in actions of sysctl and proc handler for conf.all.forwarding
From: Pavel Emelyanov @ 2007-12-05 9:58 UTC (permalink / raw)
To: David Miller; +Cc: netdev, devel
In-Reply-To: <20071205.014835.45293431.davem@davemloft.net>
David Miller wrote:
> From: Pavel Emelyanov <xemul@openvz.org>
> Date: Tue, 04 Dec 2007 13:16:45 +0300
>
>> AFAIS the net.ipv4.conf. <dev>, all and default sysctls should
>> work like this when changed (besides changing the value itself):
>>
>> <dev> : optionally do smth else
>> all : walk devices
>> default : walk devices
>>
>> The proc handler for net.ipv4.conf.all works like this:
>>
>> <dev> : flush rt cache
>> all : walk devices and flush rt cache
>> default : nothing
>>
>> while the sysctl handler works like this:
>>
>> <dev> : nothing
>> all : nothing
>> default : walk devices but don't flush the cache
>>
>> All this looks strange. Am I right that regardless of whatever
>> handler (proc or syscall) is called the behavior should be:
>>
>> <dev> : flush rt cache
>> all : walk the devices and flush the cache
>> default : walk the devices and flush the cache
>>
>> ?
>>
>> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
>
> Because, basically, nobody (and I really do mean nobody)
> uses the sysctl() method to change these things, what
> people expect is basically going to be the procfs
> access behavior.
OK. Thank you for clarification :)
> And I agree with it.
>
> The 'default' influences future settings, it should not modify
> existing devices. That's the job of 'all'.
I thought the same, and I saw that this is true for ipv6, but
ipv4 works differently :( -- changing default for some sysctls
will cause some devices to be changed as well.
I mean - devinet_copy_dflt_conf() copies the changed bit on
those devices, that have not this but marked in the "state" field.
It is called for such entries as "accept_redirects", "shared_media"
and many others. But not for "forwarding" one. That's what seemed
strange to me. Sorry, that I didn't express the idea more cleanly.
So what's the right behavior -- to propagate the default for all the
ctls on all the devices (according to their "state"), not to propagate
for all the ctls, or to keep things as they are now?
> Otherwise why have 'all' and 'default' as two different knobs
> if they do exactly the same thing? That's pointless.
>
> I've therefore dropped this patch.
>
Thanks,
Pavel
^ permalink raw reply
* Re: [PATCH 2/3] [UDP]: Restore missing inDatagrams increments
From: David Miller @ 2007-12-05 10:02 UTC (permalink / raw)
To: herbert; +Cc: dada1, wangchen, netdev
In-Reply-To: <20071205095159.GA16860@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 5 Dec 2007 20:51:59 +1100
> On Wed, Dec 05, 2007 at 10:36:40AM +0100, Eric Dumazet wrote:
> >
> > Hum, what happens if I have in my .config file :
> >
> > CONFIG_IPV6=n
>
> Thanks. Obviously I shouldn't have touched this stuff at all :)
> Let's hope this is the last problem.
>
> [UDP]: Restore missing inDatagrams increments
>
> The previous move of the the UDP inDatagrams counter caused the
> counting of encapsulated packets, SUNRPC data (as opposed to call)
> packets and RXRPC packets to go missing.
>
> This patch restores all of these.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Herbert, I applied the buggered patch and pushed it out
already, can you send me this fix relative?
I'll combine them on my next rebase, thanks.
^ permalink raw reply
* Re: [PATCH 2/3] [UDP]: Restore missing inDatagrams increments
From: Herbert Xu @ 2007-12-05 10:04 UTC (permalink / raw)
To: David Miller; +Cc: dada1, wangchen, netdev
In-Reply-To: <20071205.020248.50886654.davem@davemloft.net>
On Wed, Dec 05, 2007 at 02:02:48AM -0800, David Miller wrote:
>
> Herbert, I applied the buggered patch and pushed it out
> already, can you send me this fix relative?
>
> I'll combine them on my next rebase, thanks.
Sure, here it is.
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
--
diff --git a/include/net/udp.h b/include/net/udp.h
index 87170bb..98cb09c 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -162,6 +162,7 @@ DECLARE_SNMP_STAT(struct udp_mib, udplite_stats_in6);
if (is_udplite) SNMP_INC_STATS_USER(udplite_stats_in6, field); \
else SNMP_INC_STATS_USER(udp_stats_in6, field); } while(0)
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
#define UDPX_INC_STATS_BH(sk, field) \
do { \
if ((sk)->sk_family == AF_INET) \
@@ -169,6 +170,9 @@ DECLARE_SNMP_STAT(struct udp_mib, udplite_stats_in6);
else \
UDP6_INC_STATS_BH(field, 0); \
} while (0);
+#else
+#define UDPX_INC_STATS_BH(sk, field) UDP_INC_STATS_BH(field, 0)
+#endif
/* /proc */
struct udp_seq_afinfo {
^ permalink raw reply related
* Re: [PATCH net-2.6.25 10/11][INET] Eliminate difference in actions of sysctl and proc handler for conf.all.forwarding
From: David Miller @ 2007-12-05 10:06 UTC (permalink / raw)
To: xemul; +Cc: netdev, devel
In-Reply-To: <47567638.9080208@openvz.org>
From: Pavel Emelyanov <xemul@openvz.org>
Date: Wed, 05 Dec 2007 12:58:16 +0300
> David Miller wrote:
> > The 'default' influences future settings, it should not modify
> > existing devices. That's the job of 'all'.
>
> I thought the same, and I saw that this is true for ipv6, but
> ipv4 works differently :( -- changing default for some sysctls
> will cause some devices to be changed as well.
>
> I mean - devinet_copy_dflt_conf() copies the changed bit on
> those devices, that have not this but marked in the "state" field.
> It is called for such entries as "accept_redirects", "shared_media"
> and many others. But not for "forwarding" one. That's what seemed
> strange to me. Sorry, that I didn't express the idea more cleanly.
>
> So what's the right behavior -- to propagate the default for all the
> ctls on all the devices (according to their "state"), not to propagate
> for all the ctls, or to keep things as they are now?
Grrr, good question.
I remember we had all kinds of issues wrt. this which we
had to cleanup in IPV6 in particular.
People complained that once a device was loaded and present,
you couldn't set the 'default' and expect it to influence
the settings.
The user is pretty much screwed in one way or the other.
For example:
1) If 'default' propagates to all devices, any specific
setting for a device is lost.
2) If 'default' does not propagate, there is no way to
have 'default' influence devices which have already
been loaded.
I think both behaviors are bad, and the whole problem is that sysctls
acting as defaults cannot have sane semantics because devices get
loaded before userspace can sanely start making changes to such sysctl
'defaults'.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox