All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] CLUSTERIP: Multiple CLUSTERIP patches
@ 2005-08-30 12:15 KOVACS Krisztian
  2005-08-30 12:15 ` [PATCH 1/3] CLUSTERIP: fix memcpy() length typo KOVACS Krisztian
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: KOVACS Krisztian @ 2005-08-30 12:15 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Harald Welte

  Hi,

  I've created three patches for CLUSTERIP. The first two are bug fixes,
while the third one tries to make CLUSTERIP more efficient by changing the
internal representation of the nodes we're responsible for.

  I'd be especially thankful for comments on the second and third patches.
The second patch changes the management of config entries significantly
and it's a very error prone area anyway. The third patch probably needs
more discussion, I seems to work, but some of this seq_file thing is still
a mistery for me... :)

--
 Regards,
  Krisztian Kovacs

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] CLUSTERIP: fix memcpy() length typo
  2005-08-30 12:15 [PATCH 0/3] CLUSTERIP: Multiple CLUSTERIP patches KOVACS Krisztian
@ 2005-08-30 12:15 ` KOVACS Krisztian
  2005-08-30 14:05   ` Harald Welte
  2005-08-30 12:15 ` [PATCH 2/3] CLUSTERIP: introduce reference counting for entries KOVACS Krisztian
  2005-08-30 12:15 ` [PATCH 3/3] CLUSTERIP: use a bitmap to store node responsibility data KOVACS Krisztian
  2 siblings, 1 reply; 13+ messages in thread
From: KOVACS Krisztian @ 2005-08-30 12:15 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Harald Welte

[-- Attachment #1: 01-config_init_memcpy_typo.patch --]
[-- Type: text/plain, Size: 873 bytes --]

CLUSTERIP: fix memcpy() length typo

Fix a trivial typo in clusterip_config_init().


Signed-off-by: KOVACS Krisztian <hidden@balabit.hu>


Index: linux-2.6.13/net/ipv4/netfilter/ipt_CLUSTERIP.c
===================================================================
--- linux-2.6.13.orig/net/ipv4/netfilter/ipt_CLUSTERIP.c	2005-08-30 13:41:22.059915425 +0200
+++ linux-2.6.13/net/ipv4/netfilter/ipt_CLUSTERIP.c	2005-08-30 14:12:40.317423733 +0200
@@ -144,7 +144,7 @@
 	memcpy(&c->clustermac, &i->clustermac, ETH_ALEN);
 	c->num_total_nodes = i->num_total_nodes;
 	c->num_local_nodes = i->num_local_nodes;
-	memcpy(&c->local_nodes, &i->local_nodes, sizeof(&c->local_nodes));
+	memcpy(&c->local_nodes, &i->local_nodes, sizeof(c->local_nodes));
 	c->hash_mode = i->hash_mode;
 	c->hash_initval = i->hash_initval;
 	atomic_set(&c->refcount, 1);

--
 Regards,
  Krisztian Kovacs
 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 2/3] CLUSTERIP: introduce reference counting for entries
  2005-08-30 12:15 [PATCH 0/3] CLUSTERIP: Multiple CLUSTERIP patches KOVACS Krisztian
  2005-08-30 12:15 ` [PATCH 1/3] CLUSTERIP: fix memcpy() length typo KOVACS Krisztian
@ 2005-08-30 12:15 ` KOVACS Krisztian
  2005-08-30 14:07   ` Harald Welte
  2005-08-30 12:15 ` [PATCH 3/3] CLUSTERIP: use a bitmap to store node responsibility data KOVACS Krisztian
  2 siblings, 1 reply; 13+ messages in thread
From: KOVACS Krisztian @ 2005-08-30 12:15 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Harald Welte

[-- Attachment #1: 02-add_entry_refcount.patch --]
[-- Type: text/plain, Size: 4657 bytes --]

CLUSTERIP: introduce reference counting for entries

The CLUSTERIP target creates a procfs entry for all different cluster
IPs.  Although more than one rules can refer to a single cluster IP (and
thus a single config structure), removal of the procfs entry is done
unconditionally in destroy(). In more complicated situations involving
deferred dereferencing of the config structure by procfs and creating a
new rule with the same cluster IP it's also possible that no entry will
be created for the new rule.

This patch fixes the problem by counting the number of entries
referencing a given config structure and moving the config list
manipulation and procfs entry deletion parts to the
clusterip_config_entry_put() function.


Signed-off-by: KOVACS Krisztian <hidden@balabit.hu>


Index: linux-2.6.13/net/ipv4/netfilter/ipt_CLUSTERIP.c
===================================================================
--- linux-2.6.13.orig/net/ipv4/netfilter/ipt_CLUSTERIP.c	2005-08-30 14:12:40.317423733 +0200
+++ linux-2.6.13/net/ipv4/netfilter/ipt_CLUSTERIP.c	2005-08-30 14:12:43.365348289 +0200
@@ -49,6 +49,7 @@
 struct clusterip_config {
 	struct list_head list;			/* list of all configs */
 	atomic_t refcount;			/* reference count */
+	atomic_t entries;			/* number of entries referencing us */
 
 	u_int32_t clusterip;			/* the IP address */
 	u_int8_t clustermac[ETH_ALEN];		/* the MAC address */
@@ -76,23 +77,44 @@
 #endif
 
 static inline void
-clusterip_config_get(struct clusterip_config *c) {
+clusterip_config_get(struct clusterip_config *c)
+{
 	atomic_inc(&c->refcount);
 }
 
 static inline void
-clusterip_config_put(struct clusterip_config *c) {
-	if (atomic_dec_and_test(&c->refcount)) {
+clusterip_config_put(struct clusterip_config *c)
+{
+	if (atomic_dec_and_test(&c->refcount))
+		kfree(c);
+}
+
+static inline void
+clusterip_config_entry_get(struct clusterip_config *c)
+{
+	atomic_inc(&c->entries);
+}
+
+static inline void
+clusterip_config_entry_put(struct clusterip_config *c)
+{
+	if (atomic_dec_and_test(&c->entries)) {
 		write_lock_bh(&clusterip_lock);
 		list_del(&c->list);
 		write_unlock_bh(&clusterip_lock);
+
 		dev_mc_delete(c->dev, c->clustermac, ETH_ALEN, 0);
 		dev_put(c->dev);
-		kfree(c);
+
+		/* In case anyone still accesses the file, the open/close
+		 * functions are also incrementing the refcount on their own,
+		 * so it's safe to remove the entry even if it's in use. */
+#ifdef CONFIG_PROC_FS
+		remove_proc_entry(c->pde->name, c->pde->parent);
+#endif
 	}
 }
 
-
 static struct clusterip_config *
 __clusterip_config_find(u_int32_t clusterip)
 {
@@ -111,7 +133,7 @@
 }
 
 static inline struct clusterip_config *
-clusterip_config_find_get(u_int32_t clusterip)
+clusterip_config_find_get(u_int32_t clusterip, int entry)
 {
 	struct clusterip_config *c;
 
@@ -122,6 +144,8 @@
 		return NULL;
 	}
 	atomic_inc(&c->refcount);
+	if (entry)
+		atomic_inc(&c->entries);
 	read_unlock_bh(&clusterip_lock);
 
 	return c;
@@ -148,6 +172,7 @@
 	c->hash_mode = i->hash_mode;
 	c->hash_initval = i->hash_initval;
 	atomic_set(&c->refcount, 1);
+	atomic_set(&c->entries, 1);
 
 #ifdef CONFIG_PROC_FS
 	/* create proc dir entry */
@@ -415,7 +440,14 @@
 
 	/* FIXME: further sanity checks */
 
-	config = clusterip_config_find_get(e->ip.dst.s_addr);
+	/* If this is not a completely new entry and already has a config
+	 * attached, simply increase the entry refcount and return */
+	if (cipinfo->config != NULL) {
+		clusterip_config_entry_get(cipinfo->config);
+		return 1;
+	}
+
+	config = clusterip_config_find_get(e->ip.dst.s_addr, 1);
 	if (!config) {
 		if (!(cipinfo->flags & CLUSTERIP_FLAG_NEW)) {
 			printk(KERN_WARNING "CLUSTERIP: no config found for %u.%u.%u.%u, need 'new'\n", NIPQUAD(e->ip.dst.s_addr));
@@ -455,13 +487,10 @@
 {
 	struct ipt_clusterip_tgt_info *cipinfo = matchinfo;
 
-	/* we first remove the proc entry and then drop the reference
-	 * count.  In case anyone still accesses the file, the open/close
-	 * functions are also incrementing the refcount on their own */
-#ifdef CONFIG_PROC_FS
-	remove_proc_entry(cipinfo->config->pde->name,
-			  cipinfo->config->pde->parent);
-#endif
+	/* if no more entries are referencing the config, remove it
+	 * from the list and destroy the proc entry */
+	clusterip_config_entry_put(cipinfo->config);
+
 	clusterip_config_put(cipinfo->config);
 }
 
@@ -533,7 +562,7 @@
 
 	/* if there is no clusterip configuration for the arp reply's 
 	 * source ip, we don't want to mangle it */
-	c = clusterip_config_find_get(payload->src_ip);
+	c = clusterip_config_find_get(payload->src_ip, 0);
 	if (!c)
 		return NF_ACCEPT;
 

--
 Regards,
  Krisztian Kovacs
 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 3/3] CLUSTERIP: use a bitmap to store node responsibility data
  2005-08-30 12:15 [PATCH 0/3] CLUSTERIP: Multiple CLUSTERIP patches KOVACS Krisztian
  2005-08-30 12:15 ` [PATCH 1/3] CLUSTERIP: fix memcpy() length typo KOVACS Krisztian
  2005-08-30 12:15 ` [PATCH 2/3] CLUSTERIP: introduce reference counting for entries KOVACS Krisztian
@ 2005-08-30 12:15 ` KOVACS Krisztian
  2005-08-30 13:29   ` Pablo Neira
  2 siblings, 1 reply; 13+ messages in thread
From: KOVACS Krisztian @ 2005-08-30 12:15 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Harald Welte

[-- Attachment #1: 03-responsibility_bitmap.patch --]
[-- Type: text/plain, Size: 7395 bytes --]

CLUSTERIP: use a bitmap to store node responsibility data

Instead of maintaining an array containing a list of nodes this instance
is responsible for let's use a simple bitmap. This provides the
following features:

  * clusterip_responsible() and the add_node()/delete_node() operations
    become very simple and don't need locking
  * the config structure is much smaller

In spite of the completely different internal data representation the
user-space interface remains almost unchanged; the only difference is
that the proc file does not list nodes in the order they were added.
(The target info structure remains the same.)


Signed-off-by: KOVACS Krisztian <hidden@balabit.hu>


Index: linux-2.6.13/net/ipv4/netfilter/ipt_CLUSTERIP.c
===================================================================
--- linux-2.6.13.orig/net/ipv4/netfilter/ipt_CLUSTERIP.c	2005-08-30 14:12:43.365348289 +0200
+++ linux-2.6.13/net/ipv4/netfilter/ipt_CLUSTERIP.c	2005-08-30 14:12:46.077281161 +0200
@@ -13,6 +13,7 @@
 #include <linux/config.h>
 #include <linux/proc_fs.h>
 #include <linux/jhash.h>
+#include <linux/bitops.h>
 #include <linux/skbuff.h>
 #include <linux/ip.h>
 #include <linux/tcp.h>
@@ -30,7 +31,7 @@
 #include <linux/netfilter_ipv4/ipt_CLUSTERIP.h>
 #include <linux/netfilter_ipv4/ip_conntrack.h>
 
-#define CLUSTERIP_VERSION "0.7"
+#define CLUSTERIP_VERSION "0.8"
 
 #define DEBUG_CLUSTERIP
 
@@ -55,8 +56,7 @@
 	u_int8_t clustermac[ETH_ALEN];		/* the MAC address */
 	struct net_device *dev;			/* device */
 	u_int16_t num_total_nodes;		/* total number of nodes */
-	u_int16_t num_local_nodes;		/* number of local nodes */
-	u_int16_t local_nodes[CLUSTERIP_MAX_NODES];	/* node number array */
+	unsigned long local_nodes;		/* node number array */
 
 #ifdef CONFIG_PROC_FS
 	struct proc_dir_entry *pde;		/* proc dir entry */
@@ -67,8 +67,7 @@
 
 static LIST_HEAD(clusterip_configs);
 
-/* clusterip_lock protects the clusterip_configs list _AND_ the configurable
- * data within all structurses (num_local_nodes, local_nodes[]) */
+/* clusterip_lock protects the clusterip_configs list */
 static DEFINE_RWLOCK(clusterip_lock);
 
 #ifdef CONFIG_PROC_FS
@@ -151,6 +150,17 @@
 	return c;
 }
 
+static void
+clusterip_config_init_nodelist(struct clusterip_config *c,
+			       const struct ipt_clusterip_tgt_info *i)
+{
+	int n;
+
+	for (n = 0; n < i->num_local_nodes; n++) {
+		set_bit(i->local_nodes[n] - 1, &c->local_nodes);
+	}
+}
+
 static struct clusterip_config *
 clusterip_config_init(struct ipt_clusterip_tgt_info *i, u_int32_t ip,
 			struct net_device *dev)
@@ -167,8 +177,7 @@
 	c->clusterip = ip;
 	memcpy(&c->clustermac, &i->clustermac, ETH_ALEN);
 	c->num_total_nodes = i->num_total_nodes;
-	c->num_local_nodes = i->num_local_nodes;
-	memcpy(&c->local_nodes, &i->local_nodes, sizeof(c->local_nodes));
+	clusterip_config_init_nodelist(c, i);
 	c->hash_mode = i->hash_mode;
 	c->hash_initval = i->hash_initval;
 	atomic_set(&c->refcount, 1);
@@ -196,53 +205,28 @@
 static int
 clusterip_add_node(struct clusterip_config *c, u_int16_t nodenum)
 {
-	int i;
-
-	write_lock_bh(&clusterip_lock);
 
-	if (c->num_local_nodes >= CLUSTERIP_MAX_NODES
-	    || nodenum > CLUSTERIP_MAX_NODES) {
-		write_unlock_bh(&clusterip_lock);
+	if (nodenum == 0 ||
+	    nodenum > c->num_total_nodes)
 		return 1;
-	}
-
-	/* check if we alrady have this number in our array */
-	for (i = 0; i < c->num_local_nodes; i++) {
-		if (c->local_nodes[i] == nodenum) {
-			write_unlock_bh(&clusterip_lock);
-			return 1;
-		}
-	}
 
-	c->local_nodes[c->num_local_nodes++] = nodenum;
+	/* check if we already have this number in our bitfield */
+	if (test_and_set_bit(nodenum - 1, &c->local_nodes))
+		return 1;
 
-	write_unlock_bh(&clusterip_lock);
 	return 0;
 }
 
 static int
 clusterip_del_node(struct clusterip_config *c, u_int16_t nodenum)
 {
-	int i;
-
-	write_lock_bh(&clusterip_lock);
-
-	if (c->num_local_nodes <= 1 || nodenum > CLUSTERIP_MAX_NODES) {
-		write_unlock_bh(&clusterip_lock);
+	if (nodenum == 0 ||
+	    nodenum > c->num_total_nodes)
 		return 1;
-	}
 		
-	for (i = 0; i < c->num_local_nodes; i++) {
-		if (c->local_nodes[i] == nodenum) {
-			int size = sizeof(u_int16_t)*(c->num_local_nodes-(i+1));
-			memmove(&c->local_nodes[i], &c->local_nodes[i+1], size);
-			c->num_local_nodes--;
-			write_unlock_bh(&clusterip_lock);
-			return 0;
-		}
-	}
+	if (test_and_clear_bit(nodenum - 1, &c->local_nodes))
+		return 0;
 
-	write_unlock_bh(&clusterip_lock);
 	return 1;
 }
 
@@ -310,25 +294,7 @@
 static inline int
 clusterip_responsible(struct clusterip_config *config, u_int32_t hash)
 {
-	int i;
-
-	read_lock_bh(&clusterip_lock);
-
-	if (config->num_local_nodes == 0) {
-		read_unlock_bh(&clusterip_lock);
-		return 0;
-	}
-
-	for (i = 0; i < config->num_local_nodes; i++) {
-		if (config->local_nodes[i] == hash) {
-			read_unlock_bh(&clusterip_lock);
-			return 1;
-		}
-	}
-
-	read_unlock_bh(&clusterip_lock);
-
-	return 0;
+	return test_bit(hash - 1, &config->local_nodes);
 }
 
 /*********************************************************************** 
@@ -603,56 +569,69 @@
 
 #ifdef CONFIG_PROC_FS
 
+struct clusterip_seq_position {
+	unsigned int pos;	/* position */
+	unsigned int weight;	/* number of bits set == size */
+	unsigned int bit;	/* current bit */
+	unsigned long val;	/* current value */
+};
+
 static void *clusterip_seq_start(struct seq_file *s, loff_t *pos)
 {
 	struct proc_dir_entry *pde = s->private;
 	struct clusterip_config *c = pde->data;
-	unsigned int *nodeidx;
-
-	read_lock_bh(&clusterip_lock);
-	if (*pos >= c->num_local_nodes)
+	unsigned int weight;
+	u_int32_t local_nodes;
+	struct clusterip_seq_position *idx;
+
+	/* FIXME: possible race */
+	local_nodes = c->local_nodes;
+	weight = hweight32(local_nodes);
+	if (*pos >= weight)
 		return NULL;
 
-	nodeidx = kmalloc(sizeof(unsigned int), GFP_KERNEL);
-	if (!nodeidx)
+	idx = kmalloc(sizeof(struct clusterip_seq_position), GFP_KERNEL);
+	if (!idx)
 		return ERR_PTR(-ENOMEM);
 
-	*nodeidx = *pos;
-	return nodeidx;
+	idx->pos = *pos;
+	idx->weight = weight;
+	idx->bit = ffs(local_nodes);
+	idx->val = local_nodes;
+	clear_bit(idx->bit - 1, &idx->val);
+
+	return idx;
 }
 
 static void *clusterip_seq_next(struct seq_file *s, void *v, loff_t *pos)
 {
-	struct proc_dir_entry *pde = s->private;
-	struct clusterip_config *c = pde->data;
-	unsigned int *nodeidx = (unsigned int *)v;
+	struct clusterip_seq_position *idx = (struct clusterip_seq_position *)v;
 
-	*pos = ++(*nodeidx);
-	if (*pos >= c->num_local_nodes) {
+	*pos = ++idx->pos;
+	if (*pos >= idx->weight) {
 		kfree(v);
 		return NULL;
 	}
-	return nodeidx;
+	idx->bit = ffs(idx->val);
+	clear_bit(idx->bit - 1, &idx->val);
+	return idx;
 }
 
 static void clusterip_seq_stop(struct seq_file *s, void *v)
 {
 	kfree(v);
-
-	read_unlock_bh(&clusterip_lock);
 }
 
 static int clusterip_seq_show(struct seq_file *s, void *v)
 {
-	struct proc_dir_entry *pde = s->private;
-	struct clusterip_config *c = pde->data;
-	unsigned int *nodeidx = (unsigned int *)v;
+	struct clusterip_seq_position *idx = (struct clusterip_seq_position *)v;
 
-	if (*nodeidx != 0) 
+	if (idx->pos != 0) 
 		seq_putc(s, ',');
-	seq_printf(s, "%u", c->local_nodes[*nodeidx]);
 
-	if (*nodeidx == c->num_local_nodes-1)
+	seq_printf(s, "%u", idx->bit);
+
+	if (idx->pos == idx->weight - 1)
 		seq_putc(s, '\n');
 
 	return 0;

--
 Regards,
  Krisztian Kovacs
 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] CLUSTERIP: use a bitmap to store node responsibility data
  2005-08-30 12:15 ` [PATCH 3/3] CLUSTERIP: use a bitmap to store node responsibility data KOVACS Krisztian
@ 2005-08-30 13:29   ` Pablo Neira
  2005-08-30 13:34     ` KOVACS Krisztian
  0 siblings, 1 reply; 13+ messages in thread
From: Pablo Neira @ 2005-08-30 13:29 UTC (permalink / raw)
  To: KOVACS Krisztian; +Cc: Harald Welte, netfilter-devel

Hi Krisztian,

KOVACS Krisztian wrote:
> @@ -55,8 +56,7 @@
>  	u_int8_t clustermac[ETH_ALEN];		/* the MAC address */
>  	struct net_device *dev;			/* device */
>  	u_int16_t num_total_nodes;		/* total number of nodes */
> -	u_int16_t num_local_nodes;		/* number of local nodes */
> -	u_int16_t local_nodes[CLUSTERIP_MAX_NODES];	/* node number array */
> +	unsigned long local_nodes;		/* node number array */

I'm afraid that this breaks backward compatibility. Although CLUSTERIP 
is marked as experimental.

@Harald: Should we respect backward compatibility in this case?

--
Pablo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] CLUSTERIP: use a bitmap to store node responsibility data
  2005-08-30 13:29   ` Pablo Neira
@ 2005-08-30 13:34     ` KOVACS Krisztian
  2005-08-30 15:18       ` Pablo Neira
  0 siblings, 1 reply; 13+ messages in thread
From: KOVACS Krisztian @ 2005-08-30 13:34 UTC (permalink / raw)
  To: Pablo Neira; +Cc: Harald Welte, netfilter-devel


  Hi,

On Tuesday 30 August 2005 15.29, Pablo Neira wrote:
> KOVACS Krisztian wrote:
> > @@ -55,8 +56,7 @@
> >  	u_int8_t clustermac[ETH_ALEN];		/* the MAC address */
> >  	struct net_device *dev;			/* device */
> >  	u_int16_t num_total_nodes;		/* total number of nodes */
> > -	u_int16_t num_local_nodes;		/* number of local nodes */
> > -	u_int16_t local_nodes[CLUSTERIP_MAX_NODES];	/* node number array
> > */ +	unsigned long local_nodes;		/* node number array */
>
> I'm afraid that this breaks backward compatibility. Although
> CLUSTERIP is marked as experimental.

  IMHO this does not break compatibility, since this change only affects 
the internal clusterip_config structure, not the target info structure 
itself. (And the module does conversion in checkentry().)

-- 
 Regards,
  Krisztian Kovacs

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] CLUSTERIP: fix memcpy() length typo
  2005-08-30 12:15 ` [PATCH 1/3] CLUSTERIP: fix memcpy() length typo KOVACS Krisztian
@ 2005-08-30 14:05   ` Harald Welte
  0 siblings, 0 replies; 13+ messages in thread
From: Harald Welte @ 2005-08-30 14:05 UTC (permalink / raw)
  To: KOVACS Krisztian; +Cc: netfilter-devel

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

On Tue, Aug 30, 2005 at 02:15:37PM +0200, KOVACS Krisztian wrote:
> CLUSTERIP: fix memcpy() length typo
> 
> Fix a trivial typo in clusterip_config_init().

thanks, I wonder how often I do this.  It's one of my age-old number 1
programming errors... quite embarrassing :(

I've applied it to netfilte-2.6.14 and will submit it to DaveM soon.

-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] CLUSTERIP: introduce reference counting for entries
  2005-08-30 12:15 ` [PATCH 2/3] CLUSTERIP: introduce reference counting for entries KOVACS Krisztian
@ 2005-08-30 14:07   ` Harald Welte
  2005-08-30 15:26     ` KOVACS Krisztian
  0 siblings, 1 reply; 13+ messages in thread
From: Harald Welte @ 2005-08-30 14:07 UTC (permalink / raw)
  To: KOVACS Krisztian; +Cc: netfilter-devel

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

On Tue, Aug 30, 2005 at 02:15:38PM +0200, KOVACS Krisztian wrote:
> CLUSTERIP: introduce reference counting for entries

Thanks for your patches 2 and 3.  I want to review them in more detail
and need to re-read the CLUSTERIP code first.  Expect some feedback in
the next two days.

-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] CLUSTERIP: use a bitmap to store node responsibility data
  2005-08-30 13:34     ` KOVACS Krisztian
@ 2005-08-30 15:18       ` Pablo Neira
  0 siblings, 0 replies; 13+ messages in thread
From: Pablo Neira @ 2005-08-30 15:18 UTC (permalink / raw)
  To: KOVACS Krisztian; +Cc: Harald Welte, netfilter-devel

KOVACS Krisztian wrote:
> On Tuesday 30 August 2005 15.29, Pablo Neira wrote:
> 
>>KOVACS Krisztian wrote:
>>
>>>@@ -55,8 +56,7 @@
>>> 	u_int8_t clustermac[ETH_ALEN];		/* the MAC address */
>>> 	struct net_device *dev;			/* device */
>>> 	u_int16_t num_total_nodes;		/* total number of nodes */
>>>-	u_int16_t num_local_nodes;		/* number of local nodes */
>>>-	u_int16_t local_nodes[CLUSTERIP_MAX_NODES];	/* node number array
>>>*/ +	unsigned long local_nodes;		/* node number array */
>>
>>I'm afraid that this breaks backward compatibility. Although
>>CLUSTERIP is marked as experimental.
> 
> 
>   IMHO this does not break compatibility, since this change only affects 
> the internal clusterip_config structure, not the target info structure 
> itself. (And the module does conversion in checkentry().)

You're right, sorry for the noise.

--
Pablo

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] CLUSTERIP: introduce reference counting for entries
  2005-08-30 14:07   ` Harald Welte
@ 2005-08-30 15:26     ` KOVACS Krisztian
  2005-09-13 12:54       ` Harald Welte
  0 siblings, 1 reply; 13+ messages in thread
From: KOVACS Krisztian @ 2005-08-30 15:26 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Harald Welte


  Hi,

On Tuesday 30 August 2005 16.07, Harald Welte wrote:
> Thanks for your patches 2 and 3.  I want to review them in more
> detail and need to re-read the CLUSTERIP code first.  Expect some
> feedback in the next two days.

  OK, thanks. BTW, I've just found a serious bug in patch 2, I knew I
was missing something... So please apply this on top of patch 2.

Index: linux-2.6.13/net/ipv4/netfilter/ipt_CLUSTERIP.c
===================================================================
--- linux-2.6.13.orig/net/ipv4/netfilter/ipt_CLUSTERIP.c	2005-08-30 
16:59:07.702210333 +0200
+++ linux-2.6.13/net/ipv4/netfilter/ipt_CLUSTERIP.c	2005-08-30 
16:50:27.431088379 +0200
@@ -444,6 +444,7 @@
 	 * attached, simply increase the entry refcount and return */
 	if (cipinfo->config != NULL) {
 		clusterip_config_entry_get(cipinfo->config);
+                clusterip_config_get(cipinfo->config);
 		return 1;
 	}

-- 
 Regards,
  Krisztian Kovacs

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] CLUSTERIP: introduce reference counting for entries
  2005-08-30 15:26     ` KOVACS Krisztian
@ 2005-09-13 12:54       ` Harald Welte
  2005-09-13 15:16         ` KOVACS Krisztian
  0 siblings, 1 reply; 13+ messages in thread
From: Harald Welte @ 2005-09-13 12:54 UTC (permalink / raw)
  To: KOVACS Krisztian; +Cc: netfilter-devel

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

On Tue, Aug 30, 2005 at 05:26:07PM +0200, KOVACS Krisztian wrote:
> 
>   Hi,
> 
> On Tuesday 30 August 2005 16.07, Harald Welte wrote:
> > Thanks for your patches 2 and 3.  I want to review them in more
> > detail and need to re-read the CLUSTERIP code first.  Expect some
> > feedback in the next two days.
> 
>   OK, thanks. BTW, I've just found a serious bug in patch 2, I knew I
> was missing something... So please apply this on top of patch 2.
 
as usual, the two days got a bit long.  But now I've reviewed the old
code and your changes.  I've applied them to my git tree with some minor
modifications/clarifications.

-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] CLUSTERIP: introduce reference counting for entries
  2005-09-13 12:54       ` Harald Welte
@ 2005-09-13 15:16         ` KOVACS Krisztian
  2005-09-23 14:00           ` Harald Welte
  0 siblings, 1 reply; 13+ messages in thread
From: KOVACS Krisztian @ 2005-09-13 15:16 UTC (permalink / raw)
  To: Harald Welte, netfilter-devel


  Hi,

On Tuesday 13 September 2005 14.54, Harald Welte wrote:
> On Tue, Aug 30, 2005 at 05:26:07PM +0200, KOVACS Krisztian wrote:
> > On Tuesday 30 August 2005 16.07, Harald Welte wrote:
> > > Thanks for your patches 2 and 3.  I want to review them in more
> > > detail and need to re-read the CLUSTERIP code first.  Expect some
> > > feedback in the next two days.
> >
> >   OK, thanks. BTW, I've just found a serious bug in patch 2, I knew
> > I was missing something... So please apply this on top of patch 2.
>
> as usual, the two days got a bit long.  But now I've reviewed the old
> code and your changes.  I've applied them to my git tree with some
> minor modifications/clarifications.

  Thanks a lot. One more thing: what's up with the updated CLUSTERIP 
userspace?
 
http://lists.netfilter.org/pipermail/netfilter-devel/2005-August/021165.html

-- 
 Regards,
  Krisztian Kovacs

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] CLUSTERIP: introduce reference counting for entries
  2005-09-13 15:16         ` KOVACS Krisztian
@ 2005-09-23 14:00           ` Harald Welte
  0 siblings, 0 replies; 13+ messages in thread
From: Harald Welte @ 2005-09-23 14:00 UTC (permalink / raw)
  To: KOVACS Krisztian; +Cc: netfilter-devel

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

On Tue, Sep 13, 2005 at 05:16:57PM +0200, KOVACS Krisztian wrote:
> > as usual, the two days got a bit long.  But now I've reviewed the old
> > code and your changes.  I've applied them to my git tree with some
> > minor modifications/clarifications.
> 
>   Thanks a lot. One more thing: what's up with the updated CLUSTERIP 
> userspace?

submitted meanwhile.

-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2005-09-23 14:00 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-08-30 12:15 [PATCH 0/3] CLUSTERIP: Multiple CLUSTERIP patches KOVACS Krisztian
2005-08-30 12:15 ` [PATCH 1/3] CLUSTERIP: fix memcpy() length typo KOVACS Krisztian
2005-08-30 14:05   ` Harald Welte
2005-08-30 12:15 ` [PATCH 2/3] CLUSTERIP: introduce reference counting for entries KOVACS Krisztian
2005-08-30 14:07   ` Harald Welte
2005-08-30 15:26     ` KOVACS Krisztian
2005-09-13 12:54       ` Harald Welte
2005-09-13 15:16         ` KOVACS Krisztian
2005-09-23 14:00           ` Harald Welte
2005-08-30 12:15 ` [PATCH 3/3] CLUSTERIP: use a bitmap to store node responsibility data KOVACS Krisztian
2005-08-30 13:29   ` Pablo Neira
2005-08-30 13:34     ` KOVACS Krisztian
2005-08-30 15:18       ` Pablo Neira

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.