* Re: [PATCH, netfilter] NUMA aware ipv4/netfilter/ip_tables.c
From: Andi Kleen @ 2005-09-20 16:30 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, netfilter-devel
In-Reply-To: <432FDAC5.3040801@cosmosbay.com>
On Tuesday 20 September 2005 11:47, Eric Dumazet wrote:
> +#ifdef CONFIG_NUMA
> + struct mempolicy *oldpol;
> + struct mempolicy prefnodepolicy = {
> + .refcnt = ATOMIC_INIT(1),
> + .policy = MPOL_PREFERRED,
> + .v.preferred_node = cpu_to_node(cpu),
> + };
> + oldpol = current->mempolicy;
> + current->mempolicy = &prefnodepolicy;
> +#endif
I would prefer if random code didn't mess with mempolicy internals
like this. Better just call sys_set_mempolicy()
-Andi
^ permalink raw reply
* Re: [PATCH, netfilter] NUMA aware ipv4/netfilter/ip_tables.c
From: Eric Dumazet @ 2005-09-20 17:02 UTC (permalink / raw)
To: Andi Kleen; +Cc: netdev, netfilter-devel
In-Reply-To: <200509201830.20689.ak@suse.de>
[-- Attachment #1: Type: text/plain, Size: 1845 bytes --]
Andi Kleen a écrit :
> On Tuesday 20 September 2005 11:47, Eric Dumazet wrote:
>
>>+#ifdef CONFIG_NUMA
>>+ struct mempolicy *oldpol;
>>+ struct mempolicy prefnodepolicy = {
>>+ .refcnt = ATOMIC_INIT(1),
>>+ .policy = MPOL_PREFERRED,
>>+ .v.preferred_node = cpu_to_node(cpu),
>>+ };
>>+ oldpol = current->mempolicy;
>>+ current->mempolicy = &prefnodepolicy;
>>+#endif
>
>
> I would prefer if random code didn't mess with mempolicy internals
> like this. Better just call sys_set_mempolicy()
>
> -Andi
>
>
Thank you Andi
Is this new patch OK for you ?
Hi all
Part of the performance problem we have with netfilter is memory allocation is
not NUMA aware, but 'only' SMP aware (ie each CPU normally touch separate
cache lines)
Even with small iptables rules, the cost of this misplacement can be high on
common workloads.
Instead of using one vmalloc() area (located in the node of the iptables
process), we now vmalloc() an area for each possible CPU, using NUMA policy
(MPOL_PREFERRED) so that memory should be allocated in the CPU's node if possible.
If the size of ipt_table is small enough (less than one page), we use
kmalloc_node() instead of vmalloc(), to use less memory (and less TLB entries)
in small setups.
This patch try to use local node memory in expensive translate_table()
function (and others), but doesnt bother to bind the task to the current CPU.
Note : I also optimize get_counters(), using a SET_COUNTER() for the first
cpu, avoiding a memset() and ADD_COUNTER() if SMP on other cpus.
Thank you
Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
[-- Attachment #2: patch_ip_tables_numa --]
[-- Type: text/plain, Size: 13390 bytes --]
--- linux-2.6.14-rc1.p1/net/ipv4/netfilter/ip_tables.c 2005-09-19 19:56:12.000000000 +0200
+++ linux-2.6.14-rc1/net/ipv4/netfilter/ip_tables.c 2005-09-20 20:56:19.000000000 +0200
@@ -17,6 +17,7 @@
#include <linux/skbuff.h>
#include <linux/kmod.h>
#include <linux/vmalloc.h>
+#include <linux/mempolicy.h>
#include <linux/netdevice.h>
#include <linux/module.h>
#include <linux/tcp.h>
@@ -82,11 +83,6 @@
context stops packets coming through and allows user context to read
the counters or update the rules.
- To be cache friendly on SMP, we arrange them like so:
- [ n-entries ]
- ... cache-align padding ...
- [ n-entries ]
-
Hence the start of any table is given by get_table() below. */
/* The table itself */
@@ -104,19 +100,14 @@
unsigned int underflow[NF_IP_NUMHOOKS];
/* ipt_entry tables: one per CPU */
- char entries[0] ____cacheline_aligned;
+ void *entries[NR_CPUS];
};
static LIST_HEAD(ipt_target);
static LIST_HEAD(ipt_match);
static LIST_HEAD(ipt_tables);
#define ADD_COUNTER(c,b,p) do { (c).bcnt += (b); (c).pcnt += (p); } while(0)
-
-#ifdef CONFIG_SMP
-#define TABLE_OFFSET(t,p) (SMP_ALIGN((t)->size)*(p))
-#else
-#define TABLE_OFFSET(t,p) 0
-#endif
+#define SET_COUNTER(c,b,p) do { (c).bcnt = (b); (c).pcnt = (p); } while(0)
#if 0
#define down(x) do { printk("DOWN:%u:" #x "\n", __LINE__); down(x); } while(0)
@@ -289,8 +280,7 @@
read_lock_bh(&table->lock);
IP_NF_ASSERT(table->valid_hooks & (1 << hook));
- table_base = (void *)table->private->entries
- + TABLE_OFFSET(table->private, smp_processor_id());
+ table_base = (void *)table->private->entries[smp_processor_id()];
e = get_entry(table_base, table->private->hook_entry[hook]);
#ifdef CONFIG_NETFILTER_DEBUG
@@ -562,7 +552,7 @@
/* Figures out from what hook each rule can be called: returns 0 if
there are loops. Puts hook bitmask in comefrom. */
static int
-mark_source_chains(struct ipt_table_info *newinfo, unsigned int valid_hooks)
+mark_source_chains(struct ipt_table_info *newinfo, unsigned int valid_hooks, void *entry0)
{
unsigned int hook;
@@ -571,7 +561,7 @@
for (hook = 0; hook < NF_IP_NUMHOOKS; hook++) {
unsigned int pos = newinfo->hook_entry[hook];
struct ipt_entry *e
- = (struct ipt_entry *)(newinfo->entries + pos);
+ = (struct ipt_entry *)(entry0 + pos);
if (!(valid_hooks & (1 << hook)))
continue;
@@ -621,13 +611,13 @@
goto next;
e = (struct ipt_entry *)
- (newinfo->entries + pos);
+ (entry0 + pos);
} while (oldpos == pos + e->next_offset);
/* Move along one */
size = e->next_offset;
e = (struct ipt_entry *)
- (newinfo->entries + pos + size);
+ (entry0 + pos + size);
e->counters.pcnt = pos;
pos += size;
} else {
@@ -644,7 +634,7 @@
newpos = pos + e->next_offset;
}
e = (struct ipt_entry *)
- (newinfo->entries + newpos);
+ (entry0 + newpos);
e->counters.pcnt = pos;
pos = newpos;
}
@@ -854,6 +844,7 @@
translate_table(const char *name,
unsigned int valid_hooks,
struct ipt_table_info *newinfo,
+ void *entry0,
unsigned int size,
unsigned int number,
const unsigned int *hook_entries,
@@ -874,11 +865,11 @@
duprintf("translate_table: size %u\n", newinfo->size);
i = 0;
/* Walk through entries, checking offsets. */
- ret = IPT_ENTRY_ITERATE(newinfo->entries, newinfo->size,
+ ret = IPT_ENTRY_ITERATE(entry0, newinfo->size,
check_entry_size_and_hooks,
newinfo,
- newinfo->entries,
- newinfo->entries + size,
+ entry0,
+ entry0 + size,
hook_entries, underflows, &i);
if (ret != 0)
return ret;
@@ -906,25 +897,24 @@
}
}
- if (!mark_source_chains(newinfo, valid_hooks))
+ if (!mark_source_chains(newinfo, valid_hooks, entry0))
return -ELOOP;
/* Finally, each sanity check must pass */
i = 0;
- ret = IPT_ENTRY_ITERATE(newinfo->entries, newinfo->size,
+ ret = IPT_ENTRY_ITERATE(entry0, newinfo->size,
check_entry, name, size, &i);
if (ret != 0) {
- IPT_ENTRY_ITERATE(newinfo->entries, newinfo->size,
+ IPT_ENTRY_ITERATE(entry0, newinfo->size,
cleanup_entry, &i);
return ret;
}
/* And one copy for every other CPU */
- for (i = 1; i < num_possible_cpus(); i++) {
- memcpy(newinfo->entries + SMP_ALIGN(newinfo->size)*i,
- newinfo->entries,
- SMP_ALIGN(newinfo->size));
+ for_each_cpu(i) {
+ if (newinfo->entries[i] && newinfo->entries[i] != entry0)
+ memcpy(newinfo->entries[i], entry0, newinfo->size);
}
return ret;
@@ -940,15 +930,12 @@
#ifdef CONFIG_NETFILTER_DEBUG
{
- struct ipt_entry *table_base;
- unsigned int i;
+ int cpu;
- for (i = 0; i < num_possible_cpus(); i++) {
- table_base =
- (void *)newinfo->entries
- + TABLE_OFFSET(newinfo, i);
-
- table_base->comefrom = 0xdead57ac;
+ for_each_cpu(cpu) {
+ struct ipt_entry *table_base = newinfo->entries[cpu];
+ if (table_base)
+ table_base->comefrom = 0xdead57ac;
}
}
#endif
@@ -972,6 +959,7 @@
}
/* Gets counters. */
+#ifdef CONFIG_SMP
static inline int
add_entry_to_counter(const struct ipt_entry *e,
struct ipt_counters total[],
@@ -982,22 +970,44 @@
(*i)++;
return 0;
}
+#endif
+static inline int
+set_entry_to_counter(const struct ipt_entry *e,
+ struct ipt_counters total[],
+ unsigned int *i)
+{
+ SET_COUNTER(total[*i], e->counters.bcnt, e->counters.pcnt);
+
+ (*i)++;
+ return 0;
+}
static void
get_counters(const struct ipt_table_info *t,
struct ipt_counters counters[])
{
unsigned int cpu;
+ unsigned int curcpu = raw_smp_processor_id();
unsigned int i;
- for (cpu = 0; cpu < num_possible_cpus(); cpu++) {
+ i = 0;
+ IPT_ENTRY_ITERATE(t->entries[curcpu],
+ t->size,
+ set_entry_to_counter,
+ counters,
+ &i);
+#ifdef CONFIG_SMP
+ for_each_cpu(cpu) {
+ if (cpu == curcpu)
+ continue;
i = 0;
- IPT_ENTRY_ITERATE(t->entries + TABLE_OFFSET(t, cpu),
+ IPT_ENTRY_ITERATE(t->entries[cpu],
t->size,
add_entry_to_counter,
counters,
&i);
}
+#endif
}
static int
@@ -1009,6 +1019,7 @@
struct ipt_entry *e;
struct ipt_counters *counters;
int ret = 0;
+ void *loc_cpu_entry;
/* We need atomic snapshot of counters: rest doesn't change
(other than comefrom, which userspace doesn't care
@@ -1020,13 +1031,17 @@
return -ENOMEM;
/* First, sum counters... */
- memset(counters, 0, countersize);
write_lock_bh(&table->lock);
get_counters(table->private, counters);
write_unlock_bh(&table->lock);
- /* ... then copy entire thing from CPU 0... */
- if (copy_to_user(userptr, table->private->entries, total_size) != 0) {
+ /*
+ * choose the copy that is on our node,
+ * but dont worry if the scheduler changes the CPU
+ */
+ loc_cpu_entry = table->private->entries[raw_smp_processor_id()];
+ /* ... then copy entire thing ... */
+ if (copy_to_user(userptr, loc_cpu_entry, total_size) != 0) {
ret = -EFAULT;
goto free_counters;
}
@@ -1038,7 +1053,7 @@
struct ipt_entry_match *m;
struct ipt_entry_target *t;
- e = (struct ipt_entry *)(table->private->entries + off);
+ e = (struct ipt_entry *)(loc_cpu_entry + off);
if (copy_to_user(userptr + off
+ offsetof(struct ipt_entry, counters),
&counters[num],
@@ -1107,6 +1122,60 @@
return ret;
}
+static void free_table_info(struct ipt_table_info *info)
+{
+ int cpu;
+ for_each_cpu(cpu) {
+ if (info->size <= PAGE_SIZE)
+ kfree(info->entries[cpu]);
+ else
+ vfree(info->entries[cpu]);
+ }
+ kfree(info);
+}
+
+static struct ipt_table_info *alloc_table_info(unsigned int size)
+{
+struct ipt_table_info *newinfo;
+int cpu;
+ newinfo = kzalloc(sizeof(struct ipt_table_info), GFP_KERNEL);
+ if (!newinfo)
+ return NULL;
+ newinfo->size = size;
+ for_each_cpu(cpu) {
+ if (size <= PAGE_SIZE) {
+ newinfo->entries[cpu] = kmalloc_node(size,
+ GFP_KERNEL,
+ cpu_to_node(cpu));
+ } else {
+#ifdef CONFIG_NUMA
+ struct mempolicy *oldpol;
+ mm_segment_t oldfs = get_fs();
+ DECLARE_BITMAP(mynode, MAX_NUMNODES);
+
+ oldpol = current->mempolicy;
+ get_pol(oldpol);
+ bitmap_zero(mynode, MAX_NUMNODES);
+ set_bit(cpu_to_node(cpu), mynode);
+ set_fs(KERNEL_DS);
+ sys_set_mempolicy(MPOL_PREFERRED, &mynode, MAX_NUMNODES);
+ set_fs(oldfs);
+#endif
+ newinfo->entries[cpu] = vmalloc(size);
+#ifdef CONFIG_NUMA
+ mpol_free(current->mempolicy);
+ current->mempolicy = oldpol;
+#endif
+ }
+ if (newinfo->entries[cpu] == 0) {
+ free_table_info(newinfo);
+ return NULL;
+ }
+ }
+ return newinfo;
+}
+
+
static int
do_replace(void __user *user, unsigned int len)
{
@@ -1115,6 +1184,7 @@
struct ipt_table *t;
struct ipt_table_info *newinfo, *oldinfo;
struct ipt_counters *counters;
+ void *loc_cpu_entry, *loc_cpu_old_entry;
if (copy_from_user(&tmp, user, sizeof(tmp)) != 0)
return -EFAULT;
@@ -1127,12 +1197,15 @@
if ((SMP_ALIGN(tmp.size) >> PAGE_SHIFT) + 2 > num_physpages)
return -ENOMEM;
- newinfo = vmalloc(sizeof(struct ipt_table_info)
- + SMP_ALIGN(tmp.size) * num_possible_cpus());
+ newinfo = alloc_table_info(tmp.size);
if (!newinfo)
return -ENOMEM;
-
- if (copy_from_user(newinfo->entries, user + sizeof(tmp),
+ /*
+ * choose the copy that is on our node,
+ * but dont worry if the scheduler changes the CPU
+ */
+ loc_cpu_entry = newinfo->entries[raw_smp_processor_id()];
+ if (copy_from_user(loc_cpu_entry, user + sizeof(tmp),
tmp.size) != 0) {
ret = -EFAULT;
goto free_newinfo;
@@ -1143,10 +1216,9 @@
ret = -ENOMEM;
goto free_newinfo;
}
- memset(counters, 0, tmp.num_counters * sizeof(struct ipt_counters));
ret = translate_table(tmp.name, tmp.valid_hooks,
- newinfo, tmp.size, tmp.num_entries,
+ newinfo, loc_cpu_entry, tmp.size, tmp.num_entries,
tmp.hook_entry, tmp.underflow);
if (ret != 0)
goto free_newinfo_counters;
@@ -1185,8 +1257,9 @@
/* Get the old counters. */
get_counters(oldinfo, counters);
/* Decrease module usage counts and free resource */
- IPT_ENTRY_ITERATE(oldinfo->entries, oldinfo->size, cleanup_entry,NULL);
- vfree(oldinfo);
+ loc_cpu_old_entry = oldinfo->entries[raw_smp_processor_id()];
+ IPT_ENTRY_ITERATE(loc_cpu_old_entry, oldinfo->size, cleanup_entry,NULL);
+ free_table_info(oldinfo);
if (copy_to_user(tmp.counters, counters,
sizeof(struct ipt_counters) * tmp.num_counters) != 0)
ret = -EFAULT;
@@ -1198,11 +1271,11 @@
module_put(t->me);
up(&ipt_mutex);
free_newinfo_counters_untrans:
- IPT_ENTRY_ITERATE(newinfo->entries, newinfo->size, cleanup_entry,NULL);
+ IPT_ENTRY_ITERATE(loc_cpu_entry, newinfo->size, cleanup_entry,NULL);
free_newinfo_counters:
vfree(counters);
free_newinfo:
- vfree(newinfo);
+ free_table_info(newinfo);
return ret;
}
@@ -1235,6 +1308,7 @@
struct ipt_counters_info tmp, *paddc;
struct ipt_table *t;
int ret = 0;
+ void *loc_cpu_entry;
if (copy_from_user(&tmp, user, sizeof(tmp)) != 0)
return -EFAULT;
@@ -1264,7 +1338,12 @@
}
i = 0;
- IPT_ENTRY_ITERATE(t->private->entries,
+ /*
+ * choose the copy that is on our node,
+ * but dont worry if the scheduler changes the CPU
+ */
+ loc_cpu_entry = t->private->entries[raw_smp_processor_id()];
+ IPT_ENTRY_ITERATE(loc_cpu_entry,
t->private->size,
add_counter_to_entry,
paddc->counters,
@@ -1454,29 +1533,39 @@
{
int ret;
struct ipt_table_info *newinfo;
- static struct ipt_table_info bootstrap
- = { 0, 0, 0, { 0 }, { 0 }, { } };
+ static struct ipt_table_info bootstrap = {
+ .size = 0,
+ .number = 0,
+ .initial_entries = 0,
+ .hook_entry = { 0 },
+ .underflow = { 0 },
+ .entries = {NULL }
+ };
+ void *loc_cpu_entry;
- newinfo = vmalloc(sizeof(struct ipt_table_info)
- + SMP_ALIGN(repl->size) * num_possible_cpus());
+ newinfo = alloc_table_info(repl->size);
if (!newinfo)
return -ENOMEM;
-
- memcpy(newinfo->entries, repl->entries, repl->size);
+ /*
+ * choose the copy that is on our node,
+ * but dont worry if the scheduler changes the CPU
+ */
+ loc_cpu_entry = newinfo->entries[raw_smp_processor_id()];
+ memcpy(loc_cpu_entry, repl->entries, repl->size);
ret = translate_table(table->name, table->valid_hooks,
- newinfo, repl->size,
+ newinfo, loc_cpu_entry, repl->size,
repl->num_entries,
repl->hook_entry,
repl->underflow);
if (ret != 0) {
- vfree(newinfo);
+ free_table_info(newinfo);
return ret;
}
ret = down_interruptible(&ipt_mutex);
if (ret != 0) {
- vfree(newinfo);
+ free_table_info(newinfo);
return ret;
}
@@ -1505,20 +1594,25 @@
return ret;
free_unlock:
- vfree(newinfo);
+ free_table_info(newinfo);
goto unlock;
}
void ipt_unregister_table(struct ipt_table *table)
{
+ void *loc_cpu_entry;
down(&ipt_mutex);
LIST_DELETE(&ipt_tables, table);
up(&ipt_mutex);
- /* Decrease module usage counts and free resources */
- IPT_ENTRY_ITERATE(table->private->entries, table->private->size,
+ /* Decrease module usage counts and free resources
+ * choose the copy that is on our node,
+ * but dont worry if the scheduler changes the CPU
+ */
+ loc_cpu_entry = table->private->entries[raw_smp_processor_id()];
+ IPT_ENTRY_ITERATE(loc_cpu_entry, table->private->size,
cleanup_entry, NULL);
- vfree(table->private);
+ free_table_info(table->private);
}
/* Returns 1 if the port is matched by the range, 0 otherwise */
^ permalink raw reply
* [PATCH] af_packet: Allow for > 8 byte hardware addresses.
From: Eric W. Biederman @ 2005-09-20 17:17 UTC (permalink / raw)
To: David S. Miller; +Cc: openib-general, netdev
In-Reply-To: <20050912.154527.48978091.davem@davemloft.net>
Dave sorry for the delay getting back to this...
This version of the patch adds the one memset you were clearly
asking for.
The convention is that longer addresses will simply extend
the hardeware address byte arrays at the end of sockaddr_ll and
packet_mreq.
In making this change a small information leak was also closed.
The code only initializes the hardware address bytes that are
used, but all of struct sockaddr_ll was copied to userspace.
Now we just copy sockaddr_ll to the last byte of the hardware
address used.
For error checking larger structures than our internal
maximums continue to be allowed but an error is signaled if we can
not fit the hardware address into our internal structure.
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
net/packet/af_packet.c | 65 +++++++++++++++++++++++++++++++++++-------------
1 files changed, 48 insertions(+), 17 deletions(-)
0117e4931d1884ae71c74378590bde4cc76f403a
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -36,6 +36,11 @@
* Michal Ostrowski : Module initialization cleanup.
* Ulises Alonso : Frame number limit removal and
* packet_set_ring memory leak.
+ * Eric Biederman : Allow for > 8 byte hardware addresses.
+ * The convention is that longer addresses
+ * will simply extend the hardware address
+ * byte arrays at the end of sockaddr_ll
+ * and packet_mreq.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
@@ -161,7 +166,17 @@ struct packet_mclist
int count;
unsigned short type;
unsigned short alen;
- unsigned char addr[8];
+ unsigned char addr[MAX_ADDR_LEN];
+};
+/* identical to struct packet_mreq except it has
+ * a longer address field.
+ */
+struct packet_mreq_max
+{
+ int mr_ifindex;
+ unsigned short mr_type;
+ unsigned short mr_alen;
+ unsigned char mr_address[MAX_ADDR_LEN];
};
#endif
#ifdef CONFIG_PACKET_MMAP
@@ -716,6 +731,8 @@ static int packet_sendmsg(struct kiocb *
err = -EINVAL;
if (msg->msg_namelen < sizeof(struct sockaddr_ll))
goto out;
+ if (msg->msg_namelen < (saddr->sll_halen + offsetof(struct sockaddr_ll, sll_addr)))
+ goto out;
ifindex = saddr->sll_ifindex;
proto = saddr->sll_protocol;
addr = saddr->sll_addr;
@@ -744,6 +761,12 @@ static int packet_sendmsg(struct kiocb *
if (dev->hard_header) {
int res;
err = -EINVAL;
+ if (saddr) {
+ if (saddr->sll_halen != dev->addr_len)
+ goto out_free;
+ if (saddr->sll_hatype != dev->type)
+ goto out_free;
+ }
res = dev->hard_header(skb, dev, ntohs(proto), addr, NULL, len);
if (sock->type != SOCK_DGRAM) {
skb->tail = skb->data;
@@ -1045,6 +1068,7 @@ static int packet_recvmsg(struct kiocb *
struct sock *sk = sock->sk;
struct sk_buff *skb;
int copied, err;
+ struct sockaddr_ll *sll;
err = -EINVAL;
if (flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT))
@@ -1057,16 +1081,6 @@ static int packet_recvmsg(struct kiocb *
#endif
/*
- * If the address length field is there to be filled in, we fill
- * it in now.
- */
-
- if (sock->type == SOCK_PACKET)
- msg->msg_namelen = sizeof(struct sockaddr_pkt);
- else
- msg->msg_namelen = sizeof(struct sockaddr_ll);
-
- /*
* Call the generic datagram receiver. This handles all sorts
* of horrible races and re-entrancy so we can forget about it
* in the protocol layers.
@@ -1087,6 +1101,17 @@ static int packet_recvmsg(struct kiocb *
goto out;
/*
+ * If the address length field is there to be filled in, we fill
+ * it in now.
+ */
+
+ sll = (struct sockaddr_ll*)skb->cb;
+ if (sock->type == SOCK_PACKET)
+ msg->msg_namelen = sizeof(struct sockaddr_pkt);
+ else
+ msg->msg_namelen = sll->sll_halen + offsetof(struct sockaddr_ll, sll_addr);
+
+ /*
* You lose any data beyond the buffer you gave. If it worries a
* user program they can ask the device for its MTU anyway.
*/
@@ -1166,7 +1191,7 @@ static int packet_getname(struct socket
sll->sll_hatype = 0; /* Bad: we have no ARPHRD_UNSPEC */
sll->sll_halen = 0;
}
- *uaddr_len = sizeof(*sll);
+ *uaddr_len = offsetof(struct sockaddr_ll, sll_addr) + sll->sll_halen;
return 0;
}
@@ -1199,7 +1224,7 @@ static void packet_dev_mclist(struct net
}
}
-static int packet_mc_add(struct sock *sk, struct packet_mreq *mreq)
+static int packet_mc_add(struct sock *sk, struct packet_mreq_max *mreq)
{
struct packet_sock *po = pkt_sk(sk);
struct packet_mclist *ml, *i;
@@ -1249,7 +1274,7 @@ done:
return err;
}
-static int packet_mc_drop(struct sock *sk, struct packet_mreq *mreq)
+static int packet_mc_drop(struct sock *sk, struct packet_mreq_max *mreq)
{
struct packet_mclist *ml, **mlp;
@@ -1315,11 +1340,17 @@ packet_setsockopt(struct socket *sock, i
case PACKET_ADD_MEMBERSHIP:
case PACKET_DROP_MEMBERSHIP:
{
- struct packet_mreq mreq;
- if (optlen<sizeof(mreq))
+ struct packet_mreq_max mreq;
+ int len = optlen;
+ memset(&mreq, 0, sizeof(mreq));
+ if (len < sizeof(struct packet_mreq))
return -EINVAL;
- if (copy_from_user(&mreq,optval,sizeof(mreq)))
+ if (len > sizeof(mreq))
+ len = sizeof(mreq);
+ if (copy_from_user(&mreq,optval,len))
return -EFAULT;
+ if (len < (mreq.mr_alen + offsetof(struct packet_mreq, mr_address)))
+ return -EINVAL;
if (optname == PACKET_ADD_MEMBERSHIP)
ret = packet_mc_add(sk, &mreq);
else
^ permalink raw reply
* [PATCH] [NET] socket.c: zero socket addresses before use.
From: Eric W. Biederman @ 2005-09-20 17:18 UTC (permalink / raw)
To: David S. Miller; +Cc: openib-general, netdev
In-Reply-To: <20050912.154527.48978091.davem@davemloft.net>
Dave I don't know if this is part of what you want but
zeroing the socket address buffer before use seem to be implied
by what you were asking for. So here is an additional patch
to implement that.
This is a paranoid precaution to guard against accidental
information leaks to user space or other consumers/producers
may fail to properly fail to set or read the hardware
address length. af_packet over ethernet has had at least
has one small but in this respect.
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
net/socket.c | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)
957ae0f034aa1482e42da948b2d87ae6fc13366e
diff --git a/net/socket.c b/net/socket.c
--- a/net/socket.c
+++ b/net/socket.c
@@ -1285,6 +1285,7 @@ asmlinkage long sys_bind(int fd, struct
char address[MAX_SOCK_ADDR];
int err;
+ memset(address, 0, sizeof(address));
if((sock = sockfd_lookup(fd,&err))!=NULL)
{
if((err=move_addr_to_kernel(umyaddr,addrlen,address))>=0) {
@@ -1349,6 +1350,7 @@ asmlinkage long sys_accept(int fd, struc
int err, len;
char address[MAX_SOCK_ADDR];
+ memset(address, 0, sizeof(address));
sock = sockfd_lookup(fd, &err);
if (!sock)
goto out;
@@ -1419,6 +1421,7 @@ asmlinkage long sys_connect(int fd, stru
char address[MAX_SOCK_ADDR];
int err;
+ memset(address, 0, sizeof(address));
sock = sockfd_lookup(fd, &err);
if (!sock)
goto out;
@@ -1449,6 +1452,7 @@ asmlinkage long sys_getsockname(int fd,
char address[MAX_SOCK_ADDR];
int len, err;
+ memset(address, 0, sizeof(address));
sock = sockfd_lookup(fd, &err);
if (!sock)
goto out;
@@ -1479,6 +1483,7 @@ asmlinkage long sys_getpeername(int fd,
char address[MAX_SOCK_ADDR];
int len, err;
+ memset(address, 0, sizeof(address));
if ((sock = sockfd_lookup(fd, &err))!=NULL)
{
err = security_socket_getpeername(sock);
@@ -1510,6 +1515,7 @@ asmlinkage long sys_sendto(int fd, void
struct msghdr msg;
struct iovec iov;
+ memset(address, 0, sizeof(address));
sock = sockfd_lookup(fd, &err);
if (!sock)
goto out;
@@ -1564,6 +1570,7 @@ asmlinkage long sys_recvfrom(int fd, voi
char address[MAX_SOCK_ADDR];
int err,err2;
+ memset(address, 0, sizeof(address));
sock = sockfd_lookup(fd, &err);
if (!sock)
goto out;
@@ -1705,6 +1712,7 @@ asmlinkage long sys_sendmsg(int fd, stru
struct msghdr msg_sys;
int err, ctl_len, iov_size, total_len;
+ memset(address, 0, sizeof(address));
err = -EFAULT;
if (MSG_CMSG_COMPAT & flags) {
if (get_compat_msghdr(&msg_sys, msg_compat))
@@ -1806,6 +1814,7 @@ asmlinkage long sys_recvmsg(int fd, stru
struct sockaddr __user *uaddr;
int __user *uaddr_len;
+ memset(addr, 0, sizeof(addr));
if (MSG_CMSG_COMPAT & flags) {
if (get_compat_msghdr(&msg_sys, msg_compat))
return -EFAULT;
^ permalink raw reply
* Re: TCP throttling
From: Al Boldi @ 2005-09-20 18:54 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Mikael Abrahamsson, linux-net, netdev
In-Reply-To: <20050920091922.15b5664b@dxpl.pdx.osdl.net>
Stephen Hemminger wrote:
> On Tue, 20 Sep 2005 07:19:03 +0200 (CEST)
> Mikael Abrahamsson <swmike@swm.pp.se> wrote:
> > On Tue, 20 Sep 2005, Al Boldi wrote:
> > > Could you suggest sane values for 2.4/2.6 on a 100mbit link?
> >
> > Go for approx 2-8 megs of TCP window size and buffers.
>
> I use the following for testing over 1GE. You want
> at least 2x the delay bandwidth product.
ping 10.1 -s56
(RTT=0.250ms) * 100mbit = 3125000 Bytes
How much increase in thruput should be expected?
Thanks!
--
Al
^ permalink raw reply
* [PATCH] Adds sys_set_mempolicy() in include/linux/syscalls.h , Re: [PATCH, netfilter] NUMA aware ipv4/netfilter/ip_tables.c
From: Eric Dumazet @ 2005-09-20 21:45 UTC (permalink / raw)
To: Andi Kleen, linux-kernel; +Cc: netdev, netfilter-devel
In-Reply-To: <200509201830.20689.ak@suse.de>
Andi Kleen a écrit :
> On Tuesday 20 September 2005 11:47, Eric Dumazet wrote:
>
>
>
> I would prefer if random code didn't mess with mempolicy internals
> like this. Better just call sys_set_mempolicy()
>
> -Andi
>
>
OK but this prior patch seems necessary :
- Adds sys_set_mempolicy() in include/linux/syscalls.h
Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
^ permalink raw reply
* [PATCH] Adds sys_set_mempolicy() in include/linux/syscalls.h
From: Eric Dumazet @ 2005-09-20 21:46 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Andi Kleen, linux-kernel, netfilter-devel, netdev
In-Reply-To: <433082DE.3060308@cosmosbay.com>
[-- Attachment #1: Type: text/plain, Size: 381 bytes --]
Andi Kleen a écrit :
> On Tuesday 20 September 2005 11:47, Eric Dumazet wrote:
>
>
>
> I would prefer if random code didn't mess with mempolicy internals
> like this. Better just call sys_set_mempolicy()
> -Andi
>
>
OK but this prior patch seems necessary :
- Adds sys_set_mempolicy() in include/linux/syscalls.h
Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
[-- Attachment #2: patch_add_sys_set_mempolicy --]
[-- Type: text/plain, Size: 407 bytes --]
--- linux-2.6/include/linux/syscalls.h 2005-09-06 01:20:21.000000000 +0200
+++ linux-2.6-ed/include/linux/syscalls.h 2005-09-20 23:43:07.000000000 +0200
@@ -508,5 +508,7 @@
asmlinkage long sys_ioprio_set(int which, int who, int ioprio);
asmlinkage long sys_ioprio_get(int which, int who);
+asmlinkage long sys_set_mempolicy(int mode, unsigned long __user *nmask,
+ unsigned long maxnode);
#endif
^ permalink raw reply
* Re: [patch] fix suspend/resume on b44
From: Andrew Morton @ 2005-09-20 23:26 UTC (permalink / raw)
To: Pavel Machek; +Cc: jgarzik, netdev
In-Reply-To: <20050920132811.GA4563@elf.ucw.cz>
Pavel Machek <pavel@ucw.cz> wrote:
>
> Fix suspend/resume on b44 by freeing/reacquiring irq. Otherwise it
> hangs on resume.
>
> ...
> diff --git a/drivers/net/b44.c b/drivers/net/b44.c
> --- a/drivers/net/b44.c
> +++ b/drivers/net/b44.c
> @@ -1930,6 +1930,8 @@ static int b44_suspend(struct pci_dev *p
> b44_free_rings(bp);
>
> spin_unlock_irq(&bp->lock);
> +
> + free_irq(dev->irq, dev);
> pci_disable_device(pdev);
> return 0;
> }
> @@ -1946,6 +1948,9 @@ static int b44_resume(struct pci_dev *pd
> if (!netif_running(dev))
> return 0;
>
> + if (request_irq(dev->irq, b44_interrupt, SA_SHIRQ, dev->name, dev))
> + printk(KERN_ERR PFX "%s: request_irq failed\n", dev->name);
> +
> spin_lock_irq(&bp->lock);
>
> b44_init_rings(bp);
>
Why does it hang on suspend/resume?
This came up a while back and iirc we decided that adding free_irq() to
every ->suspend() handler in the world was the wrong thing to do. Do I
misremember?
^ permalink raw reply
* Re: [PATCH] af_packet: Allow for > 8 byte hardware addresses.
From: David S. Miller @ 2005-09-21 7:11 UTC (permalink / raw)
To: ebiederm; +Cc: openib-general, netdev
In-Reply-To: <m1irwvhd5x.fsf@ebiederm.dsl.xmission.com>
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Tue, 20 Sep 2005 11:17:14 -0600
> Dave sorry for the delay getting back to this...
> This version of the patch adds the one memset you were clearly
> asking for.
Applied, thanks a lot Eric.
^ permalink raw reply
* Re: [PATCH] [NET] socket.c: zero socket addresses before use.
From: David S. Miller @ 2005-09-21 7:13 UTC (permalink / raw)
To: ebiederm; +Cc: openib-general, netdev
In-Reply-To: <m1hdcfhd40.fsf_-_@ebiederm.dsl.xmission.com>
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Tue, 20 Sep 2005 11:18:23 -0600
> Dave I don't know if this is part of what you want but
> zeroing the socket address buffer before use seem to be implied
> by what you were asking for. So here is an additional patch
> to implement that.
>
> This is a paranoid precaution to guard against accidental
> information leaks to user space or other consumers/producers
> may fail to properly fail to set or read the hardware
> address length. af_packet over ethernet has had at least
> has one small but in this respect.
I think this patch might be a bit overkill, but thanks for cooking it
up. I'm willing to be convinced otherwise though :-)
^ permalink raw reply
* RFC: struct netdevice changes for IPoIB UC support
From: Michael S. Tsirkin @ 2005-09-21 8:02 UTC (permalink / raw)
To: openib-general, netdev; +Cc: davem, linux-kernel, Dror Goldenberg
Hi!
I am working on IP over InfiniBand net device support.
Existing code in mainline kernel only supports UD (unreliable datagram)
mode of operation, with max MTU of 2Kbyte.
I'm looking into support for UC (unreliable connected) mode of operation,
which can support MTU with theorectical limit up to 2Gbyte.
As was discussed on the openib list, one of the difficulties with
IP over IB support for UC mode, is the fact that the same device
has to support sending both UC (max MTU 2Gbyte) and UD (max MTU 2Kbyte)
packets, depending on packet link address.
I propose the following simple patch to let the netdevice override
the path MTU per dst entry. The patch was tested by modifying
existing IPoIB code to use MTU of 1K for some addresses, and 2K for
others.
Please comment on this approach: does it make sense to you guys?
Please Cc me directly, I'm not on the list.
Thanks a bunch,
MST
---
Make it possible for a network device to support more than one MTU value at a
time (depending on packet link address, or other criteria).
Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il>
Index: linux-2.6.12.5/include/linux/netdevice.h
===================================================================
--- linux-2.6.12.5.orig/include/linux/netdevice.h
+++ linux-2.6.12.5/include/linux/netdevice.h
@@ -454,6 +454,10 @@ struct net_device
#define HAVE_CHANGE_MTU
int (*change_mtu)(struct net_device *dev, int new_mtu);
+#define HAVE_GET_MTU
+ u32 (*get_mtu)(struct net_device *dev,
+ struct neighbour *neigh,
+ int path_mtu);
#define HAVE_TX_TIMEOUT
void (*tx_timeout) (struct net_device *dev);
Index: linux-2.6.12.5/include/net/dst.h
===================================================================
--- linux-2.6.12.5.orig/include/net/dst.h
+++ linux-2.6.12.5/include/net/dst.h
@@ -111,7 +111,12 @@ dst_metric(const struct dst_entry *dst,
static inline u32 dst_mtu(const struct dst_entry *dst)
{
- u32 mtu = dst_metric(dst, RTAX_MTU);
+ u32 mtu;
+ if (dst->dev && dst->dev->get_mtu)
+ mtu = dst->dev->get_mtu(dst->dev, dst->neighbour,
+ dst_metric(dst, RTAX_MTU));
+ else
+ mtu = dst_metric(dst, RTAX_MTU);
/*
* Alexey put it here, so ask him about it :)
*/
--
MST
^ permalink raw reply
* Re: RFC: struct netdevice changes for IPoIB UC support
From: Herbert Xu @ 2005-09-21 8:15 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, davem, openib-general, linux-kernel
In-Reply-To: <20050921080230.GE18449@mellanox.co.il>
Michael S. Tsirkin <mst@mellanox.co.il> wrote:
>
> Please comment on this approach: does it make sense to you guys?
> Please Cc me directly, I'm not on the list.
Sorry, this doesn't make sense.
> static inline u32 dst_mtu(const struct dst_entry *dst)
> {
> - u32 mtu = dst_metric(dst, RTAX_MTU);
> + u32 mtu;
> + if (dst->dev && dst->dev->get_mtu)
> + mtu = dst->dev->get_mtu(dst->dev, dst->neighbour,
> + dst_metric(dst, RTAX_MTU));
> + else
> + mtu = dst_metric(dst, RTAX_MTU);
>From this I gather that for a given dst the MTU is actually constant.
That is, it only varies across different dst's.
In this case you should calculate the correct MTU when the dst is
created rather than here.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [patch] fix suspend/resume on b44
From: Pavel Machek @ 2005-09-21 10:20 UTC (permalink / raw)
To: Andrew Morton; +Cc: jgarzik, netdev
In-Reply-To: <20050920162635.565e4b46.akpm@osdl.org>
Hi!
> > diff --git a/drivers/net/b44.c b/drivers/net/b44.c
> > --- a/drivers/net/b44.c
> > +++ b/drivers/net/b44.c
> > @@ -1930,6 +1930,8 @@ static int b44_suspend(struct pci_dev *p
> > b44_free_rings(bp);
> >
> > spin_unlock_irq(&bp->lock);
> > +
> > + free_irq(dev->irq, dev);
> > pci_disable_device(pdev);
> > return 0;
> > }
> > @@ -1946,6 +1948,9 @@ static int b44_resume(struct pci_dev *pd
> > if (!netif_running(dev))
> > return 0;
> >
> > + if (request_irq(dev->irq, b44_interrupt, SA_SHIRQ, dev->name, dev))
> > + printk(KERN_ERR PFX "%s: request_irq failed\n", dev->name);
> > +
> > spin_lock_irq(&bp->lock);
> >
> > b44_init_rings(bp);
> >
>
> Why does it hang on suspend/resume?
>
> This came up a while back and iirc we decided that adding free_irq() to
> every ->suspend() handler in the world was the wrong thing to do. Do I
> misremember?
No, you remember right, but b44 needed that free_irq/request_irq even
because those ACPI changes. I'm not exactly sure why, something went
very wrong otherwise.
Pavel
--
Boycott Kodak -- for their patent abuse against Java.
^ permalink raw reply
* Re: [patch] fix suspend/resume on b44
From: Andrew Morton @ 2005-09-21 10:36 UTC (permalink / raw)
To: Pavel Machek; +Cc: jgarzik, netdev
In-Reply-To: <20050921102054.GE25297@atrey.karlin.mff.cuni.cz>
Pavel Machek <pavel@suse.cz> wrote:
>
> Hi!
>
> > > diff --git a/drivers/net/b44.c b/drivers/net/b44.c
> > > --- a/drivers/net/b44.c
> > > +++ b/drivers/net/b44.c
> > > @@ -1930,6 +1930,8 @@ static int b44_suspend(struct pci_dev *p
> > > b44_free_rings(bp);
> > >
> > > spin_unlock_irq(&bp->lock);
> > > +
> > > + free_irq(dev->irq, dev);
> > > pci_disable_device(pdev);
> > > return 0;
> > > }
> > > @@ -1946,6 +1948,9 @@ static int b44_resume(struct pci_dev *pd
> > > if (!netif_running(dev))
> > > return 0;
> > >
> > > + if (request_irq(dev->irq, b44_interrupt, SA_SHIRQ, dev->name, dev))
> > > + printk(KERN_ERR PFX "%s: request_irq failed\n", dev->name);
> > > +
> > > spin_lock_irq(&bp->lock);
> > >
> > > b44_init_rings(bp);
> > >
> >
> > Why does it hang on suspend/resume?
> >
> > This came up a while back and iirc we decided that adding free_irq() to
> > every ->suspend() handler in the world was the wrong thing to do. Do I
> > misremember?
>
> No, you remember right, but b44 needed that free_irq/request_irq even
> because those ACPI changes. I'm not exactly sure why, something went
> very wrong otherwise.
Well I guess we should work out what went wrong ;)
What are the symptoms? Screaming interrupt? Can't immediately see why.
Does the screaming interrupt detetor trigger and disable the IRQ Line?
^ permalink raw reply
* Re: [PATCH] [NET] socket.c: zero socket addresses before use.
From: Eric W. Biederman @ 2005-09-21 13:48 UTC (permalink / raw)
To: David S. Miller; +Cc: openib-general, netdev
In-Reply-To: <20050921.001321.78997430.davem@davemloft.net>
"David S. Miller" <davem@davemloft.net> writes:
> From: ebiederm@xmission.com (Eric W. Biederman)
> Date: Tue, 20 Sep 2005 11:18:23 -0600
>
>> Dave I don't know if this is part of what you want but
>> zeroing the socket address buffer before use seem to be implied
>> by what you were asking for. So here is an additional patch
>> to implement that.
>>
>> This is a paranoid precaution to guard against accidental
>> information leaks to user space or other consumers/producers
>> may fail to properly fail to set or read the hardware
>> address length. af_packet over ethernet has had at least
>> has one small but in this respect.
>
> I think this patch might be a bit overkill, but thanks for cooking it
> up. I'm willing to be convinced otherwise though :-)
Personally I agree. But if we don't we probably need to audit
all of the other protocols besides af_packet and see if
they could possible have the problem that af_packet did
with struct sockaddr_ll.
What happened is struct sockaddr_ll had an array of 8 bytes
where it placed a hardware address. It reported to socket.c
the length of struct sockaddr_ll for returning to user space.
The code then only filled in enough bytes for the actual hardware
address. 6 bytes in the common case of ethernet. So only 6 bytes
were ever written and we returned 8 bytes to user space.
The transmission is similar except the kernel code was responsible
for simply not caring about the additional bytes. But confused
kernel code could have the problem.
This is the same problem you were concerned about with
my new struct packet_mreq_max. The only way to take the same
precautions on the other code paths is to modify socket.c
I am concerned enough to point out the possibility and send
a patch to add some memset() as a cheap insurance plan. Auditing
the address handling for all of the rest of the network protcols is
more than I am ready to volunteer for.
Eric
^ permalink raw reply
* Re: [patch] fix suspend/resume on b44
From: Pavel Machek @ 2005-09-21 21:13 UTC (permalink / raw)
To: Andrew Morton; +Cc: jgarzik, netdev, hmacht
In-Reply-To: <20050921033653.05c448df.akpm@osdl.org>
Hi!
> > > > diff --git a/drivers/net/b44.c b/drivers/net/b44.c
> > > > --- a/drivers/net/b44.c
> > > > +++ b/drivers/net/b44.c
> > > > @@ -1930,6 +1930,8 @@ static int b44_suspend(struct pci_dev *p
> > > > b44_free_rings(bp);
> > > >
> > > > spin_unlock_irq(&bp->lock);
> > > > +
> > > > + free_irq(dev->irq, dev);
> > > > pci_disable_device(pdev);
> > > > return 0;
> > > > }
> > > > @@ -1946,6 +1948,9 @@ static int b44_resume(struct pci_dev *pd
> > > > if (!netif_running(dev))
> > > > return 0;
> > > >
> > > > + if (request_irq(dev->irq, b44_interrupt, SA_SHIRQ, dev->name, dev))
> > > > + printk(KERN_ERR PFX "%s: request_irq failed\n", dev->name);
> > > > +
> > > > spin_lock_irq(&bp->lock);
> > > >
> > > > b44_init_rings(bp);
> > > >
> > >
> > > Why does it hang on suspend/resume?
> > >
> > > This came up a while back and iirc we decided that adding free_irq() to
> > > every ->suspend() handler in the world was the wrong thing to do. Do I
> > > misremember?
> >
> > No, you remember right, but b44 needed that free_irq/request_irq even
> > because those ACPI changes. I'm not exactly sure why, something went
> > very wrong otherwise.
>
> Well I guess we should work out what went wrong ;)
>
> What are the symptoms? Screaming interrupt? Can't immediately see why.
> Does the screaming interrupt detetor trigger and disable the IRQ Line?
No, it seems like BUG() triggers in
b44. https://bugzilla.novell.com/show_bug.cgi?id=116088 is for
basically 2.6.13 kernel (but it was in something as old as 2.6.5,
too).
Setting machine into suspend to disk with loaded module b44. While
resuming,
kernel oopses and machine freezes after reloading data from
swap. Image will be
appended.
If this can not be fixed in time, we could add this module to
UNLOAD_MODULES_BEFORE_SUSPEND in powersave configuratiion.
Pavel
--
if you have sharp zaurus hardware you don't need... you know my address
^ permalink raw reply
* Re: [patch] fix suspend/resume on b44
From: Andrew Morton @ 2005-09-21 21:22 UTC (permalink / raw)
To: Pavel Machek; +Cc: jgarzik, netdev, hmacht
In-Reply-To: <20050921211332.GA2194@elf.ucw.cz>
Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > > > > diff --git a/drivers/net/b44.c b/drivers/net/b44.c
> > > > > --- a/drivers/net/b44.c
> > > > > +++ b/drivers/net/b44.c
> > > > > @@ -1930,6 +1930,8 @@ static int b44_suspend(struct pci_dev *p
> > > > > b44_free_rings(bp);
> > > > >
> > > > > spin_unlock_irq(&bp->lock);
> > > > > +
> > > > > + free_irq(dev->irq, dev);
> > > > > pci_disable_device(pdev);
> > > > > return 0;
> > > > > }
> > > > > @@ -1946,6 +1948,9 @@ static int b44_resume(struct pci_dev *pd
> > > > > if (!netif_running(dev))
> > > > > return 0;
> > > > >
> > > > > + if (request_irq(dev->irq, b44_interrupt, SA_SHIRQ, dev->name, dev))
> > > > > + printk(KERN_ERR PFX "%s: request_irq failed\n", dev->name);
> > > > > +
> > > > > spin_lock_irq(&bp->lock);
> > > > >
> > > > > b44_init_rings(bp);
> > > > >
> > > >
> > > > Why does it hang on suspend/resume?
> > > >
> > > > This came up a while back and iirc we decided that adding free_irq() to
> > > > every ->suspend() handler in the world was the wrong thing to do. Do I
> > > > misremember?
> > >
> > > No, you remember right, but b44 needed that free_irq/request_irq even
> > > because those ACPI changes. I'm not exactly sure why, something went
> > > very wrong otherwise.
> >
> > Well I guess we should work out what went wrong ;)
> >
> > What are the symptoms? Screaming interrupt? Can't immediately see why.
> > Does the screaming interrupt detetor trigger and disable the IRQ Line?
>
> No, it seems like BUG() triggers in
> b44. https://bugzilla.novell.com/show_bug.cgi?id=116088 is for
> basically 2.6.13 kernel (but it was in something as old as 2.6.5,
> too).
>
That's here:
static void b44_tx(struct b44 *bp)
{
u32 cur, cons;
cur = br32(bp, B44_DMATX_STAT) & DMATX_STAT_CDMASK;
cur /= sizeof(struct dma_desc);
/* XXX needs updating when NETIF_F_SG is supported */
for (cons = bp->tx_cons; cons != cur; cons = NEXT_TX(cons)) {
struct ring_info *rp = &bp->tx_buffers[cons];
struct sk_buff *skb = rp->skb;
if (unlikely(skb == NULL))
BUG();
So I'd assume that the newly-woken driver took an interrupt, decided that a
Tx interrupt was pending then went BUG when it discovered that it hadn't
sent anything.
Would be good to find out the value of istat in b44_interrupt() and poke
maintainers, rather than proferring strange workarounds ;)
^ permalink raw reply
* [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
From: Eric Dumazet @ 2005-09-21 21:24 UTC (permalink / raw)
To: linux-kernel, netfilter-devel, netdev; +Cc: Andi Kleen
In-Reply-To: <43308324.70403@cosmosbay.com>
Hi
I have reworked net/ipv4/ip_tables.c to boost its performance, and post three
patches.
In oprofile results, ipt_do_table() was at the first position.
It is now at 6th position, using 1/3 of the CPU it was using before.
(Tests done on a dual Xeon i386 and a dual Opteron x86_64)
Short description :
1) No more central rwlock protecting each table (filter, nat, mangle, raw),
but one lock per CPU. It avoids cache line ping pongs for each packet.
2) Loop unrollings and various code optimizations to reduce branches
mispredictions.
3) NUMA aware allocation of memory (this part was posted earlier, but got some
polishing problems)
Long description:
1) No more one rwlock_t protecting the 'curtain'
One major bottleneck on SMP machines is the fact that one rwlock
is taken in ipt_do_table() entry and exit. That 2 atomic operations are
the killer, and even if multiple readers can work together on the same table
(using read_lock_bh()), the cache line containing rwlock still must be
taken exclusively by each CPU at entry/exit of ipt_do_table().
As existing code already use separate copies of the data for each cpu, it is
very easy to convert the central rwlock to separate rwlocks, allocated
percpu (and NUMA aware).
When a cpu enters ipt_do_table(), it can locks its local copy, touching a
cache line that is not used by other cpus. Further operations are done on
'local' copy of the data.
When a sum of all counters must be done, we can write_lock each part at a
time, instead of locking all the parts, reducing the lock contention.
2) Loop unrolling
It seems that with current compilers and CFLAGS, the code from
ip_packet_match() is very bad, using lot of mispredicted conditional branches
I made some patches and generated code on i386 and x86_64
is much better.
3) NUMA allocation.
Part of the performance problem we have with netfilter is memory allocation
is not NUMA aware, but 'only' SMP aware (ie each CPU normally touch
separate cache lines)
Even with small iptables rules, the cost of this misplacement can be high
on common workloads.
Instead of using one vmalloc() area (located in the node of the iptables
process), we now allocate an area for each possible CPU, using NUMA policy
(MPOL_PREFERRED) so that memory should be allocated in the CPU's node
if possible.
If the size of ipt_table is small enough (less than one page), we use
kmalloc_node() instead of vmalloc(), to use less memory and less TLB entries)
in small setups.
Note1 : I also optimized get_counters(), using a SET_COUNTER() for the first
cpu, avoiding a memset() and ADD_COUNTER() if SMP on other cpus.
Note2 : This patch depends on another patch that declares sys_set_mempolicy()
in include/linux/syscalls.h
( http://marc.theaimsgroup.com/?l=linux-kernel&m=112725288622984&w=2 )
Thank you
Eric Dumazet
^ permalink raw reply
* [PATCH 1/3] netfilter : 3 patches to boost ip_tables performance
From: Eric Dumazet @ 2005-09-21 21:29 UTC (permalink / raw)
To: linux-kernel, netfilter-devel, netdev; +Cc: Andi Kleen
In-Reply-To: <43308324.70403@cosmosbay.com>
[-- Attachment #1: Type: text/plain, Size: 1098 bytes --]
Patch 1/3
1) No more one rwlock_t protecting the 'curtain'
One major bottleneck on SMP machines is the fact that one rwlock
is taken in ipt_do_table() entry and exit. That 2 atomic operations are
the killer, and even if multiple readers can work together on the same table
(using read_lock_bh()), the cache line containing rwlock still must be
taken exclusively by each CPU at entry/exit of ipt_do_table().
As existing code already use separate copies of the data for each cpu, it is
very easy to convert the central rwlock to separate rwlocks, allocated
percpu (and NUMA aware).
When a cpu enters ipt_do_table(), it can locks its local copy, touching a
cache line that is not used by other cpus. Further operations are done on
'local' copy of the data.
When a sum of all counters must be done, we can write_lock each part at a
time, instead of locking all the parts, reducing the lock contention.
Note1 : I also optimized get_counters(), using a SET_COUNTER() for the first
cpu, avoiding a memset() and ADD_COUNTER() if SMP on other cpus.
Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
[-- Attachment #2: patch_ip_tables_numa.1 --]
[-- Type: text/plain, Size: 8651 bytes --]
--- linux-2.6/include/linux/netfilter_ipv4/ip_tables.h 2005-09-13 05:12:09.000000000 +0200
+++ linux-2.6-ed/include/linux/netfilter_ipv4/ip_tables.h 2005-09-21 17:42:07.000000000 +0200
@@ -445,7 +445,11 @@
unsigned int valid_hooks;
/* Lock for the curtain */
+#ifdef CONFIG_SMP
+ rwlock_t *lock_p; /* one lock per cpu */
+#else
rwlock_t lock;
+#endif
/* Man behind the curtain... */
struct ipt_table_info *private;
--- linux-2.6.14-rc1.p1/net/ipv4/netfilter/ip_nat_rule.c 2005-09-13 05:12:09.000000000 +0200
+++ linux-2.6.14-rc1/net/ipv4/netfilter/ip_nat_rule.c 2005-09-21 17:44:01.000000000 +0200
@@ -93,7 +93,6 @@
static struct ipt_table nat_table = {
.name = "nat",
.valid_hooks = NAT_VALID_HOOKS,
- .lock = RW_LOCK_UNLOCKED,
.me = THIS_MODULE,
};
--- linux-2.6.14-rc1.p1/net/ipv4/netfilter/iptable_filter.c 2005-09-13 05:12:09.000000000 +0200
+++ linux-2.6.14-rc1/net/ipv4/netfilter/iptable_filter.c 2005-09-21 17:45:20.000000000 +0200
@@ -77,7 +77,6 @@
static struct ipt_table packet_filter = {
.name = "filter",
.valid_hooks = FILTER_VALID_HOOKS,
- .lock = RW_LOCK_UNLOCKED,
.me = THIS_MODULE
};
--- linux-2.6.14-rc1.p1/net/ipv4/netfilter/iptable_mangle.c 2005-09-13 05:12:09.000000000 +0200
+++ linux-2.6.14-rc1/net/ipv4/netfilter/iptable_mangle.c 2005-09-21 17:45:20.000000000 +0200
@@ -107,7 +107,6 @@
static struct ipt_table packet_mangler = {
.name = "mangle",
.valid_hooks = MANGLE_VALID_HOOKS,
- .lock = RW_LOCK_UNLOCKED,
.me = THIS_MODULE,
};
--- linux-2.6.14-rc1.p1/net/ipv4/netfilter/iptable_raw.c 2005-09-13 05:12:09.000000000 +0200
+++ linux-2.6.14-rc1/net/ipv4/netfilter/iptable_raw.c 2005-09-21 17:45:20.000000000 +0200
@@ -82,7 +82,6 @@
static struct ipt_table packet_raw = {
.name = "raw",
.valid_hooks = RAW_VALID_HOOKS,
- .lock = RW_LOCK_UNLOCKED,
.me = THIS_MODULE
};
--- linux-2.6.14-rc1.p1/net/ipv4/netfilter/ip_tables.c 2005-09-19 19:56:12.000000000 +0200
+++ linux-2.6.14-rc1/net/ipv4/netfilter/ip_tables.c 2005-09-21 23:49:13.000000000 +0200
@@ -110,6 +110,7 @@
static LIST_HEAD(ipt_target);
static LIST_HEAD(ipt_match);
static LIST_HEAD(ipt_tables);
+#define SET_COUNTER(c,b,p) do { (c).bcnt = (b); (c).pcnt = (p); } while(0)
#define ADD_COUNTER(c,b,p) do { (c).bcnt += (b); (c).pcnt += (p); } while(0)
#ifdef CONFIG_SMP
@@ -273,6 +274,9 @@
const char *indev, *outdev;
void *table_base;
struct ipt_entry *e, *back;
+#ifdef CONFIG_SMP
+ rwlock_t *lock_p;
+#endif
/* Initialization */
ip = (*pskb)->nh.iph;
@@ -287,7 +291,18 @@
* match it. */
offset = ntohs(ip->frag_off) & IP_OFFSET;
+#ifdef CONFIG_SMP
+ /*
+ * We expand read_lock_bh() here because we need to get
+ * the address of this cpu rwlock_t
+ */
+ local_bh_disable();
+ preempt_disable();
+ lock_p = per_cpu_ptr(table->lock_p, smp_processor_id());
+ _raw_read_lock(lock_p);
+#else
read_lock_bh(&table->lock);
+#endif
IP_NF_ASSERT(table->valid_hooks & (1 << hook));
table_base = (void *)table->private->entries
+ TABLE_OFFSET(table->private, smp_processor_id());
@@ -396,7 +411,11 @@
#ifdef CONFIG_NETFILTER_DEBUG
((struct ipt_entry *)table_base)->comefrom = 0xdead57ac;
#endif
+#ifdef CONFIG_SMP
+ read_unlock_bh(lock_p);
+#else
read_unlock_bh(&table->lock);
+#endif
#ifdef DEBUG_ALLOW_ALL
return NF_ACCEPT;
@@ -930,6 +949,30 @@
return ret;
}
+static void ipt_table_global_lock(struct ipt_table *table)
+{
+#ifdef CONFIG_SMP
+ int cpu;
+ for_each_cpu(cpu) {
+ write_lock_bh(per_cpu_ptr(table->lock_p, cpu));
+ }
+#else
+ write_lock_bh(&table->lock);
+#endif
+}
+
+static void ipt_table_global_unlock(struct ipt_table *table)
+{
+#ifdef CONFIG_SMP
+ int cpu;
+ for_each_cpu(cpu) {
+ write_unlock_bh(per_cpu_ptr(table->lock_p, cpu));
+ }
+#else
+ write_unlock_bh(&table->lock);
+#endif
+}
+
static struct ipt_table_info *
replace_table(struct ipt_table *table,
unsigned int num_counters,
@@ -954,24 +997,25 @@
#endif
/* Do the substitution. */
- write_lock_bh(&table->lock);
+ ipt_table_global_lock(table);
/* Check inside lock: is the old number correct? */
if (num_counters != table->private->number) {
duprintf("num_counters != table->private->number (%u/%u)\n",
num_counters, table->private->number);
- write_unlock_bh(&table->lock);
+ ipt_table_global_unlock(table);
*error = -EAGAIN;
return NULL;
}
oldinfo = table->private;
table->private = newinfo;
newinfo->initial_entries = oldinfo->initial_entries;
- write_unlock_bh(&table->lock);
+ ipt_table_global_unlock(table);
return oldinfo;
}
/* Gets counters. */
+#ifdef CONFIG_SMP
static inline int
add_entry_to_counter(const struct ipt_entry *e,
struct ipt_counters total[],
@@ -982,22 +1026,71 @@
(*i)++;
return 0;
}
+#endif
+static inline int
+set_entry_to_counter(const struct ipt_entry *e,
+ struct ipt_counters total[],
+ unsigned int *i)
+{
+ SET_COUNTER(total[*i], e->counters.bcnt, e->counters.pcnt);
+
+ (*i)++;
+ return 0;
+}
+/*
+ * if table is NULL, no locking should be done
+ */
static void
-get_counters(const struct ipt_table_info *t,
+get_counters(struct ipt_table *table,
+ const struct ipt_table_info *t,
struct ipt_counters counters[])
{
- unsigned int cpu;
unsigned int i;
+#ifdef CONFIG_SMP
+ unsigned int cpu;
+ unsigned int curcpu;
+
+ curcpu = get_cpu();
+
+ if (table)
+ write_lock_bh(per_cpu_ptr(table->lock_p, curcpu));
+ i = 0;
+ IPT_ENTRY_ITERATE(t->entries + TABLE_OFFSET(t, curcpu),
+ t->size,
+ set_entry_to_counter,
+ counters,
+ &i);
+ if (table)
+ write_unlock_bh(per_cpu_ptr(table->lock_p, curcpu));
- for (cpu = 0; cpu < num_possible_cpus(); cpu++) {
+ for_each_cpu(cpu) {
+ if (cpu == curcpu)
+ continue;
+ if (table)
+ write_lock_bh(per_cpu_ptr(table->lock_p, cpu));
i = 0;
IPT_ENTRY_ITERATE(t->entries + TABLE_OFFSET(t, cpu),
t->size,
add_entry_to_counter,
counters,
&i);
+ if (table)
+ write_unlock_bh(per_cpu_ptr(table->lock_p, cpu));
}
+ put_cpu();
+#else
+ if (table)
+ write_lock_bh(&table->lock);
+ i = 0;
+ IPT_ENTRY_ITERATE(t->entries,
+ t->size,
+ set_entry_to_counter,
+ counters,
+ &i);
+ if (table)
+ write_unlock_bh(&table->lock);
+#endif
}
static int
@@ -1020,10 +1113,7 @@
return -ENOMEM;
/* First, sum counters... */
- memset(counters, 0, countersize);
- write_lock_bh(&table->lock);
- get_counters(table->private, counters);
- write_unlock_bh(&table->lock);
+ get_counters(table, table->private, counters);
/* ... then copy entire thing from CPU 0... */
if (copy_to_user(userptr, table->private->entries, total_size) != 0) {
@@ -1183,7 +1273,7 @@
module_put(t->me);
/* Get the old counters. */
- get_counters(oldinfo, counters);
+ get_counters(NULL, oldinfo, counters);
/* Decrease module usage counts and free resource */
IPT_ENTRY_ITERATE(oldinfo->entries, oldinfo->size, cleanup_entry,NULL);
vfree(oldinfo);
@@ -1235,6 +1325,9 @@
struct ipt_counters_info tmp, *paddc;
struct ipt_table *t;
int ret = 0;
+#ifdef CONFIG_SMP
+ rwlock_t *lockp;
+#endif
if (copy_from_user(&tmp, user, sizeof(tmp)) != 0)
return -EFAULT;
@@ -1257,7 +1350,12 @@
goto free;
}
+#ifdef CONFIG_SMP
+ lockp = per_cpu_ptr(t->lock_p, get_cpu());
+ write_lock_bh(lockp);
+#else
write_lock_bh(&t->lock);
+#endif
if (t->private->number != paddc->num_counters) {
ret = -EINVAL;
goto unlock_up_free;
@@ -1270,7 +1368,12 @@
paddc->counters,
&i);
unlock_up_free:
+#ifdef CONFIG_SMP
+ write_unlock_bh(lockp);
+ put_cpu();
+#else
write_unlock_bh(&t->lock);
+#endif
up(&ipt_mutex);
module_put(t->me);
free:
@@ -1456,7 +1559,17 @@
struct ipt_table_info *newinfo;
static struct ipt_table_info bootstrap
= { 0, 0, 0, { 0 }, { 0 }, { } };
-
+#ifdef CONFIG_SMP
+ int cpu;
+ if (!table->lock_p) {
+ if ((table->lock_p = alloc_percpu(rwlock_t)) == NULL)
+ return -ENOMEM;
+ }
+ for_each_cpu(cpu)
+ rwlock_init(per_cpu_ptr(table->lock_p, cpu));
+#else
+ rwlock_init(&table->lock);
+#endif
newinfo = vmalloc(sizeof(struct ipt_table_info)
+ SMP_ALIGN(repl->size) * num_possible_cpus());
if (!newinfo)
@@ -1497,7 +1610,6 @@
/* save number of initial entries */
table->private->initial_entries = table->private->number;
- rwlock_init(&table->lock);
list_prepend(&ipt_tables, table);
unlock:
@@ -1519,6 +1631,10 @@
IPT_ENTRY_ITERATE(table->private->entries, table->private->size,
cleanup_entry, NULL);
vfree(table->private);
+#ifdef CONFIG_SMP
+ free_percpu(table->lock_p);
+ table->lock_p = NULL;
+#endif
}
/* Returns 1 if the port is matched by the range, 0 otherwise */
^ permalink raw reply
* [PATCH 2/3] netfilter : 3 patches to boost ip_tables performance
From: Eric Dumazet @ 2005-09-21 21:32 UTC (permalink / raw)
To: linux-kernel, netfilter-devel, netdev; +Cc: Andi Kleen
In-Reply-To: <43308324.70403@cosmosbay.com>
[-- Attachment #1: Type: text/plain, Size: 330 bytes --]
Patch 2/3 (please apply after Patch 1/3)
2) Loop unrolling
It seems that with current compilers and CFLAGS, the code from
ip_packet_match() is very bad, using lot of mispredicted conditional branches
I made some patches and generated code on i386 and x86_64
is much better.
Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
[-- Attachment #2: patch_ip_tables_numa.2 --]
[-- Type: text/plain, Size: 3958 bytes --]
--- linux-2.6/net/ipv4/netfilter/ip_tables.c 2005-09-21 23:55:30.000000000 +0200
+++ linux-2.6-ed/net/ipv4/netfilter/ip_tables.c 2005-09-22 00:39:29.000000000 +0200
@@ -125,6 +125,27 @@
#define up(x) do { printk("UP:%u:" #x "\n", __LINE__); up(x); } while(0)
#endif
+/*
+ * Special macro to compare IFNAMSIZ 'strings', with mask.
+ * Best results if feeded with 3 args pointing to 'unsigned long' types
+ * Loop is manually unrolled for performance reasons.
+ * gcc generates code without branches, IFNAMSIZ being a constant
+ */
+#define compare_if_lstrings(ret, devname, expect, expect_mask) \
+ ret = ((devname)[0] ^ (expect)[0]) & (expect_mask)[0]; \
+ if (IFNAMSIZ > sizeof(*devname)) \
+ ret |= ((devname)[1] ^ (expect)[1]) & (expect_mask)[1]; \
+ if (IFNAMSIZ > 2 * sizeof(*devname)) \
+ ret |= ((devname)[2] ^ (expect)[2]) & (expect_mask)[2]; \
+ if (IFNAMSIZ > 3 * sizeof(*devname)) \
+ ret |= ((devname)[3] ^ (expect)[3]) & (expect_mask)[3]; \
+ /* just in case IFNAMSIZ is enlarged */ \
+ if (IFNAMSIZ > 4 * sizeof(*devname)) { \
+ int i; \
+ for (i = 4 ; (i < IFNAMSIZ/sizeof(*devname)); i++) \
+ ret |= ((devname)[i] ^ (expect)[i]) & (expect_mask)[i]; \
+ }
+
/* Returns whether matches rule or not. */
static inline int
ip_packet_match(const struct iphdr *ip,
@@ -133,16 +154,22 @@
const struct ipt_ip *ipinfo,
int isfrag)
{
- size_t i;
+ int bool1, bool2;
unsigned long ret;
#define FWINV(bool,invflg) ((bool) ^ !!(ipinfo->invflags & invflg))
- if (FWINV((ip->saddr&ipinfo->smsk.s_addr) != ipinfo->src.s_addr,
- IPT_INV_SRCIP)
- || FWINV((ip->daddr&ipinfo->dmsk.s_addr) != ipinfo->dst.s_addr,
- IPT_INV_DSTIP)) {
- dprintf("Source or dest mismatch.\n");
+ bool1 = ((ip->saddr&ipinfo->smsk.s_addr) != ipinfo->src.s_addr);
+ bool1 ^= !!(ipinfo->invflags & IPT_INV_SRCIP);
+
+ bool2 = ((ip->daddr&ipinfo->dmsk.s_addr) != ipinfo->dst.s_addr);
+ bool2 ^= !!(ipinfo->invflags & IPT_INV_DSTIP);
+
+ if ((bool1 | bool2) != 0) {
+ if (bool1)
+ dprintf("Source%s mismatch.\n", bool2 ? " and Dest":"");
+ else
+ dprintf("Dest mismatch.\n");
dprintf("SRC: %u.%u.%u.%u. Mask: %u.%u.%u.%u. Target: %u.%u.%u.%u.%s\n",
NIPQUAD(ip->saddr),
@@ -157,27 +184,26 @@
return 0;
}
- /* Look for ifname matches; this should unroll nicely. */
- for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
- ret |= (((const unsigned long *)indev)[i]
- ^ ((const unsigned long *)ipinfo->iniface)[i])
- & ((const unsigned long *)ipinfo->iniface_mask)[i];
- }
+ compare_if_lstrings(ret,
+ (const unsigned long *)indev,
+ (const unsigned long *)ipinfo->iniface,
+ (const unsigned long *)ipinfo->iniface_mask);
- if (FWINV(ret != 0, IPT_INV_VIA_IN)) {
+ bool1 = FWINV(ret != 0, IPT_INV_VIA_IN);
+ if (bool1) {
dprintf("VIA in mismatch (%s vs %s).%s\n",
indev, ipinfo->iniface,
ipinfo->invflags&IPT_INV_VIA_IN ?" (INV)":"");
return 0;
}
- for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
- ret |= (((const unsigned long *)outdev)[i]
- ^ ((const unsigned long *)ipinfo->outiface)[i])
- & ((const unsigned long *)ipinfo->outiface_mask)[i];
- }
+ compare_if_lstrings(ret,
+ (const unsigned long *)outdev,
+ (const unsigned long *)ipinfo->outiface,
+ (const unsigned long *)ipinfo->outiface_mask);
- if (FWINV(ret != 0, IPT_INV_VIA_OUT)) {
+ bool1 = FWINV(ret != 0, IPT_INV_VIA_OUT);
+ if (bool1) {
dprintf("VIA out mismatch (%s vs %s).%s\n",
outdev, ipinfo->outiface,
ipinfo->invflags&IPT_INV_VIA_OUT ?" (INV)":"");
@@ -185,8 +211,9 @@
}
/* Check specific protocol */
- if (ipinfo->proto
- && FWINV(ip->protocol != ipinfo->proto, IPT_INV_PROTO)) {
+ bool1 = FWINV(ip->protocol != ipinfo->proto, IPT_INV_PROTO) ;
+ bool1 &= (ipinfo->proto != 0);
+ if (bool1) {
dprintf("Packet protocol %hi does not match %hi.%s\n",
ip->protocol, ipinfo->proto,
ipinfo->invflags&IPT_INV_PROTO ? " (INV)":"");
^ permalink raw reply
* [PATCH 3/3] netfilter : 3 patches to boost ip_tables performance
From: Eric Dumazet @ 2005-09-21 21:37 UTC (permalink / raw)
To: linux-kernel, netfilter-devel, netdev; +Cc: Andi Kleen
In-Reply-To: <43308324.70403@cosmosbay.com>
[-- Attachment #1: Type: text/plain, Size: 1068 bytes --]
Patch 3/3 (please apply after Patch 2/3)
3) NUMA allocation.
Part of the performance problem we have with netfilter is memory allocation
is not NUMA aware, but 'only' SMP aware (ie each CPU normally touch
separate cache lines)
Even with small iptables rules, the cost of this misplacement can be high
on common workloads.
Instead of using one vmalloc() area (located in the node of the iptables
process), we now allocate an area for each possible CPU, using NUMA policy
(MPOL_PREFERRED) so that memory should be allocated in the CPU's node
if possible.
If the size of ipt_table is small enough (less than one page), we use
kmalloc_node() instead of vmalloc(), to use less memory and less TLB entries)
in small setups.
Please note that this patch doesnt change the number of allocated bytes, only
the location of allocated zones.
Note2 : This patch depends on another patch that declares sys_set_mempolicy()
in include/linux/syscalls.h
( http://marc.theaimsgroup.com/?l=linux-kernel&m=112725288622984&w=2 )
Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
[-- Attachment #2: patch_ip_tables_numa.3 --]
[-- Type: text/plain, Size: 12981 bytes --]
--- linux-2.6/net/ipv4/netfilter/ip_tables.c 2005-09-22 00:44:34.000000000 +0200
+++ linux-2.6-ed/net/ipv4/netfilter/ip_tables.c 2005-09-22 00:57:15.000000000 +0200
@@ -17,6 +17,8 @@
#include <linux/skbuff.h>
#include <linux/kmod.h>
#include <linux/vmalloc.h>
+#include <linux/mempolicy.h>
+#include <linux/syscalls.h>
#include <linux/netdevice.h>
#include <linux/module.h>
#include <linux/tcp.h>
@@ -82,11 +84,6 @@
context stops packets coming through and allows user context to read
the counters or update the rules.
- To be cache friendly on SMP, we arrange them like so:
- [ n-entries ]
- ... cache-align padding ...
- [ n-entries ]
-
Hence the start of any table is given by get_table() below. */
/* The table itself */
@@ -104,7 +101,7 @@
unsigned int underflow[NF_IP_NUMHOOKS];
/* ipt_entry tables: one per CPU */
- char entries[0] ____cacheline_aligned;
+ void *entries[NR_CPUS];
};
static LIST_HEAD(ipt_target);
@@ -113,12 +110,6 @@
#define SET_COUNTER(c,b,p) do { (c).bcnt = (b); (c).pcnt = (p); } while(0)
#define ADD_COUNTER(c,b,p) do { (c).bcnt += (b); (c).pcnt += (p); } while(0)
-#ifdef CONFIG_SMP
-#define TABLE_OFFSET(t,p) (SMP_ALIGN((t)->size)*(p))
-#else
-#define TABLE_OFFSET(t,p) 0
-#endif
-
#if 0
#define down(x) do { printk("DOWN:%u:" #x "\n", __LINE__); down(x); } while(0)
#define down_interruptible(x) ({ int __r; printk("DOWNi:%u:" #x "\n", __LINE__); __r = down_interruptible(x); if (__r != 0) printk("ABORT-DOWNi:%u\n", __LINE__); __r; })
@@ -331,8 +322,7 @@
read_lock_bh(&table->lock);
#endif
IP_NF_ASSERT(table->valid_hooks & (1 << hook));
- table_base = (void *)table->private->entries
- + TABLE_OFFSET(table->private, smp_processor_id());
+ table_base = (void *)table->private->entries[smp_processor_id()];
e = get_entry(table_base, table->private->hook_entry[hook]);
#ifdef CONFIG_NETFILTER_DEBUG
@@ -608,7 +598,7 @@
/* Figures out from what hook each rule can be called: returns 0 if
there are loops. Puts hook bitmask in comefrom. */
static int
-mark_source_chains(struct ipt_table_info *newinfo, unsigned int valid_hooks)
+mark_source_chains(struct ipt_table_info *newinfo, unsigned int valid_hooks, void *entry0)
{
unsigned int hook;
@@ -617,7 +607,7 @@
for (hook = 0; hook < NF_IP_NUMHOOKS; hook++) {
unsigned int pos = newinfo->hook_entry[hook];
struct ipt_entry *e
- = (struct ipt_entry *)(newinfo->entries + pos);
+ = (struct ipt_entry *)(entry0 + pos);
if (!(valid_hooks & (1 << hook)))
continue;
@@ -667,13 +657,13 @@
goto next;
e = (struct ipt_entry *)
- (newinfo->entries + pos);
+ (entry0 + pos);
} while (oldpos == pos + e->next_offset);
/* Move along one */
size = e->next_offset;
e = (struct ipt_entry *)
- (newinfo->entries + pos + size);
+ (entry0 + pos + size);
e->counters.pcnt = pos;
pos += size;
} else {
@@ -690,7 +680,7 @@
newpos = pos + e->next_offset;
}
e = (struct ipt_entry *)
- (newinfo->entries + newpos);
+ (entry0 + newpos);
e->counters.pcnt = pos;
pos = newpos;
}
@@ -900,6 +890,7 @@
translate_table(const char *name,
unsigned int valid_hooks,
struct ipt_table_info *newinfo,
+ void *entry0,
unsigned int size,
unsigned int number,
const unsigned int *hook_entries,
@@ -920,11 +911,11 @@
duprintf("translate_table: size %u\n", newinfo->size);
i = 0;
/* Walk through entries, checking offsets. */
- ret = IPT_ENTRY_ITERATE(newinfo->entries, newinfo->size,
+ ret = IPT_ENTRY_ITERATE(entry0, newinfo->size,
check_entry_size_and_hooks,
newinfo,
- newinfo->entries,
- newinfo->entries + size,
+ entry0,
+ entry0 + size,
hook_entries, underflows, &i);
if (ret != 0)
return ret;
@@ -952,25 +943,24 @@
}
}
- if (!mark_source_chains(newinfo, valid_hooks))
+ if (!mark_source_chains(newinfo, valid_hooks, entry0))
return -ELOOP;
/* Finally, each sanity check must pass */
i = 0;
- ret = IPT_ENTRY_ITERATE(newinfo->entries, newinfo->size,
+ ret = IPT_ENTRY_ITERATE(entry0, newinfo->size,
check_entry, name, size, &i);
if (ret != 0) {
- IPT_ENTRY_ITERATE(newinfo->entries, newinfo->size,
+ IPT_ENTRY_ITERATE(entry0, newinfo->size,
cleanup_entry, &i);
return ret;
}
/* And one copy for every other CPU */
- for (i = 1; i < num_possible_cpus(); i++) {
- memcpy(newinfo->entries + SMP_ALIGN(newinfo->size)*i,
- newinfo->entries,
- SMP_ALIGN(newinfo->size));
+ for_each_cpu(i) {
+ if (newinfo->entries[i] && newinfo->entries[i] != entry0)
+ memcpy(newinfo->entries[i], entry0, newinfo->size);
}
return ret;
@@ -1010,15 +1000,12 @@
#ifdef CONFIG_NETFILTER_DEBUG
{
- struct ipt_entry *table_base;
- unsigned int i;
+ int cpu;
- for (i = 0; i < num_possible_cpus(); i++) {
- table_base =
- (void *)newinfo->entries
- + TABLE_OFFSET(newinfo, i);
-
- table_base->comefrom = 0xdead57ac;
+ for_each_cpu(cpu) {
+ struct ipt_entry *table_base = newinfo->entries[cpu];
+ if (table_base)
+ table_base->comefrom = 0xdead57ac;
}
}
#endif
@@ -1083,7 +1070,7 @@
if (table)
write_lock_bh(per_cpu_ptr(table->lock_p, curcpu));
i = 0;
- IPT_ENTRY_ITERATE(t->entries + TABLE_OFFSET(t, curcpu),
+ IPT_ENTRY_ITERATE(t->entries[curcpu],
t->size,
set_entry_to_counter,
counters,
@@ -1097,7 +1084,7 @@
if (table)
write_lock_bh(per_cpu_ptr(table->lock_p, cpu));
i = 0;
- IPT_ENTRY_ITERATE(t->entries + TABLE_OFFSET(t, cpu),
+ IPT_ENTRY_ITERATE(t->entries[cpu],
t->size,
add_entry_to_counter,
counters,
@@ -1110,7 +1097,7 @@
if (table)
write_lock_bh(&table->lock);
i = 0;
- IPT_ENTRY_ITERATE(t->entries,
+ IPT_ENTRY_ITERATE(t->entries[0],
t->size,
set_entry_to_counter,
counters,
@@ -1129,6 +1116,7 @@
struct ipt_entry *e;
struct ipt_counters *counters;
int ret = 0;
+ void *loc_cpu_entry;
/* We need atomic snapshot of counters: rest doesn't change
(other than comefrom, which userspace doesn't care
@@ -1142,8 +1130,12 @@
/* First, sum counters... */
get_counters(table, table->private, counters);
- /* ... then copy entire thing from CPU 0... */
- if (copy_to_user(userptr, table->private->entries, total_size) != 0) {
+ /*
+ * choose the copy that is on our node/cpu,
+ */
+ loc_cpu_entry = table->private->entries[get_cpu()];
+ /* ... then copy entire thing ... */
+ if (copy_to_user(userptr, loc_cpu_entry, total_size) != 0) {
ret = -EFAULT;
goto free_counters;
}
@@ -1155,7 +1147,7 @@
struct ipt_entry_match *m;
struct ipt_entry_target *t;
- e = (struct ipt_entry *)(table->private->entries + off);
+ e = (struct ipt_entry *)(loc_cpu_entry + off);
if (copy_to_user(userptr + off
+ offsetof(struct ipt_entry, counters),
&counters[num],
@@ -1192,6 +1184,7 @@
}
free_counters:
+ put_cpu();
vfree(counters);
return ret;
}
@@ -1224,6 +1217,60 @@
return ret;
}
+static void free_table_info(struct ipt_table_info *info)
+{
+ int cpu;
+ for_each_cpu(cpu) {
+ if (info->size <= PAGE_SIZE)
+ kfree(info->entries[cpu]);
+ else
+ vfree(info->entries[cpu]);
+ }
+ kfree(info);
+}
+
+static struct ipt_table_info *alloc_table_info(unsigned int size)
+{
+struct ipt_table_info *newinfo;
+int cpu;
+ newinfo = kzalloc(sizeof(struct ipt_table_info), GFP_KERNEL);
+ if (!newinfo)
+ return NULL;
+ newinfo->size = size;
+ for_each_cpu(cpu) {
+ if (size <= PAGE_SIZE) {
+ newinfo->entries[cpu] = kmalloc_node(size,
+ GFP_KERNEL,
+ cpu_to_node(cpu));
+ } else {
+#ifdef CONFIG_NUMA
+ struct mempolicy *oldpol;
+ mm_segment_t oldfs = get_fs();
+ DECLARE_BITMAP(mynode, MAX_NUMNODES);
+
+ oldpol = current->mempolicy;
+ mpol_get(oldpol);
+ bitmap_zero(mynode, MAX_NUMNODES);
+ set_bit(cpu_to_node(cpu), mynode);
+ set_fs(KERNEL_DS);
+ sys_set_mempolicy(MPOL_PREFERRED, mynode, MAX_NUMNODES);
+ set_fs(oldfs);
+#endif
+ newinfo->entries[cpu] = vmalloc(size);
+#ifdef CONFIG_NUMA
+ mpol_free(current->mempolicy);
+ current->mempolicy = oldpol;
+#endif
+ }
+ if (newinfo->entries[cpu] == 0) {
+ free_table_info(newinfo);
+ return NULL;
+ }
+ }
+ return newinfo;
+}
+
+
static int
do_replace(void __user *user, unsigned int len)
{
@@ -1232,6 +1279,7 @@
struct ipt_table *t;
struct ipt_table_info *newinfo, *oldinfo;
struct ipt_counters *counters;
+ void *loc_cpu_entry, *loc_cpu_old_entry;
if (copy_from_user(&tmp, user, sizeof(tmp)) != 0)
return -EFAULT;
@@ -1244,12 +1292,14 @@
if ((SMP_ALIGN(tmp.size) >> PAGE_SHIFT) + 2 > num_physpages)
return -ENOMEM;
- newinfo = vmalloc(sizeof(struct ipt_table_info)
- + SMP_ALIGN(tmp.size) * num_possible_cpus());
+ newinfo = alloc_table_info(tmp.size);
if (!newinfo)
return -ENOMEM;
-
- if (copy_from_user(newinfo->entries, user + sizeof(tmp),
+ /*
+ * choose the copy that is on our node/cpu
+ */
+ loc_cpu_entry = newinfo->entries[get_cpu()];
+ if (copy_from_user(loc_cpu_entry, user + sizeof(tmp),
tmp.size) != 0) {
ret = -EFAULT;
goto free_newinfo;
@@ -1260,10 +1310,9 @@
ret = -ENOMEM;
goto free_newinfo;
}
- memset(counters, 0, tmp.num_counters * sizeof(struct ipt_counters));
ret = translate_table(tmp.name, tmp.valid_hooks,
- newinfo, tmp.size, tmp.num_entries,
+ newinfo, loc_cpu_entry, tmp.size, tmp.num_entries,
tmp.hook_entry, tmp.underflow);
if (ret != 0)
goto free_newinfo_counters;
@@ -1302,8 +1351,10 @@
/* Get the old counters. */
get_counters(NULL, oldinfo, counters);
/* Decrease module usage counts and free resource */
- IPT_ENTRY_ITERATE(oldinfo->entries, oldinfo->size, cleanup_entry,NULL);
- vfree(oldinfo);
+ loc_cpu_old_entry = oldinfo->entries[smp_processor_id()];
+ IPT_ENTRY_ITERATE(loc_cpu_old_entry, oldinfo->size, cleanup_entry,NULL);
+ put_cpu();
+ free_table_info(oldinfo);
if (copy_to_user(tmp.counters, counters,
sizeof(struct ipt_counters) * tmp.num_counters) != 0)
ret = -EFAULT;
@@ -1315,11 +1366,12 @@
module_put(t->me);
up(&ipt_mutex);
free_newinfo_counters_untrans:
- IPT_ENTRY_ITERATE(newinfo->entries, newinfo->size, cleanup_entry,NULL);
+ IPT_ENTRY_ITERATE(loc_cpu_entry, newinfo->size, cleanup_entry,NULL);
free_newinfo_counters:
vfree(counters);
free_newinfo:
- vfree(newinfo);
+ put_cpu();
+ free_table_info(newinfo);
return ret;
}
@@ -1352,6 +1404,7 @@
struct ipt_counters_info tmp, *paddc;
struct ipt_table *t;
int ret = 0;
+ void *loc_cpu_entry;
#ifdef CONFIG_SMP
rwlock_t *lockp;
#endif
@@ -1389,7 +1442,11 @@
}
i = 0;
- IPT_ENTRY_ITERATE(t->private->entries,
+ /*
+ * choose the copy that is on our node,
+ */
+ loc_cpu_entry = t->private->entries[smp_processor_id()];
+ IPT_ENTRY_ITERATE(loc_cpu_entry,
t->private->size,
add_counter_to_entry,
paddc->counters,
@@ -1584,8 +1641,15 @@
{
int ret;
struct ipt_table_info *newinfo;
- static struct ipt_table_info bootstrap
- = { 0, 0, 0, { 0 }, { 0 }, { } };
+ static struct ipt_table_info bootstrap = {
+ .size = 0,
+ .number = 0,
+ .initial_entries = 0,
+ .hook_entry = { 0 },
+ .underflow = { 0 },
+ .entries = {NULL }
+ };
+ void *loc_cpu_entry;
#ifdef CONFIG_SMP
int cpu;
if (!table->lock_p) {
@@ -1597,26 +1661,30 @@
#else
rwlock_init(&table->lock);
#endif
- newinfo = vmalloc(sizeof(struct ipt_table_info)
- + SMP_ALIGN(repl->size) * num_possible_cpus());
+
+ newinfo = alloc_table_info(repl->size);
if (!newinfo)
return -ENOMEM;
-
- memcpy(newinfo->entries, repl->entries, repl->size);
+ /*
+ * choose the copy that is on our node/cpu
+ */
+ loc_cpu_entry = newinfo->entries[get_cpu()];
+ memcpy(loc_cpu_entry, repl->entries, repl->size);
ret = translate_table(table->name, table->valid_hooks,
- newinfo, repl->size,
+ newinfo, loc_cpu_entry, repl->size,
repl->num_entries,
repl->hook_entry,
repl->underflow);
+ put_cpu();
if (ret != 0) {
- vfree(newinfo);
+ free_table_info(newinfo);
return ret;
}
ret = down_interruptible(&ipt_mutex);
if (ret != 0) {
- vfree(newinfo);
+ free_table_info(newinfo);
return ret;
}
@@ -1644,20 +1712,25 @@
return ret;
free_unlock:
- vfree(newinfo);
+ free_table_info(newinfo);
goto unlock;
}
void ipt_unregister_table(struct ipt_table *table)
{
+ void *loc_cpu_entry;
down(&ipt_mutex);
LIST_DELETE(&ipt_tables, table);
up(&ipt_mutex);
- /* Decrease module usage counts and free resources */
- IPT_ENTRY_ITERATE(table->private->entries, table->private->size,
+ /* Decrease module usage counts and free resources
+ * choose the copy that is on our node/cpu
+ */
+ loc_cpu_entry = table->private->entries[get_cpu()];
+ IPT_ENTRY_ITERATE(loc_cpu_entry, table->private->size,
cleanup_entry, NULL);
- vfree(table->private);
+ put_cpu();
+ free_table_info(table->private);
#ifdef CONFIG_SMP
free_percpu(table->lock_p);
table->lock_p = NULL;
^ permalink raw reply
* Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
From: Christoph Lameter @ 2005-09-21 22:43 UTC (permalink / raw)
To: Eric Dumazet; +Cc: linux-kernel, netfilter-devel, netdev, Andi Kleen
In-Reply-To: <4331CFA7.50104@cosmosbay.com>
On Wed, 21 Sep 2005, Eric Dumazet wrote:
> Instead of using one vmalloc() area (located in the node of the iptables
> process), we now allocate an area for each possible CPU, using NUMA policy
> (MPOL_PREFERRED) so that memory should be allocated in the CPU's node
> if possible.
>
> If the size of ipt_table is small enough (less than one page), we use
> kmalloc_node() instead of vmalloc(), to use less memory and less TLB entries)
> in small setups.
Maybe we better introduce vmalloc_node() instead of improvising this for
several subsystems? The e1000 driver has similar issues.
^ permalink raw reply
* Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
From: David S. Miller @ 2005-09-22 0:34 UTC (permalink / raw)
To: clameter; +Cc: netdev, ak, netfilter-devel, linux-kernel, dada1
In-Reply-To: <Pine.LNX.4.62.0509211542210.13045@schroedinger.engr.sgi.com>
From: Christoph Lameter <clameter@engr.sgi.com>
Date: Wed, 21 Sep 2005 15:43:29 -0700 (PDT)
> Maybe we better introduce vmalloc_node() instead of improvising this for
> several subsystems? The e1000 driver has similar issues.
I agree.
^ permalink raw reply
* Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
From: Christoph Lameter @ 2005-09-22 1:44 UTC (permalink / raw)
To: David S. Miller; +Cc: dada1, linux-kernel, netfilter-devel, netdev, ak
In-Reply-To: <20050921.173408.122945960.davem@davemloft.net>
On Wed, 21 Sep 2005, David S. Miller wrote:
> From: Christoph Lameter <clameter@engr.sgi.com>
> Date: Wed, 21 Sep 2005 15:43:29 -0700 (PDT)
>
> > Maybe we better introduce vmalloc_node() instead of improvising this for
> > several subsystems? The e1000 driver has similar issues.
>
> I agree.
I did an implementation in June.
See http://marc.theaimsgroup.com/?l=linux-mm&m=111766643127530&w=2
Not sure if this will fit the bill. Never really tested it.
^ permalink raw reply
* Re: [PATCH] bond_main.c: fix device deregistration in init exception path
From: Jeff Garzik @ 2005-09-22 2:38 UTC (permalink / raw)
To: Andrew Morton, Florin Malita
Cc: linux-kernel, ctindel, fubar, David S. Miller, netdev
In-Reply-To: <20050917233224.2d4b3652.akpm@osdl.org>
Andrew Morton wrote:
> diff -puN drivers/net/bonding/bond_main.c~bond_mainc-fix-device-deregistration-in-init-exception drivers/net/bonding/bond_main.c
> --- devel/drivers/net/bonding/bond_main.c~bond_mainc-fix-device-deregistration-in-init-exception 2005-09-17 23:18:38.000000000 -0700
> +++ devel-akpm/drivers/net/bonding/bond_main.c 2005-09-17 23:31:02.000000000 -0700
> @@ -5039,6 +5039,14 @@ static int __init bonding_init(void)
> return 0;
>
> out_err:
> + /*
> + * rtnl_unlock() will run netdev_run_todo(), putting the
> + * thus-far-registered bonding devices into a state which
> + * unregigister_netdevice() will accept
> + */
> + rtnl_unlock();
> + rtnl_lock();
> +
Don't we want a schedule() or schedule_timeout(1) in between?
Jeff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox