Netdev List
 help / color / mirror / Atom feed
* Re: [net-next-2.6 PATCH v6 3/7 RFC] TCPCT part 1c: sysctl_tcp_cookie_size, socket option TCP_COOKIE_TRANSACTIONS
From: William Allen Simpson @ 2009-11-16 20:40 UTC (permalink / raw)
  To: Joe Perches; +Cc: Linux Kernel Network Developers
In-Reply-To: <4AFED025.1060202@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 102 bytes --]

Improved Documentation to match existing code:

   Odd values are interpreted as the next even value.

[-- Attachment #2: TCPCT+1c6+.patch --]
[-- Type: text/plain, Size: 5128 bytes --]

diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index a0e134d..820dd4b 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -164,6 +164,14 @@ tcp_congestion_control - STRING
 	additional choices may be available based on kernel configuration.
 	Default is set as part of kernel configuration.
 
+tcp_cookie_size - INTEGER
+	Default size of TCP Cookie Transactions (TCPCT) option, that may be
+	overridden on a per socket basis by the TCPCT socket option.
+	Values greater than the maximum (16) are interpreted as the maximum.
+	Values greater than zero and less than the minimum (8) are interpreted
+	as the minimum.  Odd values are interpreted as the next even value.
+	Default: 0 (off).
+
 tcp_dsack - BOOLEAN
 	Allows TCP to send "duplicate" SACKs.
 
diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index 32d7d77..eaa3113 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -102,7 +102,9 @@ enum {
 #define TCP_QUICKACK		12	/* Block/reenable quick acks */
 #define TCP_CONGESTION		13	/* Congestion control algorithm */
 #define TCP_MD5SIG		14	/* TCP MD5 Signature (RFC2385) */
+#define TCP_COOKIE_TRANSACTIONS	15	/* TCP Cookie Transactions */
 
+/* for TCP_INFO socket option */
 #define TCPI_OPT_TIMESTAMPS	1
 #define TCPI_OPT_SACK		2
 #define TCPI_OPT_WSCALE		4
@@ -174,6 +176,30 @@ struct tcp_md5sig {
 	__u8	tcpm_key[TCP_MD5SIG_MAXKEYLEN];		/* key (binary) */
 };
 
+/* for TCP_COOKIE_TRANSACTIONS (TCPCT) socket option */
+#define TCP_COOKIE_MIN		 8		/*  64-bits */
+#define TCP_COOKIE_MAX		16		/* 128-bits */
+#define TCP_COOKIE_PAIR_SIZE	(2*TCP_COOKIE_MAX)
+
+/* Flags for both getsockopt and setsockopt */
+#define TCP_COOKIE_IN_ALWAYS	(1 << 0)	/* Discard SYN without cookie */
+#define TCP_COOKIE_OUT_NEVER	(1 << 1)	/* Prohibit outgoing cookies,
+						 * supercedes everything. */
+
+/* Flags for getsockopt */
+#define TCP_S_DATA_IN		(1 << 2)	/* Was data received? */
+#define TCP_S_DATA_OUT		(1 << 3)	/* Was data sent? */
+
+/* TCP_COOKIE_TRANSACTIONS data */
+struct tcp_cookie_transactions {
+	__u16	tcpct_flags;			/* see above */
+	__u8	__tcpct_pad1;			/* zero */
+	__u8	tcpct_cookie_desired;		/* bytes */
+	__u16	tcpct_s_data_desired;		/* bytes of variable data */
+	__u16	tcpct_used;			/* bytes in value */
+	__u8	tcpct_value[TCP_MSS_DEFAULT];
+};
+
 #ifdef __KERNEL__
 
 #include <linux/skbuff.h>
@@ -227,6 +253,11 @@ struct tcp_options_received {
 	u16	mss_clamp;	/* Maximal mss, negotiated at connection setup */
 };
 
+static inline void tcp_clear_options(struct tcp_options_received *rx_opt)
+{
+	rx_opt->tstamp_ok = rx_opt->sack_ok = rx_opt->wscale_ok = rx_opt->snd_wscale = 0;
+}
+
 /* This is the max number of SACKS that we'll generate and process. It's safe
  * to increse this, although since:
  *   size = TCPOLEN_SACK_BASE_ALIGNED (4) + n * TCPOLEN_SACK_PERBLOCK (8)
@@ -435,6 +466,6 @@ static inline struct tcp_timewait_sock *tcp_twsk(const struct sock *sk)
 	return (struct tcp_timewait_sock *)sk;
 }
 
-#endif
+#endif	/* __KERNEL__ */
 
 #endif	/* _LINUX_TCP_H */
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 4a99a8e..738b65f 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -234,6 +234,7 @@ extern int sysctl_tcp_base_mss;
 extern int sysctl_tcp_workaround_signed_windows;
 extern int sysctl_tcp_slow_start_after_idle;
 extern int sysctl_tcp_max_ssthresh;
+extern int sysctl_tcp_cookie_size;
 
 extern atomic_t tcp_memory_allocated;
 extern struct percpu_counter tcp_sockets_allocated;
@@ -340,11 +341,6 @@ static inline void tcp_dec_quickack_mode(struct sock *sk,
 
 extern void tcp_enter_quickack_mode(struct sock *sk);
 
-static inline void tcp_clear_options(struct tcp_options_received *rx_opt)
-{
- 	rx_opt->tstamp_ok = rx_opt->sack_ok = rx_opt->wscale_ok = rx_opt->snd_wscale = 0;
-}
-
 #define	TCP_ECN_OK		1
 #define	TCP_ECN_QUEUE_CWR	2
 #define	TCP_ECN_DEMAND_CWR	4
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 2dcf04d..3422c54 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -714,6 +714,14 @@ static struct ctl_table ipv4_table[] = {
 	},
 	{
 		.ctl_name	= CTL_UNNUMBERED,
+		.procname	= "tcp_cookie_size",
+		.data		= &sysctl_tcp_cookie_size,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec
+	},
+	{
+		.ctl_name	= CTL_UNNUMBERED,
 		.procname	= "udp_mem",
 		.data		= &sysctl_udp_mem,
 		.maxlen		= sizeof(sysctl_udp_mem),
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 1151cb8..e59fa5a 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -59,6 +59,14 @@ int sysctl_tcp_base_mss __read_mostly = 512;
 /* By default, RFC2861 behavior.  */
 int sysctl_tcp_slow_start_after_idle __read_mostly = 1;
 
+#ifdef CONFIG_SYSCTL
+/* By default, let the user enable it. */
+int sysctl_tcp_cookie_size __read_mostly = 0;
+#else
+int sysctl_tcp_cookie_size __read_mostly = TCP_COOKIE_MAX;
+#endif
+
+
 /* Account for new data that has been sent to the network. */
 static void tcp_event_new_data_sent(struct sock *sk, struct sk_buff *skb)
 {

^ permalink raw reply related

* Re: Those gpio-mdio patches...
From: Lennart Sorensen @ 2009-11-16 20:25 UTC (permalink / raw)
  To: Lennart Sorensen; +Cc: David Miller, netdev
In-Reply-To: <20091116182145.GB15157@caffeine.csclub.uwaterloo.ca>

On Mon, Nov 16, 2009 at 01:21:45PM -0500, Lennart Sorensen wrote:
> On Fri, Nov 13, 2009 at 01:59:08PM -0800, David Miller wrote:
> > Please don't fix the issue that way.  If the routine is returning a
> > true or false value, make it return a 'bool' instead of an 'int'.
> > That way we don't have to "accept something other than 0 and 1"
> 
> Hmm.  Well I just tried to fix what was already there.  gpio-lib does
> NOT return 0 or 1, it returns 0 or 1 shifted by the gpio position in a
> gpio register, but soemthing has to convert that to a boolean value.
> Making it return a bool doesn't change the fact it needs the '!!' applied
> to make it a bool.
> 
> I can look at changing the mdio-gpio to using bool instead of integer
> where necesary.

So it looks like this would involve changing mdio-bitbang to using bool
rather than int in a couple of places.  This would affect:

./include/linux/mdio-bitbang.h
./arch/powerpc/platforms/82xx/ep8248e.c
./drivers/net/fs_enet/mii-bitbang.c
./drivers/net/phy/mdio-bitbang.c
./drivers/net/phy/mdio-gpio.c
./drivers/net/sh_eth.c

I could write up a patch for that if it makes sense.

I figure it would mean changing:

void (*set_mdc)(struct mdiobb_ctrl *ctrl, int level);
void (*set_mdio_dir)(struct mdiobb_ctrl *ctrl, int output);
void (*set_mdio_data)(struct mdiobb_ctrl *ctrl, int value);
int (*get_mdio_data)(struct mdiobb_ctrl *ctrl);

into:

void (*set_mdc)(struct mdiobb_ctrl *ctrl, bool level);
void (*set_mdio_dir)(struct mdiobb_ctrl *ctrl, bool output);
void (*set_mdio_data)(struct mdiobb_ctrl *ctrl, bool value);
bool (*get_mdio_data)(struct mdiobb_ctrl *ctrl);

Does that make sense?

Of course the only place that breaks right now is mdiobb_get_num which
assumes the value is 0 or 1.  Everywhere else appears to be fine with
0 and non-0.  Maybe putting the fix just in there or in mdiobb_get_bit
makes more sense, since then it doesn't matter what the users of
mdio-bitbang do, it will always work.

-- 
Len Sorensen

^ permalink raw reply

* Re: sparse vs. skbuff.h
From: Vegard Nossum @ 2009-11-16 20:23 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Johannes Berg, netdev
In-Reply-To: <4B01AD5F.7020402@gmail.com>

2009/11/16 Eric Dumazet <eric.dumazet@gmail.com>:
> Johannes Berg a écrit :
>> On Mon, 2009-11-16 at 20:21 +0100, Johannes Berg wrote:
>>> commit 14d18a81b5171d4433e41129619c75748b4f4d26
>>> Author: Eric Dumazet <eric.dumazet@gmail.com>
>>> Date:   Thu Oct 29 00:10:37 2009 +0000
>>>
>>>     net: fix kmemcheck annotations
>>>
>>>
>>> broke sparse endian checks on everything that includes skbuff.h because
>>> the first and only (because it's an error) thing sparse now reports is
>>> this:
>>>
>>> include/linux/skbuff.h:357:41: error: invalid bitfield specifier for type restricted __be16.
>>
>> Simply changing from
>>       __be16 protocol:16;
>> to
>>       __be16 protocol;
>>
>> but leaving it inside the kmemcheck annotation seems to do the right
>> thing. Except of course that kmemcheck will not properly check it now.
>> Maybe those annotations should simply be made to have no impact on
>> struct padding instead?
>>
>
> Hmm, I have really no idea of what is the right way to fix this stuff.
>
> Last time I did adding a non bitfield element inside the begin/end annotations,
> I was flamed, because a bitfield is a bitfield, not a char/short
>
> http://www.spinics.net/lists/netdev/msg108803.html
>
> Now sparse is complaining... What will be the next story ?

If by "I was flamed" you are referring to my reply:

http://www.spinics.net/lists/netdev/msg108825.html

then I am really sorry, because I had no intentions to insult you. In
fact, I am grateful that you are finding bugs and telling me about
them But I should also be allowed to disagree with a patch if I truly
believe it is the wrong thing to do.

For the issue in question: If the variable is turned into a
non-bitfield (as Johannes suggested), it would be fine, because now
GCC won't emit masking operations (AND, OR) when initializing it, but
a regular MOV. Also, the struct annotations do not by themselves do
anything, but they are used by kmemcheck_annotate_bitfield().

In other words, I think the right thing to do is to turn it into a
non-bitfield and move it _outside_ the bitfield annotation. Johannes,
can you make the patch and let us have a look? In the meantime I will
submit the patch that fixes the extraneous field padding in
KMEMCHECK=n kernels.


Vegard

^ permalink raw reply

* Re: [RFC PATCH] net: add dataref destructor to sk_buff
From: Gregory Haskins @ 2009-11-16 20:18 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Herbert Xu, Gregory Haskins, Michael S. Tsirkin, alacrityvm-devel,
	linux-kernel, netdev
In-Reply-To: <20091116115931.6266f9c9@nehalam>

[-- Attachment #1: Type: text/plain, Size: 1833 bytes --]

Stephen Hemminger wrote:
> On Sat, 14 Nov 2009 00:27:46 -0500
> Gregory Haskins <gregory.haskins@gmail.com> wrote:
> 
>> Stephen Hemminger wrote:

> 
>>> People have tried doing copy-less send by page flipping, but the overhead of the IPI to
>>> invalidate the TLB exceeded the overhead of the copy. There was an Intel paper on this in
>>> at Linux Symposium (Ottawa) several years ago.
>> I think you are confusing copy-less tx with copy-less rx.  You can try
>> to do copy-less rx with page flipping, which has the IPI/TLB thrashing
>> properties you mention, and I agree is problematic.  We are talking
>> about copy-less tx here, however, and therefore no page-flipping is
>> involved.  Rather, we are just posting SG lists of pages directly to the
>> NIC (assuming the nic supports HIGH_DMA, etc).  You do not need to flip
>> the page, or invalidate the TLB (and thus IPI the other cores) to do
>> this to my knowledge.
>>
> 
> If you want to do copy-less tx for all applications, you have to
> do COW to handle the trivial case of :
> 
> while (cc = read(infd, buffer, sizeof buffer)) {
>    send(outsock, buffer, cc);
> }
> 
> 

You certainly _could_ implement this as a COW I suppose, but that would
be insane.  If someone did do this, you are right: you need TLB
invalidation.

However, if I were going to actually propose the changeover of the
system calls to use zero-copy (note that I am not), it would be based on
the concept in this patch.  That is: the send() would block until the
NIC completes the DMA and the shinfo block is freed.  Alternate
implementations would be AIO based, where the shinfo destructor
signifies the generation of the completion event.

FWIW: The latter is conceptually similar to how this is being used in
AlacrityVM.

HTH

Kind Regards,
-Greg


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 267 bytes --]

^ permalink raw reply

* Re: Doubt in implementations of mean loss interval at sender side
From: Ivo Calado @ 2009-11-16 20:09 UTC (permalink / raw)
  To: Gerrit Renker, dccp, netdev
In-Reply-To: <cb00fa210911161207n5f255a16w1b750701c1bd177c@mail.gmail.com>

On Mon, Nov 9, 2009 at 4:09 AM, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> | > To sum up, here is whay I think is minimally required to satisfy the union
> | > of RFC 4340, 4342, 4828, 5348, and 5622:
> | >
> | >        struct tfrc_tx_packet_info {
> | >                u64     seqno:48,
> | >                        is_ect0:1,
> | >                        is_data_packet:1,
> | >                        is_in_loss_interval:1;
> | >                u32     send_time;
> | >                u32     rtt_estimate;
> | >                struct tfrc_tx_packet_info *next; /* FIFO */
> | >        };
> | >
> | > That would be a per-packet storage cost of about 16 bytes, plus the pointer
> | > (8 bytes on 64-bit architectures). One could avoid the pointer by defining a
> | >        u64     base_seqno;
> | > and then
> | >        struct tfrc_tx_packet_info[some constant here];
> | > and then index the array relative to the base_seqno.
> | >
> |
> | Yes, I believe that struct is enough too. But how long would be necessary
> | the struct array to be?
> |
> The problem is the same as with Ack Vectors - the array (or list) can grow
> arbitrarily large. You made a good reply, since all the questions are
> inter-related. The first two I see here are
>
>  1) the choice of data structure (array or list)
>  2) the design of a garbage-collector
>
> This includes your point from above, about the maximum size. To draw the
> analogy to Ack Vectors, at the moment they use a fixed size. On certain
> mediums (WiFi) there exist situations where even that fixed limit is
> reached, causing an overflow with Ack Vectors that have reached a size
> of 2 * 253 = 506 bytes.
>
> Looking after old data of sent packets is similar, the main difference I
> see that at some stage "unused" old entries need to be collected, to avoid
> the overflow problem which occurs when using a fixed-size structure.
>
> I find that 'Acknowledgments of Acknowledgments' is a bad idea, since it
> means implementing reliable delivery over unreliable transport; on the
> one hand DCCP is designed to be unreliable, but here suddenly is a break
> with that design decision.
>
> So point (2) will probably mean coming up with some guessed heuristics
> that will work for most scenarios, but may fail in others.
>
>
> This is why I am not a big fan of the sender-based solution: to solve (2) and
> your question from above requires a lot of testing of the garbage-collection
> and book-keeping algorithms, rather than on the actual congestion control.
>
> One can spend a lot of time going over these issues, but of what use is the
> most ingenious data structure if the overall protocol behavior does not
> deliver a useful performance to users of the protocol?

Yes, a sender-based implementation seems really complicated, mainly in
principle.

>
> | > IIb) Further remarks
> | > --------------------
> | > At first sight it would seem that storing the RTT also solves the problem
> | > of inaccurate RTTs used at the receiver. Unfortunately, this is not the
> | > case. X_recv is sampled over intervals of varying length which may or may
> | > not equal the RTT.  To factor out the effect of window counters, the sender
> | > would need to store the packet size as well and would need to use rather
> | > complicated computations - an ugly workaround.
> |
> | I didn't understand how the packet size would help and what
> | computations are needed.
> |
> The above still refers to the earlier posting about letting the sender
> supply the RTT estimate R_i in packet `i' as defined in RFC 5348, 3.2.1.
>
> Though the same section later suggests that a coarse-grained timestamp
> is sufficient, in practice the inaccuracy of the RTT means inaccurate
> X_recv, and as a consequence sub-optimal protocol performance.
>
> The problem is that the algorithm from RFC 4342, 8.1 assumes that the
> rate of change of the window counter also relates to the change of
> packet spacing (the difference between the T_i packe arrivel times).
>
> Howver, especially when using high-speed (1/10 Gbit) networking, this
> assumption often does not hold in practice. Packets are sent by the network
> card in bunches, or intervening switches/routers cause a compression of
> packet inter-arrival times. Hence it is perfectly possible that a bundle of
> packets with different Window Counter CCVal values arrive at virtually
> the same time. For instance, on a 100Mbs ethernet I have seen spikes of
> X_recv of up to 2-3 Gbits/sec. Several orders of magnitude from the
> real packet speed (not to mention the unrealistic value).
>
> So the question above was asking whether there is a way for the sender
> to "compute away" the inaccuracies reported by the receiver. Your reply
> confirms my doubts that doing this is probably not possible.
>
> To clarify, I was asking whether it would be possible for the sender to
> perform step (2) of RFC 5348, 6.2; to compensate for the fact that the
> receiver does not have a reliable RTT estimate.

I understand now the issue, thanks. Isn't better to just send the RTT estimate
to the sender, as the RFC says?

>
> For example, when receiving feedback for packet `i', it would iterate
> through the list/array, going back over as many packets as are covered
> by the send time T_i of packet `i' minus the RTT estimate R_i at that
> time, sum their packet sizes, and from that value recompute X_recv.
>
> This is a bit complicated if the garbage-collector has already purged
> older entries, so part of the garbage collector would probably have
> to watch over acknowledged packets. I add this as item
>  3) validate X_recv
> to the above running list of book-keeping items done at the sender.
>
>
> | > One thing I stumbled across while reading your code was the fact that RFC 4342
> | > leaves it open as to how many Loss Intervals to send: on the one hand it follows
> | > the suggestion of RFC 5348 to use 1+NINTERVAL=9, but on the other hand it does
> | > not restrict the number of loss intervals. Also RFC 5622 does not limit the
> | > number of Loss Intervals / Data Dropped options.
> | >
> | > If receiving n > 9 Loss Intervals, what does the sender do with the n-9 older
> | > intervals? There must be some mechanism to stop these options from growing
> | > beyond bounds, so it needs to store also which loss intervals have been
> | > acknowledged, introducing the "Acknowledgment of Acknowledgments"
> | > problem.
> |
> | In RFC 4342 section 8.6 it says that the limit of loss interval data
> | to send is 28, and RFC 5622 8.7 says 84 for dropped packets option.
> | But I don't see why to send so many data in these options.
> | Yes, the most recent 9 loss intervals are required to be reported,
> | except if the sender acknowledged previous sent loss intervals, so in
> | that case only one is required, the open interval.
> | And we can avoid the "Acknowledgment of Acknowledgments" if we always send
> | the required 9 loss intervals, I think.
> |
> | > A second point is how to compute the loss event rate when n > 9. It seems
> | > that this would mean grinding through all loss intervals using a window
> | > of 9. If that is the case, the per-packet-computation costs become very
> | > expensive.
> |
> | RFC 4342 section 8.6 suggests that only 9 loss intervals are required
> | anyway. And I believe that's enough for the computation of current
> | mean loss interval. What do you think?
> |
> Yes, absolutely, I am completely in favour of this very sensible suggestion.
>
> If people really must experiment with such outdated data, that could be
> done in truly experimental patches. Especially since RFC 5348 normatively
> recommends a value of n = 8 in section 5.4. And we are saving further
> headaches about the book-keeping/garbage collection of old data.
>
> | > II) Computational part of the implementation
> | > --------------------------------------------
> | > If only Loss Intervals alone are used, only these need to be verified
> | > before being used to alter the sender behaviour.
> | >
> | > But when one or more other DCCP options also appear, the verification is
> | >  * intra: make sure each received option is in itself consistent,
> | >  * inter: make sure options are mutually consistent.
> | >
> | > The second has a combinatorial effect, i.e. n! verifications for n options.
> | >
> <snip>
> |
> | Yes, there's a combinatorial problem in checking the options for consistence.
> | But, what if we find out that some option doesn't match against others?
> | What action would be taken?
> I add this as
>  4) define policy for dealing with options that are not mutually consistent
>
> | First, what can cause the receiver to send inconsistent options?
> | A bad implementation only?
> Yes I think that a bad implementation (whether on purpose or not) would be
> the main cause, since header options are protected even if partial
> checksums are used (RFC 4340, 9.2).
>
> But there is also the benign case mentioned at the end of RFC 4342, 9.2,
> where a receiver collapses multiple losses into a single loss event, i.e.
>  5) validate received Loss Intervals and regroup the receiver-based
>    information if necessary, without interpreting this as attempted
>    receiver misbehaviour.
>
> | Accordingly to ecn nonce echo sum algorithm, if a receiver is found to be
> | lying about loss or to be bad implemented, the sender adjusts the send rate
> | as if loss were perceived.
> | Can we do the same in this situation? If so, can we skip checking options
> | between them and only check ecn nonce sum?
> This is difficult since Ack Vectors and Loss Intervals use different
> definitions of ECN Nonce sum (last paragraph in RFC 4342, 9.1), i.e. we have
>  6) separate algorithms to compute Ack Vector/Loss Intervals ECN Nonce sum.
>
> With regard to (5) above, your suggestion gives
>  7) validate options, on mismatch other than (5) only validate ECN nonce.
>
> | If some option is wrong it show more loss (or any worse situation for the
> | receiver) or conceals loss. In the first case, I don't believe we need to care,
> | and in the second, the ecn nonce sum can reveal the bad acting of the receiver.
> Yes you are right, we need not worry if a receiver reports a higher loss rate
> than the verification done by the sender (which recomputes the data that the
> receiver already has computed) calculates.
>
> But for the second case, there is no guarantee to catch a misbehaving
> receiver, only a 50% chance at the end of many computations.

Isn't it 50% chance at each ecn verified? So, at the end we'll end up with 100%?

>
> RFC 4342, 9 suggests one way of verifying Loss Intervals / Ack Vectors:
>  5) occasionally do not send a packet, or send packet out of order.
>
> This increases complexity of book-keeping, the sender needs to keep track
> which of the sent packets was a fake send/drop. It also requires an algorithm
> to iterate over the sender data structures in order to find out whether the
> reasoning of the receiver is sane. I have doubts whether this can be done
> without sacrificing the performance of the in-kernel sender side.
>

I have doubts either. This seems to be too complicated and not much useful.

>
> | > III) Closing remarks in favour of receiver-based implementation
> | > ---------------------------------------------------------------
> | > Finally, both RFC 4342 and RFC 5622 do not explicitly discard the
> | > possibility of using a receiver-based implementation. Quoting
> | > RFC 4342, 3.2: "If it prefers, the sender can also use a loss event
> | >                rate calculated and reported by the receiver."
> | > Furthermore, the revised TFRC specification points out in section 7
> | > the advantages that a receiver-based implementation has:
> | >  * it does not mandate reliable delivery of packet loss data;
> | >  * it is robust against the loss of feedback packets;
> | >  * better suited for scalable server design.
> | >
> | > Quite likely, if the server does not have to store and validate a mass
> | > of data, it is also less prone to be toppled by DoS attacks.
> |
> | You're right. But what the RFC's says about it is almost exactly the
> | opposite, isn't? What can we do about it? I like the receiver-based design,
> | but I believe that loss intervals are interesting, mostly because  of
> | receiver behavior verification.
> |
> While writing the above reply, I was amazed to see how much of the computation
> that has already been done at the receiver needs to be done again at the sender,
> ust in order to be able to verify the data.
>
> To me this seems very inefficient.
>
> Moreover, the biggest danger I see here is spending a lot of time with the
> details of sender book-keeping and verification, just to then see that the
> performance of CCID-3/4 in practice turns out to be below the standards
> acceptable to even modest users.
>
> I think it is clearly better to prefer the simplest possible implementation
> in such cases, to better debug live protocol performance.
>
> In particular, since CCID-4 is designed to be an experimental protocol
> (i.e. if it works, RFC 5622 may mature into a Proposed Standard, if not,
> it might be superseded by a different specification).
>
> And I think that testing the actual user performance has the highest priority.

Yes, we can work more with a simpler implementation at the receiver
side and focus
on performance and test, and features too. After, we have a stable
version and good enough in performance terms,
 we can continue improving the sender side.

>
> The literature on the subject is almost exclusively done on experiences in
> ns-2 userland. Almost no Internet experiments at all have been done with DCCP.
>
> This is because the IPPROTO = 33 identifier needs to be entered
> especially into a firewall, which opens holes that firewall
> administrators don't like to open (unless the firewall is based on
> a recent Linux kernel, opening all ports for IP protocol identifier
> 33 is the only way of allowing DCCP traffic in/out of a firewall).
>
> In addition, most people use NAT at home, putting another obstacle
> on experiments. The result is then that tests are done in a lab testbed
> or in virtualisation - emulated networks.
>
> To conclude, I still think that the simpler, receiver-based implementation
> gives a better start. A 'perfect' receiver implementation is also a good
> reference point to start protocol evaluation: if the performance is bad
> despite getting things right at the receiver, then other parts of the
> protocol need investigation/improvement.

I agree. It's a risk to work on the sender at the moment, implementing
these features,
algorithms and ending with a CCID that doesn't match the expected performance.
Can you list the pending tasks in both code and tests to be done?


Cheers,

Ivo

--
Ivo Augusto Andrade Rocha Calado
MSc. Candidate
Embedded Systems and Pervasive Computing Lab - http://embedded.ufcg.edu.br
Systems and Computing Department - http://www.dsc.ufcg.edu.br
Electrical Engineering and Informatics Center - http://www.ceei.ufcg.edu.br
Federal University of Campina Grande - http://www.ufcg.edu.br

PGP: 0x03422935
Putt's Law:
      Technology is dominated by two types of people:
              Those who understand what they do not manage.
              Those who manage what they do not understand.

^ permalink raw reply

* Re: [net-next-2.6 PATCH v6 2/7 RFC] TCPCT part 1b: generate Responder Cookie
From: William Allen Simpson @ 2009-11-16 20:06 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Joe Perches, Linux Kernel Network Developers, Paul E. McKenney
In-Reply-To: <4B01710F.70800@gmail.com>

Eric Dumazet wrote:
> William Allen Simpson a écrit :
>> Do I hear an Ack?
>>
> Fine by me William
> 
> But you forgot to re-include full commit message and Signed-off-by:
> 
> Yes it seems cumbersome, but David is handling ~50 patches per day, 
> we shall give him as _perfect_ patches as possible.
> 
Agreed, I understood that I'd need to send out the _entire_ patch series
without the RFC designation before our leader would consider it.

Right now, I'm just collecting as many comments, improvements, and Acks as
possible before fetch, rebase, compiling, testing, and re-posting the
whole megillah again with "no fuzz" patches.

Hopefully, splitting this into more sub-parts (as you requested) enables
reviewing, but of course it's an exponential increase in preparation and
test time.  So, I'm trying to get some comments on every sub-part.

^ permalink raw reply

* Re: [RFC PATCH] net: add dataref destructor to sk_buff
From: Stephen Hemminger @ 2009-11-16 19:59 UTC (permalink / raw)
  To: Gregory Haskins
  Cc: Herbert Xu, Gregory Haskins, Michael S. Tsirkin, alacrityvm-devel,
	linux-kernel, netdev
In-Reply-To: <4AFE3FD2.6030403@gmail.com>

On Sat, 14 Nov 2009 00:27:46 -0500
Gregory Haskins <gregory.haskins@gmail.com> wrote:

> Stephen Hemminger wrote:
> > On Fri, 13 Nov 2009 21:27:57 -0500
> > Gregory Haskins <gregory.haskins@gmail.com> wrote:
> > 
> >> Herbert Xu wrote:
> >>> On Fri, Nov 13, 2009 at 08:33:35PM -0500, Gregory Haskins wrote:
> >>>> Well, not with respect to the overall protocol, of course not.  But with
> >>>> respect to the buffer in question, it _has_ to be.  Or am I missing
> >>>> something?
> >>> sendfile() has never guaranteed that the kernel is finished with
> >>> the underlying pages when it returns.
> >>>
> >>> Cheers,
> >> Clearly there must be _some_ mechanism to synchronize (e.g.
> >> flush/barrier) though, right?  Otherwise, that interface would seem to
> >> be quite prone to races and would likely be unusable.   So what does
> >> said flush use to know when the buffer is free?
> > 
> > No all the interfaces require a copy.
> 
> I'm sorry, but I do not think that is correct.  As others have pointed
> out, that would not appear to be true for at least sendfile.

Correct.

> 
> > Actually, sendfile makes no guarantee about synchronization
> > because the receiver of said file could be arbitrarily slow, and any attempt at locking down
> > current contents of file is a denial of service exposure.
> 
> I think you are inverting the problem space.  It is fully expected that
> changing the "file", or the pages that represent the file before the
> packet is queued would constitute the modification of the stream on the
> wire.
> 
> I am more thinking about the applications of mmap+sendfile to implement
> a zero-copy interface.  As David mentions in another mail, it seems that
> there is no sync mechanism available, so this would not appear to be a
> viable use case today, unfortunately.

yes, if you do mmap/sendfile then there is no synchronization, and the stack
can hold onto your data for an arbitrary time.  The file and mapping's can
be closed but that risks tying up all of memory.


> > 
> > People have tried doing copy-less send by page flipping, but the overhead of the IPI to
> > invalidate the TLB exceeded the overhead of the copy. There was an Intel paper on this in
> > at Linux Symposium (Ottawa) several years ago.
> 
> I think you are confusing copy-less tx with copy-less rx.  You can try
> to do copy-less rx with page flipping, which has the IPI/TLB thrashing
> properties you mention, and I agree is problematic.  We are talking
> about copy-less tx here, however, and therefore no page-flipping is
> involved.  Rather, we are just posting SG lists of pages directly to the
> NIC (assuming the nic supports HIGH_DMA, etc).  You do not need to flip
> the page, or invalidate the TLB (and thus IPI the other cores) to do
> this to my knowledge.
> 

If you want to do copy-less tx for all applications, you have to
do COW to handle the trivial case of :

while (cc = read(infd, buffer, sizeof buffer)) {
   send(outsock, buffer, cc);
}




-- 

^ permalink raw reply

* Re: [RFC] dev->refcnt long term holder
From: Eric Dumazet @ 2009-11-16 19:54 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: David S. Miller, Herbert Xu, Linux Netdev List
In-Reply-To: <20091116110207.08b60c81@nehalam>

Stephen Hemminger a écrit :
> 
> Or both..
> 

Well well well :)

I was hoping a fast path, but anyway, a linkwatch_forget_dev(dev) is probably better,
(in case "ip link del" closely follows an "ip link add / up")

I'll post something when tested.

Thanks

^ permalink raw reply

* Re: sparse vs. skbuff.h
From: Eric Dumazet @ 2009-11-16 19:51 UTC (permalink / raw)
  To: Johannes Berg; +Cc: netdev, Vegard Nossum
In-Reply-To: <1258399627.32159.41.camel@johannes.local>

Johannes Berg a écrit :
> On Mon, 2009-11-16 at 20:21 +0100, Johannes Berg wrote:
>> commit 14d18a81b5171d4433e41129619c75748b4f4d26
>> Author: Eric Dumazet <eric.dumazet@gmail.com>
>> Date:   Thu Oct 29 00:10:37 2009 +0000
>>
>>     net: fix kmemcheck annotations
>>
>>
>> broke sparse endian checks on everything that includes skbuff.h because
>> the first and only (because it's an error) thing sparse now reports is
>> this:
>>
>> include/linux/skbuff.h:357:41: error: invalid bitfield specifier for type restricted __be16.
> 
> Simply changing from
> 	__be16 protocol:16;
> to
> 	__be16 protocol;
> 
> but leaving it inside the kmemcheck annotation seems to do the right
> thing. Except of course that kmemcheck will not properly check it now.
> Maybe those annotations should simply be made to have no impact on
> struct padding instead?
> 

Hmm, I have really no idea of what is the right way to fix this stuff.

Last time I did adding a non bitfield element inside the begin/end annotations,
I was flamed, because a bitfield is a bitfield, not a char/short

http://www.spinics.net/lists/netdev/msg108803.html

Now sparse is complaining... What will be the next story ?


^ permalink raw reply

* Re: sparse vs. skbuff.h
From: Johannes Berg @ 2009-11-16 19:27 UTC (permalink / raw)
  To: netdev; +Cc: Eric Dumazet, Vegard Nossum
In-Reply-To: <1258399271.32159.39.camel@johannes.local>

[-- Attachment #1: Type: text/plain, Size: 825 bytes --]

On Mon, 2009-11-16 at 20:21 +0100, Johannes Berg wrote:
> commit 14d18a81b5171d4433e41129619c75748b4f4d26
> Author: Eric Dumazet <eric.dumazet@gmail.com>
> Date:   Thu Oct 29 00:10:37 2009 +0000
> 
>     net: fix kmemcheck annotations
> 
> 
> broke sparse endian checks on everything that includes skbuff.h because
> the first and only (because it's an error) thing sparse now reports is
> this:
> 
> include/linux/skbuff.h:357:41: error: invalid bitfield specifier for type restricted __be16.

Simply changing from
	__be16 protocol:16;
to
	__be16 protocol;

but leaving it inside the kmemcheck annotation seems to do the right
thing. Except of course that kmemcheck will not properly check it now.
Maybe those annotations should simply be made to have no impact on
struct padding instead?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply

* sparse vs. skbuff.h
From: Johannes Berg @ 2009-11-16 19:21 UTC (permalink / raw)
  To: netdev; +Cc: Eric Dumazet

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

commit 14d18a81b5171d4433e41129619c75748b4f4d26
Author: Eric Dumazet <eric.dumazet@gmail.com>
Date:   Thu Oct 29 00:10:37 2009 +0000

    net: fix kmemcheck annotations


broke sparse endian checks on everything that includes skbuff.h because
the first and only (because it's an error) thing sparse now reports is
this:

include/linux/skbuff.h:357:41: error: invalid bitfield specifier for type restricted __be16.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply

* Re: [RFC] dev->refcnt long term holder
From: Stephen Hemminger @ 2009-11-16 19:02 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: David S. Miller, Herbert Xu, Linux Netdev List
In-Reply-To: <4B019381.2010509@gmail.com>

On Mon, 16 Nov 2009 19:01:37 +0100
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> time ip link del eth3.103 ; time ip link del eth3.104 ; time ip link del eth3.105
> 
> real	0m0.266s
> user	0m0.000s
> sys	0m0.001s
> 
> real	0m0.770s
> user	0m0.000s
> sys	0m0.000s
> 
> real	0m1.022s
> user	0m0.000s
> sys	0m0.000s
> 
> 
> One problem of current schem in vlan dismantle phase is the
> holding of device done by following chain :
> 
> vlan_dev_stop() ->
> 	netif_carrier_off(dev) ->
> 		linkwatch_fire_event(dev) ->
> 			dev_hold() ...
> 
> And __linkwatch_run_queue() run up to one second later...
> 
> Is following patch one way to avoid the problem, or should
> we add a new linkwatch_forgetpro_device(dev) method to immediately
> release the device reference (and unlink device from the list) ?
> (This would probably need a doubly linked list instead of single link list)

Or both..

-- 

^ permalink raw reply

* Re: [PATCH] ixgbe: Fixing EEH handler to handle more than one error
From: Waskiewicz Jr, Peter P @ 2009-11-16 19:01 UTC (permalink / raw)
  To: David Miller
  Cc: leitao@linux.vnet.ibm.com, netdev@vger.kernel.org,
	Waskiewicz Jr, Peter P, Kirsher, Jeffrey T
In-Reply-To: <20091115.221628.243358818.davem@davemloft.net>

On Sun, 15 Nov 2009, David Miller wrote:

> From: leitao@linux.vnet.ibm.com
> Date: Tue, 10 Nov 2009 13:37:47 -0500
> 
> > After commmit 4b77b0a2ba27d64f58f16d8d4d48d8319dda36ff EEH breaks
> > after the second error, since it calls pci_restore_state()
> > but it returns 0, since pci->state_saved is false.
> > 
> > So, this patch just call pci_save_state() after pci_restore_state().
> > 
> > Signed-off-by: Breno Leitao <leitao@linux.vnet.ibm.com>
> 
> Intel folks, are you integrating or looking at this patch?

This patch looks correct and makes sense to me.  I'm going to ACK it now, 
but we'll still pull it in and test it.  If we see any issues, we'll 
report them with possible fixes.  But at this point, I think this patch is 
fine.

Acked-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>

^ permalink raw reply

* Re: [PATCH] net/can: add driver for mscan family & mpc52xx_mscan
From: Wolfram Sang @ 2009-11-16 18:44 UTC (permalink / raw)
  To: Grant Likely
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
	netdev-u79uwXL29TY76Z2rM5mHXA, David Miller, Wolfgang Grandegger,
	linuxppc-dev-mnsaURCQ41sdnm+yROfE0A
In-Reply-To: <fa686aa40911130939x54ac53f9x173a875a5a4435d3-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>


[-- Attachment #1.1: Type: text/plain, Size: 506 bytes --]

Hi Grant,

Wolfgang commented on some points already, I will pick up the other remarks,
just one question:

> > +       clk_src = of_get_property(np, "fsl,mscan-clk-src", NULL);
> > +       if (clk_src && strcmp(clk_src, "ip") == 0)
> 
> Should protect against non-null.  strncmp() maybe?

"ip" is null-terminated, or what do you mean?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

^ permalink raw reply

* Re: Those gpio-mdio patches...
From: Lennart Sorensen @ 2009-11-16 18:21 UTC (permalink / raw)
  To: David Miller; +Cc: lsorense, netdev
In-Reply-To: <20091113.135908.249908732.davem@davemloft.net>

On Fri, Nov 13, 2009 at 01:59:08PM -0800, David Miller wrote:
> Please don't fix the issue that way.  If the routine is returning a
> true or false value, make it return a 'bool' instead of an 'int'.
> That way we don't have to "accept something other than 0 and 1"

Hmm.  Well I just tried to fix what was already there.  gpio-lib does
NOT return 0 or 1, it returns 0 or 1 shifted by the gpio position in a
gpio register, but soemthing has to convert that to a boolean value.
Making it return a bool doesn't change the fact it needs the '!!' applied
to make it a bool.

I can look at changing the mdio-gpio to using bool instead of integer
where necesary.

-- 
Len Sorensen

^ permalink raw reply

* [RFC] dev->refcnt long term holder
From: Eric Dumazet @ 2009-11-16 18:01 UTC (permalink / raw)
  To: David S. Miller, Herbert Xu, Stephen Hemminger; +Cc: Linux Netdev List

time ip link del eth3.103 ; time ip link del eth3.104 ; time ip link del eth3.105

real	0m0.266s
user	0m0.000s
sys	0m0.001s

real	0m0.770s
user	0m0.000s
sys	0m0.000s

real	0m1.022s
user	0m0.000s
sys	0m0.000s


One problem of current schem in vlan dismantle phase is the
holding of device done by following chain :

vlan_dev_stop() ->
	netif_carrier_off(dev) ->
		linkwatch_fire_event(dev) ->
			dev_hold() ...

And __linkwatch_run_queue() run up to one second later...

Is following patch one way to avoid the problem, or should
we add a new linkwatch_forgetpro_device(dev) method to immediately
release the device reference (and unlink device from the list) ?
(This would probably need a doubly linked list instead of single link list)

Thanks

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 4198ec5..17216f9 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -512,7 +512,6 @@ static int vlan_dev_stop(struct net_device *dev)
 	if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr))
 		dev_unicast_delete(real_dev, dev->dev_addr);
 
-	netif_carrier_off(dev);
 	return 0;
 }
 

^ permalink raw reply related

* RE: Sharing VF device among Xen VMs
From: Rose, Gregory V @ 2009-11-16 17:39 UTC (permalink / raw)
  To: Satish Chowdhury, netdev@vger.kernel.org
In-Reply-To: <be5d34890911141149v45ba176ax5705d681def3470e@mail.gmail.com>

>-----Original Message-----
>From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
>On Behalf Of Satish Chowdhury
>Sent: Saturday, November 14, 2009 11:50 AM
>To: netdev@vger.kernel.org
>Subject: Sharing VF device among Xen VMs
>
>Hi,
>
>I am trying to verify a situation where VMs share a VF device of Intel
>82576 dual port for data traffic.
>
>Setup:
>Case -1: On a Vt-d machine Xen(xen-1) is installed. On dom0 multiple
>VFs are created for 82576ET dual port card. One of the VF is
>pass-through to a VM.  Now, the VM again has Xen (xen-2) installed.
>So, the VMs of Xen-2 have to share the passthrough VF device for data
>traffic.
>
>Will I be able to send data from Xen-2 VMs to external world?
>
>I had issues while creating VMs for Xen2. So, could not do the
>experiment.
>
>Case-2:  On Xen-1 itself I loaded igbvf driver. Changed xen
>configuration to make VF as default interface on dom0. Now VMs of
>Xen-1 should share the VF device.
>
>Ping from VF interface on VM  and PF ip address works.
>Ping between VMs goes through.
>But, ping from domU to another machine on same network on switch doesn't
>work.
>
>The arp broadcast request goes out through VF interface. But the arp
>reply doesn't reach VF interface, they get routed to PF interface. If
>PF interface to the bridge on dom0 then ping from VMs to external
>machine work.
>
>In my experiment, the arp reply that reaches the NIC, has mac address
>of interface on VM(domU). 82576 performs L2 filtering based on VF MAC
>address. So, packet is not queued to VF interface.
>
>Is it possible to add VMs mac address to L2 filtering pool of the NIC?
>
>Regards,
>-Satish
>--
[Rose, Gregory V] 

No, there is no tool for this right now.  Extensions to the iproute2 package are under review internally and either a set of proposed patches or an RFC should be forthcoming soon.

You are correct that you need to be able to add the MAC addresses of VMs using the bridge interface to the L2 filter table of the NIC before this setup will work correctly.

For now the 82576 in SR-IOV modes of operation only supports communication among the VFs.  You should not try to use the physical function as a bridge interface until the tool I mentioned above becomes available.

- Greg

^ permalink raw reply

* Re: [PATCH net-next 1/2] Phonet: put protocols array under RCU
From: Rémi Denis-Courmont @ 2009-11-16 17:34 UTC (permalink / raw)
  To: paulmck; +Cc: netdev, Rémi Denis-Courmont
In-Reply-To: <20091116172617.GC6746@linux.vnet.ibm.com>

Le lundi 16 novembre 2009 19:26:17 Paul E. McKenney, vous avez écrit :
> On Fri, Nov 13, 2009 at 05:01:18PM +0200, Rémi Denis-Courmont wrote:
> > From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> >
> > Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> > ---
> >  net/phonet/af_phonet.c |   20 +++++++++++---------
> >  1 files changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/net/phonet/af_phonet.c b/net/phonet/af_phonet.c
> > index 8d3a55b..ed65da2 100644
> > --- a/net/phonet/af_phonet.c
> > +++ b/net/phonet/af_phonet.c
> > @@ -35,7 +35,6 @@
> >
> >  /* Transport protocol registration */
> >  static struct phonet_protocol *proto_tab[PHONET_NPROTO] __read_mostly;
> > -static DEFINE_SPINLOCK(proto_tab_lock);
> >
> >  static struct phonet_protocol *phonet_proto_get(int protocol)
> >  {
> > @@ -44,11 +43,11 @@ static struct phonet_protocol *phonet_proto_get(int
> > protocol) if (protocol >= PHONET_NPROTO)
> >  		return NULL;
> >
> > -	spin_lock(&proto_tab_lock);
> > +	rcu_read_lock();
> >  	pp = proto_tab[protocol];
> 
> Don't we need an rcu_dereference() in here somewhere?
> 
> Perhaps something like the following?

Err yes, we do.

-- 
Rémi Denis-Courmont
http://www.remlab.net/

^ permalink raw reply

* Re: [PATCH net-next 1/2] Phonet: put protocols array under RCU
From: Paul E. McKenney @ 2009-11-16 17:26 UTC (permalink / raw)
  To: Rémi Denis-Courmont; +Cc: netdev, Rémi Denis-Courmont
In-Reply-To: <1258124479-1167-1-git-send-email-remi@remlab.net>

On Fri, Nov 13, 2009 at 05:01:18PM +0200, Rémi Denis-Courmont wrote:
> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> 
> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> ---
>  net/phonet/af_phonet.c |   20 +++++++++++---------
>  1 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/net/phonet/af_phonet.c b/net/phonet/af_phonet.c
> index 8d3a55b..ed65da2 100644
> --- a/net/phonet/af_phonet.c
> +++ b/net/phonet/af_phonet.c
> @@ -35,7 +35,6 @@
> 
>  /* Transport protocol registration */
>  static struct phonet_protocol *proto_tab[PHONET_NPROTO] __read_mostly;
> -static DEFINE_SPINLOCK(proto_tab_lock);
> 
>  static struct phonet_protocol *phonet_proto_get(int protocol)
>  {
> @@ -44,11 +43,11 @@ static struct phonet_protocol *phonet_proto_get(int protocol)
>  	if (protocol >= PHONET_NPROTO)
>  		return NULL;
> 
> -	spin_lock(&proto_tab_lock);
> +	rcu_read_lock();
>  	pp = proto_tab[protocol];

Don't we need an rcu_dereference() in here somewhere?

Perhaps something like the following?

	pp = rcu_dereference(proto_tab[protocol]);

							Thanx, Paul

>  	if (pp && !try_module_get(pp->prot->owner))
>  		pp = NULL;
> -	spin_unlock(&proto_tab_lock);
> +	rcu_read_unlock();
> 
>  	return pp;
>  }
> @@ -439,6 +438,8 @@ static struct packet_type phonet_packet_type __read_mostly = {
>  	.func = phonet_rcv,
>  };
> 
> +static DEFINE_MUTEX(proto_tab_lock);
> +
>  int __init_or_module phonet_proto_register(int protocol,
>  						struct phonet_protocol *pp)
>  {
> @@ -451,12 +452,12 @@ int __init_or_module phonet_proto_register(int protocol,
>  	if (err)
>  		return err;
> 
> -	spin_lock(&proto_tab_lock);
> +	mutex_lock(&proto_tab_lock);
>  	if (proto_tab[protocol])
>  		err = -EBUSY;
>  	else
> -		proto_tab[protocol] = pp;
> -	spin_unlock(&proto_tab_lock);
> +		rcu_assign_pointer(proto_tab[protocol], pp);
> +	mutex_unlock(&proto_tab_lock);
> 
>  	return err;
>  }
> @@ -464,10 +465,11 @@ EXPORT_SYMBOL(phonet_proto_register);
> 
>  void phonet_proto_unregister(int protocol, struct phonet_protocol *pp)
>  {
> -	spin_lock(&proto_tab_lock);
> +	mutex_lock(&proto_tab_lock);
>  	BUG_ON(proto_tab[protocol] != pp);
> -	proto_tab[protocol] = NULL;
> -	spin_unlock(&proto_tab_lock);
> +	rcu_assign_pointer(proto_tab[protocol], NULL);
> +	mutex_unlock(&proto_tab_lock);
> +	synchronize_rcu();
>  	proto_unregister(pp->prot);
>  }
>  EXPORT_SYMBOL(phonet_proto_unregister);
> -- 
> 1.6.3.3
> 
> --
> 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

^ permalink raw reply

* Re: [RFC PATCH] net: add dataref destructor to sk_buff
From: Avi Kivity @ 2009-11-16 17:08 UTC (permalink / raw)
  To: David Miller
  Cc: gregory.haskins, herbert, ghaskins, mst, alacrityvm-devel,
	linux-kernel, netdev
In-Reply-To: <20091113.190438.78469912.davem@davemloft.net>

On 11/14/2009 05:04 AM, David Miller wrote:
> From: Gregory Haskins<gregory.haskins@gmail.com>
> Date: Fri, 13 Nov 2009 20:33:35 -0500
>
>    
>> Well, not with respect to the overall protocol, of course not.  But with
>> respect to the buffer in question, it _has_ to be.  Or am I missing
>> something?
>>      
> sendfile() absolutely, and positively, is not.
>
> Any entity can write to the pages being send via sendfile(), at will,
> and those writes will show up in the packet stream if they occur
> before the NIC DMA's the memory backed by those pages into it's
> buffer.
>
> There is zero data synchronization whatsoever, we don't lock the
> pages, we don't block their usage while they are queued up in the
> socket send queue, nothing like that.
>
>    

But it must maintain a reference count on the page being dmaed and drop 
it only after dma is complete.  Otherwise we risk the page being 
recycled and arbitrary memory sent out on the wire; and an application 
can trivially cause this by truncate()ing a sendfile.

> The user returns long before it every hits the wire and there is zero
> "notification" to the user that the pages in question for the
> sendfile() request are no longer in use.
>    

The put_page() is a notification except it doesn't reach the caller.  
Gregory's patch (and previous shared info destructor patches) is an 
attempt to make it reach the caller, IIUC.

-- 
error compiling committee.c: too many arguments to function

^ permalink raw reply

* Re: [PATCH 1/2] rps: core implementation
From: Tom Herbert @ 2009-11-16 17:02 UTC (permalink / raw)
  To: Andi Kleen; +Cc: David Miller, netdev
In-Reply-To: <87hbt0kaae.fsf@basil.nowhere.org>

>> +     case __constant_htons(ETH_P_IPV6):
>> +             if (!pskb_may_pull(skb, sizeof(*ip6)))
>> +                     return -1;
>> +
>> +             ip6 = (struct ipv6hdr *) skb->data;
>> +             ip_proto = ip6->nexthdr;
>> +             addr1 = ip6->saddr.s6_addr32[3];
>> +             addr2 = ip6->daddr.s6_addr32[3];
>
> Why only [3] ? Is this future proof?
>
No.  But it's same as inet6_ehashfn :-)

>> +     for_each_cpu_mask_nr(cpu, __get_cpu_var(rps_remote_softirq_cpus)) {
>> +             struct softnet_data *queue = &per_cpu(softnet_data, cpu);
>> +             __smp_call_function_single(cpu, &queue->csd, 0);
>
> How do you get around the standard deadlocks with IPI called from
> irq disabled section?
>

What are the standard deadlocks?  Looks like __send_remote_softirq
will call __smp_call_function with irq's disabled...

> And why are the interrupts are disabled here anyways?
>

Protects rps_remote_softirq_cpus.


> It's a standard pet peeve of me, but it's quite unlikely you'll
> get any useful entropy at this time of kernel startup.
>
> Normally it's always the same.
>
Would it make sense to just use skb_tx_hashrnd for the receive hash
key also (renaming it to be more general)?


>> +     if (err)
>> +             return err;
>> +
>> +     rtnl_lock();
>
> It seems weird to do user parsing while holding that lock.
> Better first set up and allocate and then finally initialize global state.

Yes.  We could build a new map each time and then insert it into the
device structure using an rcu lock (which I hope would be sufficient
locking)

Thanks,
Tom

^ permalink raw reply

* Re: [net-next-2.6 PATCH v2] allow access to sysfs_groups member
From: Stephen Hemminger @ 2009-11-16 16:46 UTC (permalink / raw)
  To: Kurt Van Dijck; +Cc: Oliver Hartkopp, Wolfgang Grandegger, netdev
In-Reply-To: <20091114165408.GA283@e-circ.dyndns.org>

On Sat, 14 Nov 2009 17:54:09 +0100
Kurt Van Dijck <kurt.van.dijck@eia.be> wrote:

> On Fri, Nov 13, 2009 at 02:27:38PM -0800, Stephen Hemminger wrote:
> > On Fri, 13 Nov 2009 11:51:57 +0100
> > Kurt Van Dijck <kurt.van.dijck@eia.be> wrote:
> [...]
> > EXPORT_SYMBOL_GPL() for all device/sysfs related stuff.
> Ok, no problem
> > 
> > Also, need some way to BUG() if this is done after device has
> > been registered.  
> Ok. I gave it a try. I _think_ I did it write, but a second look would
> not harm :-). I was not able to run a real test yet.
> > 
> > Another way to add sub-directories which is what bridge, bonding,
> > and others do is to use another kobject. I think this is what you
> > want for the case of two CAN objects under one netdevice.
> In fact, I wanted to add this for cards that have multiple seperate CAN
> network device, combined on 1 PCI or PCMCIA device.
> In the 'add network' uevent, a udev rule could find properties of the
> card (device/ symlink). Right now, there is no way to tell if a network
> device is bus 1 or 2 on the card. in CAN, there's no such thing as a MAC
> address.
> I encountered this issue with a softing CAN card (not yet in mainline),
> and there are other drivers in the socketCAN queue that have the same
> problem.
> 
> The ethernet cards with multiple busses (that I've seen yet :-) )
> combine multiple PCI devices on 1 card, and identification of the
> 'instance on the card' can happen with the sysfs properties delivered by
> the PCI bus.
> CAN devices with multiple busses typically are combined all together on
> 1 single device on the PCI or PCMCIA bus.
> 
> So, I wanted to add a 'channel' property in /sys/class/net/canX, which
> could indicate the instance on the device. Such property must be
> installed by the driver, not the bus the device is on.
> This patch allows me to have this channel property present at the moment
> of the uevent.
> 
> I can imagine other subsystems may benefit from this too.

Okay by me.

Acked-by: Stephen Hemminger <shemminger@vyatta.com>


-- 

^ permalink raw reply

* Re: [PATCH 1/2] rps: core implementation
From: Tom Herbert @ 2009-11-16 16:43 UTC (permalink / raw)
  To: David Miller; +Cc: netdev
In-Reply-To: <20091116.031914.65020185.davem@davemloft.net>

On Mon, Nov 16, 2009 at 3:19 AM, David Miller <davem@davemloft.net> wrote:
> From: Tom Herbert <therbert@google.com>
> Date: Tue, 10 Nov 2009 22:53:17 -0800
>
>> +             /* Schedule NAPI for backlog device */
>> +             if (napi_schedule_prep(&queue->backlog)) {
>> +                     if (cpu != smp_processor_id()) {
>> +                             cpu_set(cpu,
>> +                                 get_cpu_var(rps_remote_softirq_cpus));
>> +                             __raise_softirq_irqoff(NET_RPS_SOFTIRQ);
>> +                     } else
>> +                             __napi_schedule(&queue->backlog);
>> +             }
>> +             goto enqueue;
>
> {,__}send_remote_softirq() doesn't work? :-)
>
NET_RPS_SOFTIRQ is intended to provide coalescing of IPIs.

send_remote_softirq could be used, but we would also need to get the
napi structure on the remote cpu poll list so that would probably need
to be protected by locks (something like __napi_schedule_oncpu could
be defined).  Would this be better to do?

^ permalink raw reply

* Re: [PATCH] ifb: add multi-queue support
From: Stephen Hemminger @ 2009-11-16 16:39 UTC (permalink / raw)
  To: xiaosuo; +Cc: David S. Miller, Patrick McHardy, Eric Dumazet, Tom Herbert,
	netdev
In-Reply-To: <4AFCE273.7010901@gmail.com>

On Fri, 13 Nov 2009 12:37:07 +0800
Changli Gao <xiaosuo@gmail.com> wrote:

> ifb: add multi-queue support
> 
> Add multi-queue support, and one kernel thread is created for per queue.
> It can used to emulate multi-queue NIC in software, and distribute work
> among CPUs.

My $.02 is that receive packet steering RPS should be done generically at
receive layer. Then all the CPU, mapping and configuration issues can be
done once, not just for IFB, Bridge, VLAN, ... The number of users of IFB
is small, and setup is complex. Steering packets in IFB is optimizing only
a rarely used corner.

Layered link services like IFB need to be multi-threaded lockless to maintain
the advantages of multi-queue and RPS.

-- 

^ permalink raw reply

* Re: pull request: wireless-2.6 2009-11-16
From: John W. Linville @ 2009-11-16 16:32 UTC (permalink / raw)
  To: davem; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <20091116162655.GD14410@tuxdriver.com>

On Mon, Nov 16, 2009 at 11:26:55AM -0500, John W. Linville wrote:

> activity in ipw2200, so it the revert should be set.  Plus, I've
> tested the revert locally.

s/so it the revert should be set/so the revert should be safe/

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox