Netdev List
 help / color / mirror / Atom feed
* [PATCH 7/7] Consolidate frag queues freeing
From: Pavel Emelyanov @ 2007-10-16 14:07 UTC (permalink / raw)
  To: David Miller; +Cc: Linux Netdev List, devel
In-Reply-To: <4714C0AF.1000209@openvz.org>

Since we now allocate the queues in inet_fragment.c, we
can safely free it in the same place. The ->destructor
callback thus becomes optional for inet_frags.

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

---

diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
index 470b056..a75f8cb 100644
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -136,7 +136,9 @@ void inet_frag_destroy(struct inet_frag_queue *q, struct inet_frags *f,
 		*work -= f->qsize;
 	atomic_sub(f->qsize, &f->mem);
 
-	f->destructor(q);
+	if (f->destructor)
+		f->destructor(q);
+	kfree(q);
 
 }
 EXPORT_SYMBOL(inet_frag_destroy);
diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index 62dcb79..34e790d 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -171,7 +171,6 @@ static __inline__ void ip4_frag_free(struct inet_frag_queue *q)
 	qp = container_of(q, struct ipq, q);
 	if (qp->peer)
 		inet_putpeer(qp->peer);
-	kfree(qp);
 }
 
 
diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
index 411daf5..a70a482 100644
--- a/net/ipv6/netfilter/nf_conntrack_reasm.c
+++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
@@ -130,11 +130,6 @@ static inline void frag_kfree_skb(struct sk_buff *skb, unsigned int *work)
 	kfree_skb(skb);
 }
 
-static void nf_frag_free(struct inet_frag_queue *q)
-{
-	kfree(container_of(q, struct nf_ct_frag6_queue, q));
-}
-
 /* Destruction primitives. */
 
 static __inline__ void fq_put(struct nf_ct_frag6_queue *fq)
@@ -682,7 +677,7 @@ int nf_ct_frag6_init(void)
 	nf_frags.ctl = &nf_frags_ctl;
 	nf_frags.hashfn = nf_hashfn;
 	nf_frags.constructor = ip6_frag_init;
-	nf_frags.destructor = nf_frag_free;
+	nf_frags.destructor = NULL;
 	nf_frags.skb_free = nf_skb_free;
 	nf_frags.qsize = sizeof(struct nf_ct_frag6_queue);
 	nf_frags.match = ip6_frag_match;
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index 01766bc..76c88a9 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -175,11 +175,6 @@ void ip6_frag_init(struct inet_frag_queue *q, void *a)
 }
 EXPORT_SYMBOL(ip6_frag_init);
 
-static void ip6_frag_free(struct inet_frag_queue *fq)
-{
-	kfree(container_of(fq, struct frag_queue, q));
-}
-
 /* Destruction primitives. */
 
 static __inline__ void fq_put(struct frag_queue *fq)
@@ -645,7 +640,7 @@ void __init ipv6_frag_init(void)
 	ip6_frags.ctl = &ip6_frags_ctl;
 	ip6_frags.hashfn = ip6_hashfn;
 	ip6_frags.constructor = ip6_frag_init;
-	ip6_frags.destructor = ip6_frag_free;
+	ip6_frags.destructor = NULL;
 	ip6_frags.skb_free = NULL;
 	ip6_frags.qsize = sizeof(struct frag_queue);
 	ip6_frags.match = ip6_frag_match;
-- 
1.5.3.4


^ permalink raw reply related

* [PATCH 6/7] Remove no longer needed ->equal callback
From: Pavel Emelyanov @ 2007-10-16 14:05 UTC (permalink / raw)
  To: David Miller; +Cc: Linux Netdev List, devel
In-Reply-To: <4714C0AF.1000209@openvz.org>

Since this callback is used to check for conflicts in
hashtable when inserting a newly created frag queue, we can
do the same by checking for matching the queue with the 
argument, used to create one.

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

---

diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h
index 6429926..954def4 100644
--- a/include/net/inet_frag.h
+++ b/include/net/inet_frag.h
@@ -43,8 +43,6 @@ struct inet_frags {
 						void *arg);
 	void			(*destructor)(struct inet_frag_queue *);
 	void			(*skb_free)(struct sk_buff *);
-	int			(*equal)(struct inet_frag_queue *q1,
-					 struct inet_frag_queue *q2);
 	int			(*match)(struct inet_frag_queue *q,
 						void *arg);
 	void			(*frag_expire)(unsigned long data);
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 005853a..ae328b6 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -378,7 +378,6 @@ static inline int ipv6_prefix_equal(const struct in6_addr *a1,
 }
 
 struct inet_frag_queue;
-int ip6_frag_equal(struct inet_frag_queue *q1, struct inet_frag_queue *q2);
 
 struct ip6_create_arg {
 	__be32 id;
diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
index 08901b4..470b056 100644
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -174,7 +174,7 @@ int inet_frag_evictor(struct inet_frags *f)
 EXPORT_SYMBOL(inet_frag_evictor);
 
 static struct inet_frag_queue *inet_frag_intern(struct inet_frag_queue *qp_in,
-		struct inet_frags *f, unsigned int hash)
+		struct inet_frags *f, unsigned int hash, void *arg)
 {
 	struct inet_frag_queue *qp;
 #ifdef CONFIG_SMP
@@ -188,7 +188,7 @@ static struct inet_frag_queue *inet_frag_intern(struct inet_frag_queue *qp_in,
 	 * promoted read lock to write lock.
 	 */
 	hlist_for_each_entry(qp, n, &f->hash[hash], list) {
-		if (f->equal(qp, qp_in)) {
+		if (f->match(qp, arg)) {
 			atomic_inc(&qp->refcnt);
 			write_unlock(&f->lock);
 			qp_in->last_in |= COMPLETE;
@@ -235,7 +235,7 @@ static struct inet_frag_queue *inet_frag_create(struct inet_frags *f,
 	if (q == NULL)
 		return NULL;
 
-	return inet_frag_intern(q, f, hash);
+	return inet_frag_intern(q, f, hash, arg);
 }
 
 struct inet_frag_queue *inet_frag_find(struct inet_frags *f, void *key,
diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index 46f8de6..62dcb79 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -128,20 +128,6 @@ static unsigned int ip4_hashfn(struct inet_frag_queue *q)
 	return ipqhashfn(ipq->id, ipq->saddr, ipq->daddr, ipq->protocol);
 }
 
-static int ip4_frag_equal(struct inet_frag_queue *q1,
-		struct inet_frag_queue *q2)
-{
-	struct ipq *qp1, *qp2;
-
-	qp1 = container_of(q1, struct ipq, q);
-	qp2 = container_of(q2, struct ipq, q);
-	return (qp1->id == qp2->id &&
-			qp1->saddr == qp2->saddr &&
-			qp1->daddr == qp2->daddr &&
-			qp1->protocol == qp2->protocol &&
-			qp1->user == qp2->user);
-}
-
 static int ip4_frag_match(struct inet_frag_queue *q, void *a)
 {
 	struct ipq *qp;
@@ -631,7 +617,6 @@ void __init ipfrag_init(void)
 	ip4_frags.destructor = ip4_frag_free;
 	ip4_frags.skb_free = NULL;
 	ip4_frags.qsize = sizeof(struct ipq);
-	ip4_frags.equal = ip4_frag_equal;
 	ip4_frags.match = ip4_frag_match;
 	ip4_frags.frag_expire = ip_expire;
 	inet_frags_init(&ip4_frags);
diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
index 1ab52ef..411daf5 100644
--- a/net/ipv6/netfilter/nf_conntrack_reasm.c
+++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
@@ -686,7 +686,6 @@ int nf_ct_frag6_init(void)
 	nf_frags.skb_free = nf_skb_free;
 	nf_frags.qsize = sizeof(struct nf_ct_frag6_queue);
 	nf_frags.match = ip6_frag_match;
-	nf_frags.equal = ip6_frag_equal;
 	nf_frags.frag_expire = nf_ct_frag6_expire;
 	inet_frags_init(&nf_frags);
 
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index 11fffe7..01766bc 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -143,18 +143,6 @@ static unsigned int ip6_hashfn(struct inet_frag_queue *q)
 	return ip6qhashfn(fq->id, &fq->saddr, &fq->daddr);
 }
 
-int ip6_frag_equal(struct inet_frag_queue *q1, struct inet_frag_queue *q2)
-{
-	struct frag_queue *fq1, *fq2;
-
-	fq1 = container_of(q1, struct frag_queue, q);
-	fq2 = container_of(q2, struct frag_queue, q);
-	return (fq1->id == fq2->id &&
-			ipv6_addr_equal(&fq2->saddr, &fq1->saddr) &&
-			ipv6_addr_equal(&fq2->daddr, &fq1->daddr));
-}
-EXPORT_SYMBOL(ip6_frag_equal);
-
 int ip6_frag_match(struct inet_frag_queue *q, void *a)
 {
 	struct frag_queue *fq;
@@ -661,7 +649,6 @@ void __init ipv6_frag_init(void)
 	ip6_frags.skb_free = NULL;
 	ip6_frags.qsize = sizeof(struct frag_queue);
 	ip6_frags.match = ip6_frag_match;
-	ip6_frags.equal = ip6_frag_equal;
 	ip6_frags.frag_expire = ip6_frag_expire;
 	inet_frags_init(&ip6_frags);
 }
-- 
1.5.3.4


^ permalink raw reply related

* Re: [PATCH 2/3] [NET_CLS_ACT] Use skb_act_clone
From: Herbert Xu @ 2007-10-16 14:06 UTC (permalink / raw)
  To: jamal; +Cc: Patrick McHardy, David Miller, Alexey Kuznetsov, netdev
In-Reply-To: <1192540951.4480.108.camel@localhost>

On Tue, Oct 16, 2007 at 09:22:31AM -0400, jamal wrote:
>
> Does the copying of tc_verd in __copy_skb_header() not cover that?

Yep, I'm pretty sure it does.

In fact this patch completely resolves my concerns about tc_verd.

Thanks!
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply

* [PATCH 5/7] Consolidate xxx_find() in fragment management
From: Pavel Emelyanov @ 2007-10-16 14:03 UTC (permalink / raw)
  To: David Miller; +Cc: Linux Netdev List, devel
In-Reply-To: <4714C0AF.1000209@openvz.org>

Here we need another callback ->match to check whether the
entry found in hash matches the key passed. The key used 
is the same as the creation argument for inet_frag_create.

Yet again, this ->match is the same for netfilter and ipv6.
Running a frew steps forward - this callback will later
replace the ->equal one.

Since the inet_frag_find() uses the already consolidated
inet_frag_create() remove the xxx_frag_create from protocol
codes.

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

---

diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h
index e33072b..6429926 100644
--- a/include/net/inet_frag.h
+++ b/include/net/inet_frag.h
@@ -45,6 +45,8 @@ struct inet_frags {
 	void			(*skb_free)(struct sk_buff *);
 	int			(*equal)(struct inet_frag_queue *q1,
 					 struct inet_frag_queue *q2);
+	int			(*match)(struct inet_frag_queue *q,
+						void *arg);
 	void			(*frag_expire)(unsigned long data);
 };
 
@@ -55,8 +57,8 @@ void inet_frag_kill(struct inet_frag_queue *q, struct inet_frags *f);
 void inet_frag_destroy(struct inet_frag_queue *q,
 				struct inet_frags *f, int *work);
 int inet_frag_evictor(struct inet_frags *f);
-struct inet_frag_queue *inet_frag_create(struct inet_frags *f,
-		void *create_arg, unsigned int hash);
+struct inet_frag_queue *inet_frag_find(struct inet_frags *f, void *key,
+		unsigned int hash);
 
 static inline void inet_frag_put(struct inet_frag_queue *q, struct inet_frags *f)
 {
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 9dc99bf..005853a 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -387,6 +387,7 @@ struct ip6_create_arg {
 };
 
 void ip6_frag_init(struct inet_frag_queue *q, void *a);
+int ip6_frag_match(struct inet_frag_queue *q, void *a);
 
 static inline int ipv6_addr_any(const struct in6_addr *a)
 {
diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
index 0124885..08901b4 100644
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -226,8 +226,8 @@ static struct inet_frag_queue *inet_frag_alloc(struct inet_frags *f, void *arg)
 	return q;
 }
 
-struct inet_frag_queue *inet_frag_create(struct inet_frags *f, void *arg,
-		unsigned int hash)
+static struct inet_frag_queue *inet_frag_create(struct inet_frags *f,
+		void *arg, unsigned int hash)
 {
 	struct inet_frag_queue *q;
 
@@ -237,4 +237,23 @@ struct inet_frag_queue *inet_frag_create(struct inet_frags *f, void *arg,
 
 	return inet_frag_intern(q, f, hash);
 }
-EXPORT_SYMBOL(inet_frag_create);
+
+struct inet_frag_queue *inet_frag_find(struct inet_frags *f, void *key,
+		unsigned int hash)
+{
+	struct inet_frag_queue *q;
+	struct hlist_node *n;
+
+	read_lock(&f->lock);
+	hlist_for_each_entry(q, n, &f->hash[hash], list) {
+		if (f->match(q, key)) {
+			atomic_inc(&q->refcnt);
+			read_unlock(&f->lock);
+			return q;
+		}
+	}
+	read_unlock(&f->lock);
+
+	return inet_frag_create(f, key, hash);
+}
+EXPORT_SYMBOL(inet_frag_find);
diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index db29c3c..46f8de6 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -142,6 +142,19 @@ static int ip4_frag_equal(struct inet_frag_queue *q1,
 			qp1->user == qp2->user);
 }
 
+static int ip4_frag_match(struct inet_frag_queue *q, void *a)
+{
+	struct ipq *qp;
+	struct ip4_create_arg *arg = a;
+
+	qp = container_of(q, struct ipq, q);
+	return (qp->id == arg->iph->id &&
+			qp->saddr == arg->iph->saddr &&
+			qp->daddr == arg->iph->daddr &&
+			qp->protocol == arg->iph->protocol &&
+			qp->user == arg->user);
+}
+
 /* Memory Tracking Functions. */
 static __inline__ void frag_kfree_skb(struct sk_buff *skb, int *work)
 {
@@ -235,18 +248,20 @@ out:
 	ipq_put(qp);
 }
 
-/* Creation primitives. */
-
-/* Add an entry to the 'ipq' queue for a newly received IP datagram. */
-static struct ipq *ip_frag_create(struct iphdr *iph, u32 user, unsigned int h)
+/* Find the correct entry in the "incomplete datagrams" queue for
+ * this IP datagram, and create new one, if nothing is found.
+ */
+static inline struct ipq *ip_find(struct iphdr *iph, u32 user)
 {
 	struct inet_frag_queue *q;
 	struct ip4_create_arg arg;
+	unsigned int hash;
 
 	arg.iph = iph;
 	arg.user = user;
+	hash = ipqhashfn(iph->id, iph->saddr, iph->daddr, iph->protocol);
 
-	q = inet_frag_create(&ip4_frags, &arg, h);
+	q = inet_frag_find(&ip4_frags, &arg, hash);
 	if (q == NULL)
 		goto out_nomem;
 
@@ -257,37 +272,6 @@ out_nomem:
 	return NULL;
 }
 
-/* Find the correct entry in the "incomplete datagrams" queue for
- * this IP datagram, and create new one, if nothing is found.
- */
-static inline struct ipq *ip_find(struct iphdr *iph, u32 user)
-{
-	__be16 id = iph->id;
-	__be32 saddr = iph->saddr;
-	__be32 daddr = iph->daddr;
-	__u8 protocol = iph->protocol;
-	unsigned int hash;
-	struct ipq *qp;
-	struct hlist_node *n;
-
-	read_lock(&ip4_frags.lock);
-	hash = ipqhashfn(id, saddr, daddr, protocol);
-	hlist_for_each_entry(qp, n, &ip4_frags.hash[hash], q.list) {
-		if (qp->id == id		&&
-		    qp->saddr == saddr	&&
-		    qp->daddr == daddr	&&
-		    qp->protocol == protocol &&
-		    qp->user == user) {
-			atomic_inc(&qp->q.refcnt);
-			read_unlock(&ip4_frags.lock);
-			return qp;
-		}
-	}
-	read_unlock(&ip4_frags.lock);
-
-	return ip_frag_create(iph, user, hash);
-}
-
 /* Is the fragment too far ahead to be part of ipq? */
 static inline int ip_frag_too_far(struct ipq *qp)
 {
@@ -648,6 +632,7 @@ void __init ipfrag_init(void)
 	ip4_frags.skb_free = NULL;
 	ip4_frags.qsize = sizeof(struct ipq);
 	ip4_frags.equal = ip4_frag_equal;
+	ip4_frags.match = ip4_frag_match;
 	ip4_frags.frag_expire = ip_expire;
 	inet_frags_init(&ip4_frags);
 }
diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
index 72451e2..1ab52ef 100644
--- a/net/ipv6/netfilter/nf_conntrack_reasm.c
+++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
@@ -176,18 +176,19 @@ out:
 
 /* Creation primitives. */
 
-static struct nf_ct_frag6_queue *
-nf_ct_frag6_create(unsigned int hash, __be32 id, struct in6_addr *src,
-		struct in6_addr *dst)
+static __inline__ struct nf_ct_frag6_queue *
+fq_find(__be32 id, struct in6_addr *src, struct in6_addr *dst)
 {
 	struct inet_frag_queue *q;
 	struct ip6_create_arg arg;
+	unsigned int hash;
 
 	arg.id = id;
 	arg.src = src;
 	arg.dst = dst;
+	hash = ip6qhashfn(id, src, dst);
 
-	q = inet_frag_create(&nf_frags, &arg, hash);
+	q = inet_frag_find(&nf_frags, &arg, hash);
 	if (q == NULL)
 		goto oom;
 
@@ -198,28 +199,6 @@ oom:
 	return NULL;
 }
 
-static __inline__ struct nf_ct_frag6_queue *
-fq_find(__be32 id, struct in6_addr *src, struct in6_addr *dst)
-{
-	struct nf_ct_frag6_queue *fq;
-	struct hlist_node *n;
-	unsigned int hash = ip6qhashfn(id, src, dst);
-
-	read_lock(&nf_frags.lock);
-	hlist_for_each_entry(fq, n, &nf_frags.hash[hash], q.list) {
-		if (fq->id == id &&
-		    ipv6_addr_equal(src, &fq->saddr) &&
-		    ipv6_addr_equal(dst, &fq->daddr)) {
-			atomic_inc(&fq->q.refcnt);
-			read_unlock(&nf_frags.lock);
-			return fq;
-		}
-	}
-	read_unlock(&nf_frags.lock);
-
-	return nf_ct_frag6_create(hash, id, src, dst);
-}
-
 
 static int nf_ct_frag6_queue(struct nf_ct_frag6_queue *fq, struct sk_buff *skb,
 			     struct frag_hdr *fhdr, int nhoff)
@@ -706,6 +685,7 @@ int nf_ct_frag6_init(void)
 	nf_frags.destructor = nf_frag_free;
 	nf_frags.skb_free = nf_skb_free;
 	nf_frags.qsize = sizeof(struct nf_ct_frag6_queue);
+	nf_frags.match = ip6_frag_match;
 	nf_frags.equal = ip6_frag_equal;
 	nf_frags.frag_expire = nf_ct_frag6_expire;
 	inet_frags_init(&nf_frags);
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index ce87340..11fffe7 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -155,6 +155,18 @@ int ip6_frag_equal(struct inet_frag_queue *q1, struct inet_frag_queue *q2)
 }
 EXPORT_SYMBOL(ip6_frag_equal);
 
+int ip6_frag_match(struct inet_frag_queue *q, void *a)
+{
+	struct frag_queue *fq;
+	struct ip6_create_arg *arg = a;
+
+	fq = container_of(q, struct frag_queue, q);
+	return (fq->id == arg->id &&
+			ipv6_addr_equal(&fq->saddr, arg->src) &&
+			ipv6_addr_equal(&fq->daddr, arg->dst));
+}
+EXPORT_SYMBOL(ip6_frag_match);
+
 /* Memory Tracking Functions. */
 static inline void frag_kfree_skb(struct sk_buff *skb, int *work)
 {
@@ -245,20 +257,20 @@ out:
 	fq_put(fq);
 }
 
-/* Creation primitives. */
-
-static struct frag_queue *
-ip6_frag_create(__be32 id, struct in6_addr *src, struct in6_addr *dst,
-		struct inet6_dev *idev, unsigned int hash)
+static __inline__ struct frag_queue *
+fq_find(__be32 id, struct in6_addr *src, struct in6_addr *dst,
+	struct inet6_dev *idev)
 {
 	struct inet_frag_queue *q;
 	struct ip6_create_arg arg;
+	unsigned int hash;
 
 	arg.id = id;
 	arg.src = src;
 	arg.dst = dst;
+	hash = ip6qhashfn(id, src, dst);
 
-	q = inet_frag_create(&ip6_frags, &arg, hash);
+	q = inet_frag_find(&ip6_frags, &arg, hash);
 	if (q == NULL)
 		goto oom;
 
@@ -269,31 +281,6 @@ oom:
 	return NULL;
 }
 
-static __inline__ struct frag_queue *
-fq_find(__be32 id, struct in6_addr *src, struct in6_addr *dst,
-	struct inet6_dev *idev)
-{
-	struct frag_queue *fq;
-	struct hlist_node *n;
-	unsigned int hash;
-
-	read_lock(&ip6_frags.lock);
-	hash = ip6qhashfn(id, src, dst);
-	hlist_for_each_entry(fq, n, &ip6_frags.hash[hash], q.list) {
-		if (fq->id == id &&
-		    ipv6_addr_equal(src, &fq->saddr) &&
-		    ipv6_addr_equal(dst, &fq->daddr)) {
-			atomic_inc(&fq->q.refcnt);
-			read_unlock(&ip6_frags.lock);
-			return fq;
-		}
-	}
-	read_unlock(&ip6_frags.lock);
-
-	return ip6_frag_create(id, src, dst, idev, hash);
-}
-
-
 static int ip6_frag_queue(struct frag_queue *fq, struct sk_buff *skb,
 			   struct frag_hdr *fhdr, int nhoff)
 {
@@ -673,6 +660,7 @@ void __init ipv6_frag_init(void)
 	ip6_frags.destructor = ip6_frag_free;
 	ip6_frags.skb_free = NULL;
 	ip6_frags.qsize = sizeof(struct frag_queue);
+	ip6_frags.match = ip6_frag_match;
 	ip6_frags.equal = ip6_frag_equal;
 	ip6_frags.frag_expire = ip6_frag_expire;
 	inet_frags_init(&ip6_frags);
-- 
1.5.3.4


^ permalink raw reply related

* [PATCH 4/7] Consolidate xxx_frag_create()
From: Pavel Emelyanov @ 2007-10-16 14:00 UTC (permalink / raw)
  To: David Miller; +Cc: Linux Netdev List, devel
In-Reply-To: <4714C0AF.1000209@openvz.org>

This one uses the xxx_frag_intern() and xxx_frag_alloc()
routines, which are already consolidated, so remove them
from protocol code (as promised).

The ->constructor callback is used to init the rest of
the frag queue and it is the same for netfilter and ipv6.

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

---

diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h
index 412b858..e33072b 100644
--- a/include/net/inet_frag.h
+++ b/include/net/inet_frag.h
@@ -39,6 +39,8 @@ struct inet_frags {
 	struct inet_frags_ctl	*ctl;
 
 	unsigned int		(*hashfn)(struct inet_frag_queue *);
+	void			(*constructor)(struct inet_frag_queue *q,
+						void *arg);
 	void			(*destructor)(struct inet_frag_queue *);
 	void			(*skb_free)(struct sk_buff *);
 	int			(*equal)(struct inet_frag_queue *q1,
@@ -53,9 +55,8 @@ void inet_frag_kill(struct inet_frag_queue *q, struct inet_frags *f);
 void inet_frag_destroy(struct inet_frag_queue *q,
 				struct inet_frags *f, int *work);
 int inet_frag_evictor(struct inet_frags *f);
-struct inet_frag_queue *inet_frag_intern(struct inet_frag_queue *q,
-		struct inet_frags *f, unsigned int hash);
-struct inet_frag_queue *inet_frag_alloc(struct inet_frags *f);
+struct inet_frag_queue *inet_frag_create(struct inet_frags *f,
+		void *create_arg, unsigned int hash);
 
 static inline void inet_frag_put(struct inet_frag_queue *q, struct inet_frags *f)
 {
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index ff12697..9dc99bf 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -380,6 +380,14 @@ static inline int ipv6_prefix_equal(const struct in6_addr *a1,
 struct inet_frag_queue;
 int ip6_frag_equal(struct inet_frag_queue *q1, struct inet_frag_queue *q2);
 
+struct ip6_create_arg {
+	__be32 id;
+	struct in6_addr *src;
+	struct in6_addr *dst;
+};
+
+void ip6_frag_init(struct inet_frag_queue *q, void *a);
+
 static inline int ipv6_addr_any(const struct in6_addr *a)
 {
 	return ((a->s6_addr32[0] | a->s6_addr32[1] | 
diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
index 22539fb..0124885 100644
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -173,7 +173,7 @@ int inet_frag_evictor(struct inet_frags *f)
 }
 EXPORT_SYMBOL(inet_frag_evictor);
 
-struct inet_frag_queue *inet_frag_intern(struct inet_frag_queue *qp_in,
+static struct inet_frag_queue *inet_frag_intern(struct inet_frag_queue *qp_in,
 		struct inet_frags *f, unsigned int hash)
 {
 	struct inet_frag_queue *qp;
@@ -208,9 +208,8 @@ struct inet_frag_queue *inet_frag_intern(struct inet_frag_queue *qp_in,
 	write_unlock(&f->lock);
 	return qp;
 }
-EXPORT_SYMBOL(inet_frag_intern);
 
-struct inet_frag_queue *inet_frag_alloc(struct inet_frags *f)
+static struct inet_frag_queue *inet_frag_alloc(struct inet_frags *f, void *arg)
 {
 	struct inet_frag_queue *q;
 	
@@ -218,6 +217,7 @@ struct inet_frag_queue *inet_frag_alloc(struct inet_frags *f)
 	if (q == NULL)
 		return NULL;
 
+	f->constructor(q, arg);
 	atomic_add(f->qsize, &f->mem);
 	setup_timer(&q->timer, f->frag_expire, (unsigned long)q);
 	spin_lock_init(&q->lock);
@@ -225,4 +225,16 @@ struct inet_frag_queue *inet_frag_alloc(struct inet_frags *f)
 
 	return q;
 }
-EXPORT_SYMBOL(inet_frag_alloc);
+
+struct inet_frag_queue *inet_frag_create(struct inet_frags *f, void *arg,
+		unsigned int hash)
+{
+	struct inet_frag_queue *q;
+
+	q = inet_frag_alloc(f, arg);
+	if (q == NULL)
+		return NULL;
+
+	return inet_frag_intern(q, f, hash);
+}
+EXPORT_SYMBOL(inet_frag_create);
diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index 2cf8cdc..db29c3c 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -108,6 +108,11 @@ int ip_frag_mem(void)
 static int ip_frag_reasm(struct ipq *qp, struct sk_buff *prev,
 			 struct net_device *dev);
 
+struct ip4_create_arg {
+	struct iphdr *iph;
+	u32 user;
+};
+
 static unsigned int ipqhashfn(__be16 id, __be32 saddr, __be32 daddr, u8 prot)
 {
 	return jhash_3words((__force u32)id << 16 | prot,
@@ -146,6 +151,20 @@ static __inline__ void frag_kfree_skb(struct sk_buff *skb, int *work)
 	kfree_skb(skb);
 }
 
+static void ip4_frag_init(struct inet_frag_queue *q, void *a)
+{
+	struct ipq *qp = container_of(q, struct ipq, q);
+	struct ip4_create_arg *arg = a;
+
+	qp->protocol = arg->iph->protocol;
+	qp->id = arg->iph->id;
+	qp->saddr = arg->iph->saddr;
+	qp->daddr = arg->iph->daddr;
+	qp->user = arg->user;
+	qp->peer = sysctl_ipfrag_max_dist ?
+		inet_getpeer(arg->iph->saddr, 1) : NULL;
+}
+
 static __inline__ void ip4_frag_free(struct inet_frag_queue *q)
 {
 	struct ipq *qp;
@@ -156,14 +175,6 @@ static __inline__ void ip4_frag_free(struct inet_frag_queue *q)
 	kfree(qp);
 }
 
-static __inline__ struct ipq *frag_alloc_queue(void)
-{
-	struct inet_frag_queue *q;
-
-	q = inet_frag_alloc(&ip4_frags);
-	return q ? container_of(q, struct ipq, q) : NULL;
-}
-
 
 /* Destruction primitives. */
 
@@ -226,30 +237,20 @@ out:
 
 /* Creation primitives. */
 
-static struct ipq *ip_frag_intern(struct ipq *qp_in, unsigned int hash)
-{
-	struct inet_frag_queue *q;
-
-	q = inet_frag_intern(&qp_in->q, &ip4_frags, hash);
-	return container_of(q, struct ipq, q);
-}
-
 /* Add an entry to the 'ipq' queue for a newly received IP datagram. */
 static struct ipq *ip_frag_create(struct iphdr *iph, u32 user, unsigned int h)
 {
-	struct ipq *qp;
+	struct inet_frag_queue *q;
+	struct ip4_create_arg arg;
 
-	if ((qp = frag_alloc_queue()) == NULL)
-		goto out_nomem;
+	arg.iph = iph;
+	arg.user = user;
 
-	qp->protocol = iph->protocol;
-	qp->id = iph->id;
-	qp->saddr = iph->saddr;
-	qp->daddr = iph->daddr;
-	qp->user = user;
-	qp->peer = sysctl_ipfrag_max_dist ? inet_getpeer(iph->saddr, 1) : NULL;
+	q = inet_frag_create(&ip4_frags, &arg, h);
+	if (q == NULL)
+		goto out_nomem;
 
-	return ip_frag_intern(qp, h);
+	return container_of(q, struct ipq, q);
 
 out_nomem:
 	LIMIT_NETDEBUG(KERN_ERR "ip_frag_create: no memory left !\n");
@@ -642,6 +643,7 @@ void __init ipfrag_init(void)
 {
 	ip4_frags.ctl = &ip4_frags_ctl;
 	ip4_frags.hashfn = ip4_hashfn;
+	ip4_frags.constructor = ip4_frag_init;
 	ip4_frags.destructor = ip4_frag_free;
 	ip4_frags.skb_free = NULL;
 	ip4_frags.qsize = sizeof(struct ipq);
diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
index 29a42d2..72451e2 100644
--- a/net/ipv6/netfilter/nf_conntrack_reasm.c
+++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
@@ -135,14 +135,6 @@ static void nf_frag_free(struct inet_frag_queue *q)
 	kfree(container_of(q, struct nf_ct_frag6_queue, q));
 }
 
-static inline struct nf_ct_frag6_queue *frag_alloc_queue(void)
-{
-	struct inet_frag_queue *q;
-
-	q = inet_frag_alloc(&nf_frags);
-	return q ? container_of(q, struct nf_ct_frag6_queue, q) : NULL;
-}
-
 /* Destruction primitives. */
 
 static __inline__ void fq_put(struct nf_ct_frag6_queue *fq)
@@ -184,33 +176,25 @@ out:
 
 /* Creation primitives. */
 
-static struct nf_ct_frag6_queue *nf_ct_frag6_intern(unsigned int hash,
-					  struct nf_ct_frag6_queue *fq_in)
+static struct nf_ct_frag6_queue *
+nf_ct_frag6_create(unsigned int hash, __be32 id, struct in6_addr *src,
+		struct in6_addr *dst)
 {
 	struct inet_frag_queue *q;
+	struct ip6_create_arg arg;
 
-	q = inet_frag_intern(&fq_in->q, &nf_frags, hash);
-	return container_of(q, struct nf_ct_frag6_queue, q);
-}
-
-
-static struct nf_ct_frag6_queue *
-nf_ct_frag6_create(unsigned int hash, __be32 id, struct in6_addr *src,				   struct in6_addr *dst)
-{
-	struct nf_ct_frag6_queue *fq;
+	arg.id = id;
+	arg.src = src;
+	arg.dst = dst;
 
-	if ((fq = frag_alloc_queue()) == NULL) {
-		pr_debug("Can't alloc new queue\n");
+	q = inet_frag_create(&nf_frags, &arg, hash);
+	if (q == NULL)
 		goto oom;
-	}
 
-	fq->id = id;
-	ipv6_addr_copy(&fq->saddr, src);
-	ipv6_addr_copy(&fq->daddr, dst);
-
-	return nf_ct_frag6_intern(hash, fq);
+	return container_of(q, struct nf_ct_frag6_queue, q);
 
 oom:
+	pr_debug("Can't alloc new queue\n");
 	return NULL;
 }
 
@@ -718,6 +702,7 @@ int nf_ct_frag6_init(void)
 {
 	nf_frags.ctl = &nf_frags_ctl;
 	nf_frags.hashfn = nf_hashfn;
+	nf_frags.constructor = ip6_frag_init;
 	nf_frags.destructor = nf_frag_free;
 	nf_frags.skb_free = nf_skb_free;
 	nf_frags.qsize = sizeof(struct nf_ct_frag6_queue);
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index 21913c7..ce87340 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -164,17 +164,20 @@ static inline void frag_kfree_skb(struct sk_buff *skb, int *work)
 	kfree_skb(skb);
 }
 
-static void ip6_frag_free(struct inet_frag_queue *fq)
+void ip6_frag_init(struct inet_frag_queue *q, void *a)
 {
-	kfree(container_of(fq, struct frag_queue, q));
+	struct frag_queue *fq = container_of(q, struct frag_queue, q);
+	struct ip6_create_arg *arg = a;
+
+	fq->id = arg->id;
+	ipv6_addr_copy(&fq->saddr, arg->src);
+	ipv6_addr_copy(&fq->daddr, arg->dst);
 }
+EXPORT_SYMBOL(ip6_frag_init);
 
-static inline struct frag_queue *frag_alloc_queue(void)
+static void ip6_frag_free(struct inet_frag_queue *fq)
 {
-	struct inet_frag_queue *q;
-
-	q = inet_frag_alloc(&ip6_frags);
-	return q ? container_of(q, struct frag_queue, q) : NULL;
+	kfree(container_of(fq, struct frag_queue, q));
 }
 
 /* Destruction primitives. */
@@ -244,31 +247,22 @@ out:
 
 /* Creation primitives. */
 
-
-static struct frag_queue *ip6_frag_intern(struct frag_queue *fq_in,
-		unsigned int hash)
-{
-	struct inet_frag_queue *q;
-
-	q = inet_frag_intern(&fq_in->q, &ip6_frags, hash);
-	return container_of(q, struct frag_queue, q);
-}
-
-
 static struct frag_queue *
 ip6_frag_create(__be32 id, struct in6_addr *src, struct in6_addr *dst,
 		struct inet6_dev *idev, unsigned int hash)
 {
-	struct frag_queue *fq;
+	struct inet_frag_queue *q;
+	struct ip6_create_arg arg;
 
-	if ((fq = frag_alloc_queue()) == NULL)
-		goto oom;
+	arg.id = id;
+	arg.src = src;
+	arg.dst = dst;
 
-	fq->id = id;
-	ipv6_addr_copy(&fq->saddr, src);
-	ipv6_addr_copy(&fq->daddr, dst);
+	q = inet_frag_create(&ip6_frags, &arg, hash);
+	if (q == NULL)
+		goto oom;
 
-	return ip6_frag_intern(fq, hash);
+	return container_of(q, struct frag_queue, q);
 
 oom:
 	IP6_INC_STATS_BH(idev, IPSTATS_MIB_REASMFAILS);
@@ -675,6 +669,7 @@ void __init ipv6_frag_init(void)
 
 	ip6_frags.ctl = &ip6_frags_ctl;
 	ip6_frags.hashfn = ip6_hashfn;
+	ip6_frags.constructor = ip6_frag_init;
 	ip6_frags.destructor = ip6_frag_free;
 	ip6_frags.skb_free = NULL;
 	ip6_frags.qsize = sizeof(struct frag_queue);
-- 
1.5.3.4


^ permalink raw reply related

* [PATCH 3/7] Consolidate xxx_frag_alloc()
From: Pavel Emelyanov @ 2007-10-16 13:57 UTC (permalink / raw)
  To: David Miller; +Cc: Linux Netdev List, devel
In-Reply-To: <4714C0AF.1000209@openvz.org>

Just perform the kzalloc() allocation and setup common
fields in the inet_frag_queue(). Then return the result
to the caller to initialize the rest.

The inet_frag_alloc() may return NULL, so check the 
return value before doing the container_of(). This looks 
ugly, but the xxx_frag_alloc() will be removed soon.

The xxx_expire() timer callbacks are patches, 
because the argument is now the inet_frag_queue, not 
the protocol specific queue.

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

---

diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h
index 133e187..412b858 100644
--- a/include/net/inet_frag.h
+++ b/include/net/inet_frag.h
@@ -43,6 +43,7 @@ struct inet_frags {
 	void			(*skb_free)(struct sk_buff *);
 	int			(*equal)(struct inet_frag_queue *q1,
 					 struct inet_frag_queue *q2);
+	void			(*frag_expire)(unsigned long data);
 };
 
 void inet_frags_init(struct inet_frags *);
@@ -54,6 +55,7 @@ void inet_frag_destroy(struct inet_frag_queue *q,
 int inet_frag_evictor(struct inet_frags *f);
 struct inet_frag_queue *inet_frag_intern(struct inet_frag_queue *q,
 		struct inet_frags *f, unsigned int hash);
+struct inet_frag_queue *inet_frag_alloc(struct inet_frags *f);
 
 static inline void inet_frag_put(struct inet_frag_queue *q, struct inet_frags *f)
 {
diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
index 15054eb..22539fb 100644
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -209,3 +209,20 @@ struct inet_frag_queue *inet_frag_intern(struct inet_frag_queue *qp_in,
 	return qp;
 }
 EXPORT_SYMBOL(inet_frag_intern);
+
+struct inet_frag_queue *inet_frag_alloc(struct inet_frags *f)
+{
+	struct inet_frag_queue *q;
+	
+	q = kzalloc(f->qsize, GFP_ATOMIC);
+	if (q == NULL)
+		return NULL;
+
+	atomic_add(f->qsize, &f->mem);
+	setup_timer(&q->timer, f->frag_expire, (unsigned long)q);
+	spin_lock_init(&q->lock);
+	atomic_set(&q->refcnt, 1);
+
+	return q;
+}
+EXPORT_SYMBOL(inet_frag_alloc);
diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index 4b1bbbe..2cf8cdc 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -158,12 +158,10 @@ static __inline__ void ip4_frag_free(struct inet_frag_queue *q)
 
 static __inline__ struct ipq *frag_alloc_queue(void)
 {
-	struct ipq *qp = kzalloc(sizeof(struct ipq), GFP_ATOMIC);
+	struct inet_frag_queue *q;
 
-	if (!qp)
-		return NULL;
-	atomic_add(sizeof(struct ipq), &ip4_frags.mem);
-	return qp;
+	q = inet_frag_alloc(&ip4_frags);
+	return q ? container_of(q, struct ipq, q) : NULL;
 }
 
 
@@ -199,7 +197,9 @@ static void ip_evictor(void)
  */
 static void ip_expire(unsigned long arg)
 {
-	struct ipq *qp = (struct ipq *) arg;
+	struct ipq *qp;
+	
+	qp = container_of((struct inet_frag_queue *) arg, struct ipq, q);
 
 	spin_lock(&qp->q.lock);
 
@@ -249,13 +249,6 @@ static struct ipq *ip_frag_create(struct iphdr *iph, u32 user, unsigned int h)
 	qp->user = user;
 	qp->peer = sysctl_ipfrag_max_dist ? inet_getpeer(iph->saddr, 1) : NULL;
 
-	/* Initialize a timer for this entry. */
-	init_timer(&qp->q.timer);
-	qp->q.timer.data = (unsigned long) qp;	/* pointer to queue	*/
-	qp->q.timer.function = ip_expire;		/* expire function	*/
-	spin_lock_init(&qp->q.lock);
-	atomic_set(&qp->q.refcnt, 1);
-
 	return ip_frag_intern(qp, h);
 
 out_nomem:
@@ -653,6 +646,7 @@ void __init ipfrag_init(void)
 	ip4_frags.skb_free = NULL;
 	ip4_frags.qsize = sizeof(struct ipq);
 	ip4_frags.equal = ip4_frag_equal;
+	ip4_frags.frag_expire = ip_expire;
 	inet_frags_init(&ip4_frags);
 }
 
diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
index d7dc444..29a42d2 100644
--- a/net/ipv6/netfilter/nf_conntrack_reasm.c
+++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
@@ -137,13 +137,10 @@ static void nf_frag_free(struct inet_frag_queue *q)
 
 static inline struct nf_ct_frag6_queue *frag_alloc_queue(void)
 {
-	struct nf_ct_frag6_queue *fq;
+	struct inet_frag_queue *q;
 
-	fq = kzalloc(sizeof(struct nf_ct_frag6_queue), GFP_ATOMIC);
-	if (fq == NULL)
-		return NULL;
-	atomic_add(sizeof(struct nf_ct_frag6_queue), &nf_frags.mem);
-	return fq;
+	q = inet_frag_alloc(&nf_frags);
+	return q ? container_of(q, struct nf_ct_frag6_queue, q) : NULL;
 }
 
 /* Destruction primitives. */
@@ -168,7 +165,10 @@ static void nf_ct_frag6_evictor(void)
 
 static void nf_ct_frag6_expire(unsigned long data)
 {
-	struct nf_ct_frag6_queue *fq = (struct nf_ct_frag6_queue *) data;
+	struct nf_ct_frag6_queue *fq;
+	
+	fq = container_of((struct inet_frag_queue *)data,
+			struct nf_ct_frag6_queue, q);
 
 	spin_lock(&fq->q.lock);
 
@@ -208,10 +208,6 @@ nf_ct_frag6_create(unsigned int hash, __be32 id, struct in6_addr *src,				   str
 	ipv6_addr_copy(&fq->saddr, src);
 	ipv6_addr_copy(&fq->daddr, dst);
 
-	setup_timer(&fq->q.timer, nf_ct_frag6_expire, (unsigned long)fq);
-	spin_lock_init(&fq->q.lock);
-	atomic_set(&fq->q.refcnt, 1);
-
 	return nf_ct_frag6_intern(hash, fq);
 
 oom:
@@ -726,6 +722,7 @@ int nf_ct_frag6_init(void)
 	nf_frags.skb_free = nf_skb_free;
 	nf_frags.qsize = sizeof(struct nf_ct_frag6_queue);
 	nf_frags.equal = ip6_frag_equal;
+	nf_frags.frag_expire = nf_ct_frag6_expire;
 	inet_frags_init(&nf_frags);
 
 	return 0;
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index 73ea204..21913c7 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -171,12 +171,10 @@ static void ip6_frag_free(struct inet_frag_queue *fq)
 
 static inline struct frag_queue *frag_alloc_queue(void)
 {
-	struct frag_queue *fq = kzalloc(sizeof(struct frag_queue), GFP_ATOMIC);
+	struct inet_frag_queue *q;
 
-	if(!fq)
-		return NULL;
-	atomic_add(sizeof(struct frag_queue), &ip6_frags.mem);
-	return fq;
+	q = inet_frag_alloc(&ip6_frags);
+	return q ? container_of(q, struct frag_queue, q) : NULL;
 }
 
 /* Destruction primitives. */
@@ -205,9 +203,11 @@ static void ip6_evictor(struct inet6_dev *idev)
 
 static void ip6_frag_expire(unsigned long data)
 {
-	struct frag_queue *fq = (struct frag_queue *) data;
+	struct frag_queue *fq;
 	struct net_device *dev = NULL;
 
+	fq = container_of((struct inet_frag_queue *)data, struct frag_queue, q);
+
 	spin_lock(&fq->q.lock);
 
 	if (fq->q.last_in & COMPLETE)
@@ -268,12 +268,6 @@ ip6_frag_create(__be32 id, struct in6_addr *src, struct in6_addr *dst,
 	ipv6_addr_copy(&fq->saddr, src);
 	ipv6_addr_copy(&fq->daddr, dst);
 
-	init_timer(&fq->q.timer);
-	fq->q.timer.function = ip6_frag_expire;
-	fq->q.timer.data = (long) fq;
-	spin_lock_init(&fq->q.lock);
-	atomic_set(&fq->q.refcnt, 1);
-
 	return ip6_frag_intern(fq, hash);
 
 oom:
@@ -685,5 +679,6 @@ void __init ipv6_frag_init(void)
 	ip6_frags.skb_free = NULL;
 	ip6_frags.qsize = sizeof(struct frag_queue);
 	ip6_frags.equal = ip6_frag_equal;
+	ip6_frags.frag_expire = ip6_frag_expire;
 	inet_frags_init(&ip6_frags);
 }
-- 
1.5.3.4


^ permalink raw reply related

* [PATCH 2/7] Consolidate xxx_frag_intern
From: Pavel Emelyanov @ 2007-10-16 13:53 UTC (permalink / raw)
  To: David Miller; +Cc: Linux Netdev List, devel
In-Reply-To: <4714C0AF.1000209@openvz.org>

This routine checks for the existence of a given entry
in the hash table and inserts the new one if needed.

The ->equal callback is used to compare two frag_queue-s
together, but this one is temporary and will be removed
later. The netfilter code and the ipv6 one use the same
routine to compare frags.

The inet_frag_intern() always returns non-NULL pointer,
so convert the inet_frag_queue into protocol specific
one (with the container_of) without any checks.

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

---

diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h
index 911c2cd..133e187 100644
--- a/include/net/inet_frag.h
+++ b/include/net/inet_frag.h
@@ -41,6 +41,8 @@ struct inet_frags {
 	unsigned int		(*hashfn)(struct inet_frag_queue *);
 	void			(*destructor)(struct inet_frag_queue *);
 	void			(*skb_free)(struct sk_buff *);
+	int			(*equal)(struct inet_frag_queue *q1,
+					 struct inet_frag_queue *q2);
 };
 
 void inet_frags_init(struct inet_frags *);
@@ -50,6 +52,8 @@ void inet_frag_kill(struct inet_frag_queue *q, struct inet_frags *f);
 void inet_frag_destroy(struct inet_frag_queue *q,
 				struct inet_frags *f, int *work);
 int inet_frag_evictor(struct inet_frags *f);
+struct inet_frag_queue *inet_frag_intern(struct inet_frag_queue *q,
+		struct inet_frags *f, unsigned int hash);
 
 static inline void inet_frag_put(struct inet_frag_queue *q, struct inet_frags *f)
 {
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index cc796cb..ff12697 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -377,6 +377,9 @@ static inline int ipv6_prefix_equal(const struct in6_addr *a1,
 				   prefixlen);
 }
 
+struct inet_frag_queue;
+int ip6_frag_equal(struct inet_frag_queue *q1, struct inet_frag_queue *q2);
+
 static inline int ipv6_addr_any(const struct in6_addr *a)
 {
 	return ((a->s6_addr32[0] | a->s6_addr32[1] | 
diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
index 484cf51..15054eb 100644
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -172,3 +172,40 @@ int inet_frag_evictor(struct inet_frags *f)
 	return evicted;
 }
 EXPORT_SYMBOL(inet_frag_evictor);
+
+struct inet_frag_queue *inet_frag_intern(struct inet_frag_queue *qp_in,
+		struct inet_frags *f, unsigned int hash)
+{
+	struct inet_frag_queue *qp;
+#ifdef CONFIG_SMP
+	struct hlist_node *n;
+#endif
+
+	write_lock(&f->lock);
+#ifdef CONFIG_SMP
+	/* With SMP race we have to recheck hash table, because
+	 * such entry could be created on other cpu, while we
+	 * promoted read lock to write lock.
+	 */
+	hlist_for_each_entry(qp, n, &f->hash[hash], list) {
+		if (f->equal(qp, qp_in)) {
+			atomic_inc(&qp->refcnt);
+			write_unlock(&f->lock);
+			qp_in->last_in |= COMPLETE;
+			inet_frag_put(qp_in, f);
+			return qp;
+		}
+	}
+#endif
+	qp = qp_in;
+	if (!mod_timer(&qp->timer, jiffies + f->ctl->timeout))
+		atomic_inc(&qp->refcnt);
+
+	atomic_inc(&qp->refcnt);
+	hlist_add_head(&qp->list, &f->hash[hash]);
+	list_add_tail(&qp->lru_list, &f->lru_list);
+	f->nqueues++;
+	write_unlock(&f->lock);
+	return qp;
+}
+EXPORT_SYMBOL(inet_frag_intern);
diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index d12a18b..4b1bbbe 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -123,6 +123,20 @@ static unsigned int ip4_hashfn(struct inet_frag_queue *q)
 	return ipqhashfn(ipq->id, ipq->saddr, ipq->daddr, ipq->protocol);
 }
 
+static int ip4_frag_equal(struct inet_frag_queue *q1,
+		struct inet_frag_queue *q2)
+{
+	struct ipq *qp1, *qp2;
+
+	qp1 = container_of(q1, struct ipq, q);
+	qp2 = container_of(q2, struct ipq, q);
+	return (qp1->id == qp2->id &&
+			qp1->saddr == qp2->saddr &&
+			qp1->daddr == qp2->daddr &&
+			qp1->protocol == qp2->protocol &&
+			qp1->user == qp2->user);
+}
+
 /* Memory Tracking Functions. */
 static __inline__ void frag_kfree_skb(struct sk_buff *skb, int *work)
 {
@@ -214,43 +228,10 @@ out:
 
 static struct ipq *ip_frag_intern(struct ipq *qp_in, unsigned int hash)
 {
-	struct ipq *qp;
-#ifdef CONFIG_SMP
-	struct hlist_node *n;
-#endif
+	struct inet_frag_queue *q;
 
-	write_lock(&ip4_frags.lock);
-#ifdef CONFIG_SMP
-	/* With SMP race we have to recheck hash table, because
-	 * such entry could be created on other cpu, while we
-	 * promoted read lock to write lock.
-	 */
-	hlist_for_each_entry(qp, n, &ip4_frags.hash[hash], q.list) {
-		if (qp->id == qp_in->id		&&
-		    qp->saddr == qp_in->saddr	&&
-		    qp->daddr == qp_in->daddr	&&
-		    qp->protocol == qp_in->protocol &&
-		    qp->user == qp_in->user) {
-			atomic_inc(&qp->q.refcnt);
-			write_unlock(&ip4_frags.lock);
-			qp_in->q.last_in |= COMPLETE;
-			ipq_put(qp_in);
-			return qp;
-		}
-	}
-#endif
-	qp = qp_in;
-
-	if (!mod_timer(&qp->q.timer, jiffies + ip4_frags_ctl.timeout))
-		atomic_inc(&qp->q.refcnt);
-
-	atomic_inc(&qp->q.refcnt);
-	hlist_add_head(&qp->q.list, &ip4_frags.hash[hash]);
-	INIT_LIST_HEAD(&qp->q.lru_list);
-	list_add_tail(&qp->q.lru_list, &ip4_frags.lru_list);
-	ip4_frags.nqueues++;
-	write_unlock(&ip4_frags.lock);
-	return qp;
+	q = inet_frag_intern(&qp_in->q, &ip4_frags, hash);
+	return container_of(q, struct ipq, q);
 }
 
 /* Add an entry to the 'ipq' queue for a newly received IP datagram. */
@@ -671,6 +652,7 @@ void __init ipfrag_init(void)
 	ip4_frags.destructor = ip4_frag_free;
 	ip4_frags.skb_free = NULL;
 	ip4_frags.qsize = sizeof(struct ipq);
+	ip4_frags.equal = ip4_frag_equal;
 	inet_frags_init(&ip4_frags);
 }
 
diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
index 726fafd..d7dc444 100644
--- a/net/ipv6/netfilter/nf_conntrack_reasm.c
+++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
@@ -187,37 +187,10 @@ out:
 static struct nf_ct_frag6_queue *nf_ct_frag6_intern(unsigned int hash,
 					  struct nf_ct_frag6_queue *fq_in)
 {
-	struct nf_ct_frag6_queue *fq;
-#ifdef CONFIG_SMP
-	struct hlist_node *n;
-#endif
-
-	write_lock(&nf_frags.lock);
-#ifdef CONFIG_SMP
-	hlist_for_each_entry(fq, n, &nf_frags.hash[hash], q.list) {
-		if (fq->id == fq_in->id &&
-		    ipv6_addr_equal(&fq_in->saddr, &fq->saddr) &&
-		    ipv6_addr_equal(&fq_in->daddr, &fq->daddr)) {
-			atomic_inc(&fq->q.refcnt);
-			write_unlock(&nf_frags.lock);
-			fq_in->q.last_in |= COMPLETE;
-			fq_put(fq_in);
-			return fq;
-		}
-	}
-#endif
-	fq = fq_in;
+	struct inet_frag_queue *q;
 
-	if (!mod_timer(&fq->q.timer, jiffies + nf_frags_ctl.timeout))
-		atomic_inc(&fq->q.refcnt);
-
-	atomic_inc(&fq->q.refcnt);
-	hlist_add_head(&fq->q.list, &nf_frags.hash[hash]);
-	INIT_LIST_HEAD(&fq->q.lru_list);
-	list_add_tail(&fq->q.lru_list, &nf_frags.lru_list);
-	nf_frags.nqueues++;
-	write_unlock(&nf_frags.lock);
-	return fq;
+	q = inet_frag_intern(&fq_in->q, &nf_frags, hash);
+	return container_of(q, struct nf_ct_frag6_queue, q);
 }
 
 
@@ -752,6 +725,7 @@ int nf_ct_frag6_init(void)
 	nf_frags.destructor = nf_frag_free;
 	nf_frags.skb_free = nf_skb_free;
 	nf_frags.qsize = sizeof(struct nf_ct_frag6_queue);
+	nf_frags.equal = ip6_frag_equal;
 	inet_frags_init(&nf_frags);
 
 	return 0;
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index 0a1bf43..73ea204 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -143,6 +143,18 @@ static unsigned int ip6_hashfn(struct inet_frag_queue *q)
 	return ip6qhashfn(fq->id, &fq->saddr, &fq->daddr);
 }
 
+int ip6_frag_equal(struct inet_frag_queue *q1, struct inet_frag_queue *q2)
+{
+	struct frag_queue *fq1, *fq2;
+
+	fq1 = container_of(q1, struct frag_queue, q);
+	fq2 = container_of(q2, struct frag_queue, q);
+	return (fq1->id == fq2->id &&
+			ipv6_addr_equal(&fq2->saddr, &fq1->saddr) &&
+			ipv6_addr_equal(&fq2->daddr, &fq1->daddr));
+}
+EXPORT_SYMBOL(ip6_frag_equal);
+
 /* Memory Tracking Functions. */
 static inline void frag_kfree_skb(struct sk_buff *skb, int *work)
 {
@@ -236,37 +248,10 @@ out:
 static struct frag_queue *ip6_frag_intern(struct frag_queue *fq_in,
 		unsigned int hash)
 {
-	struct frag_queue *fq;
-#ifdef CONFIG_SMP
-	struct hlist_node *n;
-#endif
-
-	write_lock(&ip6_frags.lock);
-#ifdef CONFIG_SMP
-	hlist_for_each_entry(fq, n, &ip6_frags.hash[hash], q.list) {
-		if (fq->id == fq_in->id &&
-		    ipv6_addr_equal(&fq_in->saddr, &fq->saddr) &&
-		    ipv6_addr_equal(&fq_in->daddr, &fq->daddr)) {
-			atomic_inc(&fq->q.refcnt);
-			write_unlock(&ip6_frags.lock);
-			fq_in->q.last_in |= COMPLETE;
-			fq_put(fq_in);
-			return fq;
-		}
-	}
-#endif
-	fq = fq_in;
-
-	if (!mod_timer(&fq->q.timer, jiffies + ip6_frags_ctl.timeout))
-		atomic_inc(&fq->q.refcnt);
+	struct inet_frag_queue *q;
 
-	atomic_inc(&fq->q.refcnt);
-	hlist_add_head(&fq->q.list, &ip6_frags.hash[hash]);
-	INIT_LIST_HEAD(&fq->q.lru_list);
-	list_add_tail(&fq->q.lru_list, &ip6_frags.lru_list);
-	ip6_frags.nqueues++;
-	write_unlock(&ip6_frags.lock);
-	return fq;
+	q = inet_frag_intern(&fq_in->q, &ip6_frags, hash);
+	return container_of(q, struct frag_queue, q);
 }
 
 
@@ -699,5 +684,6 @@ void __init ipv6_frag_init(void)
 	ip6_frags.destructor = ip6_frag_free;
 	ip6_frags.skb_free = NULL;
 	ip6_frags.qsize = sizeof(struct frag_queue);
+	ip6_frags.equal = ip6_frag_equal;
 	inet_frags_init(&ip6_frags);
 }
-- 
1.5.3.4


^ permalink raw reply related

* [PATCH 1/7] Omit double hash calculations in xxx_frag_intern
From: Pavel Emelyanov @ 2007-10-16 13:48 UTC (permalink / raw)
  To: David Miller; +Cc: Linux Netdev List, devel
In-Reply-To: <4714C0AF.1000209@openvz.org>

Since the hash value is already calculated in xxx_find, we can 
simply use it later. This is already done in netfilter code, 
so make the same in ipv4 and ipv6.

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

---

>From 5bcb464538cf504a9f6cf3be33dbd1a5ff18b693 Mon Sep 17 00:00:00 2001
From: Pavel <pavel@xemulnb.sw.ru>
Date: Tue, 16 Oct 2007 14:40:30 +0400
Subject: [PATCH 1/7] Pass the hash value as an extra argument

---
 net/ipv4/ip_fragment.c |   11 ++++-------
 net/ipv6/reassembly.c  |   11 +++++------
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index 443b3f8..d12a18b 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -212,17 +212,14 @@ out:
 
 /* Creation primitives. */
 
-static struct ipq *ip_frag_intern(struct ipq *qp_in)
+static struct ipq *ip_frag_intern(struct ipq *qp_in, unsigned int hash)
 {
 	struct ipq *qp;
 #ifdef CONFIG_SMP
 	struct hlist_node *n;
 #endif
-	unsigned int hash;
 
 	write_lock(&ip4_frags.lock);
-	hash = ipqhashfn(qp_in->id, qp_in->saddr, qp_in->daddr,
-			 qp_in->protocol);
 #ifdef CONFIG_SMP
 	/* With SMP race we have to recheck hash table, because
 	 * such entry could be created on other cpu, while we
@@ -257,7 +254,7 @@ static struct ipq *ip_frag_intern(struct ipq *qp_in)
 }
 
 /* Add an entry to the 'ipq' queue for a newly received IP datagram. */
-static struct ipq *ip_frag_create(struct iphdr *iph, u32 user)
+static struct ipq *ip_frag_create(struct iphdr *iph, u32 user, unsigned int h)
 {
 	struct ipq *qp;
 
@@ -278,7 +275,7 @@ static struct ipq *ip_frag_create(struct iphdr *iph, u32 user)
 	spin_lock_init(&qp->q.lock);
 	atomic_set(&qp->q.refcnt, 1);
 
-	return ip_frag_intern(qp);
+	return ip_frag_intern(qp, h);
 
 out_nomem:
 	LIMIT_NETDEBUG(KERN_ERR "ip_frag_create: no memory left !\n");
@@ -313,7 +310,7 @@ static inline struct ipq *ip_find(struct iphdr *iph, u32 user)
 	}
 	read_unlock(&ip4_frags.lock);
 
-	return ip_frag_create(iph, user);
+	return ip_frag_create(iph, user, hash);
 }
 
 /* Is the fragment too far ahead to be part of ipq? */
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index 6ad19cf..0a1bf43 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -233,16 +233,15 @@ out:
 /* Creation primitives. */
 
 
-static struct frag_queue *ip6_frag_intern(struct frag_queue *fq_in)
+static struct frag_queue *ip6_frag_intern(struct frag_queue *fq_in,
+		unsigned int hash)
 {
 	struct frag_queue *fq;
-	unsigned int hash;
 #ifdef CONFIG_SMP
 	struct hlist_node *n;
 #endif
 
 	write_lock(&ip6_frags.lock);
-	hash = ip6qhashfn(fq_in->id, &fq_in->saddr, &fq_in->daddr);
 #ifdef CONFIG_SMP
 	hlist_for_each_entry(fq, n, &ip6_frags.hash[hash], q.list) {
 		if (fq->id == fq_in->id &&
@@ -273,7 +272,7 @@ static struct frag_queue *ip6_frag_intern(struct frag_queue *fq_in)
 
 static struct frag_queue *
 ip6_frag_create(__be32 id, struct in6_addr *src, struct in6_addr *dst,
-		struct inet6_dev *idev)
+		struct inet6_dev *idev, unsigned int hash)
 {
 	struct frag_queue *fq;
 
@@ -290,7 +289,7 @@ ip6_frag_create(__be32 id, struct in6_addr *src, struct in6_addr *dst,
 	spin_lock_init(&fq->q.lock);
 	atomic_set(&fq->q.refcnt, 1);
 
-	return ip6_frag_intern(fq);
+	return ip6_frag_intern(fq, hash);
 
 oom:
 	IP6_INC_STATS_BH(idev, IPSTATS_MIB_REASMFAILS);
@@ -318,7 +317,7 @@ fq_find(__be32 id, struct in6_addr *src, struct in6_addr *dst,
 	}
 	read_unlock(&ip6_frags.lock);
 
-	return ip6_frag_create(id, src, dst, idev);
+	return ip6_frag_create(id, src, dst, idev, hash);
 }
 
 
-- 
1.5.3.4


^ permalink raw reply related

* [PATCH 0/7] Next step in consolidating the IP fragment management
From: Pavel Emelyanov @ 2007-10-16 13:46 UTC (permalink / raw)
  To: David Miller; +Cc: Linux Netdev List, devel

This set concerns the xxx_find() routine only. This one
is used to obtain the fragment queue depending on some
criteria, and this can be consolidated.

The consolidation goes step-by-step, consolidating one
sub-routine at a time, so some functions, exports and 
callbacks that are introduced in patches are temporary
and are removed in some subsequent patch.

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

^ permalink raw reply

* Re: [PATCH 2/3] [NET_CLS_ACT] Use skb_act_clone
From: jamal @ 2007-10-16 13:22 UTC (permalink / raw)
  To: Patrick McHardy; +Cc: David Miller, Alexey Kuznetsov, Herbert Xu, netdev
In-Reply-To: <4714B445.1020900@trash.net>

On Tue, 2007-16-10 at 14:53 +0200, Patrick McHardy wrote:
> jamal wrote:
> 
> [Can't quote for some reason ..]

Sorry, trying to be clever with git-format-patch but chickened out
sending using git-am - so patch was just attached (Dave seems to be 
friendly to such an approach when he applies)

> I don't think this change is safe since we might loose the
> redirect counter and have endless redirect loops. ipip for
> example calls skb_realloc_headroom, which clones the skb
> when delta > 0, so it seems we really need to copy tc_verd
> in skb_clone(). Generally the assumption that drivers don't
> clone packets doesn't seem safe.

Does the copying of tc_verd in __copy_skb_header() not cover that?

cheers,
jamal



^ permalink raw reply

* Re: [PATCH 3/3] [NET_DOC] Document some simple rules for actions
From: jamal @ 2007-10-16 13:09 UTC (permalink / raw)
  To: Herbert Xu; +Cc: David Miller, Alexey Kuznetsov, Patrick McHardy, netdev
In-Reply-To: <20071016125134.GA11319@gondor.apana.org.au>

On Tue, 2007-16-10 at 20:51 +0800, Herbert Xu wrote:

> But I still think the distinction between skb_clone and
> skb_copy_header is wrong.
> 
> When you munge a packet, it's still going to go back up
> the stack and be processed along the same path.  Therefore
> you should be calling pskb_expand_head.

Dang, just noticed i said skb_expand() in the doc - is that the 
issue? I will resend that one patch.
We do call pskb_expand_head() - example look at act_pedit.c

> In other words, this does not explain why skb_copy/pskb_copy
> and skb_copy_expand should differ from skb_clone.  As far as
> I can see all these functions should behave in the same manner
> with respect to tc_verd


We need to avoid actions from infinetely looping (eg when they ask for
being reclassified) in a directed graph. That loop ttl count  must be
reset to zero if we branch, because that is a new directed path. This is
always true and therefore that field must always be reset. 
I have a feeling i am going on a tangent if that doesnt respond to your
comment above.

cheers,
jamal


^ permalink raw reply

* Re: [PATCH 2/3] [NET_CLS_ACT] Use skb_act_clone
From: Patrick McHardy @ 2007-10-16 12:53 UTC (permalink / raw)
  To: hadi; +Cc: David Miller, Alexey Kuznetsov, Herbert Xu, netdev
In-Reply-To: <1192537921.4480.80.camel@localhost>

jamal wrote:

[Can't quote for some reason ..]

I don't think this change is safe since we might loose the
redirect counter and have endless redirect loops. ipip for
example calls skb_realloc_headroom, which clones the skb
when delta > 0, so it seems we really need to copy tc_verd
in skb_clone(). Generally the assumption that drivers don't
clone packets doesn't seem safe.

^ permalink raw reply

* Re: [PATCH 3/3] [NET_DOC] Document some simple rules for actions
From: Herbert Xu @ 2007-10-16 12:51 UTC (permalink / raw)
  To: jamal; +Cc: David Miller, Alexey Kuznetsov, Patrick McHardy, netdev
In-Reply-To: <1192538011.4480.84.camel@localhost>

On Tue, Oct 16, 2007 at 08:33:31AM -0400, jamal wrote:
>
> +2) If you munge any packet thou shalt call skb_expand in the case
> +someone else is referencing the skb. After that you "own" the skb. 
> +You must also tell us if it is ok to munge the packet (TC_OK2MUNGE), 
> +this way any action downstream can stomp on the packet.

Thanks for the documentation!

But I still think the distinction between skb_clone and
skb_copy_header is wrong.

When you munge a packet, it's still going to go back up
the stack and be processed along the same path.  Therefore
you should be calling pskb_expand_head.

In other words, this does not explain why skb_copy/pskb_copy
and skb_copy_expand should differ from skb_clone.  As far as
I can see all these functions should behave in the same manner
with respect to tc_verd.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply

* [PATCH 3/3] [NET_DOC] Document some simple rules for actions
From: jamal @ 2007-10-16 12:33 UTC (permalink / raw)
  To: David Miller; +Cc: Alexey Kuznetsov, Herbert Xu, Patrick McHardy, netdev

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

cheers,
jamal


[-- Attachment #2: 0003-NET_DOC-Document-some-simple-rules-for-actions --]
[-- Type: text/plain, Size: 1966 bytes --]

[NET_DOC] Document some simple rules for actions

This patch adds documentation on what is expected of an
action which branches away from the action-graph or when it
needs to trample on actins. It also describes what is expected of
users of such actions.

Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>
---
 Documentation/networking/tc-actions-env-rules.txt |   29 +++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/Documentation/networking/tc-actions-env-rules.txt b/Documentation/networking/tc-actions-env-rules.txt
new file mode 100644
index 0000000..c684f48
--- /dev/null
+++ b/Documentation/networking/tc-actions-env-rules.txt
@@ -0,0 +1,29 @@
+
+The "enviromental" rules for authors of any new tc actions are:   
+
+1) If you stealeth or borroweth any packet thou shalt be branching 
+from the righteous path and thou shalt cloneth. 
+
+For example if your action queues a packet to be processed later
+or intentionaly branches by redirecting a packet then you need to 
+clone the packet. 
+There are certain fields in the skb tc_verd that need to be reset so we
+avoid loops etc. A few are generic enough so much so that skb_act_clone() 
+resets them for you. So invoke skb_act_clone() rather than skb_clone()
+
+2) If you munge any packet thou shalt call skb_expand in the case
+someone else is referencing the skb. After that you "own" the skb. 
+You must also tell us if it is ok to munge the packet (TC_OK2MUNGE), 
+this way any action downstream can stomp on the packet.
+
+3) dropping packets you dont own is a nono. You simply return
+TC_ACT_SHOT to the caller and they will drop it.
+
+The "enviromental" rules for callers of actions (qdiscs etc) are:
+
+*) thou art responsible for freeing anything returned as being
+TC_ACT_SHOT/STOLEN/QUEUED. If none of TC_ACT_SHOT/STOLEN/QUEUED is
+returned then all is great and you dont need to do anything.
+
+Post on netdev if something is unclear.
+
-- 
1.4.4.1.gaed4


^ permalink raw reply related

* [PATCH 2/3] [NET_CLS_ACT] Use skb_act_clone
From: jamal @ 2007-10-16 12:32 UTC (permalink / raw)
  To: David Miller; +Cc: Alexey Kuznetsov, Herbert Xu, Patrick McHardy, netdev

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


cheers,
jamal

[-- Attachment #2: 0002-NET_CLS_ACT-Use-skb_act_clone --]
[-- Type: text/plain, Size: 1314 bytes --]

[NET_CLS_ACT] Use skb_act_clone
clean skb_clone of any signs of CONFIG_NET_CLS_ACT and
have mirred us skb_act_clone()

Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>
---
 net/core/skbuff.c      |    7 -------
 net/sched/act_mirred.c |    2 +-
 2 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 70d9b5d..593a8da 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -415,13 +415,6 @@ static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb)
 	n->hdr_len = skb->nohdr ? skb_headroom(skb) : skb->hdr_len;
 	n->nohdr = 0;
 	n->destructor = NULL;
-#ifdef CONFIG_NET_CLS_ACT
-	/* FIXME What is this and why don't we do it in copy_skb_header? */
-	n->tc_verd = SET_TC_VERD(n->tc_verd,0);
-	n->tc_verd = CLR_TC_OK2MUNGE(n->tc_verd);
-	n->tc_verd = CLR_TC_MUNGED(n->tc_verd);
-	C(iif);
-#endif
 	C(truesize);
 	atomic_set(&n->users, 1);
 	C(head);
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index fd7bca4..c3fde91 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -166,7 +166,7 @@ bad_mirred:
 		return TC_ACT_SHOT;
 	}
 
-	skb2 = skb_clone(skb, GFP_ATOMIC);
+	skb2 = skb_act_clone(skb, GFP_ATOMIC);
 	if (skb2 == NULL)
 		goto bad_mirred;
 	if (m->tcfm_eaction != TCA_EGRESS_MIRROR &&
-- 
1.4.4.1.gaed4


^ permalink raw reply related

* [PATCH 1/3] [NET_CLS_ACT] Introduce skb_act_clone
From: jamal @ 2007-10-16 12:29 UTC (permalink / raw)
  To: David Miller; +Cc: Alexey Kuznetsov, Herbert Xu, Patrick McHardy, netdev

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


cheers,
jamal

[-- Attachment #2: 0001-NET_CLS_ACT-Introduce-skb_act_clone --]
[-- Type: text/plain, Size: 1024 bytes --]

[NET_CLS_ACT] Introduce skb_act_clone
Reworked skb_clone looks uglier with the single ifdef CONFIG_NET_CLS_ACT
This patch introduces skb_act_clone which will replace skb_clone
in tc actions

Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>
---
 include/net/sch_generic.h |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index a02ec9e..c926551 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -316,4 +316,19 @@ static inline u32 qdisc_l2t(struct qdisc_rate_table* rtab, unsigned int pktlen)
 	return rtab->data[slot];
 }
 
+#ifdef CONFIG_NET_CLS_ACT
+static inline struct sk_buff *skb_act_clone(struct sk_buff *skb, gfp_t gfp_mask)
+{
+	struct sk_buff *n = skb_clone(skb, gfp_mask);
+
+	if (n) {
+		n->tc_verd = SET_TC_VERD(n->tc_verd, 0);
+		n->tc_verd = CLR_TC_OK2MUNGE(n->tc_verd);
+		n->tc_verd = CLR_TC_MUNGED(n->tc_verd);
+		n->iif = skb->iif;
+	}
+	return n;
+}
+#endif
+
 #endif
-- 
1.4.4.1.gaed4


^ permalink raw reply related

* [PATCHES] cleanup skb_clone
From: jamal @ 2007-10-16 12:22 UTC (permalink / raw)
  To: David Miller; +Cc: Alexey Kuznetsov, Herbert Xu, Patrick McHardy, netdev

I am going to send three patches:
1) Introduces skb_act_clone
2) Uses skb_act_clone
3) documents expect behavior of actions in case of cloning or copying
skbs.

cheers,
jamal


^ permalink raw reply

* Re: Resend: [IPROUTE2 PATCH] Interface group as new ip link option
From: jamal @ 2007-10-16 12:08 UTC (permalink / raw)
  To: panther; +Cc: netdev, netfilter-devel
In-Reply-To: <4714A4C3.10103@balabit.hu>

On Tue, 2007-16-10 at 13:47 +0200, Laszlo Attila Toth wrote:

> Hm, the main problem (for me) is that it requires more then one value 
> per net device and it should be limited. But if I say it is 5 for 
> instance, anyone can say it is not enough for him.

Yes, those pesky humans, you give them an inch and they want a mile ;->
I think 32 for example is better than 1. So if you use a u32 variable
doesnt pollute the netdevice that much - and used as a bitmap, then
setting any of the 32 bits implies the device is in that group. This of
course limits the group ids to be (1..32). Your mileage may vary.

> ip link set ppp0 group 10
> also if "ip link set..." works for multiple interfaces it is already 
> done. If it is not, it will affect positively all other settings :)

nice.
The part that is not very useful today with ifconfig or ip link ls is i
have only two choices: Either i ask for all devices or for one; i cant
ask for something in the middle. If i can group them together then i can
solve that problem sith some user space plumbing (which avoids 10K
netlink messages). 
"get ppp*" may actually be a totally separate call which also traverses
the list in the kernel and just returns the matching devices. 

cheers,
jamal


^ permalink raw reply

* NF_IP_POST_ROUTING hook question (2.4)
From: Yakov Lerner @ 2007-10-16 12:03 UTC (permalink / raw)
  To: netdev
In-Reply-To: <f36b08ee0710160328h3a45d18k41a32dcb8f6f3d4e@mail.gmail.com>

In the NF_IP_POST_ROUTING hook function, how can I
tell that packet originated locally vs was forwarded ?
I need to hook NF_IP_POST_ROUTING not NF_IP_FORWARD.

I'd expect that 'net_device *in' argument to be  not NULL for
forwarded packets.

But what I see is NULL for both forwarded  and locally sent packets
(in NF_IP_POST_ROUTING hook).
Which field of skb can  tell me that packet originated locally ?

Yakov

^ permalink raw reply

* Re: Resend: [IPROUTE2 PATCH] Interface group as new ip link option
From: Laszlo Attila Toth @ 2007-10-16 11:47 UTC (permalink / raw)
  To: hadi; +Cc: netdev, netfilter-devel
In-Reply-To: <1192534002.4480.52.camel@localhost>

jamal írta:
> On Tue, 2007-16-10 at 13:05 +0200, Laszlo Attila Toth wrote:
> 
>> That sounds great but for what whould you like to use? It may help me 
>> for the implementation.
> 
> For example i may want to add a tc rule to a group of interfaces.
> tc BNF roughly looks like:
> tc <object> <operation> <target>
> 
> Where targe is := <dev devname>
> It may be useful to extend "target" to include alternatively a group of
> devices.

Ok, I see.

> 
>> Currently it is not available since the the 
>> net_device structure holds the group id and for this get operation an 
>> iteration (of net_devices) may be necessary...
> 
> I understand - but i would rather do the iteration in the kernel than in
> user space and save myself a gazillion netlink messages.

You are right - that's faster in the kernel and useless to move it to 
userspace.

> 
>> We didn't want to use multiple groups in favour of masked group ids. 
>> What kind of usage needs other implementation?
> 
> Same lazyperson use cases as above; in one case i may want to set a rule
> to group = {eth0, eth1, eth10} and in another {eth0,eth5}. I realize
> this may be more involved something clever for an implementation (eg
> using a bitmap instead of a straight int) - so if it not a simple thing,
> just defer it to some later time.

Hm, the main problem (for me) is that it requires more then one value 
per net device and it should be limited. But if I say it is 5 for 
instance, anyone can say it is not enough for him.


> 
> Ok, now that you are asking, heres another one feature request for
> you;-> It would be nice if i can set a group based on a regular
> expression of a devices name; eg "ppp*". Use case for this, off top of
> my head (without looking at your syntax):
> 
> ip set group 10 ppp*

ip link set ppp0 group 10
also if "ip link set..." works for multiple interfaces it is already 
done. If it is not, it will affect positively all other settings :)

-- 
Attila

^ permalink raw reply

* Re: Resend: [IPROUTE2 PATCH] Interface group as new ip link option
From: jamal @ 2007-10-16 11:26 UTC (permalink / raw)
  To: panther; +Cc: netdev, netfilter-devel
In-Reply-To: <47149B04.30607@balabit.hu>

On Tue, 2007-16-10 at 13:05 +0200, Laszlo Attila Toth wrote:

> That sounds great but for what whould you like to use? It may help me 
> for the implementation.

For example i may want to add a tc rule to a group of interfaces.
tc BNF roughly looks like:
tc <object> <operation> <target>

Where targe is := <dev devname>
It may be useful to extend "target" to include alternatively a group of
devices.
The way i would see that done is to add a little wrapper to tc to first
ask the kernel for a list of interfaces and then invoke each with the
equivalent of current syntax.
Did that make sense?

> Currently it is not available since the the 
> net_device structure holds the group id and for this get operation an 
> iteration (of net_devices) may be necessary...

I understand - but i would rather do the iteration in the kernel than in
user space and save myself a gazillion netlink messages.

> We didn't want to use multiple groups in favour of masked group ids. 
> What kind of usage needs other implementation?

Same lazyperson use cases as above; in one case i may want to set a rule
to group = {eth0, eth1, eth10} and in another {eth0,eth5}. I realize
this may be more involved something clever for an implementation (eg
using a bitmap instead of a straight int) - so if it not a simple thing,
just defer it to some later time.

Ok, now that you are asking, heres another one feature request for
you;-> It would be nice if i can set a group based on a regular
expression of a devices name; eg "ppp*". Use case for this, off top of
my head (without looking at your syntax):

ip set group 10 ppp*

And equivalent to "ifconfig ppp*" so i dont list 10K interfaces
ip list group 10 or pass any 5K messages to user space

cheers,
jamal


^ permalink raw reply

* Re: Resend: [IPROUTE2 PATCH] Interface group as new ip link option
From: Laszlo Attila Toth @ 2007-10-16 11:05 UTC (permalink / raw)
  To: hadi; +Cc: netdev, netfilter-devel
In-Reply-To: <1192531516.4480.33.camel@localhost>

jamal írta:
> On Tue, 2007-16-10 at 11:03 +0200, Laszlo Attila Toth wrote:
>> Interfaces can be grouped and each group has an unique positive integer ID.
>> It can be set via ip link.
> 
> Feature request: Can you also implement a get operation. Perhaps
> something that returns from the kernel a list of ifindices when one
> passes a group to it?

That sounds great but for what whould you like to use? It may help me 
for the implementation. Currently it is not available since the the 
net_device structure holds the group id and for this get operation an 
iteration (of net_devices) may be necessary...
> 
> BTW, does it make sense to have one interface in multiple groups?

We didn't want to use multiple groups in favour of masked group ids. 
What kind of usage needs other implementation?

--
Attila

^ permalink raw reply

* Re: Resend: [IPROUTE2 PATCH] Interface group as new ip link option
From: jamal @ 2007-10-16 10:45 UTC (permalink / raw)
  To: Laszlo Attila Toth; +Cc: netdev, netfilter-devel
In-Reply-To: <a6713ba13c49b7fa20073d8abf1862480c2799e0.1192457385.git.panther@balabit.hu>

On Tue, 2007-16-10 at 11:03 +0200, Laszlo Attila Toth wrote:
> Interfaces can be grouped and each group has an unique positive integer ID.
> It can be set via ip link.

Feature request: Can you also implement a get operation. Perhaps
something that returns from the kernel a list of ifindices when one
passes a group to it?

BTW, does it make sense to have one interface in multiple groups?

cheers,
jamal


^ permalink raw reply

* Re: [PATCH] Update function names in /proc/net/tcp documentation
From: Herbert Xu @ 2007-10-16 10:34 UTC (permalink / raw)
  To: Jean Delvare; +Cc: rick.jones2, netdev
In-Reply-To: <20071016104636.7d633bad@hyperion.delvare>

On Tue, Oct 16, 2007 at 10:46:36AM +0200, Jean Delvare wrote:
>
> Thanks for the information. Now I'm worried that most people use
> netstat and not ss, while it seems that netstat doesn't have tcp_diag
> support. Would it make sense to add tcp_diag support to netstat? Or
> should we simply invite system admins to switch from netstat to ss
> (maybe by adding a netstat-like formatting option)?

Personally getting netstat to use inet_diag would seem to be
easier than retraining everybody's fingers to type ss :)

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply

* Re: Will RFC1146 (tcp alternative checksum options) be implemented in Linux tcp stack ?
From: Matti Aarnio @ 2007-10-16 10:28 UTC (permalink / raw)
  To: Yanping Du; +Cc: netdev
In-Reply-To: <887901.30138.qm@web55205.mail.re4.yahoo.com>

On Mon, Oct 15, 2007 at 06:15:11PM -0700, Yanping Du wrote:
> Hi,
> 
>   We found the standard 16-bit tcp checksum is not
> strong enough in some cases. Is there any roadmap on
> implementing RFC1146 (tcp alternative checksum
> options) in Linux tcp stack ? If yes, how soon will
> that be in ?

If you want to pay to somebody to implement it, possibly soon.
Otherwise it is up to whims of capable kernel coders, who
usually have their hands quite full.

The tcp alternate checksum option does have severe performance
hinderance issue.  It may be implemented by intermingling it
into standard tcp_checksum_and_copy routine, but in cases
where the hardware can do tcp checksum offloading, it definitely
does not improve the performance.

Your alternate approach could be to use a IPSEC VPN tunnel,
which will detect an attempt to alter data ( = transmission
error ) even when the original TCP does not detect anything.

Yet third way could be to do it in application: ssh datastream
as one example - or tunnel via SSH port forward.

Need for alternate checksums does appear to indicate that you
are using bad quality transmission system -- those are not
usually very high speed things, and then hacks like IPSEC or
SSH tunnels are excellent choices.

Nevertheless, adding checksum complexity/size on each packet
does not guarantee error detection, it merely lowers the
probability of error detection failure.  There _will_ happen
an error which wasn't detected, but will it be once a minute
or once a year, or once per billion years..

Bad quality ( = error introducing ) transmission system will
also make speedy data transmission that much more difficult,
thus my presumption that it isn't very fast link...

Anyway, we have seen data corruption on disk, over the network
etc.  Having application level MD5 checksumming of the datastream
every N kilobytes has its own attractions.  Including embedding
those checksums in the stored datastreams.


> Please kindly copy reply to my email address as I've
> not subscribed the netdev@ mailing list at present.
> 
>   http://www.faqs.org/rfcs/rfc1146.html
> 
> Thanks!
> -Yanping

/Matti Aarnio

^ 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