Netdev List
 help / color / mirror / Atom feed
* [PATCH 10/10] [ACKVEC]: Separate option parsing from CCID processing
From: Gerrit Renker @ 2008-02-21  9:09 UTC (permalink / raw)
  To: acme; +Cc: dccp, netdev, Gerrit Renker
In-Reply-To: <1203584968-8957-10-git-send-email-gerrit@erg.abdn.ac.uk>

This patch replaces an almost identical replication of code: large parts
of dccp_parse_options() re-appeared as ccid2_ackvector() in ccid2.c.

Apart from the duplication, this caused two more problems:
 1. CCIDs should not need to be concerned with parsing header options;
 2. one can not assume that Ack Vectors appear as a contiguous area within an
    skb, it is legal to insert other options and/or padding in between. The
    current code would throw an error and stop reading in such a case.

The patch provides a new data structure and associated list housekeeping.

Only small changes were necessary to integrate with CCID-2: data structure
initialisation, adapt list traversal routine, and add call to the provided
cleanup routine.

The latter also lead to fixing the following BUG: CCID-2 so far ignored
Ack Vectors on all packets other than Ack/DataAck, which is incorrect,
since Ack Vectors can be present on any packet that has an Ack field.

Details:
--------
 * received Ack Vectors are parsed by dccp_parse_options() alone, which passes
   the result on to the CCID-specific routine ccid_hc_tx_parse_options();
 * CCIDs interested in using/decoding Ack Vector information will add code
   to fetch parsed Ack Vectors via this interface;
 * a data structure, `struct dccp_ackvec_parsed' is provided as interface;
 * this structure arranges Ack Vectors of the same skb into a FIFO order;
 * a doubly-linked list is used to keep the required FIFO code small.

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/ackvec.c      |   28 ++++++++++
 net/dccp/ackvec.h      |   19 +++++++
 net/dccp/ccids/ccid2.c |  136 +++++++++++++++---------------------------------
 net/dccp/ccids/ccid2.h |    2 +
 net/dccp/options.c     |   17 ++++---
 5 files changed, 102 insertions(+), 100 deletions(-)

--- a/net/dccp/ackvec.h
+++ b/net/dccp/ackvec.h
@@ -112,4 +112,23 @@ static inline bool dccp_ackvec_is_empty(const struct dccp_ackvec *av)
 {
 	return av->av_overflow == 0 && av->av_buf_head == av->av_buf_tail;
 }
+
+/**
+ * struct dccp_ackvec_parsed  -  Record offsets of Ack Vectors in skb
+ * @vec:	start of vector (offset into skb)
+ * @len:	length of @vec
+ * @nonce:	whether @vec had an ECN nonce of 0 or 1
+ * @node:	FIFO - arranged in descending order of ack_ackno
+ * This structure is used by CCIDs to access Ack Vectors in a received skb.
+ */
+struct dccp_ackvec_parsed {
+	u8		 *vec,
+			 len,
+			 nonce:1;
+	struct list_head node;
+};
+
+extern int dccp_ackvec_parsed_add(struct list_head *head,
+				  u8 *vec, u8 len, u8 nonce);
+extern void dccp_ackvec_parsed_cleanup(struct list_head *parsed_chunks);
 #endif /* _ACKVEC_H */
--- a/net/dccp/ackvec.c
+++ b/net/dccp/ackvec.c
@@ -345,6 +345,34 @@ free_records:
 	}
 }
 
+/*
+ *	Routines to keep track of Ack Vectors received in an skb
+ */
+int dccp_ackvec_parsed_add(struct list_head *head, u8 *vec, u8 len, u8 nonce)
+{
+	struct dccp_ackvec_parsed *new = kmalloc(sizeof(*new), GFP_ATOMIC);
+
+	if (new == NULL)
+		return -ENOBUFS;
+	new->vec   = vec;
+	new->len   = len;
+	new->nonce = nonce;
+
+	list_add_tail(&new->node, head);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dccp_ackvec_parsed_add);
+
+void dccp_ackvec_parsed_cleanup(struct list_head *parsed_chunks)
+{
+	struct dccp_ackvec_parsed *cur, *next;
+
+	list_for_each_entry_safe(cur, next, parsed_chunks, node)
+		kfree(cur);
+	INIT_LIST_HEAD(parsed_chunks);
+}
+EXPORT_SYMBOL_GPL(dccp_ackvec_parsed_cleanup);
+
 int __init dccp_ackvec_init(void)
 {
 	dccp_ackvec_slab = kmem_cache_create("dccp_ackvec",
--- a/net/dccp/options.c
+++ b/net/dccp/options.c
@@ -135,13 +135,6 @@ int dccp_parse_options(struct sock *sk, struct dccp_request_sock *dreq,
 			if (rc)
 				goto out_invalid_option;
 			break;
-		case DCCPO_ACK_VECTOR_0:
-		case DCCPO_ACK_VECTOR_1:
-			if (dccp_packet_without_ack(skb))   /* RFC 4340, 11.4 */
-				break;
-			dccp_pr_debug("%s Ack Vector (len=%u)\n", dccp_role(sk),
-				      len);
-			break;
 		case DCCPO_TIMESTAMP:
 			if (len != 4)
 				goto out_invalid_option;
@@ -229,6 +222,16 @@ int dccp_parse_options(struct sock *sk, struct dccp_request_sock *dreq,
 						     value) != 0)
 				goto out_invalid_option;
 			break;
+		case DCCPO_ACK_VECTOR_0:
+		case DCCPO_ACK_VECTOR_1:
+			if (dccp_packet_without_ack(skb))   /* RFC 4340, 11.4 */
+				break;
+			/*
+			 * Ack vectors are processed by the TX CCID if it is
+			 * interested. The RX CCID need not parse Ack Vectors,
+			 * since it is only interested in clearing old state.
+			 * Fall through.
+			 */
 		case DCCPO_MIN_TX_CCID_SPECIFIC ... DCCPO_MAX_TX_CCID_SPECIFIC:
 			if (ccid_hc_tx_parse_options(dp->dccps_hc_tx_ccid, sk,
 						     opt, len, value - options,
--- a/net/dccp/ccids/ccid2.h
+++ b/net/dccp/ccids/ccid2.h
@@ -47,6 +47,7 @@ struct ccid2_seq {
  * @ccid2hctx_lastrtt -time RTT was last measured
  * @ccid2hctx_rpseq - last consecutive seqno
  * @ccid2hctx_rpdupack - dupacks since rpseq
+ * @ccid2hctx_av_chunks: list of Ack Vectors received on current skb
 */
 struct ccid2_hc_tx_sock {
 	u32			ccid2hctx_cwnd;
@@ -66,6 +67,7 @@ struct ccid2_hc_tx_sock {
 	int			ccid2hctx_rpdupack;
 	unsigned long		ccid2hctx_last_cong;
 	u64			ccid2hctx_high_ack;
+	struct list_head	ccid2hctx_av_chunks;
 };
 
 struct ccid2_hc_rx_sock {
--- a/net/dccp/ccids/ccid2.c
+++ b/net/dccp/ccids/ccid2.c
@@ -320,68 +320,6 @@ static void ccid2_hc_tx_packet_sent(struct sock *sk, int more, unsigned int len)
 #endif
 }
 
-/* XXX Lame code duplication!
- * returns -1 if none was found.
- * else returns the next offset to use in the function call.
- */
-static int ccid2_ackvector(struct sock *sk, struct sk_buff *skb, int offset,
-			   unsigned char **vec, unsigned char *veclen)
-{
-	const struct dccp_hdr *dh = dccp_hdr(skb);
-	unsigned char *options = (unsigned char *)dh + dccp_hdr_len(skb);
-	unsigned char *opt_ptr;
-	const unsigned char *opt_end = (unsigned char *)dh +
-					(dh->dccph_doff * 4);
-	unsigned char opt, len;
-	unsigned char *value;
-
-	BUG_ON(offset < 0);
-	options += offset;
-	opt_ptr = options;
-	if (opt_ptr >= opt_end)
-		return -1;
-
-	while (opt_ptr != opt_end) {
-		opt   = *opt_ptr++;
-		len   = 0;
-		value = NULL;
-
-		/* Check if this isn't a single byte option */
-		if (opt > DCCPO_MAX_RESERVED) {
-			if (opt_ptr == opt_end)
-				goto out_invalid_option;
-
-			len = *opt_ptr++;
-			if (len < 3)
-				goto out_invalid_option;
-			/*
-			 * Remove the type and len fields, leaving
-			 * just the value size
-			 */
-			len     -= 2;
-			value   = opt_ptr;
-			opt_ptr += len;
-
-			if (opt_ptr > opt_end)
-				goto out_invalid_option;
-		}
-
-		switch (opt) {
-		case DCCPO_ACK_VECTOR_0:
-		case DCCPO_ACK_VECTOR_1:
-			*vec	= value;
-			*veclen = len;
-			return offset + (opt_ptr - options);
-		}
-	}
-
-	return -1;
-
-out_invalid_option:
-	DCCP_BUG("Invalid option - this should not happen (previous parsing)!");
-	return -1;
-}
-
 static void ccid2_hc_tx_kill_rto_timer(struct sock *sk)
 {
 	struct ccid2_hc_tx_sock *hctx = ccid2_hc_tx_sk(sk);
@@ -502,15 +440,28 @@ static void ccid2_congestion_event(struct sock *sk, struct ccid2_seq *seqp)
 		ccid2_change_l_ack_ratio(sk, hctx->ccid2hctx_cwnd);
 }
 
+static int ccid2_hc_tx_parse_options(struct sock *sk, unsigned char option,
+				     unsigned char len, u16 idx,
+				     unsigned char *value)
+{
+	struct ccid2_hc_tx_sock *hctx = ccid2_hc_tx_sk(sk);
+
+	switch (option) {
+	case DCCPO_ACK_VECTOR_0:
+	case DCCPO_ACK_VECTOR_1:
+		return dccp_ackvec_parsed_add(&hctx->ccid2hctx_av_chunks, value,
+					      len, option - DCCPO_ACK_VECTOR_0);
+	}
+	return 0;
+}
+
 static void ccid2_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)
 {
 	struct dccp_sock *dp = dccp_sk(sk);
 	struct ccid2_hc_tx_sock *hctx = ccid2_hc_tx_sk(sk);
+	struct dccp_ackvec_parsed *avp;
 	u64 ackno, seqno;
 	struct ccid2_seq *seqp;
-	unsigned char *vector;
-	unsigned char veclen;
-	int offset = 0;
 	int done = 0;
 	unsigned int maxincr = 0;
 
@@ -545,17 +496,12 @@ static void ccid2_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)
 	}
 
 	/* check forward path congestion */
-	/* still didn't send out new data packets */
-	if (hctx->ccid2hctx_seqh == hctx->ccid2hctx_seqt)
+	if (dccp_packet_without_ack(skb))
 		return;
 
-	switch (DCCP_SKB_CB(skb)->dccpd_type) {
-	case DCCP_PKT_ACK:
-	case DCCP_PKT_DATAACK:
-		break;
-	default:
-		return;
-	}
+	/* still didn't send out new data packets */
+	if (hctx->ccid2hctx_seqh == hctx->ccid2hctx_seqt)
+		goto done;
 
 	ackno = DCCP_SKB_CB(skb)->dccpd_ack_seq;
 	if (after48(ackno, hctx->ccid2hctx_high_ack))
@@ -579,15 +525,16 @@ static void ccid2_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)
 		maxincr = DIV_ROUND_UP(dp->dccps_l_ack_ratio, 2);
 
 	/* go through all ack vectors */
-	while ((offset = ccid2_ackvector(sk, skb, offset,
-					 &vector, &veclen)) != -1) {
+	list_for_each_entry(avp, &hctx->ccid2hctx_av_chunks, node) {
 		/* go through this ack vector */
-		while (veclen--) {
-			u64 ackno_end_rl = SUB48(ackno, dccp_ackvec_runlen(vector));
+		for (; avp->len--; avp->vec++) {
+			u64 ackno_end_rl = SUB48(ackno,
+						 dccp_ackvec_runlen(avp->vec));
 
-			ccid2_pr_debug("ackvec start:%llu end:%llu\n",
+			ccid2_pr_debug("ackvec %llu |%u,%u|\n",
 				       (unsigned long long)ackno,
-				       (unsigned long long)ackno_end_rl);
+				       dccp_ackvec_state(avp->vec) >> 6,
+				       dccp_ackvec_runlen(avp->vec));
 			/* if the seqno we are analyzing is larger than the
 			 * current ackno, then move towards the tail of our
 			 * seqnos.
@@ -606,7 +553,7 @@ static void ccid2_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)
 			 * run length
 			 */
 			while (between48(seqp->ccid2s_seq,ackno_end_rl,ackno)) {
-				const u8 state = dccp_ackvec_state(vector);
+				const u8 state = dccp_ackvec_state(avp->vec);
 
 				/* new packet received or marked */
 				if (state != DCCPAV_NOT_RECEIVED &&
@@ -633,7 +580,6 @@ static void ccid2_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)
 				break;
 
 			ackno = SUB48(ackno_end_rl, 1);
-			vector++;
 		}
 		if (done)
 			break;
@@ -697,6 +643,8 @@ static void ccid2_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)
 	}
 
 	ccid2_hc_tx_check_sanity(hctx);
+done:
+	dccp_ackvec_parsed_cleanup(&hctx->ccid2hctx_av_chunks);
 }
 
 static int ccid2_hc_tx_init(struct ccid *ccid, struct sock *sk)
@@ -731,6 +679,7 @@ static int ccid2_hc_tx_init(struct ccid *ccid, struct sock *sk)
 	hctx->ccid2hctx_last_cong = jiffies;
 	setup_timer(&hctx->ccid2hctx_rtotimer, ccid2_hc_tx_rto_expire,
 			(unsigned long)sk);
+	INIT_LIST_HEAD(&hctx->ccid2hctx_av_chunks);
 
 	ccid2_hc_tx_check_sanity(hctx);
 	return 0;
@@ -766,17 +715,18 @@ static void ccid2_hc_rx_packet_recv(struct sock *sk, struct sk_buff *skb)
 }
 
 static struct ccid_operations ccid2 = {
-	.ccid_id		= DCCPC_CCID2,
-	.ccid_name		= "TCP-like",
-	.ccid_owner		= THIS_MODULE,
-	.ccid_hc_tx_obj_size	= sizeof(struct ccid2_hc_tx_sock),
-	.ccid_hc_tx_init	= ccid2_hc_tx_init,
-	.ccid_hc_tx_exit	= ccid2_hc_tx_exit,
-	.ccid_hc_tx_send_packet	= ccid2_hc_tx_send_packet,
-	.ccid_hc_tx_packet_sent	= ccid2_hc_tx_packet_sent,
-	.ccid_hc_tx_packet_recv	= ccid2_hc_tx_packet_recv,
-	.ccid_hc_rx_obj_size	= sizeof(struct ccid2_hc_rx_sock),
-	.ccid_hc_rx_packet_recv	= ccid2_hc_rx_packet_recv,
+	.ccid_id		  = DCCPC_CCID2,
+	.ccid_name		  = "TCP-like",
+	.ccid_owner		  = THIS_MODULE,
+	.ccid_hc_tx_obj_size	  = sizeof(struct ccid2_hc_tx_sock),
+	.ccid_hc_tx_init	  = ccid2_hc_tx_init,
+	.ccid_hc_tx_exit	  = ccid2_hc_tx_exit,
+	.ccid_hc_tx_send_packet	  = ccid2_hc_tx_send_packet,
+	.ccid_hc_tx_packet_sent	  = ccid2_hc_tx_packet_sent,
+	.ccid_hc_tx_parse_options = ccid2_hc_tx_parse_options,
+	.ccid_hc_tx_packet_recv	  = ccid2_hc_tx_packet_recv,
+	.ccid_hc_rx_obj_size	  = sizeof(struct ccid2_hc_rx_sock),
+	.ccid_hc_rx_packet_recv	  = ccid2_hc_rx_packet_recv,
 };
 
 #ifdef CONFIG_IP_DCCP_CCID2_DEBUG

^ permalink raw reply

* [PATCH 09/10] [ACKVEC]: Remove old infrastructure
From: Gerrit Renker @ 2008-02-21  9:09 UTC (permalink / raw)
  To: acme; +Cc: dccp, netdev, Gerrit Renker
In-Reply-To: <1203584968-8957-9-git-send-email-gerrit@erg.abdn.ac.uk>

This removes
 * functions for which updates have been provided in the preceding patches and
 * the @av_vec_len field - it is no longer necessary since the buffer length is
   now always computed dynamically;
 * conditional debugging code (CONFIG_IP_DCCP_ACKVEC).

The reason for removing the conditional debugging code is that Ack Vectors are
an almost inevitable necessity - RFC 4341 says that for CCID-2, Ack Vectors must
be used. Furthermore, the code would be only interesting for coding - after some
extensive testing with this patch set, having the debug code around is no longer
of real help.

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/Kconfig       |    3 -
 net/dccp/Makefile      |    5 +-
 net/dccp/ackvec.c      |  252 ------------------------------------------------
 net/dccp/ackvec.h      |   79 +---------------
 net/dccp/ccids/Kconfig |    1 -
 5 files changed, 3 insertions(+), 337 deletions(-)

--- a/net/dccp/ackvec.c
+++ b/net/dccp/ackvec.c
@@ -9,19 +9,10 @@
  *      under the terms of the GNU General Public License as published by the
  *      Free Software Foundation; version 2 of the License;
  */
-
-#include "ackvec.h"
 #include "dccp.h"
-
-#include <linux/dccp.h>
-#include <linux/init.h>
-#include <linux/errno.h>
 #include <linux/kernel.h>
-#include <linux/skbuff.h>
 #include <linux/slab.h>
 
-#include <net/sock.h>
-
 static struct kmem_cache *dccp_ackvec_slab;
 static struct kmem_cache *dccp_ackvec_record_slab;
 
@@ -284,249 +275,6 @@ void dccp_ackvec_input(struct dccp_ackvec *av, u64 seqno, u8 state)
 	}
 }
 
-/*
- * If several packets are missing, the HC-Receiver may prefer to enter multiple
- * bytes with run length 0, rather than a single byte with a larger run length;
- * this simplifies table updates if one of the missing packets arrives.
- */
-static inline int dccp_ackvec_set_buf_head_state(struct dccp_ackvec *av,
-						 const unsigned int packets,
-						 const unsigned char state)
-{
-	unsigned int gap;
-	long new_head;
-
-	if (av->av_vec_len + packets > DCCPAV_MAX_ACKVEC_LEN)
-		return -ENOBUFS;
-
-	gap	 = packets - 1;
-	new_head = av->av_buf_head - packets;
-
-	if (new_head < 0) {
-		if (gap > 0) {
-			memset(av->av_buf, DCCPAV_NOT_RECEIVED,
-			       gap + new_head + 1);
-			gap = -new_head;
-		}
-		new_head += DCCPAV_MAX_ACKVEC_LEN;
-	}
-
-	av->av_buf_head = new_head;
-
-	if (gap > 0)
-		memset(av->av_buf + av->av_buf_head + 1,
-		       DCCPAV_NOT_RECEIVED, gap);
-
-	av->av_buf[av->av_buf_head] = state;
-	av->av_vec_len += packets;
-	return 0;
-}
-
-/*
- * Implements the RFC 4340, Appendix A
- */
-int dccp_ackvec_add(struct dccp_ackvec *av, const struct sock *sk,
-		    const u64 ackno, const u8 state)
-{
-	u8 *cur_head = av->av_buf + av->av_buf_head,
-	   *buf_end  = av->av_buf + DCCPAV_MAX_ACKVEC_LEN;
-	/*
-	 * Check at the right places if the buffer is full, if it is, tell the
-	 * caller to start dropping packets till the HC-Sender acks our ACK
-	 * vectors, when we will free up space in av_buf.
-	 *
-	 * We may well decide to do buffer compression, etc, but for now lets
-	 * just drop.
-	 *
-	 * From Appendix A.1.1 (`New Packets'):
-	 *
-	 *	Of course, the circular buffer may overflow, either when the
-	 *	HC-Sender is sending data at a very high rate, when the
-	 *	HC-Receiver's acknowledgements are not reaching the HC-Sender,
-	 *	or when the HC-Sender is forgetting to acknowledge those acks
-	 *	(so the HC-Receiver is unable to clean up old state). In this
-	 *	case, the HC-Receiver should either compress the buffer (by
-	 *	increasing run lengths when possible), transfer its state to
-	 *	a larger buffer, or, as a last resort, drop all received
-	 *	packets, without processing them whatsoever, until its buffer
-	 *	shrinks again.
-	 */
-
-	/* See if this is the first ackno being inserted */
-	if (av->av_vec_len == 0) {
-		*cur_head = state;
-		av->av_vec_len = 1;
-	} else if (after48(ackno, av->av_buf_ackno)) {
-		const u64 delta = dccp_delta_seqno(av->av_buf_ackno, ackno);
-
-		/*
-		 * Look if the state of this packet is the same as the
-		 * previous ackno and if so if we can bump the head len.
-		 */
-		if (delta == 1 && dccp_ackvec_state(cur_head) == state &&
-		    dccp_ackvec_runlen(cur_head) < DCCPAV_MAX_RUNLEN)
-			*cur_head += 1;
-		else if (dccp_ackvec_set_buf_head_state(av, delta, state))
-			return -ENOBUFS;
-	} else {
-		/*
-		 * A.1.2.  Old Packets
-		 *
-		 *	When a packet with Sequence Number S <= buf_ackno
-		 *	arrives, the HC-Receiver will scan the table for
-		 *	the byte corresponding to S. (Indexing structures
-		 *	could reduce the complexity of this scan.)
-		 */
-		u64 delta = dccp_delta_seqno(ackno, av->av_buf_ackno);
-
-		while (1) {
-			const u8 len = dccp_ackvec_runlen(cur_head);
-			/*
-			 * valid packets not yet in av_buf have a reserved
-			 * entry, with a len equal to 0.
-			 */
-			if (*cur_head == DCCPAV_NOT_RECEIVED && delta == 0) {
-				dccp_pr_debug("Found %llu reserved seat!\n",
-					      (unsigned long long)ackno);
-				*cur_head = state;
-				goto out;
-			}
-			/* len == 0 means one packet */
-			if (delta < len + 1)
-				goto out_duplicate;
-
-			delta -= len + 1;
-			if (++cur_head == buf_end)
-				cur_head = av->av_buf;
-		}
-	}
-
-	av->av_buf_ackno = ackno;
-out:
-	return 0;
-
-out_duplicate:
-	/* Duplicate packet */
-	dccp_pr_debug("Received a dup or already considered lost "
-		      "packet: %llu\n", (unsigned long long)ackno);
-	return -EILSEQ;
-}
-
-static void dccp_ackvec_throw_record(struct dccp_ackvec *av,
-				     struct dccp_ackvec_record *avr)
-{
-	struct dccp_ackvec_record *next;
-
-	/* sort out vector length */
-	if (av->av_buf_head <= avr->avr_ack_ptr)
-		av->av_vec_len = avr->avr_ack_ptr - av->av_buf_head;
-	else
-		av->av_vec_len = DCCPAV_MAX_ACKVEC_LEN - 1 -
-				 av->av_buf_head + avr->avr_ack_ptr;
-
-	/* free records */
-	list_for_each_entry_safe_from(avr, next, &av->av_records, avr_node) {
-		list_del(&avr->avr_node);
-		kmem_cache_free(dccp_ackvec_record_slab, avr);
-	}
-}
-
-void dccp_ackvec_check_rcv_ackno(struct dccp_ackvec *av, struct sock *sk,
-				 const u64 ackno)
-{
-	struct dccp_ackvec_record *avr;
-
-	/*
-	 * If we traverse backwards, it should be faster when we have large
-	 * windows. We will be receiving ACKs for stuff we sent a while back
-	 * -sorbo.
-	 */
-	list_for_each_entry_reverse(avr, &av->av_records, avr_node) {
-		if (ackno == avr->avr_ack_seqno) {
-			dccp_pr_debug("%s ACK packet 0, len=%d, ack_seqno=%llu, "
-				      "ack_ackno=%llu, ACKED!\n",
-				      dccp_role(sk), avr->avr_ack_runlen,
-				      (unsigned long long)avr->avr_ack_seqno,
-				      (unsigned long long)avr->avr_ack_ackno);
-			dccp_ackvec_throw_record(av, avr);
-			break;
-		} else if (avr->avr_ack_seqno > ackno)
-			break; /* old news */
-	}
-}
-
-static void dccp_ackvec_check_rcv_ackvector(struct dccp_ackvec *av,
-					    struct sock *sk, u64 *ackno,
-					    const unsigned char len,
-					    const unsigned char *vector)
-{
-	unsigned char i;
-	struct dccp_ackvec_record *avr;
-
-	/* Check if we actually sent an ACK vector */
-	if (list_empty(&av->av_records))
-		return;
-
-	i = len;
-	/*
-	 * XXX
-	 * I think it might be more efficient to work backwards. See comment on
-	 * rcv_ackno. -sorbo.
-	 */
-	avr = list_entry(av->av_records.next, struct dccp_ackvec_record, avr_node);
-	while (i--) {
-		const u8 rl = dccp_ackvec_runlen(vector);
-		u64 ackno_end_rl;
-
-		dccp_set_seqno(&ackno_end_rl, *ackno - rl);
-
-		/*
-		 * If our AVR sequence number is greater than the ack, go
-		 * forward in the AVR list until it is not so.
-		 */
-		list_for_each_entry_from(avr, &av->av_records, avr_node) {
-			if (!after48(avr->avr_ack_seqno, *ackno))
-				goto found;
-		}
-		/* End of the av_records list, not found, exit */
-		break;
-found:
-		if (between48(avr->avr_ack_seqno, ackno_end_rl, *ackno)) {
-			if (dccp_ackvec_state(vector) != DCCPAV_NOT_RECEIVED) {
-				dccp_pr_debug("%s ACK vector 0, len=%d, "
-					      "ack_seqno=%llu, ack_ackno=%llu, "
-					      "ACKED!\n",
-					      dccp_role(sk), len,
-					      (unsigned long long)
-					      avr->avr_ack_seqno,
-					      (unsigned long long)
-					      avr->avr_ack_ackno);
-				dccp_ackvec_throw_record(av, avr);
-				break;
-			}
-			/*
-			 * If it wasn't received, continue scanning... we might
-			 * find another one.
-			 */
-		}
-
-		dccp_set_seqno(ackno, ackno_end_rl - 1);
-		++vector;
-	}
-}
-
-int dccp_ackvec_parse(struct sock *sk, const struct sk_buff *skb,
-		      u64 *ackno, const u8 opt, const u8 *value, const u8 len)
-{
-	if (len > DCCP_SINGLE_OPT_MAXLEN)
-		return -1;
-
-	/* dccp_ackvector_print(DCCP_SKB_CB(skb)->dccpd_ack_seq, value, len); */
-	dccp_ackvec_check_rcv_ackvector(dccp_sk(sk)->dccps_hc_rx_ackvec, sk,
-					ackno, len, value);
-	return 0;
-}
-
 /**
  * dccp_ackvec_clear_state  -  Perform house-keeping / garbage-collection
  * This routine is called when the peer acknowledges the receipt of Ack Vectors
--- a/net/dccp/Kconfig
+++ b/net/dccp/Kconfig
@@ -25,9 +25,6 @@ config INET_DCCP_DIAG
 	def_tristate y if (IP_DCCP = y && INET_DIAG = y)
 	def_tristate m
 
-config IP_DCCP_ACKVEC
-	bool
-
 source "net/dccp/ccids/Kconfig"
 
 menu "DCCP Kernel Hacking"
--- a/net/dccp/ccids/Kconfig
+++ b/net/dccp/ccids/Kconfig
@@ -4,7 +4,6 @@ menu "DCCP CCIDs Configuration (EXPERIMENTAL)"
 config IP_DCCP_CCID2
 	tristate "CCID2 (TCP-Like) (EXPERIMENTAL)"
 	def_tristate IP_DCCP
-	select IP_DCCP_ACKVEC
 	---help---
 	  CCID 2, TCP-like Congestion Control, denotes Additive Increase,
 	  Multiplicative Decrease (AIMD) congestion control with behavior
--- a/net/dccp/Makefile
+++ b/net/dccp/Makefile
@@ -1,6 +1,7 @@
 obj-$(CONFIG_IP_DCCP) += dccp.o dccp_ipv4.o
 
-dccp-y := ccid.o feat.o input.o minisocks.o options.o output.o proto.o timer.o
+dccp-y := ccid.o feat.o input.o minisocks.o options.o \
+	  output.o proto.o timer.o ackvec.o
 
 dccp_ipv4-y := ipv4.o
 
@@ -8,8 +9,6 @@ dccp_ipv4-y := ipv4.o
 obj-$(subst y,$(CONFIG_IP_DCCP),$(CONFIG_IPV6)) += dccp_ipv6.o
 dccp_ipv6-y := ipv6.o
 
-dccp-$(CONFIG_IP_DCCP_ACKVEC) += ackvec.o
-
 obj-$(CONFIG_INET_DCCP_DIAG) += dccp_diag.o
 obj-$(CONFIG_NET_DCCPPROBE) += dccp_probe.o
 
--- a/net/dccp/ackvec.h
+++ b/net/dccp/ackvec.h
@@ -62,7 +62,6 @@ static inline u8 dccp_ackvec_state(const u8 *cell)
  *		   %DCCP_SINGLE_OPT_MAXLEN cells in the live portion of @av_buf
  * @av_overflow:   if 1 then buf_head == buf_tail indicates buffer wraparound
  * @av_records:	   list of %dccp_ackvec_record (Ack Vectors sent previously)
- * @av_veclen:	   length of the live portion of @av_buf
  */
 struct dccp_ackvec {
 	u8			av_buf[DCCPAV_MAX_ACKVEC_LEN];
@@ -73,7 +72,6 @@ struct dccp_ackvec {
 	bool			av_buf_nonce[DCCPAV_NUM_ACKVECS];
 	u8			av_overflow:1;
 	struct list_head	av_records;
-	u16			av_vec_len;
 };
 
 /** struct dccp_ackvec_record - Records information about sent Ack Vectors
@@ -99,25 +97,12 @@ struct dccp_ackvec_record {
 	u8		 avr_ack_nonce:1;
 };
 
-struct sock;
-struct sk_buff;
-
-#ifdef CONFIG_IP_DCCP_ACKVEC
-extern int dccp_ackvec_init(void);
+extern int  dccp_ackvec_init(void);
 extern void dccp_ackvec_exit(void);
 
 extern struct dccp_ackvec *dccp_ackvec_alloc(const gfp_t priority);
 extern void dccp_ackvec_free(struct dccp_ackvec *av);
 
-extern int dccp_ackvec_add(struct dccp_ackvec *av, const struct sock *sk,
-			   const u64 ackno, const u8 state);
-
-extern void dccp_ackvec_check_rcv_ackno(struct dccp_ackvec *av,
-					struct sock *sk, const u64 ackno);
-extern int dccp_ackvec_parse(struct sock *sk, const struct sk_buff *skb,
-			     u64 *ackno, const u8 opt,
-			     const u8 *value, const u8 len);
-
 extern void dccp_ackvec_input(struct dccp_ackvec *av, u64 seqno, u8 state);
 extern int  dccp_ackvec_update_records(struct dccp_ackvec *av, u64 seq, u8 sum);
 extern void dccp_ackvec_clear_state(struct dccp_ackvec *av, const u64 ackno);
@@ -127,66 +112,4 @@ static inline bool dccp_ackvec_is_empty(const struct dccp_ackvec *av)
 {
 	return av->av_overflow == 0 && av->av_buf_head == av->av_buf_tail;
 }
-#else /* CONFIG_IP_DCCP_ACKVEC */
-static inline int dccp_ackvec_init(void)
-{
-	return 0;
-}
-
-static inline void dccp_ackvec_exit(void)
-{
-}
-
-static inline struct dccp_ackvec *dccp_ackvec_alloc(const gfp_t priority)
-{
-	return NULL;
-}
-
-static inline void dccp_ackvec_free(struct dccp_ackvec *av)
-{
-}
-
-static inline void dccp_ackvec_input(struct dccp_ackvec *av, u64 seq, u8 state)
-{
-
-}
-
-static inline int dccp_ackvec_add(struct dccp_ackvec *av, const struct sock *sk,
-				  const u64 ackno, const u8 state)
-{
-	return -1;
-}
-
-static inline void dccp_ackvec_clear_state(struct dccp_ackvec *av,
-					    const u64 ackno)
-{
-}
-
-static inline void dccp_ackvec_check_rcv_ackno(struct dccp_ackvec *av,
-					       struct sock *sk, const u64 ackno)
-{
-}
-
-static inline int dccp_ackvec_parse(struct sock *sk, const struct sk_buff *skb,
-				    const u64 *ackno, const u8 opt,
-				    const u8 *value, const u8 len)
-{
-	return -1;
-}
-
-static inline int dccp_ackvec_update_records(struct dccp_ackvec *av, u64 seq, u8 nonce)
-{
-	return -1;
-}
-
-static inline u16 dccp_ackvec_buflen(const struct dccp_ackvec *av)
-{
-	return 0;
-}
-
-static inline bool dccp_ackvec_is_empty(const struct dccp_ackvec *av)
-{
-	return true;
-}
-#endif /* CONFIG_IP_DCCP_ACKVEC */
 #endif /* _ACKVEC_H */

^ permalink raw reply

* [PATCH 02/10] [ACKVEC]: Ack Vector interface clean-up
From: Gerrit Renker @ 2008-02-21  9:09 UTC (permalink / raw)
  To: acme; +Cc: dccp, netdev, Gerrit Renker
In-Reply-To: <1203584968-8957-2-git-send-email-gerrit@erg.abdn.ac.uk>

This patch brings the Ack Vector interface up to date. Its main purpose is
to lay the basis for the subsequent patches of this set, which will use the
new data structure fields and routines.

There are no real algorithmic changes, rather an adaptation:

 (1) Replaced the static Ack Vector size (2) with a #define so that it can
     be adapted (with low loss / Ack Ratio, a value of 1 works, so 2 seems
     to be sufficient for the moment) and added a solution so that computing
     the ECN nonce will continue to work - even with larger Ack Vectors.

 (2) Replaced the #defines for Ack Vector states with a complete enum.

 (3) Replaced #defines to compute Ack Vector length and state with general
     purpose routines (inlines), and updated code to use these.

 (4) Added a `tail' field (conversion to circular buffer in subsequent patch).

 (5) Updated the (outdated) documentation for Ack Vector struct.

 (6) All sequence number containers now trimmed to 48 bits.

 (7) Removal of unused bits:
     * removed dccpav_ack_nonce from struct dccp_ackvec, since this is already
       redundantly stored in the `dccpavr_ack_nonce' (of Ack Vector record);
     * removed unused function to print Ack Vectors;
     * removed Elapsed Time for Ack Vectors (it was nowhere used);
     * replaced semantics of dccpavr_sent_len with dccpavr_ack_runlen, since
       the code needs to be able to remember the old run length;
     * reduced the de-/allocation routines (redundant / duplicate tests).


Justification for removing Elapsed Time information [can be removed]:
---------------------------------------------------------------------
 1. The Elapsed Time information for Ack Vectors was nowhere used in the code.
 2. DCCP does not implement rate-based pacing of acknowledgments. The only
    recommendation for always including Elapsed Time is in section 11.3 of
    RFC 4340: "Receivers that rate-pace acknowledgements SHOULD [...]
    include Elapsed Time options". But such is not the case here.
 3. It does not really improve estimation accuracy. The Elapsed Time field only
    records the time between the arrival of the last acknowledgeable packet and
    the time the Ack Vector is sent out. Since Linux does not (yet) implement
    delayed Acks, the time difference will typically be small, since often the
    arrival of a data packet triggers sending feedback at the HC-receiver.


Justification for changes in de-/allocation routines [can be removed]:
----------------------------------------------------------------------
  * INIT_LIST_HEAD in dccp_ackvec_record_new was redundant, since the list
    pointers were later overwritten when the node was added via list_add();
  * dccp_ackvec_record_new() was called in a single place only;
  * calls to list_del_init() before calling dccp_ackvec_record_delete() were
    redundant, since subsequently the entire element was k-freed;
  * since all calls to dccp_ackvec_record_delete() were preceded to a call to
    list_del_init(), the WARN_ON test would never evaluate to true;
  * since all calls to dccp_ackvec_record_delete() were made from within
    list_for_each_entry_safe(), the test for avr == NULL was redundant;
  * list_empty() in ackvec_free was redundant, since the same condition is
    embedded in the loop condition of the subsequent list_for_each_entry_safe().


Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/ackvec.c      |  205 +++++++++++++++--------------------------------
 net/dccp/ackvec.h      |  103 +++++++++++++-----------
 net/dccp/ccids/ccid2.c |   13 +--
 net/dccp/input.c       |    6 +-
 4 files changed, 129 insertions(+), 198 deletions(-)

--- a/net/dccp/ackvec.h
+++ b/net/dccp/ackvec.h
@@ -3,83 +3,92 @@
 /*
  *  net/dccp/ackvec.h
  *
- *  An implementation of the DCCP protocol
+ *  An implementation of Ack Vectors for the DCCP protocol
+ *  Copyright (c) 2007 University of Aberdeen, Scotland, UK
  *  Copyright (c) 2005 Arnaldo Carvalho de Melo <acme@mandriva.com>
- *
  *	This program is free software; you can redistribute it and/or modify it
  *	under the terms of the GNU General Public License version 2 as
  *	published by the Free Software Foundation.
  */
 
 #include <linux/compiler.h>
-#include <linux/ktime.h>
 #include <linux/list.h>
 #include <linux/types.h>
 
 /* maximum size of a single TLV-encoded option (sans type/len bytes) */
 #define DCCP_SINGLE_OPT_MAXLEN  253
-/* We can spread an ack vector across multiple options */
-#define DCCP_MAX_ACKVEC_LEN (DCCP_SINGLE_OPT_MAXLEN * 2)
+/*
+ * Ack Vector buffer space is static, in multiples of %DCCP_SINGLE_OPT_MAXLEN,
+ * the maximum size of a single Ack Vector. Setting %DCCPAV_NUM_ACKVECS to 1
+ * will be sufficient for most cases of low Ack Ratios, using a value of 2 gives
+ * more headroom if Ack Ratio is higher or when the sender acknowledges slowly.
+ */
+#define DCCPAV_NUM_ACKVECS	2
+#define DCCPAV_MAX_ACKVEC_LEN	(DCCP_SINGLE_OPT_MAXLEN * DCCPAV_NUM_ACKVECS)
+
+enum dccp_ackvec_states {
+	DCCPAV_RECEIVED =	0x00,
+	DCCPAV_ECN_MARKED =	0x40,
+	DCCPAV_RESERVED =	0x80,
+	DCCPAV_NOT_RECEIVED =	0xC0
+};
+#define DCCPAV_MAX_RUNLEN	0x3F
 
-#define DCCP_ACKVEC_STATE_RECEIVED	0
-#define DCCP_ACKVEC_STATE_ECN_MARKED	(1 << 6)
-#define DCCP_ACKVEC_STATE_NOT_RECEIVED	(3 << 6)
+static inline u8 dccp_ackvec_runlen(const u8 *cell)
+{
+	return *cell & DCCPAV_MAX_RUNLEN;
+}
 
-#define DCCP_ACKVEC_STATE_MASK		0xC0 /* 11000000 */
-#define DCCP_ACKVEC_LEN_MASK		0x3F /* 00111111 */
+static inline u8 dccp_ackvec_state(const u8 *cell)
+{
+	return *cell & ~DCCPAV_MAX_RUNLEN;
+}
 
-/** struct dccp_ackvec - ack vector
- *
- * This data structure is the one defined in RFC 4340, Appendix A.
- *
- * @av_buf_head - circular buffer head
- * @av_buf_tail - circular buffer tail
- * @av_buf_ackno - ack # of the most recent packet acknowledgeable in the
- *		       buffer (i.e. %av_buf_head)
- * @av_buf_nonce - the one-bit sum of the ECN Nonces on all packets acked
- * 		       by the buffer with State 0
+/** struct dccp_ackvec - Ack Vector main data structure
  *
- * Additionally, the HC-Receiver must keep some information about the
- * Ack Vectors it has recently sent. For each packet sent carrying an
- * Ack Vector, it remembers four variables:
+ * This implements a fixed-size circular buffer within an array and is largely
+ * based on Appendix A of RFC 4340.
  *
- * @av_records - list of dccp_ackvec_record
- * @av_ack_nonce - the one-bit sum of the ECN Nonces for all State 0.
- *
- * @av_time - the time in usecs
- * @av_buf - circular buffer of acknowledgeable packets
+ * @av_buf:	   circular buffer storage area
+ * @av_buf_head:   head index; begin of live portion in @av_buf
+ * @av_buf_tail:   tail index; first index _after_ the live portion in @av_buf
+ * @av_buf_ackno:  highest seqno of acknowledgeable packet recorded in @av_buf
+ * @av_buf_nonce:  ECN nonce sums, each covering subsequent segments of up to
+ *		   %DCCP_SINGLE_OPT_MAXLEN cells in the live portion of @av_buf
+ * @av_records:	   list of %dccp_ackvec_record (Ack Vectors sent previously)
+ * @av_veclen:	   length of the live portion of @av_buf
  */
 struct dccp_ackvec {
-	u64			av_buf_ackno;
-	struct list_head	av_records;
-	ktime_t			av_time;
+	u8			av_buf[DCCPAV_MAX_ACKVEC_LEN];
 	u16			av_buf_head;
+	u16			av_buf_tail;
+	u64			av_buf_ackno:48;
+	bool			av_buf_nonce[DCCPAV_NUM_ACKVECS];
+	struct list_head	av_records;
 	u16			av_vec_len;
-	u8			av_buf_nonce;
-	u8			av_ack_nonce;
-	u8			av_buf[DCCP_MAX_ACKVEC_LEN];
 };
 
-/** struct dccp_ackvec_record - ack vector record
+/** struct dccp_ackvec_record - Records information about sent Ack Vectors
  *
- * ACK vector record as defined in Appendix A of spec.
+ * These list entries define the additional information which the HC-Receiver
+ * keeps about recently-sent Ack Vectors; again refer to RFC 4340, Appendix A.
  *
- * The list is sorted by avr_ack_seqno
+ * @avr_node:	    the list node in @av_records
+ * @avr_ack_seqno:  sequence number of the packet the Ack Vector was sent on
+ * @avr_ack_ackno:  the Ack number that this record/Ack Vector refers to
+ * @avr_ack_ptr:    pointer into @av_buf where this record starts
+ * @avr_ack_runlen: run length of @avr_ack_ptr at the time of sending
+ * @avr_ack_nonce:  the sum of @av_buf_nonce's at the time this record was sent
  *
- * @avr_node - node in av_records
- * @avr_ack_seqno - sequence number of the packet this record was sent on
- * @avr_ack_ackno - sequence number being acknowledged
- * @avr_ack_ptr - pointer into av_buf where this record starts
- * @avr_ack_nonce - av_ack_nonce at the time this record was sent
- * @avr_sent_len - lenght of the record in av_buf
+ * The list as a whole is sorted in descending order by @avr_ack_seqno.
  */
 struct dccp_ackvec_record {
 	struct list_head avr_node;
-	u64		 avr_ack_seqno;
-	u64		 avr_ack_ackno;
+	u64		 avr_ack_seqno:48;
+	u64		 avr_ack_ackno:48;
 	u16		 avr_ack_ptr;
-	u16		 avr_sent_len;
-	u8		 avr_ack_nonce;
+	u8		 avr_ack_runlen;
+	u8		 avr_ack_nonce:1;
 };
 
 struct sock;
--- a/net/dccp/ackvec.c
+++ b/net/dccp/ackvec.c
@@ -1,7 +1,8 @@
 /*
  *  net/dccp/ackvec.c
  *
- *  An implementation of the DCCP protocol
+ *  An implementation of Ack Vectors for the DCCP protocol
+ *  Copyright (c) 2007 University of Aberdeen, Scotland, UK
  *  Copyright (c) 2005 Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
  *
  *      This program is free software; you can redistribute it and/or modify it
@@ -24,24 +25,34 @@
 static struct kmem_cache *dccp_ackvec_slab;
 static struct kmem_cache *dccp_ackvec_record_slab;
 
-static struct dccp_ackvec_record *dccp_ackvec_record_new(void)
+struct dccp_ackvec *dccp_ackvec_alloc(const gfp_t priority)
 {
-	struct dccp_ackvec_record *avr =
-			kmem_cache_alloc(dccp_ackvec_record_slab, GFP_ATOMIC);
+	struct dccp_ackvec *av = kmem_cache_alloc(dccp_ackvec_slab, priority);
 
-	if (avr != NULL)
-		INIT_LIST_HEAD(&avr->avr_node);
+	if (av != NULL) {
+		av->av_buf_head	 = DCCPAV_MAX_ACKVEC_LEN - 1;
+		av->av_vec_len	 = 0;
+		memset(av->av_buf_nonce, 0, sizeof(av->av_buf_nonce));
+		INIT_LIST_HEAD(&av->av_records);
+	}
+	return av;
+}
 
-	return avr;
+static void dccp_ackvec_purge_records(struct dccp_ackvec *av)
+{
+	struct dccp_ackvec_record *cur, *next;
+
+	list_for_each_entry_safe(cur, next, &av->av_records, avr_node)
+		kmem_cache_free(dccp_ackvec_record_slab, cur);
+	INIT_LIST_HEAD(&av->av_records);
 }
 
-static void dccp_ackvec_record_delete(struct dccp_ackvec_record *avr)
+void dccp_ackvec_free(struct dccp_ackvec *av)
 {
-	if (unlikely(avr == NULL))
-		return;
-	/* Check if deleting a linked record */
-	WARN_ON(!list_empty(&avr->avr_node));
-	kmem_cache_free(dccp_ackvec_record_slab, avr);
+	if (likely(av != NULL)) {
+		dccp_ackvec_purge_records(av);
+		kmem_cache_free(dccp_ackvec_slab, av);
+	}
 }
 
 static void dccp_ackvec_insert_avr(struct dccp_ackvec *av,
@@ -69,24 +80,16 @@ int dccp_insert_option_ackvec(struct sock *sk, struct sk_buff *skb)
 	struct dccp_ackvec *av = dp->dccps_hc_rx_ackvec;
 	/* Figure out how many options do we need to represent the ackvec */
 	const u16 nr_opts = DIV_ROUND_UP(av->av_vec_len, DCCP_SINGLE_OPT_MAXLEN);
-	u16 len = av->av_vec_len + 2 * nr_opts, i;
-	u32 elapsed_time;
+	u16 len = av->av_vec_len + 2 * nr_opts;
+	u8 i, nonce = 0;
 	const unsigned char *tail, *from;
 	unsigned char *to;
 	struct dccp_ackvec_record *avr;
-	suseconds_t delta;
 
 	if (DCCP_SKB_CB(skb)->dccpd_opt_len + len > DCCP_MAX_OPT_LEN)
 		return -1;
 
-	delta = ktime_us_delta(ktime_get_real(), av->av_time);
-	elapsed_time = delta / 10;
-
-	if (elapsed_time != 0 &&
-	    dccp_insert_option_elapsed_time(sk, skb, elapsed_time))
-		return -1;
-
-	avr = dccp_ackvec_record_new();
+	avr = kmem_cache_alloc(dccp_ackvec_record_slab, GFP_ATOMIC);
 	if (avr == NULL)
 		return -1;
 
@@ -95,7 +98,7 @@ int dccp_insert_option_ackvec(struct sock *sk, struct sk_buff *skb)
 	to   = skb_push(skb, len);
 	len  = av->av_vec_len;
 	from = av->av_buf + av->av_buf_head;
-	tail = av->av_buf + DCCP_MAX_ACKVEC_LEN;
+	tail = av->av_buf + DCCPAV_MAX_ACKVEC_LEN;
 
 	for (i = 0; i < nr_opts; ++i) {
 		int copylen = len;
@@ -103,7 +106,13 @@ int dccp_insert_option_ackvec(struct sock *sk, struct sk_buff *skb)
 		if (len > DCCP_SINGLE_OPT_MAXLEN)
 			copylen = DCCP_SINGLE_OPT_MAXLEN;
 
-		*to++ = DCCPO_ACK_VECTOR_0;
+		/*
+		 * RFC 4340, 12.2: Encode the Nonce Echo for this Ack Vector via
+		 * its type; ack_nonce is the sum of all individual buf_nonce's.
+		 */
+		nonce ^= av->av_buf_nonce[i];
+
+		*to++ = DCCPO_ACK_VECTOR_0 + av->av_buf_nonce[i];
 		*to++ = copylen + 2;
 
 		/* Check if buf_head wraps */
@@ -124,75 +133,24 @@ int dccp_insert_option_ackvec(struct sock *sk, struct sk_buff *skb)
 	}
 
 	/*
-	 *	From RFC 4340, A.2:
-	 *
-	 *	For each acknowledgement it sends, the HC-Receiver will add an
-	 *	acknowledgement record.  ack_seqno will equal the HC-Receiver
-	 *	sequence number it used for the ack packet; ack_ptr will equal
-	 *	buf_head; ack_ackno will equal buf_ackno; and ack_nonce will
-	 *	equal buf_nonce.
+	 * Each sent Ack Vector is recorded in the list, as per A.2 of RFC 4340.
 	 */
-	avr->avr_ack_seqno = DCCP_SKB_CB(skb)->dccpd_seq;
-	avr->avr_ack_ptr   = av->av_buf_head;
-	avr->avr_ack_ackno = av->av_buf_ackno;
-	avr->avr_ack_nonce = av->av_buf_nonce;
-	avr->avr_sent_len  = av->av_vec_len;
+	avr->avr_ack_seqno  = DCCP_SKB_CB(skb)->dccpd_seq;
+	avr->avr_ack_ptr    = av->av_buf_head;
+	avr->avr_ack_ackno  = av->av_buf_ackno;
+	avr->avr_ack_nonce  = nonce;
+	avr->avr_ack_runlen = dccp_ackvec_runlen(av->av_buf + av->av_buf_head);
 
 	dccp_ackvec_insert_avr(av, avr);
 
 	dccp_pr_debug("%s ACK Vector 0, len=%d, ack_seqno=%llu, "
 		      "ack_ackno=%llu\n",
-		      dccp_role(sk), avr->avr_sent_len,
+		      dccp_role(sk), avr->avr_ack_runlen,
 		      (unsigned long long)avr->avr_ack_seqno,
 		      (unsigned long long)avr->avr_ack_ackno);
 	return 0;
 }
 
-struct dccp_ackvec *dccp_ackvec_alloc(const gfp_t priority)
-{
-	struct dccp_ackvec *av = kmem_cache_alloc(dccp_ackvec_slab, priority);
-
-	if (av != NULL) {
-		av->av_buf_head	 = DCCP_MAX_ACKVEC_LEN - 1;
-		av->av_buf_ackno = UINT48_MAX + 1;
-		av->av_buf_nonce = 0;
-		av->av_time	 = ktime_set(0, 0);
-		av->av_vec_len	 = 0;
-		INIT_LIST_HEAD(&av->av_records);
-	}
-
-	return av;
-}
-
-void dccp_ackvec_free(struct dccp_ackvec *av)
-{
-	if (unlikely(av == NULL))
-		return;
-
-	if (!list_empty(&av->av_records)) {
-		struct dccp_ackvec_record *avr, *next;
-
-		list_for_each_entry_safe(avr, next, &av->av_records, avr_node) {
-			list_del_init(&avr->avr_node);
-			dccp_ackvec_record_delete(avr);
-		}
-	}
-
-	kmem_cache_free(dccp_ackvec_slab, av);
-}
-
-static inline u8 dccp_ackvec_state(const struct dccp_ackvec *av,
-				   const u32 index)
-{
-	return av->av_buf[index] & DCCP_ACKVEC_STATE_MASK;
-}
-
-static inline u8 dccp_ackvec_len(const struct dccp_ackvec *av,
-				 const u32 index)
-{
-	return av->av_buf[index] & DCCP_ACKVEC_LEN_MASK;
-}
-
 /*
  * If several packets are missing, the HC-Receiver may prefer to enter multiple
  * bytes with run length 0, rather than a single byte with a larger run length;
@@ -205,7 +163,7 @@ static inline int dccp_ackvec_set_buf_head_state(struct dccp_ackvec *av,
 	unsigned int gap;
 	long new_head;
 
-	if (av->av_vec_len + packets > DCCP_MAX_ACKVEC_LEN)
+	if (av->av_vec_len + packets > DCCPAV_MAX_ACKVEC_LEN)
 		return -ENOBUFS;
 
 	gap	 = packets - 1;
@@ -213,18 +171,18 @@ static inline int dccp_ackvec_set_buf_head_state(struct dccp_ackvec *av,
 
 	if (new_head < 0) {
 		if (gap > 0) {
-			memset(av->av_buf, DCCP_ACKVEC_STATE_NOT_RECEIVED,
+			memset(av->av_buf, DCCPAV_NOT_RECEIVED,
 			       gap + new_head + 1);
 			gap = -new_head;
 		}
-		new_head += DCCP_MAX_ACKVEC_LEN;
+		new_head += DCCPAV_MAX_ACKVEC_LEN;
 	}
 
 	av->av_buf_head = new_head;
 
 	if (gap > 0)
 		memset(av->av_buf + av->av_buf_head + 1,
-		       DCCP_ACKVEC_STATE_NOT_RECEIVED, gap);
+		       DCCPAV_NOT_RECEIVED, gap);
 
 	av->av_buf[av->av_buf_head] = state;
 	av->av_vec_len += packets;
@@ -237,6 +195,8 @@ static inline int dccp_ackvec_set_buf_head_state(struct dccp_ackvec *av,
 int dccp_ackvec_add(struct dccp_ackvec *av, const struct sock *sk,
 		    const u64 ackno, const u8 state)
 {
+	u8 *cur_head = av->av_buf + av->av_buf_head,
+	   *buf_end  = av->av_buf + DCCPAV_MAX_ACKVEC_LEN;
 	/*
 	 * Check at the right places if the buffer is full, if it is, tell the
 	 * caller to start dropping packets till the HC-Sender acks our ACK
@@ -261,7 +221,7 @@ int dccp_ackvec_add(struct dccp_ackvec *av, const struct sock *sk,
 
 	/* See if this is the first ackno being inserted */
 	if (av->av_vec_len == 0) {
-		av->av_buf[av->av_buf_head] = state;
+		*cur_head = state;
 		av->av_vec_len = 1;
 	} else if (after48(ackno, av->av_buf_ackno)) {
 		const u64 delta = dccp_delta_seqno(av->av_buf_ackno, ackno);
@@ -270,10 +230,9 @@ int dccp_ackvec_add(struct dccp_ackvec *av, const struct sock *sk,
 		 * Look if the state of this packet is the same as the
 		 * previous ackno and if so if we can bump the head len.
 		 */
-		if (delta == 1 &&
-		    dccp_ackvec_state(av, av->av_buf_head) == state &&
-		    dccp_ackvec_len(av, av->av_buf_head) < DCCP_ACKVEC_LEN_MASK)
-			av->av_buf[av->av_buf_head]++;
+		if (delta == 1 && dccp_ackvec_state(cur_head) == state &&
+		    dccp_ackvec_runlen(cur_head) < DCCPAV_MAX_RUNLEN)
+			*cur_head += 1;
 		else if (dccp_ackvec_set_buf_head_state(av, delta, state))
 			return -ENOBUFS;
 	} else {
@@ -286,21 +245,17 @@ int dccp_ackvec_add(struct dccp_ackvec *av, const struct sock *sk,
 		 *	could reduce the complexity of this scan.)
 		 */
 		u64 delta = dccp_delta_seqno(ackno, av->av_buf_ackno);
-		u32 index = av->av_buf_head;
 
 		while (1) {
-			const u8 len = dccp_ackvec_len(av, index);
-			const u8 state = dccp_ackvec_state(av, index);
+			const u8 len = dccp_ackvec_runlen(cur_head);
 			/*
 			 * valid packets not yet in av_buf have a reserved
 			 * entry, with a len equal to 0.
 			 */
-			if (state == DCCP_ACKVEC_STATE_NOT_RECEIVED &&
-			    len == 0 && delta == 0) { /* Found our
-							 reserved seat! */
+			if (*cur_head == DCCPAV_NOT_RECEIVED && delta == 0) {
 				dccp_pr_debug("Found %llu reserved seat!\n",
 					      (unsigned long long)ackno);
-				av->av_buf[index] = state;
+				*cur_head = state;
 				goto out;
 			}
 			/* len == 0 means one packet */
@@ -308,13 +263,12 @@ int dccp_ackvec_add(struct dccp_ackvec *av, const struct sock *sk,
 				goto out_duplicate;
 
 			delta -= len + 1;
-			if (++index == DCCP_MAX_ACKVEC_LEN)
-				index = 0;
+			if (++cur_head == buf_end)
+				cur_head = av->av_buf;
 		}
 	}
 
 	av->av_buf_ackno = ackno;
-	av->av_time = ktime_get_real();
 out:
 	return 0;
 
@@ -325,31 +279,6 @@ out_duplicate:
 	return -EILSEQ;
 }
 
-#ifdef CONFIG_IP_DCCP_DEBUG
-void dccp_ackvector_print(const u64 ackno, const unsigned char *vector, int len)
-{
-	dccp_pr_debug_cat("ACK vector len=%d, ackno=%llu |", len,
-			 (unsigned long long)ackno);
-
-	while (len--) {
-		const u8 state = (*vector & DCCP_ACKVEC_STATE_MASK) >> 6;
-		const u8 rl = *vector & DCCP_ACKVEC_LEN_MASK;
-
-		dccp_pr_debug_cat("%d,%d|", state, rl);
-		++vector;
-	}
-
-	dccp_pr_debug_cat("\n");
-}
-
-void dccp_ackvec_print(const struct dccp_ackvec *av)
-{
-	dccp_ackvector_print(av->av_buf_ackno,
-			     av->av_buf + av->av_buf_head,
-			     av->av_vec_len);
-}
-#endif
-
 static void dccp_ackvec_throw_record(struct dccp_ackvec *av,
 				     struct dccp_ackvec_record *avr)
 {
@@ -359,13 +288,13 @@ static void dccp_ackvec_throw_record(struct dccp_ackvec *av,
 	if (av->av_buf_head <= avr->avr_ack_ptr)
 		av->av_vec_len = avr->avr_ack_ptr - av->av_buf_head;
 	else
-		av->av_vec_len = DCCP_MAX_ACKVEC_LEN - 1 -
+		av->av_vec_len = DCCPAV_MAX_ACKVEC_LEN - 1 -
 				 av->av_buf_head + avr->avr_ack_ptr;
 
 	/* free records */
 	list_for_each_entry_safe_from(avr, next, &av->av_records, avr_node) {
-		list_del_init(&avr->avr_node);
-		dccp_ackvec_record_delete(avr);
+		list_del(&avr->avr_node);
+		kmem_cache_free(dccp_ackvec_record_slab, avr);
 	}
 }
 
@@ -383,7 +312,7 @@ void dccp_ackvec_check_rcv_ackno(struct dccp_ackvec *av, struct sock *sk,
 		if (ackno == avr->avr_ack_seqno) {
 			dccp_pr_debug("%s ACK packet 0, len=%d, ack_seqno=%llu, "
 				      "ack_ackno=%llu, ACKED!\n",
-				      dccp_role(sk), 1,
+				      dccp_role(sk), avr->avr_ack_runlen,
 				      (unsigned long long)avr->avr_ack_seqno,
 				      (unsigned long long)avr->avr_ack_ackno);
 			dccp_ackvec_throw_record(av, avr);
@@ -413,7 +342,7 @@ static void dccp_ackvec_check_rcv_ackvector(struct dccp_ackvec *av,
 	 */
 	avr = list_entry(av->av_records.next, struct dccp_ackvec_record, avr_node);
 	while (i--) {
-		const u8 rl = *vector & DCCP_ACKVEC_LEN_MASK;
+		const u8 rl = dccp_ackvec_runlen(vector);
 		u64 ackno_end_rl;
 
 		dccp_set_seqno(&ackno_end_rl, *ackno - rl);
@@ -430,8 +359,7 @@ static void dccp_ackvec_check_rcv_ackvector(struct dccp_ackvec *av,
 		break;
 found:
 		if (between48(avr->avr_ack_seqno, ackno_end_rl, *ackno)) {
-			const u8 state = *vector & DCCP_ACKVEC_STATE_MASK;
-			if (state != DCCP_ACKVEC_STATE_NOT_RECEIVED) {
+			if (dccp_ackvec_state(vector) != DCCPAV_NOT_RECEIVED) {
 				dccp_pr_debug("%s ACK vector 0, len=%d, "
 					      "ack_seqno=%llu, ack_ackno=%llu, "
 					      "ACKED!\n",
@@ -474,10 +402,9 @@ int __init dccp_ackvec_init(void)
 	if (dccp_ackvec_slab == NULL)
 		goto out_err;
 
-	dccp_ackvec_record_slab =
-			kmem_cache_create("dccp_ackvec_record",
-					  sizeof(struct dccp_ackvec_record),
-					  0, SLAB_HWCACHE_ALIGN, NULL);
+	dccp_ackvec_record_slab = kmem_cache_create("dccp_ackvec_record",
+					     sizeof(struct dccp_ackvec_record),
+					     0, SLAB_HWCACHE_ALIGN, NULL);
 	if (dccp_ackvec_record_slab == NULL)
 		goto out_destroy_slab;
 
--- a/net/dccp/ccids/ccid2.c
+++ b/net/dccp/ccids/ccid2.c
@@ -583,8 +583,7 @@ static void ccid2_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)
 					 &vector, &veclen)) != -1) {
 		/* go through this ack vector */
 		while (veclen--) {
-			const u8 rl = *vector & DCCP_ACKVEC_LEN_MASK;
-			u64 ackno_end_rl = SUB48(ackno, rl);
+			u64 ackno_end_rl = SUB48(ackno, dccp_ackvec_runlen(vector));
 
 			ccid2_pr_debug("ackvec start:%llu end:%llu\n",
 				       (unsigned long long)ackno,
@@ -607,17 +606,15 @@ static void ccid2_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)
 			 * run length
 			 */
 			while (between48(seqp->ccid2s_seq,ackno_end_rl,ackno)) {
-				const u8 state = *vector &
-						 DCCP_ACKVEC_STATE_MASK;
+				const u8 state = dccp_ackvec_state(vector);
 
 				/* new packet received or marked */
-				if (state != DCCP_ACKVEC_STATE_NOT_RECEIVED &&
+				if (state != DCCPAV_NOT_RECEIVED &&
 				    !seqp->ccid2s_acked) {
-					if (state ==
-					    DCCP_ACKVEC_STATE_ECN_MARKED) {
+					if (state == DCCPAV_ECN_MARKED)
 						ccid2_congestion_event(sk,
 								       seqp);
-					} else
+					else
 						ccid2_new_ack(sk, seqp,
 							      &maxincr);
 
--- a/net/dccp/input.c
+++ b/net/dccp/input.c
@@ -377,8 +377,7 @@ int dccp_rcv_established(struct sock *sk, struct sk_buff *skb,
 
 	if (dp->dccps_hc_rx_ackvec != NULL &&
 	    dccp_ackvec_add(dp->dccps_hc_rx_ackvec, sk,
-			    DCCP_SKB_CB(skb)->dccpd_seq,
-			    DCCP_ACKVEC_STATE_RECEIVED))
+			    DCCP_SKB_CB(skb)->dccpd_seq, DCCPAV_RECEIVED))
 		goto discard;
 	dccp_deliver_input_to_ccids(sk, skb);
 
@@ -627,8 +626,7 @@ int dccp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
 
 		if (dp->dccps_hc_rx_ackvec != NULL &&
 		    dccp_ackvec_add(dp->dccps_hc_rx_ackvec, sk,
-				    DCCP_SKB_CB(skb)->dccpd_seq,
-				    DCCP_ACKVEC_STATE_RECEIVED))
+				    DCCP_SKB_CB(skb)->dccpd_seq, DCCPAV_RECEIVED))
 			goto discard;
 
 		dccp_deliver_input_to_ccids(sk, skb);

^ permalink raw reply

* New sparse warning in net/mac80211/debugfs_sta.c
From: Harvey Harrison @ 2008-02-21  9:34 UTC (permalink / raw)
  To: David Miller, Joe Perches; +Cc: linux-netdev

commit 0795af5729b18218767fab27c44b1384f72dc9ad
Author: Joe Perches <joe@perches.com>
Date:   Wed Oct 3 17:59:30 2007 -0700

    [NET]: Introduce and use print_mac() and DECLARE_MAC_BUF()
    
    This is nicer than the MAC_FMT stuff.
    
    Signed-off-by: Joe Perches <joe@perches.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Introduced:
net/mac80211/debugfs_sta.c: In function ‘ieee80211_sta_debugfs_add’:
net/mac80211/debugfs_sta.c:211: warning: statement with no effect

Does print_mac modify the mac buffer in-place, or return a new buffer?

Harvey


^ permalink raw reply

* Re: [PATCH][PPPOL2TP]: Fix SMP oops in pppol2tp driver
From: James Chapman @ 2008-02-21  9:53 UTC (permalink / raw)
  To: Jarek Poplawski; +Cc: David Miller, Paul Mackerras, netdev
In-Reply-To: <20080221085959.GA12944@ff.dom.local>

Jarek Poplawski wrote:
> On Wed, Feb 20, 2008 at 10:37:57PM +0000, James Chapman wrote:
>> Jarek Poplawski wrote:
>>
>>>>> (testing patch #1)
>>> But I hope you tested with the fixed (take 2) version of this patch...
>> Yes I did. :)
>>
>> But I just got another lockdep error (attached).
>>
>>> Since it's quite experimental (testing) this patch could be wrong
>>> as it is, but I hope it should show the proper way to solve this
>>> problem. Probably you did some of these, but here are a few of my
>>> suggestions for testing this:
>>>
>>> 1) try my patch with your full bh locking changing patch;
>>> 2) add while loops to these trylocks on failure, with e.g.  __delay(1);
>>>    this should work like full locks again, but there should be no (this
>>>    kind of) lockdep reports;
>> Hmm, isn't this just bypassing the lockdep checks?
> 
> Yes! But it's only for debugging: to find if this change in locking
> is to be blamed for these new lockups. It should effectively work just
> like without this patch, but without this lockdep warning. So, if
> after such change lockups still happen, then it would seem you didn't
> test this enough before. Otherwise the new patch is to blame and needs
> reworking.

The lockups still happen, but I think they are now due to a different 
problem, as you say.

Some background on this issue might be useful to help get feedback from 
others on the list. This issue was first reported by an ISP who found 
random lockups if an L2TP tunnel carrying hundreds/thousands of L2TP 
sessions went down due to a network outage and then recovered itself. On 
recovery, all of the tunnel's sessions (PPP) are created rapidly. 
Sometimes the tunnel would recover just fine, but other times not. The 
ISP put some effort into reproducing the problem and found that 
repeatedly creating/deleting a tunnel with lots of L2TP sessions would 
cause the failure after a random time between a few minutes and several 
hours. The original lockdep trace came from the ISP. I initially 
couldn't reproduce the problem but I borrowed two equivalent quad-core 
systems and can now reproduce it. Subsequent lockdep traces have been 
from my testing.

The _bh locking fixes in pppol2tp combined with your ppp_generic change 
solved that problem. So I then added data traffic into the mix (since 
this will happen in a real network) and found that lockups still happen. 
But the lockdep trace in this case is different, as you noted.

Does PPPoE stress the PPP setup code as much as this scenario? I guess 
in theory it could if lots of PPPoE clients connected at the same time, 
but there is no aggregate tunnel like there is with L2TP to cause all 
sessions to connect simultaneously. Perhaps PPTP also suffers from these 
issues? Perhaps not because it tends to be used only in VPN setups where 
there is only 1 session per tunnel.

>>> 3) I send here another testing patch with this second way to do this:
>>>    on the write side, but it's even more "experimental" and only a
>>>    proof of concept (should be applied on vanilla ppp_generic).
>> I'll look over it. I think I need to take a step back and look at what's  
>> happening in more detail though.
> 
> This is something completely new and changes all the picture: the xmit
> path wasn't expected (at least by me) to be called in softirq context
> at all, and there were no traces of this on previous reports. But,
> since lockdep always stops after the first warning, there could be
> even more surprises like this in the future. I'll check this report.

Doesn't the TX softirq do transmits if they've been queued up?

-- 
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development


^ permalink raw reply

* Re: New sparse warning in net/mac80211/debugfs_sta.c
From: Johannes Berg @ 2008-02-21  9:54 UTC (permalink / raw)
  To: Harvey Harrison; +Cc: David Miller, Joe Perches, linux-netdev
In-Reply-To: <1203586467.20345.14.camel@brick>

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


On Thu, 2008-02-21 at 01:34 -0800, Harvey Harrison wrote:
> commit 0795af5729b18218767fab27c44b1384f72dc9ad
> Author: Joe Perches <joe@perches.com>
> Date:   Wed Oct 3 17:59:30 2007 -0700
> 
>     [NET]: Introduce and use print_mac() and DECLARE_MAC_BUF()
>     
>     This is nicer than the MAC_FMT stuff.
>     
>     Signed-off-by: Joe Perches <joe@perches.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> Introduced:
> net/mac80211/debugfs_sta.c: In function ‘ieee80211_sta_debugfs_add’:
> net/mac80211/debugfs_sta.c:211: warning: statement with no effect
> 
> Does print_mac modify the mac buffer in-place, or return a new buffer?

It modifies the buffer, and I think it's more likely that the warning
was introduced by

commit 8f789c48448aed74fe1c07af76de8f04adacec7d
Author: David S. Miller <davem@davemloft.net>
Date:   Mon Feb 18 16:50:22 2008 -0800

    [NET]: Elminate spurious print_mac() calls.

since __pure indicates that the function has no side effects apart from
the return value (IIRC)

johannes

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

^ permalink raw reply

* Re: New sparse warning in net/mac80211/debugfs_sta.c
From: Harvey Harrison @ 2008-02-21 10:01 UTC (permalink / raw)
  To: David Miller; +Cc: joe, netdev
In-Reply-To: <20080221.015743.222059206.davem@davemloft.net>

On Thu, 2008-02-21 at 01:57 -0800, David Miller wrote:
> From: Harvey Harrison <harvey.harrison@gmail.com>
> Date: Thu, 21 Feb 2008 01:34:27 -0800
> 
> > commit 0795af5729b18218767fab27c44b1384f72dc9ad
> > Author: Joe Perches <joe@perches.com>
> > Date:   Wed Oct 3 17:59:30 2007 -0700
> > 
> >     [NET]: Introduce and use print_mac() and DECLARE_MAC_BUF()
> >     
> >     This is nicer than the MAC_FMT stuff.
> >     
> >     Signed-off-by: Joe Perches <joe@perches.com>
> >     Signed-off-by: David S. Miller <davem@davemloft.net>
> > 
> > Introduced:
> > net/mac80211/debugfs_sta.c: In function ‘ieee80211_sta_debugfs_add’:
> > net/mac80211/debugfs_sta.c:211: warning: statement with no effect
> > 
> > Does print_mac modify the mac buffer in-place, or return a new buffer?
> 
> It modifies the buffer in place.
> 
> The warning is likely not from Joe's patch, but more likely
> from the __pure attribute I recently added to print_mac()'s
> declaration.  If so, we'll need to perhaps rework things.

In this case, it's being passed to a debugfs create function, could it
instead use sysfs_format_mac?

Harvey


^ permalink raw reply

* Re: New sparse warning in net/mac80211/debugfs_sta.c
From: David Miller @ 2008-02-21 10:03 UTC (permalink / raw)
  To: johannes; +Cc: harvey.harrison, joe, netdev
In-Reply-To: <1203587699.17534.135.camel@johannes.berg>

From: Johannes Berg <johannes@sipsolutions.net>
Date: Thu, 21 Feb 2008 10:54:59 +0100

> It modifies the buffer, and I think it's more likely that the warning
> was introduced by
 ...
>     [NET]: Elminate spurious print_mac() calls.

Right and that's what I just suggested in another reply.

Harvey how did you "narrow it down" to Joe's patch?  Did you really
bisect to figure it out, or did you just guess?  You said "commit X
introduced" and if you really didn't bisect it down to that
such a claim is very misleading.

Thanks.

^ permalink raw reply

* Re: New sparse warning in net/mac80211/debugfs_sta.c
From: David Miller @ 2008-02-21 10:05 UTC (permalink / raw)
  To: harvey.harrison; +Cc: joe, netdev
In-Reply-To: <1203588079.20345.15.camel@brick>

From: Harvey Harrison <harvey.harrison@gmail.com>
Date: Thu, 21 Feb 2008 02:01:19 -0800

> In this case, it's being passed to a debugfs create function, could it
> instead use sysfs_format_mac?

Just assigning print_mac() to a local variable then passing that to
debugfs_create_dir() will make the warning go away.

>From another perspective adding that __pure attribute to print_mac()
might not have been the best idea.  But I can't think of another
way to elimitate the "passing print_mac() args to pr_debug()
still generates calls to print_mac() even when DEBUG is not
defined" problem :-/


^ permalink raw reply

* Re: New sparse warning in net/mac80211/debugfs_sta.c
From: Harvey Harrison @ 2008-02-21 10:08 UTC (permalink / raw)
  To: David Miller; +Cc: johannes, joe, netdev
In-Reply-To: <20080221.020343.190715910.davem@davemloft.net>

On Thu, 2008-02-21 at 02:03 -0800, David Miller wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Thu, 21 Feb 2008 10:54:59 +0100
> 
> > It modifies the buffer, and I think it's more likely that the warning
> > was introduced by
>  ...
> >     [NET]: Elminate spurious print_mac() calls.
> 
> Right and that's what I just suggested in another reply.
> 
> Harvey how did you "narrow it down" to Joe's patch?  Did you really
> bisect to figure it out, or did you just guess?  You said "commit X
> introduced" and if you really didn't bisect it down to that
> such a claim is very misleading.

Sorry, that was misleading, it was a simple-minded git-log/git-blame
very short look, saw that line introduced here.

Really, that should have read:

Sparse warning introduced since -rc2:
..etc

Harvey's best guess is this commit...

I'll be more careful about this language going forward.

Cheers,

Harvey


^ permalink raw reply

* hello
From: diana @ 2008-02-21  9:36 UTC (permalink / raw)




Blessed one, 

My name  is Mrs. Deborah Traywick
I am 59 years old and I was diagnosed for cancer 
for about 2 years ago, i am a dying woman and i 
have decided to donate the sum of $5.5 million dollars.)
to you for the good of humanity.

Contact my lawyer with this email: 

Name: Peter J. Wallace. 
Email: 4peterjwallace@myway.com

Tell him that I have WILLED 5.5M to you by 
quoting my personal reference number Qe/yr/088/51/wsa/ld
and I have also notified him that I am WILLING that 
amount to you.

Please use the funds well and always 
extend the good work to othersI don't know you but 
I have been directed to do this. I wish you the best in your life.   
 

Regards,

Deborah Traywick

----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.


^ permalink raw reply

* Re: New sparse warning in net/mac80211/debugfs_sta.c
From: David Miller @ 2008-02-21 10:12 UTC (permalink / raw)
  To: harvey.harrison; +Cc: johannes, joe, netdev
In-Reply-To: <1203588498.20345.20.camel@brick>

From: Harvey Harrison <harvey.harrison@gmail.com>
Date: Thu, 21 Feb 2008 02:08:18 -0800

> Really, that should have read:
> 
> Sparse warning introduced since -rc2:
> ..etc
> 
> Harvey's best guess is this commit...

I think the commit you originally blamed got added in 2.6.24,
so it would be very difficult for it to cause a post-2.6.25-rc2
regression :-)


^ permalink raw reply

* Re: New sparse warning in net/mac80211/debugfs_sta.c
From: Harvey Harrison @ 2008-02-21 10:14 UTC (permalink / raw)
  To: David Miller; +Cc: joe, netdev
In-Reply-To: <20080221.020554.259219477.davem@davemloft.net>

On Thu, 2008-02-21 at 02:05 -0800, David Miller wrote:
> From: Harvey Harrison <harvey.harrison@gmail.com>
> Date: Thu, 21 Feb 2008 02:01:19 -0800
> 
> > In this case, it's being passed to a debugfs create function, could it
> > instead use sysfs_format_mac?
> 
> Just assigning print_mac() to a local variable then passing that to
> debugfs_create_dir() will make the warning go away.

Well, in this case I'd suggest either reverting the __pure annotation
or just ignoring the warning.

Harvey


^ permalink raw reply

* Re: New sparse warning in net/mac80211/debugfs_sta.c
From: Harvey Harrison @ 2008-02-21 10:16 UTC (permalink / raw)
  To: David Miller; +Cc: johannes, joe, netdev
In-Reply-To: <20080221.021228.09081608.davem@davemloft.net>

On Thu, 2008-02-21 at 02:12 -0800, David Miller wrote:
> From: Harvey Harrison <harvey.harrison@gmail.com>
> Date: Thu, 21 Feb 2008 02:08:18 -0800
> 
> > Really, that should have read:
> > 
> > Sparse warning introduced since -rc2:
> > ..etc
> > 
> > Harvey's best guess is this commit...
> 
> I think the commit you originally blamed got added in 2.6.24,
> so it would be very difficult for it to cause a post-2.6.25-rc2
> regression :-)
> 

I don't view it as a regression, just seeing if warning maintainers
incrementally when new sparse warnings get in ends up being well
received.

In addition to trying to lower the noise level of the existing
warnings, might as well try to stem the tide of new ones.

Harvey




^ permalink raw reply

* Re: New sparse warning in net/mac80211/debugfs_sta.c
From: Johannes Berg @ 2008-02-21 10:17 UTC (permalink / raw)
  To: David Miller; +Cc: harvey.harrison, joe, netdev
In-Reply-To: <20080221.020554.259219477.davem@davemloft.net>

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


On Thu, 2008-02-21 at 02:05 -0800, David Miller wrote:
> From: Harvey Harrison <harvey.harrison@gmail.com>
> Date: Thu, 21 Feb 2008 02:01:19 -0800
> 
> > In this case, it's being passed to a debugfs create function, could it
> > instead use sysfs_format_mac?
> 
> Just assigning print_mac() to a local variable then passing that to
> debugfs_create_dir() will make the warning go away.

Indeed, either of those will work.

> From another perspective adding that __pure attribute to print_mac()
> might not have been the best idea.  But I can't think of another
> way to elimitate the "passing print_mac() args to pr_debug()
> still generates calls to print_mac() even when DEBUG is not
> defined" problem :-/

Yeah, I saw that discussion. I think it's fine, it's just something we
need to be aware of. In fact, I Joe had a patch (that seems to have
gotten lost?) to make DECLARE_MAC_BUF() declare a structure with the u8
pointer in it instead to get type checking for the args, which would
make our code there not even compile, and imho rightfully so. I'll send
in a patch to fix this (via John) and Joe can resend his patch to get
typechecking there.

johannes

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

^ permalink raw reply

* [PATCH] mac80211: fix debugfs_sta print_mac() warning
From: Johannes Berg @ 2008-02-21 10:22 UTC (permalink / raw)
  To: John Linville
  Cc: David Miller, joe-6d6DIl74uiNBDgjK7y7TUQ,
	netdev-u79uwXL29TY76Z2rM5mHXA, Harvey Harrison, linux-wireless
In-Reply-To: <1203588993.20345.27.camel@brick>

When print_mac() was marked as __pure to avoid emitting a function
call in pr_debug() scenarios, a warning in this code surfaced since
it relies on the fact that the buffer is modified and doesn't use
the return value. This patch makes it use the return value instead.

Signed-off-by: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Reported-by: Harvey Harrison <harvey.harrison-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 net/mac80211/debugfs_sta.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

--- everything.orig/net/mac80211/debugfs_sta.c	2008-02-21 11:17:48.000000000 +0100
+++ everything/net/mac80211/debugfs_sta.c	2008-02-21 11:17:59.000000000 +0100
@@ -297,12 +297,13 @@ STA_OPS_WR(agg_status);
 void ieee80211_sta_debugfs_add(struct sta_info *sta)
 {
 	struct dentry *stations_dir = sta->local->debugfs.stations;
-	DECLARE_MAC_BUF(mac);
+	DECLARE_MAC_BUF(mbuf);
+	u8 *mac;
 
 	if (!stations_dir)
 		return;
 
-	print_mac(mac, sta->addr);
+	mac = print_mac(mbuf, sta->addr);
 
 	sta->debugfs.dir = debugfs_create_dir(mac, stations_dir);
 	if (!sta->debugfs.dir)

^ permalink raw reply

* [PATCH 1/2] [IPV6]: Add ORCHID prefix to address label table
From: Juha-Matti Tapio @ 2008-02-21 10:08 UTC (permalink / raw)
  To: netdev

Add a new label for Overlay Routable Cryptographic Hash Identifiers
(RFC 4843) prefix 2001:10::/28 to help proper source address
selection.

ORCHID addresses are used by for example Host Identity Protocol. They are 
global and routable, but they currently need support from both endpoints 
and therefore mixing regular and ORCHID addresses for source and 
destination is a bad idea in general case.

Signed-off-by: Juha-Matti Tapio <jmtapio@verkkotelakka.net>
---
 net/ipv6/addrlabel.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/net/ipv6/addrlabel.c b/net/ipv6/addrlabel.c
index a3c5a72..3a8b3f5 100644
--- a/net/ipv6/addrlabel.c
+++ b/net/ipv6/addrlabel.c
@@ -58,6 +58,7 @@ static struct ip6addrlbl_table
  * ::ffff:0:0/96	V4MAPPED	4
  * fc00::/7		N/A		5		ULA (RFC 4193)
  * 2001::/32		N/A		6		Teredo (RFC 4380)
+ * 2001:10::/28		N/A		7		ORCHID (RFC 4843)
  *
  * Note: 0xffffffff is used if we do not have any policies.
  */
@@ -85,6 +86,10 @@ static const __initdata struct ip6addrlbl_init_table
 		.prefix = &(struct in6_addr){{{ 0x20, 0x01 }}},
 		.prefixlen = 32,
 		.label = 6,
+	},{	/* 2001:10::/28 */
+		.prefix = &(struct in6_addr){{{ 0x20, 0x01, 0x00, 0x10 }}},
+		.prefixlen = 28,
+		.label = 7,
 	},{	/* ::ffff:0:0 */
 		.prefix = &(struct in6_addr){{{ [10] = 0xff, [11] = 0xff }}},
 		.prefixlen = 96,
-- 
1.5.3.8


^ permalink raw reply related

* [PATCH 2/2] [IPV6]: Fix source address selection for ORCHID addresses
From: Juha-Matti Tapio @ 2008-02-21 10:08 UTC (permalink / raw)
  To: netdev

Skip the prefix length matching in source address selection for
orchid -> non-orchid addresses.

Overlay Routable Cryptographic Hash IDentifiers (RFC 4843,
2001:10::/28) are currenty not globally reachable. Without this
check a host with an ORCHID address can end up preferring those over
regular addresses when talking to other regular hosts in the 2001::/16
range thus breaking non-orchid connections.

Signed-off-by: Juha-Matti Tapio <jmtapio@verkkotelakka.net>
---
 include/net/ipv6.h  |   10 ++++++++++
 net/ipv6/addrconf.c |    5 +++++
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index c0c019f..67e024a 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -384,6 +384,16 @@ static inline int ipv6_addr_v4mapped(const struct in6_addr *a)
 }
 
 /*
+ * Check for a RFC 4843 ORCHID address 
+ * (Overlay Routable Cryptographic Hash Identifiers)
+ */
+static inline int ipv6_addr_orchid(const struct in6_addr *a)
+{
+	return ((a->s6_addr32[0] & htonl(0xfffffff0))
+		== htonl(0x20010010));
+}
+
+/*
  * find the first different bit between two addresses
  * length of address must be a multiple of 32bits
  */
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index e40213d..2474d20 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1125,6 +1125,11 @@ int ipv6_dev_get_saddr(struct net_device *daddr_dev,
 			if (hiscore.rule < 7)
 				hiscore.rule++;
 #endif
+
+			/* Skip rule 8 for orchid -> non-orchid address pairs. */
+			if (ipv6_addr_orchid(&ifa->addr) && !ipv6_addr_orchid(daddr))
+				continue;
+
 			/* Rule 8: Use longest matching prefix */
 			if (hiscore.rule < 8) {
 				hiscore.matchlen = ipv6_addr_diff(&ifa_result->addr, daddr);
-- 
1.5.3.8


^ permalink raw reply related

* Re: [PATCH] [NETFILTER]: fix ebtable targets return
From: Patrick McHardy @ 2008-02-21 10:50 UTC (permalink / raw)
  To: David Miller; +Cc: joonwpark81, netfilter-devel, netdev
In-Reply-To: <20080220.214533.205446881.davem@davemloft.net>

David Miller wrote:
> From: Joonwoo Park <joonwpark81@gmail.com>
> Date: Thu, 21 Feb 2008 14:36:32 +0900
> 
>> The function ebt_do_table doesn't take NF_DROP as a verdict from the targets.
>>
>> Signed-off-by: Joonwoo Park <joonwpark81@gmail.com>
> 
> Whoops, good catch :-)
> 
> Patrick, if you want you can just signoff on this and I can
> put it directly into my tree.

Good catch indeed, thanks.

Signed-off-by: Patrick McHardy <kaber@trash.net>

^ permalink raw reply

* Re: ni52.c warnings explosion
From: Alan Cox @ 2008-02-21 10:56 UTC (permalink / raw)
  To: Harvey Harrison; +Cc: David Miller, linux-netdev
In-Reply-To: <1203578784.20345.4.camel@brick>

On Wed, 20 Feb 2008 23:26:24 -0800
Harvey Harrison <harvey.harrison@gmail.com> wrote:

> Dave,
> 
> Somewhere between 2.6.25-rc1 and -rc2 something changed that produces a
> few hundred sparse warnings in ni52.c.

I gave it a massive clean up but I've not annotated it
> 
> I see Alan touched it last.
> 
> drivers/net/ni52.c:219:15: warning: incorrect type in argument 1 (different address spaces)
> drivers/net/ni52.c:219:15:    expected void const volatile [noderef] <asn:2>*addr
> drivers/net/ni52.c:219:15:    got unsigned char *<noident>
> 
> If you want the full log, let me know.

Throw it my way. I'll go through it but its probably mostly about
annotating it now it uses io spaces.

^ permalink raw reply

* Re: 2.6.25-rc2-mm1 - several bugs and a crash
From: Patrick McHardy @ 2008-02-21 11:28 UTC (permalink / raw)
  To: Tilman Schmidt; +Cc: Andrew Morton, linux-kernel, netdev, netfilter-devel
In-Reply-To: <47BC982C.7050402@imap.cc>

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

Tilman Schmidt wrote:
> Still, X came up fine, I could log in (Gnome feeling subjectively
> a bit sluggish), call up a web page from the Internet in Firefox,
> and start perusing the logs, when the whole system froze: neither
> mouse nor keyboard would react anymore, and only the Wind^Wreset
> button would put me back in control. After rebooting into the
> previous, non-mm kernel I found this in the syslog:
> 
> Feb 20 17:22:40 xenon kernel: [   48.180297] BUG: using smp_processor_id() in preemptible [00000000] code: ntpdate/3562
> Feb 20 17:22:40 xenon kernel: [   48.180297] caller is __nf_conntrack_find+0x9b/0xeb [nf_conntrack]
> Feb 20 17:22:40 xenon kernel: [   48.180297] Pid: 3562, comm: ntpdate Not tainted 2.6.25-rc2-mm1-testing #1
> Feb 20 17:22:40 xenon kernel: [   48.180297]  [<c02015b9>] debug_smp_processor_id+0x99/0xb0


Could you test whether this patch fixes the netfilter
warnings please?


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

commit 736b33102292be0d75be1e950ca9bcd5361db7dd
Author: Patrick McHardy <kaber@trash.net>
Date:   Thu Feb 21 12:26:01 2008 +0100

    [NETFILTER]: nf_conntrack: fix smp_processor_id() in preemptible code warning
    
    Since we're using RCU for the conntrack hash now, we need to avoid
    getting preempted or interrupted by BHs while changing the stats.
    
    Fixes warning reported by Tilman Schmidt <tilman@imap.cc> when using
    preemptible RCU:
    
    [   48.180297] BUG: using smp_processor_id() in preemptible [00000000] code: ntpdate/3562
    [   48.180297] caller is __nf_conntrack_find+0x9b/0xeb [nf_conntrack]
    [   48.180297] Pid: 3562, comm: ntpdate Not tainted 2.6.25-rc2-mm1-testing #1
    [   48.180297]  [<c02015b9>] debug_smp_processor_id+0x99/0xb0
    [   48.180297]  [<fac643a7>] __nf_conntrack_find+0x9b/0xeb [nf_conntrack]
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 327e847..b77eb56 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -256,13 +256,19 @@ __nf_conntrack_find(const struct nf_conntrack_tuple *tuple)
 	struct hlist_node *n;
 	unsigned int hash = hash_conntrack(tuple);
 
+	/* Disable BHs the entire time since we normally need to disable them
+	 * at least once for the stats anyway.
+	 */
+	local_bh_disable();
 	hlist_for_each_entry_rcu(h, n, &nf_conntrack_hash[hash], hnode) {
 		if (nf_ct_tuple_equal(tuple, &h->tuple)) {
 			NF_CT_STAT_INC(found);
+			local_bh_enable();
 			return h;
 		}
 		NF_CT_STAT_INC(searched);
 	}
+	local_bh_enable();
 
 	return NULL;
 }
@@ -400,17 +406,20 @@ nf_conntrack_tuple_taken(const struct nf_conntrack_tuple *tuple,
 	struct hlist_node *n;
 	unsigned int hash = hash_conntrack(tuple);
 
-	rcu_read_lock();
+	/* Disable BHs the entire time since we need to disable them at
+	 * least once for the stats anyway.
+	 */
+	rcu_read_lock_bh();
 	hlist_for_each_entry_rcu(h, n, &nf_conntrack_hash[hash], hnode) {
 		if (nf_ct_tuplehash_to_ctrack(h) != ignored_conntrack &&
 		    nf_ct_tuple_equal(tuple, &h->tuple)) {
 			NF_CT_STAT_INC(found);
-			rcu_read_unlock();
+			rcu_read_unlock_bh();
 			return 1;
 		}
 		NF_CT_STAT_INC(searched);
 	}
-	rcu_read_unlock();
+	rcu_read_unlock_bh();
 
 	return 0;
 }

^ permalink raw reply related

* Re: [NETFILTER]: Introduce nf_inet_address
From: David Woodhouse @ 2008-02-21 11:52 UTC (permalink / raw)
  To: Patrick McHardy; +Cc: netdev, Jan Engelhardt, David S. Miller, varekova
In-Reply-To: <47BAEB6C.1030609@trash.net>


On Tue, 2008-02-19 at 15:45 +0100, Patrick McHardy wrote:
> That would break iptables compilation, which already includes
> linux/in.h in some files. I guess the best fix for now is to
> include netinet/in.h in busybox and long-term clean this up
> properly.

Yeah, that makes sense.

Can we push the change to __u32 (or uint32_t) for 2.6.24? Or is there
something obvious we should be doing in busybox which we aren't? I don't
quite understand why this u_int32_t crap doesn't work at _all_ when it
evidently used to at least in some environments.

-- 
dwmw2


^ permalink raw reply

* Re: [NETFILTER]: Introduce nf_inet_address
From: Patrick McHardy @ 2008-02-21 12:00 UTC (permalink / raw)
  To: David Woodhouse; +Cc: netdev, Jan Engelhardt, David S. Miller, varekova
In-Reply-To: <1203594725.15409.48.camel@shinybook.infradead.org>

David Woodhouse wrote:
> On Tue, 2008-02-19 at 15:45 +0100, Patrick McHardy wrote:
>> That would break iptables compilation, which already includes
>> linux/in.h in some files. I guess the best fix for now is to
>> include netinet/in.h in busybox and long-term clean this up
>> properly.
> 
> Yeah, that makes sense.
> 
> Can we push the change to __u32 (or uint32_t) for 2.6.24? Or is there
> something obvious we should be doing in busybox which we aren't? I don't
> quite understand why this u_int32_t crap doesn't work at _all_ when it
> evidently used to at least in some environments.


I already sent it to Dave for 2.6.25 (I assume that what you meant,
it was introduced after 2.6.24), its currently sitting in net-2.6
and should hit Linus' tree next time he pulls from Dave.



^ permalink raw reply

* [PATCH] Don't create tunnels with '%' in name.
From: Pavel Emelyanov @ 2008-02-21 12:05 UTC (permalink / raw)
  To: David Miller; +Cc: Linux Netdev List, devel

Four tunnel drivers (ip_gre, ipip, ip6_tunnel and sit) can
receive a pre-defined name for a device from the userspace.
Since these drivers call the register_netdevice() after this 
(rtnl_lock is held), the device's name may contain a '%' 
character.

Not sure how bad is this to have a device with a '%' in its
name, but all the other places either use the register_netdev(),
or explicitly call dev_alloc_name() before registering, i.e. 
do not allow for such names.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>

---

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 63f6917..6b9744f 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -274,19 +274,24 @@ static struct ip_tunnel * ipgre_tunnel_locate(struct ip_tunnel_parm *parms, int
 	if (!dev)
 	  return NULL;
 
+	if (strchr(name, '%')) {
+		if (dev_alloc_name(dev, name) < 0)
+			goto failed_free;
+	}
+
 	dev->init = ipgre_tunnel_init;
 	nt = netdev_priv(dev);
 	nt->parms = *parms;
 
-	if (register_netdevice(dev) < 0) {
-		free_netdev(dev);
-		goto failed;
-	}
+	if (register_netdevice(dev) < 0)
+		goto failed_free;
 
 	dev_hold(dev);
 	ipgre_tunnel_link(nt);
 	return nt;
 
+failed_free:
+	free_netdev(dev);
 failed:
 	return NULL;
 }
diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
index da28158..118e7d9 100644
--- a/net/ipv4/ipip.c
+++ b/net/ipv4/ipip.c
@@ -236,19 +236,24 @@ static struct ip_tunnel * ipip_tunnel_locate(struct ip_tunnel_parm *parms, int c
 	if (dev == NULL)
 		return NULL;
 
+	if (strchr(name, '%')) {
+		if (dev_alloc_name(dev, name) < 0)
+			goto failed_free;
+	}
+
 	nt = netdev_priv(dev);
 	dev->init = ipip_tunnel_init;
 	nt->parms = *parms;
 
-	if (register_netdevice(dev) < 0) {
-		free_netdev(dev);
-		goto failed;
-	}
+	if (register_netdevice(dev) < 0)
+		goto failed_free;
 
 	dev_hold(dev);
 	ipip_tunnel_link(nt);
 	return nt;
 
+failed_free:
+	free_netdev(dev);
 failed:
 	return NULL;
 }
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index cd94064..fa83d70 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -245,17 +245,24 @@ static struct ip6_tnl *ip6_tnl_create(struct ip6_tnl_parm *p)
 	if (dev == NULL)
 		goto failed;
 
+	if (strchr(name, '%')) {
+		if (dev_alloc_name(dev, name) < 0)
+			goto failed_free;
+	}
+
 	t = netdev_priv(dev);
 	dev->init = ip6_tnl_dev_init;
 	t->parms = *p;
 
-	if ((err = register_netdevice(dev)) < 0) {
-		free_netdev(dev);
-		goto failed;
-	}
+	if ((err = register_netdevice(dev)) < 0)
+		goto failed_free;
+
 	dev_hold(dev);
 	ip6_tnl_link(t);
 	return t;
+
+failed_free:
+	free_netdev(dev);
 failed:
 	return NULL;
 }
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index e77239d..a09a6b0 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -179,6 +179,11 @@ static struct ip_tunnel * ipip6_tunnel_locate(struct ip_tunnel_parm *parms, int
 	if (dev == NULL)
 		return NULL;
 
+	if (strchr(name, '%')) {
+		if (dev_alloc_name(dev, name) < 0)
+			goto failed_free;
+	}
+
 	nt = netdev_priv(dev);
 	dev->init = ipip6_tunnel_init;
 	nt->parms = *parms;
@@ -186,16 +191,16 @@ static struct ip_tunnel * ipip6_tunnel_locate(struct ip_tunnel_parm *parms, int
 	if (parms->i_flags & SIT_ISATAP)
 		dev->priv_flags |= IFF_ISATAP;
 
-	if (register_netdevice(dev) < 0) {
-		free_netdev(dev);
-		goto failed;
-	}
+	if (register_netdevice(dev) < 0)
+		goto failed_free;
 
 	dev_hold(dev);
 
 	ipip6_tunnel_link(nt);
 	return nt;
 
+failed_free:
+	free_netdev(dev);
 failed:
 	return NULL;
 }

^ permalink raw reply related

* Re: [PATCH][PPPOL2TP]: Fix SMP oops in pppol2tp driver
From: Jarek Poplawski @ 2008-02-21 12:08 UTC (permalink / raw)
  To: James Chapman; +Cc: David Miller, Paul Mackerras, netdev
In-Reply-To: <47BD4A34.7070606@katalix.com>

On Thu, Feb 21, 2008 at 09:53:56AM +0000, James Chapman wrote:
> Jarek Poplawski wrote:
...
> The _bh locking fixes in pppol2tp combined with your ppp_generic change  
> solved that problem. So I then added data traffic into the mix (since  
> this will happen in a real network) and found that lockups still happen.  
> But the lockdep trace in this case is different, as you noted.

I'm not sure what do you mean by "solved that problem": lack of lockups
or lack of this kind of lockdep reports. This lockdep report shows a
real danger in this case, probably very little probable, unless a lot
of tries. So if you think just this kind of lockup happend there, this
is would be nice. But there could be something less nice too: we are
"fighting" with lockdep in one place but the lockup happens somewhere
else for some totally different reason...

> Does PPPoE stress the PPP setup code as much as this scenario? I guess  
> in theory it could if lots of PPPoE clients connected at the same time,  
> but there is no aggregate tunnel like there is with L2TP to cause all  
> sessions to connect simultaneously. Perhaps PPTP also suffers from these  
> issues? Perhaps not because it tends to be used only in VPN setups where  
> there is only 1 session per tunnel.

I don't know this code enough, but it seems it should be easier to
maintain or debug if there are similar solutions where possible.

>>>> 3) I send here another testing patch with this second way to do this:
>>>>    on the write side, but it's even more "experimental" and only a
>>>>    proof of concept (should be applied on vanilla ppp_generic).
>>> I'll look over it. I think I need to take a step back and look at 
>>> what's  happening in more detail though.
>>
>> This is something completely new and changes all the picture: the xmit
>> path wasn't expected (at least by me) to be called in softirq context
>> at all, and there were no traces of this on previous reports. But,
>> since lockdep always stops after the first warning, there could be
>> even more surprises like this in the future. I'll check this report.
>
> Doesn't the TX softirq do transmits if they've been queued up?

I've probably too much looked at these reports, and should've expected
this could happen. Probably queueing could be separated, but since
there could be no queue at all and it's done like this, then this
current proof of concept seems to be dead end, and we have to go back
to fixing sk_dst_lock handling and my patches could be dumped...

So if I don't miss something again (and I need more time for this new
report) you should try to fix the problem not reported originally by
lockdep, but forseen by David(!): we need to avoid any path like:
ppp_generic -> pppol2tp -> something, which could take sk_dst_lock
while holding any ppp_generic writing lock:  they all are softirq
"safe" (i.e. endangered). David gave some example, but I'm not sure
you did your patch like this (sk_dst_set()). Probably ip_queue_xmit()
can't work with this too.

Another, probably simpler way would be to move almost all pppol2tp_xmit
code to a workqueue: this should let to break most of dependencies with
ppp_generic locks, but I don't know how much it would affect other
things (e.g. performance). So you should really rethink these things.

Jarek P.

^ 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