* e1000: Detected Tx Unit Hang
From: Bernd Schubert @ 2008-02-15 22:52 UTC (permalink / raw)
To: netdev
Hello,
I can't login to one of our servers and just got this in an ipmi sol
session:
[18169.209181] e1000: eth0: e1000_clean_tx_irq: Detected Tx Unit Hang
[18169.209183] Tx Queue <0>
[18169.209184] TDH <e3>
[18169.209185] TDT <e3>
[18169.209186] next_to_use <e3>
[18169.209187] next_to_clean <bd>
[18169.209188] buffer_info[next_to_clean]
[18169.209189] time_stamp <10043e4d2>
[18169.209190] next_to_watch <be>
[18169.209191] jiffies <10043e6f6>
[18169.209192] next_to_watch.status <1>
[18169.256978] e1000: eth2: e1000_clean_tx_irq: Detected Tx Unit Hang
[18169.256979] Tx Queue <0>
[18169.256980] TDH <de>
[18169.256982] TDT <de>
[18169.256983] next_to_use <de>
[18169.256984] next_to_clean <bc>
[18169.256985] buffer_info[next_to_clean]
[18169.256986] time_stamp <10043e511>
[18169.256987] next_to_watch <bd>
[18169.256988] jiffies <10043e701>
[18169.256989] next_to_watch.status <1>
This is with 2.6.22.18. Is there any chance to recover the system? For some
reasons I would prefer not to reboot now.
Thanks,
Bernd
^ permalink raw reply
* Re: tbench regression in 2.6.25-rc1
From: David Miller @ 2008-02-15 23:22 UTC (permalink / raw)
To: dada1; +Cc: yanmin_zhang, herbert, linux-kernel, netdev
In-Reply-To: <47B59FFC.4030603@cosmosbay.com>
From: Eric Dumazet <dada1@cosmosbay.com>
Date: Fri, 15 Feb 2008 15:21:48 +0100
> On linux-2.6.25-rc1 x86_64 :
>
> offsetof(struct dst_entry, lastuse)=0xb0
> offsetof(struct dst_entry, __refcnt)=0xb8
> offsetof(struct dst_entry, __use)=0xbc
> offsetof(struct dst_entry, next)=0xc0
>
> So it should be optimal... I dont know why tbench prefers __refcnt being
> on 0xc0, since in this case lastuse will be on a different cache line...
>
> Each incoming IP packet will need to change lastuse, __refcnt and __use,
> so keeping them in the same cache line is a win.
>
> I suspect then that even this patch could help tbench, since it avoids
> writing lastuse...
I think your suspicions are right, and even moreso
it helps to keep __refcnt out of the same cache line
as input/output/ops which are read-almost-entirely :-)
I haven't done an exhaustive analysis, but it seems that
the write traffic to lastuse and __refcnt are about the
same. However if we find that __refcnt gets hit more
than lastuse in this workload, it explains the regression.
^ permalink raw reply
* Re: [PATCH] [XFRM] Add CONFIG_INET dependency to CONFIG_XFRM_STATISTICS
From: David Miller @ 2008-02-15 23:24 UTC (permalink / raw)
To: johfel; +Cc: netdev
In-Reply-To: <1203088123.29269.1.camel@localhost>
From: Johann Felix Soden <johfel@gmx.de>
Date: Fri, 15 Feb 2008 16:08:43 +0100
> From: Johann Felix Soden <johfel@users.sourceforge.net>
>
> With INET=n and XFRM_STATISTICS=y I get the following build failure:
>
> net/built-in.o: In function `xfrm_init':
> (.init.text+0xcd7): undefined reference to `snmp_mib_init'
>
> Signed-off-by: Johann Felix Soden <johfel@users.sourceforge.net>
> CC: Masahide NAKAMURA <nakam@linux-ipv6.org>
A fix for this has been in my net-2.6 tree for a few days,
and Linus pulled it in this morning.
^ permalink raw reply
* Re: e1000: Detected Tx Unit Hang
From: Kok, Auke @ 2008-02-15 23:29 UTC (permalink / raw)
To: Bernd Schubert; +Cc: netdev
In-Reply-To: <fp553k$9r8$1@ger.gmane.org>
Bernd Schubert wrote:
> Hello,
>
> I can't login to one of our servers and just got this in an ipmi sol
> session:
>
> [18169.209181] e1000: eth0: e1000_clean_tx_irq: Detected Tx Unit Hang
> [18169.209183] Tx Queue <0>
> [18169.209184] TDH <e3>
> [18169.209185] TDT <e3>
> [18169.209186] next_to_use <e3>
> [18169.209187] next_to_clean <bd>
> [18169.209188] buffer_info[next_to_clean]
> [18169.209189] time_stamp <10043e4d2>
> [18169.209190] next_to_watch <be>
> [18169.209191] jiffies <10043e6f6>
> [18169.209192] next_to_watch.status <1>
> [18169.256978] e1000: eth2: e1000_clean_tx_irq: Detected Tx Unit Hang
> [18169.256979] Tx Queue <0>
> [18169.256980] TDH <de>
> [18169.256982] TDT <de>
> [18169.256983] next_to_use <de>
> [18169.256984] next_to_clean <bc>
> [18169.256985] buffer_info[next_to_clean]
> [18169.256986] time_stamp <10043e511>
> [18169.256987] next_to_watch <bd>
> [18169.256988] jiffies <10043e701>
> [18169.256989] next_to_watch.status <1>
>
> This is with 2.6.22.18. Is there any chance to recover the system? For some
> reasons I would prefer not to reboot now.
if that's all you have then it was false alarm. there should be a 'netdev timeout
- link reset' following those messages. can you send some more context on those
messages?
in real tx hang cases, the hardware is reset within 2 seconds, and everything
continues as normal.
Auke
^ permalink raw reply
* Re: pull request: wireless-2.6 'fixes' 2008-02-15
From: David Miller @ 2008-02-16 0:16 UTC (permalink / raw)
To: linville-2XuSBdqkA4R54TAoqtyWWQ
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20080215191751.GA3054-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
From: "John W. Linville" <linville-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Date: Fri, 15 Feb 2008 14:17:51 -0500
> git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-2.6.git fixes
Pulled and pushed out.
If you haven't noticed, I'm going to try and keep my net-2.6
tree from rebasing for a while and see how well that works.
So you can feel free to just keep applying patches to your
fixes branch and I can pull them in cleanly.
^ permalink raw reply
* Re: [git patches] net driver fixes
From: David Miller @ 2008-02-16 0:16 UTC (permalink / raw)
To: jeff; +Cc: netdev, linux-kernel
In-Reply-To: <20080215160314.GA13609@havoc.gtf.org>
From: Jeff Garzik <jeff@garzik.org>
Date: Fri, 15 Feb 2008 11:03:14 -0500
> Please pull from 'upstream-davem' branch of
> master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/netdev-2.6.git upstream-davem
Pulled and pushed out.
As I mentioned to John Linville just now, I'm going to try
and keep net-2.6 running as long as I can without rebasing.
So you can just keep piling patches into your upstream-davem
branch without fear of my turning your world upside down
with a rebase :-)
^ permalink raw reply
* Re: e1000: Detected Tx Unit Hang
From: Bernd Schubert @ 2008-02-16 0:26 UTC (permalink / raw)
To: Kok, Auke; +Cc: netdev
In-Reply-To: <47B6204D.8060503@intel.com>
On Saturday 16 February 2008, Kok, Auke wrote:
> Bernd Schubert wrote:
> > Hello,
> >
> > I can't login to one of our servers and just got this in an ipmi sol
> > session:
> >
> > [18169.209181] e1000: eth0: e1000_clean_tx_irq: Detected Tx Unit Hang
> > [18169.209183] Tx Queue <0>
> > [18169.209184] TDH <e3>
> > [18169.209185] TDT <e3>
> > [18169.209186] next_to_use <e3>
> > [18169.209187] next_to_clean <bd>
> > [18169.209188] buffer_info[next_to_clean]
> > [18169.209189] time_stamp <10043e4d2>
> > [18169.209190] next_to_watch <be>
> > [18169.209191] jiffies <10043e6f6>
> > [18169.209192] next_to_watch.status <1>
> > [18169.256978] e1000: eth2: e1000_clean_tx_irq: Detected Tx Unit Hang
> > [18169.256979] Tx Queue <0>
> > [18169.256980] TDH <de>
> > [18169.256982] TDT <de>
> > [18169.256983] next_to_use <de>
> > [18169.256984] next_to_clean <bc>
> > [18169.256985] buffer_info[next_to_clean]
> > [18169.256986] time_stamp <10043e511>
> > [18169.256987] next_to_watch <bd>
> > [18169.256988] jiffies <10043e701>
> > [18169.256989] next_to_watch.status <1>
> >
> > This is with 2.6.22.18. Is there any chance to recover the system? For
> > some reasons I would prefer not to reboot now.
>
> if that's all you have then it was false alarm. there should be a 'netdev
> timeout - link reset' following those messages. can you send some more
> context on those messages?
All I presently know is that there are 20 servers and login doesn't work any
more - sysrq+t does show me it hangs in fuse, which is accessing the
underlying nfs (we are using unionfs-fuse). While I checked the sysrq-t
output suddenly these e1000 messages appeared.
Thinking a bit about it, it either could be 2.6.22.18 has an e1000 bug, which
2.6.22.X didn't have (X=16, I think, but I'm not sure) or someone
mis-configured the switch/network environment today.
Hmm, now that I think about the last part, there already had been other
networking problems today, which were supposed to be fixed several hours ago.
Seems they didn't fix it properly.
>
> in real tx hang cases, the hardware is reset within 2 seconds, and
> everything continues as normal.
Thanks, this gives me hope I don't need to reboot the serves (reboot would
mean I would need to start 60 md-raid rebuilds...).
Thanks,
Bernd
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Andrew Morton @ 2008-02-16 0:40 UTC (permalink / raw)
To: paulmck; +Cc: linux-kernel, shemminger, davem, netdev, dipankar, ego, herbert
In-Reply-To: <20080213220024.GA30729@linux.vnet.ibm.com>
On Wed, 13 Feb 2008 14:00:24 -0800
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> Hello!
>
> This is an updated version of the patch posted last November:
>
> http://archives.free.net.ph/message/20071201.003721.cd6ff17c.en.html
>
> This new version permits arguments with side effects, for example:
>
> rcu_assign_pointer(global_p, p++);
>
> and also verifies that the arguments are pointers, while still avoiding
> the unnecessary memory barrier when assigning NULL to a pointer.
> This memory-barrier avoidance means that rcu_assign_pointer() is now only
> permitted for pointers (not array indexes), and so this version emits a
> compiler warning if the first argument is not a pointer. I built a "make
> allyesconfig" version on an x86 system, and received no such warnings.
> If RCU is ever applied to array indexes, then the second patch in this
> series should be applied, and the resulting rcu_assign_index() be used.
>
> Given the rather surprising history of subtlely broken implementations of
> rcu_assign_pointer(), I took the precaution of generating a full set of
> test cases and verified that memory barriers and compiler warnings were
> emitted when required. I guess it is the simple things that get you...
>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>
> rcupdate.h | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff -urpNa -X dontdiff linux-2.6.24/include/linux/rcupdate.h linux-2.6.24-rap/include/linux/rcupdate.h
> --- linux-2.6.24/include/linux/rcupdate.h 2008-01-24 14:58:37.000000000 -0800
> +++ linux-2.6.24-rap/include/linux/rcupdate.h 2008-02-13 13:36:47.000000000 -0800
> @@ -270,12 +270,20 @@ extern struct lockdep_map rcu_lock_map;
> * structure after the pointer assignment. More importantly, this
> * call documents which pointers will be dereferenced by RCU read-side
> * code.
> + *
> + * Throws a compiler warning for non-pointer arguments.
> + *
> + * Does not insert a memory barrier for a NULL pointer.
> */
>
> -#define rcu_assign_pointer(p, v) ({ \
> - smp_wmb(); \
> - (p) = (v); \
> - })
> +#define rcu_assign_pointer(p, v) \
> + ({ \
> + typeof(*p) *_________p1 = (v); \
> + \
> + if (!__builtin_constant_p(v) || (_________p1 != NULL)) \
> + smp_wmb(); \
> + (p) = _________p1; \
> + })
>
umm...
net/netfilter/core.c: In function 'nf_register_afinfo':
net/netfilter/core.c:39: warning: initialization discards qualifiers from pointer target type
net/netfilter/nf_log.c: In function 'nf_log_register':
net/netfilter/nf_log.c:37: warning: initialization discards qualifiers from pointer target type
net/netfilter/nf_queue.c: In function 'nf_register_queue_handler':
net/netfilter/nf_queue.c:38: warning: initialization discards qualifiers from pointer target type
^ permalink raw reply
* Re: [git patches] net driver fixes
From: Jeff Garzik @ 2008-02-16 0:52 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-kernel
In-Reply-To: <20080215.161651.32391345.davem@davemloft.net>
David Miller wrote:
> From: Jeff Garzik <jeff@garzik.org>
> Date: Fri, 15 Feb 2008 11:03:14 -0500
>
>> Please pull from 'upstream-davem' branch of
>> master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/netdev-2.6.git upstream-davem
>
> Pulled and pushed out.
>
> As I mentioned to John Linville just now, I'm going to try
> and keep net-2.6 running as long as I can without rebasing.
>
> So you can just keep piling patches into your upstream-davem
> branch without fear of my turning your world upside down
> with a rebase :-)
Thanks, it's appreciated!
Jeff
^ permalink raw reply
* [PATCH] (02/15/08 Linus git) Smack unlabeled outgoing ambient packets - v4
From: Casey Schaufler @ 2008-02-15 23:24 UTC (permalink / raw)
To: akpm, torvalds, linux-kernel, netdev, paul.moore, casey
From: Casey Schaufler <casey@schaufler-ca.com>
Smack uses CIPSO labeling, but allows for unlabeled packets
by specifying an "ambient" label that is applied to incoming
unlabeled packets. Because the other end of the connection
may dislike IP options, and ssh is one know application that
behaves thus, it is prudent to respond in kind. This patch
changes the network labeling behavior such that an outgoing
packet that would be given a CIPSO label that matches the
ambient label is left unlabeled. An "unlbl" domain is added
and the netlabel defaulting mechanism invoked rather than
assuming that everything is CIPSO. Locking has been added
around changes to the ambient label as the mechanisms used
to do so are more involved.
Cleaned up some issues noted in review.
Make smk_cipso_doi() static.
Create a hook for the new security_secctx_to_secid()
using existing underlying code.
Fill in audit data for netlbl domain calls.
Collapse unnecessary multiple assignments.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
security/smack/smack_lsm.c | 36 ++++++++++++++++----
security/smack/smackfs.c | 61 ++++++++++++++++++++++++++---------
2 files changed, 74 insertions(+), 23 deletions(-)
diff -uprN -X linux-2.6.25-g0215-base//Documentation/dontdiff linux-2.6.25-g0215-base/security/smack/smackfs.c linux-2.6.25-g0215/security/smack/smackfs.c
--- linux-2.6.25-g0215-base/security/smack/smackfs.c 2008-02-15 14:25:37.000000000 -0800
+++ linux-2.6.25-g0215/security/smack/smackfs.c 2008-02-15 14:30:36.000000000 -0800
@@ -24,6 +24,7 @@
#include <net/cipso_ipv4.h>
#include <linux/seq_file.h>
#include <linux/ctype.h>
+#include <linux/audit.h>
#include "smack.h"
/*
@@ -45,6 +46,7 @@ enum smk_inos {
*/
static DEFINE_MUTEX(smack_list_lock);
static DEFINE_MUTEX(smack_cipso_lock);
+static DEFINE_MUTEX(smack_ambient_lock);
/*
* This is the "ambient" label for network traffic.
@@ -342,6 +344,9 @@ void smk_cipso_doi(void)
struct cipso_v4_doi *doip;
struct netlbl_audit audit_info;
+ audit_info.loginuid = audit_get_loginuid(current);
+ audit_info.secid = smack_to_secid(current->security);
+
rc = netlbl_cfg_map_del(NULL, &audit_info);
if (rc != 0)
printk(KERN_WARNING "%s:%d remove rc = %d\n",
@@ -363,6 +368,30 @@ void smk_cipso_doi(void)
__func__, __LINE__, rc);
}
+/**
+ * smk_unlbl_ambient - initialize the unlabeled domain
+ */
+void smk_unlbl_ambient(char *oldambient)
+{
+ int rc;
+ struct netlbl_audit audit_info;
+
+ audit_info.loginuid = audit_get_loginuid(current);
+ audit_info.secid = smack_to_secid(current->security);
+
+ if (oldambient != NULL) {
+ rc = netlbl_cfg_map_del(oldambient, &audit_info);
+ if (rc != 0)
+ printk(KERN_WARNING "%s:%d remove rc = %d\n",
+ __func__, __LINE__, rc);
+ }
+
+ rc = netlbl_cfg_unlbl_add_map(smack_net_ambient, &audit_info);
+ if (rc != 0)
+ printk(KERN_WARNING "%s:%d add rc = %d\n",
+ __func__, __LINE__, rc);
+}
+
/*
* Seq_file read operations for /smack/cipso
*/
@@ -709,7 +738,6 @@ static ssize_t smk_read_ambient(struct f
size_t cn, loff_t *ppos)
{
ssize_t rc;
- char out[SMK_LABELLEN];
int asize;
if (*ppos != 0)
@@ -717,23 +745,18 @@ static ssize_t smk_read_ambient(struct f
/*
* Being careful to avoid a problem in the case where
* smack_net_ambient gets changed in midstream.
- * Since smack_net_ambient is always set with a value
- * from the label list, including initially, and those
- * never get freed, the worst case is that the pointer
- * gets changed just after this strncpy, in which case
- * the value passed up is incorrect. Locking around
- * smack_net_ambient wouldn't be any better than this
- * copy scheme as by the time the caller got to look
- * at the ambient value it would have cleared the lock
- * and been changed.
*/
- strncpy(out, smack_net_ambient, SMK_LABELLEN);
- asize = strlen(out) + 1;
+ mutex_lock(&smack_ambient_lock);
- if (cn < asize)
- return -EINVAL;
+ asize = strlen(smack_net_ambient) + 1;
+
+ if (cn >= asize)
+ rc = simple_read_from_buffer(buf, cn, ppos,
+ smack_net_ambient, asize);
+ else
+ rc = -EINVAL;
- rc = simple_read_from_buffer(buf, cn, ppos, out, asize);
+ mutex_unlock(&smack_ambient_lock);
return rc;
}
@@ -751,6 +774,7 @@ static ssize_t smk_write_ambient(struct
size_t count, loff_t *ppos)
{
char in[SMK_LABELLEN];
+ char *oldambient;
char *smack;
if (!capable(CAP_MAC_ADMIN))
@@ -766,7 +790,13 @@ static ssize_t smk_write_ambient(struct
if (smack == NULL)
return -EINVAL;
+ mutex_lock(&smack_ambient_lock);
+
+ oldambient = smack_net_ambient;
smack_net_ambient = smack;
+ smk_unlbl_ambient(oldambient);
+
+ mutex_unlock(&smack_ambient_lock);
return count;
}
@@ -974,6 +1004,7 @@ static int __init init_smk_fs(void)
sema_init(&smack_write_sem, 1);
smk_cipso_doi();
+ smk_unlbl_ambient(NULL);
return err;
}
diff -uprN -X linux-2.6.25-g0215-base//Documentation/dontdiff linux-2.6.25-g0215-base/security/smack/smack_lsm.c linux-2.6.25-g0215/security/smack/smack_lsm.c
--- linux-2.6.25-g0215-base/security/smack/smack_lsm.c 2008-02-15 14:25:37.000000000 -0800
+++ linux-2.6.25-g0215/security/smack/smack_lsm.c 2008-02-15 14:31:21.000000000 -0800
@@ -1251,9 +1251,8 @@ static void smack_to_secattr(char *smack
switch (smack_net_nltype) {
case NETLBL_NLTYPE_CIPSOV4:
- nlsp->domain = NULL;
- nlsp->flags = NETLBL_SECATTR_DOMAIN;
- nlsp->flags |= NETLBL_SECATTR_MLS_LVL;
+ nlsp->domain = kstrdup(smack, GFP_ATOMIC);
+ nlsp->flags = NETLBL_SECATTR_DOMAIN | NETLBL_SECATTR_MLS_LVL;
rc = smack_to_cipso(smack, &cipso);
if (rc == 0) {
@@ -1282,15 +1281,14 @@ static int smack_netlabel(struct sock *s
{
struct socket_smack *ssp;
struct netlbl_lsm_secattr secattr;
- int rc = 0;
+ int rc;
ssp = sk->sk_security;
netlbl_secattr_init(&secattr);
smack_to_secattr(ssp->smk_out, &secattr);
- if (secattr.flags != NETLBL_SECATTR_NONE)
- rc = netlbl_sock_setattr(sk, &secattr);
-
+ rc = netlbl_sock_setattr(sk, &secattr);
netlbl_secattr_destroy(&secattr);
+
return rc;
}
@@ -1313,6 +1311,7 @@ static int smack_inode_setsecurity(struc
struct inode_smack *nsp = inode->i_security;
struct socket_smack *ssp;
struct socket *sock;
+ int rc = 0;
if (value == NULL || size > SMK_LABELLEN)
return -EACCES;
@@ -1341,7 +1340,10 @@ static int smack_inode_setsecurity(struc
ssp->smk_in = sp;
else if (strcmp(name, XATTR_SMACK_IPOUT) == 0) {
ssp->smk_out = sp;
- return smack_netlabel(sock->sk);
+ rc = smack_netlabel(sock->sk);
+ if (rc != 0)
+ printk(KERN_WARNING "Smack: \"%s\" netlbl error %d.\n",
+ __func__, -rc);
} else
return -EOPNOTSUPP;
@@ -2214,6 +2216,9 @@ static void smack_sock_graft(struct sock
ssp->smk_packet[0] = '\0';
rc = smack_netlabel(sk);
+ if (rc != 0)
+ printk(KERN_WARNING "Smack: \"%s\" netlbl error %d.\n",
+ __func__, -rc);
}
/**
@@ -2346,6 +2351,20 @@ static int smack_secid_to_secctx(u32 sec
}
/*
+ * smack_secctx_to_secid - return the secid for a smack label
+ * @secdata: smack label
+ * @seclen: how long result is
+ * @secid: outgoing integer
+ *
+ * Exists for audit and networking code.
+ */
+static int smack_secctx_to_secid(char *secdata, u32 seclen, u32 *secid)
+{
+ *secid = smack_to_secid(secdata);
+ return 0;
+}
+
+/*
* smack_release_secctx - don't do anything.
* @key_ref: unused
* @context: unused
@@ -2475,6 +2494,7 @@ static struct security_operations smack_
.key_permission = smack_key_permission,
#endif /* CONFIG_KEYS */
.secid_to_secctx = smack_secid_to_secctx,
+ .secctx_to_secid = smack_secctx_to_secid,
.release_secctx = smack_release_secctx,
};
^ permalink raw reply
* Re: [PATCH 1/2] remove rcu_assign_pointer(NULL) penalty with type/macro safety
From: Paul E. McKenney @ 2008-02-16 1:23 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, shemminger, davem, netdev, dipankar, ego, herbert
In-Reply-To: <20080215164024.7fd356fa.akpm@linux-foundation.org>
On Fri, Feb 15, 2008 at 04:40:24PM -0800, Andrew Morton wrote:
> On Wed, 13 Feb 2008 14:00:24 -0800
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
>
> > Hello!
> >
> > This is an updated version of the patch posted last November:
> >
> > http://archives.free.net.ph/message/20071201.003721.cd6ff17c.en.html
> >
> > This new version permits arguments with side effects, for example:
> >
> > rcu_assign_pointer(global_p, p++);
> >
> > and also verifies that the arguments are pointers, while still avoiding
> > the unnecessary memory barrier when assigning NULL to a pointer.
> > This memory-barrier avoidance means that rcu_assign_pointer() is now only
> > permitted for pointers (not array indexes), and so this version emits a
> > compiler warning if the first argument is not a pointer. I built a "make
> > allyesconfig" version on an x86 system, and received no such warnings.
> > If RCU is ever applied to array indexes, then the second patch in this
> > series should be applied, and the resulting rcu_assign_index() be used.
> >
> > Given the rather surprising history of subtlely broken implementations of
> > rcu_assign_pointer(), I took the precaution of generating a full set of
> > test cases and verified that memory barriers and compiler warnings were
> > emitted when required. I guess it is the simple things that get you...
> >
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> >
> > rcupdate.h | 16 ++++++++++++----
> > 1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff -urpNa -X dontdiff linux-2.6.24/include/linux/rcupdate.h linux-2.6.24-rap/include/linux/rcupdate.h
> > --- linux-2.6.24/include/linux/rcupdate.h 2008-01-24 14:58:37.000000000 -0800
> > +++ linux-2.6.24-rap/include/linux/rcupdate.h 2008-02-13 13:36:47.000000000 -0800
> > @@ -270,12 +270,20 @@ extern struct lockdep_map rcu_lock_map;
> > * structure after the pointer assignment. More importantly, this
> > * call documents which pointers will be dereferenced by RCU read-side
> > * code.
> > + *
> > + * Throws a compiler warning for non-pointer arguments.
> > + *
> > + * Does not insert a memory barrier for a NULL pointer.
> > */
> >
> > -#define rcu_assign_pointer(p, v) ({ \
> > - smp_wmb(); \
> > - (p) = (v); \
> > - })
> > +#define rcu_assign_pointer(p, v) \
> > + ({ \
> > + typeof(*p) *_________p1 = (v); \
> > + \
> > + if (!__builtin_constant_p(v) || (_________p1 != NULL)) \
> > + smp_wmb(); \
> > + (p) = _________p1; \
> > + })
> >
>
> umm...
>
> net/netfilter/core.c: In function 'nf_register_afinfo':
> net/netfilter/core.c:39: warning: initialization discards qualifiers from pointer target type
> net/netfilter/nf_log.c: In function 'nf_log_register':
> net/netfilter/nf_log.c:37: warning: initialization discards qualifiers from pointer target type
> net/netfilter/nf_queue.c: In function 'nf_register_queue_handler':
> net/netfilter/nf_queue.c:38: warning: initialization discards qualifiers from pointer target type
Hmmm... Netfilter compiles cleanly here. My guess is that your gcc
is more fastidious about const declarations. Could you please either
let me know what arch/gcc-settings you are using, or, alternatively,
see if the following patch fixes things up? The comparison against
NULL should at least emit warnings for non-pointer types -- not as
good as an error, but better than emitting bogus warnings.
So I guess I should stick with simple things like preemptable RCU instead
of the much more difficult task of outsmarting gcc...
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
rcupdate.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- rcupdate.h.old 2008-02-15 17:18:50.000000000 -0800
+++ rcupdate.h 2008-02-15 17:18:52.000000000 -0800
@@ -176,7 +176,7 @@ struct rcu_head {
#define rcu_assign_pointer(p, v) \
({ \
- typeof(*p) *_________p1 = (v); \
+ typeof(p) _________p1 = (v); \
\
if (!__builtin_constant_p(v) || ((_________p1) != NULL)) \
smp_wmb(); \
^ permalink raw reply
* Re: [PATCH] [XFRM] Add CONFIG_INET dependency to CONFIG_XFRM_STATISTICS
From: Masahide NAKAMURA @ 2008-02-16 2:20 UTC (permalink / raw)
To: David Miller; +Cc: johfel, netdev
In-Reply-To: <20080215.152400.65001698.davem@davemloft.net>
Saturday 16 February 2008 08:24, David Miller wrote:
> From: Johann Felix Soden <johfel@gmx.de>
> Date: Fri, 15 Feb 2008 16:08:43 +0100
>
> > From: Johann Felix Soden <johfel@users.sourceforge.net>
> >
> > With INET=n and XFRM_STATISTICS=y I get the following build failure:
> >
> > net/built-in.o: In function `xfrm_init':
> > (.init.text+0xcd7): undefined reference to `snmp_mib_init'
> >
> > Signed-off-by: Johann Felix Soden <johfel@users.sourceforge.net>
> > CC: Masahide NAKAMURA <nakam@linux-ipv6.org>
>
> A fix for this has been in my net-2.6 tree for a few days,
> and Linus pulled it in this morning.
Thank you guys for taking care of it.
--
Masahide NAKAMURA
^ permalink raw reply
* Re: [PATCH] (02/15/08 Linus git) Smack unlabeled outgoing ambient packets - v4
From: Paul Moore @ 2008-02-16 3:13 UTC (permalink / raw)
To: casey; +Cc: akpm, torvalds, linux-kernel, netdev
In-Reply-To: <47B61F29.3040203@schaufler-ca.com>
On Friday 15 February 2008 6:24:25 pm Casey Schaufler wrote:
> From: Casey Schaufler <casey@schaufler-ca.com>
>
> Smack uses CIPSO labeling, but allows for unlabeled packets
> by specifying an "ambient" label that is applied to incoming
> unlabeled packets. Because the other end of the connection
> may dislike IP options, and ssh is one know application that
> behaves thus, it is prudent to respond in kind. This patch
> changes the network labeling behavior such that an outgoing
> packet that would be given a CIPSO label that matches the
> ambient label is left unlabeled. An "unlbl" domain is added
> and the netlabel defaulting mechanism invoked rather than
> assuming that everything is CIPSO. Locking has been added
> around changes to the ambient label as the mechanisms used
> to do so are more involved.
>
> Cleaned up some issues noted in review.
> Make smk_cipso_doi() static.
> Create a hook for the new security_secctx_to_secid()
> using existing underlying code.
> Fill in audit data for netlbl domain calls.
> Collapse unnecessary multiple assignments.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Looks good to me, thanks for making those changes.
Acked-by: Paul Moore <paul.moore@hp.com>
> ---
>
> security/smack/smack_lsm.c | 36 ++++++++++++++++----
> security/smack/smackfs.c | 61 ++++++++++++++++++++++++++---------
> 2 files changed, 74 insertions(+), 23 deletions(-)
>
> diff -uprN -X linux-2.6.25-g0215-base//Documentation/dontdiff
> linux-2.6.25-g0215-base/security/smack/smackfs.c
> linux-2.6.25-g0215/security/smack/smackfs.c ---
> linux-2.6.25-g0215-base/security/smack/smackfs.c 2008-02-15
> 14:25:37.000000000 -0800 +++
> linux-2.6.25-g0215/security/smack/smackfs.c 2008-02-15 14:30:36.000000000
> -0800 @@ -24,6 +24,7 @@
> #include <net/cipso_ipv4.h>
> #include <linux/seq_file.h>
> #include <linux/ctype.h>
> +#include <linux/audit.h>
> #include "smack.h"
>
> /*
> @@ -45,6 +46,7 @@ enum smk_inos {
> */
> static DEFINE_MUTEX(smack_list_lock);
> static DEFINE_MUTEX(smack_cipso_lock);
> +static DEFINE_MUTEX(smack_ambient_lock);
>
> /*
> * This is the "ambient" label for network traffic.
> @@ -342,6 +344,9 @@ void smk_cipso_doi(void)
> struct cipso_v4_doi *doip;
> struct netlbl_audit audit_info;
>
> + audit_info.loginuid = audit_get_loginuid(current);
> + audit_info.secid = smack_to_secid(current->security);
> +
> rc = netlbl_cfg_map_del(NULL, &audit_info);
> if (rc != 0)
> printk(KERN_WARNING "%s:%d remove rc = %d\n",
> @@ -363,6 +368,30 @@ void smk_cipso_doi(void)
> __func__, __LINE__, rc);
> }
>
> +/**
> + * smk_unlbl_ambient - initialize the unlabeled domain
> + */
> +void smk_unlbl_ambient(char *oldambient)
> +{
> + int rc;
> + struct netlbl_audit audit_info;
> +
> + audit_info.loginuid = audit_get_loginuid(current);
> + audit_info.secid = smack_to_secid(current->security);
> +
> + if (oldambient != NULL) {
> + rc = netlbl_cfg_map_del(oldambient, &audit_info);
> + if (rc != 0)
> + printk(KERN_WARNING "%s:%d remove rc = %d\n",
> + __func__, __LINE__, rc);
> + }
> +
> + rc = netlbl_cfg_unlbl_add_map(smack_net_ambient, &audit_info);
> + if (rc != 0)
> + printk(KERN_WARNING "%s:%d add rc = %d\n",
> + __func__, __LINE__, rc);
> +}
> +
> /*
> * Seq_file read operations for /smack/cipso
> */
> @@ -709,7 +738,6 @@ static ssize_t smk_read_ambient(struct f
> size_t cn, loff_t *ppos)
> {
> ssize_t rc;
> - char out[SMK_LABELLEN];
> int asize;
>
> if (*ppos != 0)
> @@ -717,23 +745,18 @@ static ssize_t smk_read_ambient(struct f
> /*
> * Being careful to avoid a problem in the case where
> * smack_net_ambient gets changed in midstream.
> - * Since smack_net_ambient is always set with a value
> - * from the label list, including initially, and those
> - * never get freed, the worst case is that the pointer
> - * gets changed just after this strncpy, in which case
> - * the value passed up is incorrect. Locking around
> - * smack_net_ambient wouldn't be any better than this
> - * copy scheme as by the time the caller got to look
> - * at the ambient value it would have cleared the lock
> - * and been changed.
> */
> - strncpy(out, smack_net_ambient, SMK_LABELLEN);
> - asize = strlen(out) + 1;
> + mutex_lock(&smack_ambient_lock);
>
> - if (cn < asize)
> - return -EINVAL;
> + asize = strlen(smack_net_ambient) + 1;
> +
> + if (cn >= asize)
> + rc = simple_read_from_buffer(buf, cn, ppos,
> + smack_net_ambient, asize);
> + else
> + rc = -EINVAL;
>
> - rc = simple_read_from_buffer(buf, cn, ppos, out, asize);
> + mutex_unlock(&smack_ambient_lock);
>
> return rc;
> }
> @@ -751,6 +774,7 @@ static ssize_t smk_write_ambient(struct
> size_t count, loff_t *ppos)
> {
> char in[SMK_LABELLEN];
> + char *oldambient;
> char *smack;
>
> if (!capable(CAP_MAC_ADMIN))
> @@ -766,7 +790,13 @@ static ssize_t smk_write_ambient(struct
> if (smack == NULL)
> return -EINVAL;
>
> + mutex_lock(&smack_ambient_lock);
> +
> + oldambient = smack_net_ambient;
> smack_net_ambient = smack;
> + smk_unlbl_ambient(oldambient);
> +
> + mutex_unlock(&smack_ambient_lock);
>
> return count;
> }
> @@ -974,6 +1004,7 @@ static int __init init_smk_fs(void)
>
> sema_init(&smack_write_sem, 1);
> smk_cipso_doi();
> + smk_unlbl_ambient(NULL);
>
> return err;
> }
> diff -uprN -X linux-2.6.25-g0215-base//Documentation/dontdiff
> linux-2.6.25-g0215-base/security/smack/smack_lsm.c
> linux-2.6.25-g0215/security/smack/smack_lsm.c ---
> linux-2.6.25-g0215-base/security/smack/smack_lsm.c 2008-02-15
> 14:25:37.000000000 -0800 +++
> linux-2.6.25-g0215/security/smack/smack_lsm.c 2008-02-15 14:31:21.000000000
> -0800 @@ -1251,9 +1251,8 @@ static void smack_to_secattr(char *smack
>
> switch (smack_net_nltype) {
> case NETLBL_NLTYPE_CIPSOV4:
> - nlsp->domain = NULL;
> - nlsp->flags = NETLBL_SECATTR_DOMAIN;
> - nlsp->flags |= NETLBL_SECATTR_MLS_LVL;
> + nlsp->domain = kstrdup(smack, GFP_ATOMIC);
> + nlsp->flags = NETLBL_SECATTR_DOMAIN | NETLBL_SECATTR_MLS_LVL;
>
> rc = smack_to_cipso(smack, &cipso);
> if (rc == 0) {
> @@ -1282,15 +1281,14 @@ static int smack_netlabel(struct sock *s
> {
> struct socket_smack *ssp;
> struct netlbl_lsm_secattr secattr;
> - int rc = 0;
> + int rc;
>
> ssp = sk->sk_security;
> netlbl_secattr_init(&secattr);
> smack_to_secattr(ssp->smk_out, &secattr);
> - if (secattr.flags != NETLBL_SECATTR_NONE)
> - rc = netlbl_sock_setattr(sk, &secattr);
> -
> + rc = netlbl_sock_setattr(sk, &secattr);
> netlbl_secattr_destroy(&secattr);
> +
> return rc;
> }
>
> @@ -1313,6 +1311,7 @@ static int smack_inode_setsecurity(struc
> struct inode_smack *nsp = inode->i_security;
> struct socket_smack *ssp;
> struct socket *sock;
> + int rc = 0;
>
> if (value == NULL || size > SMK_LABELLEN)
> return -EACCES;
> @@ -1341,7 +1340,10 @@ static int smack_inode_setsecurity(struc
> ssp->smk_in = sp;
> else if (strcmp(name, XATTR_SMACK_IPOUT) == 0) {
> ssp->smk_out = sp;
> - return smack_netlabel(sock->sk);
> + rc = smack_netlabel(sock->sk);
> + if (rc != 0)
> + printk(KERN_WARNING "Smack: \"%s\" netlbl error %d.\n",
> + __func__, -rc);
> } else
> return -EOPNOTSUPP;
>
> @@ -2214,6 +2216,9 @@ static void smack_sock_graft(struct sock
> ssp->smk_packet[0] = '\0';
>
> rc = smack_netlabel(sk);
> + if (rc != 0)
> + printk(KERN_WARNING "Smack: \"%s\" netlbl error %d.\n",
> + __func__, -rc);
> }
>
> /**
> @@ -2346,6 +2351,20 @@ static int smack_secid_to_secctx(u32 sec
> }
>
> /*
> + * smack_secctx_to_secid - return the secid for a smack label
> + * @secdata: smack label
> + * @seclen: how long result is
> + * @secid: outgoing integer
> + *
> + * Exists for audit and networking code.
> + */
> +static int smack_secctx_to_secid(char *secdata, u32 seclen, u32 *secid)
> +{
> + *secid = smack_to_secid(secdata);
> + return 0;
> +}
> +
> +/*
> * smack_release_secctx - don't do anything.
> * @key_ref: unused
> * @context: unused
> @@ -2475,6 +2494,7 @@ static struct security_operations smack_
> .key_permission = smack_key_permission,
> #endif /* CONFIG_KEYS */
> .secid_to_secctx = smack_secid_to_secctx,
> + .secctx_to_secid = smack_secctx_to_secid,
> .release_secctx = smack_release_secctx,
> };
--
paul moore
linux security @ hp
^ permalink raw reply
* Re: include/linux/pcounter.h
From: Andrew Morton @ 2008-02-16 3:37 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, netdev, linux-kernel
In-Reply-To: <20080204014402.1c55d3fe.akpm@linux-foundation.org>
- First up, why was this added at all? We have percpu_counter.h which
has several years development invested in it. afaict it would suit the
present applications of pcounters.
If some deficiency in percpu_counters has been identified, is it
possible to correct that deficiency rather than implementing a whole new
counter type? That would be much better.
- The comments in pcounter.h appear to indicate that there is a
performance advantage (and we infer that the advantage is when the
statically-allocated flavour of pcounters is used). When compared with
percpu_counters the number of data-reference indirections is the same as
with percpu_counters, so no advantage there.
And, bizarrely, because of a quite inappropriate abstraction toy, both
flavours of pcounters add an indirect function call which I believe is
significantly more expensive than a plain old pointer indirection.
So it's quite possible that DEFINE_PCOUNTER-style counters consume more
memory and are slower than percpu_counters. They will surely be much
slower on the read side. More below.
If we really want to put some helper wrappers around
DEFINE_PER_CPU(s32) then I'd suggest that we should do that as a
standalone thing and not attempt to graft the same interface onto two
quite different types of storage (DEFINE_PER_CPU and alloc_per_cpu)
- The comment "2)" in pcounter.h (which overflows 80 cols and probably
wasn't passed through checkpatch) indicates that some other
implementation (presumably plain old DEFINE_PER_CPU) will use
NR_CPUS*(32+sizeof(void *)) bytes of storage. But DEFINE_PCOUNTER will
use as much memory as DEFINE_PER_CPU(s32) and both pcounter_alloc()-style
pcounters and percpu_counters use
num_possible_cpus()*sizeof(s32)+epsilon.
- The CONFIG_SMP=n stubs in pcounter.h are cheesy and are vulnerable to
several well-known compilation risks which I always forget. Should be
converted to regular static inlines.
- the comment in lib/pcounter.c needlessly exceeds 80 cols.
- pcounter_dyn_add() will spew a
use-of-smp_processor_id()-in-preemptible-code warning if used in places
where one could reasonably use it. The interface could do with a bit of
a rethink. Or at least some justification and documentation.
- pcounter_getval() will be disastrously inefficient if
num_possible_cpus() is much greater than num_online_cpus(). It should
use for_each_online_cpu() (as does percpu_counter), and implement a CPU
hotplug notifier (as does percpu_counter).
It will remain grossly inefficient at high CPU counts, unlike
percpu_counters, which solved this problem by doing a batched spill into
a central counter at add/sub time.
The danger here is that someone will actually use this interface in new
code. Six months later (when it's too late to fix it) the big-NUMA guys
come up and say "whaa, when our user does <this> it disabled interrupts
for N milliseconds".
- pcounter_getval() can return incorrect negative numbers. This can
cause caller malfunctions in very rare situations because callers just
don't expect the things which they're counting to go negative.
We experienced this during the evolution of percpu_counter. See
percpu_counter_sum_positive() and friends.
- pcounter_alloc() should return -ENOMEM on allocation error, not "1".
- pcounter_free() perhaps shouldn't test for (self->per_cpu_values !=
NULL), because callers shouldn't be calling it if pcounter_alloc() failed
(arguable).
afaict the whole implementation can and should be removed and replaced with
percpu_counters. I don't think there's much point in its ability to manage
DEFINE_PER_CPU counters: pcounter_getval() remains grossly inefficient (and
can return negative values) and quite a bit of new code will need to be put
in place to address that.
But perhaps there are plans to evolve it into something further in the
future, I don't know. But I would suggest that the people who have worked
upon percpu_counters (principally Gautham, Peter Z, clameter and me) be
involved in that work.
^ permalink raw reply
* [RFC] Best method to control a "transmit-only" mode on fiber NICs (specifically sky2)
From: Kyle Moffett @ 2008-02-16 3:41 UTC (permalink / raw)
To: Linux NetDev, LKML, Stephen Hemminger
Hi,
The company I'm working for has an unusual fiber NIC configuration
that we use for one of our network appliances. We connect only a
single fiber from the TX port on one NIC to the RX port on another
NIC, providing a physically-one-way path for enhanced security.
Unfortunately this doesn't work with most NIC drivers, as even with
auto-negotiation off they look for link probe pulses before they
consider the link "up" and are willing to send packets. We have been
able to use Myricom 10GigE NICs with a custom firmware image. More
recently we have patched the sky2 driver to turn on the FIB_FORCE_LNK
flag in the PHY control register; this seems to work on the
Marvell-chipset boards we have here.
What would be the preferred way to control this "force link" flag?
Right now we are accessing it using ethtool; we have added an
additional "duplex" mode: "DUPLEX_TXONLY", with a value of 2. When
you specify a speed and turn off autonegotiation ("./patched-ethtool
-s eth2 speed 1000 autoneg off duplex txonly"), it will turn on the
specified bit in the PHY control register and the link will
automatically come up. We also have one related bug-fix^Wdirty hack
for sky2 to reset the PHY a second time during netif-up after enabling
interrupts; otherwise the immediate "link up" interrupt gets lost.
Once I get approval from the company I will patch the post itself for
review.
I look forward to your comments and suggestions
Cheers,
Kyle Moffett
^ permalink raw reply
* Re: [PATCH] (02/15/08 Linus git) Smack unlabeled outgoing ambient packets - v4
From: Casey Schaufler @ 2008-02-16 4:36 UTC (permalink / raw)
To: Paul Moore, casey; +Cc: akpm, torvalds, linux-kernel, netdev
In-Reply-To: <200802152213.11661.paul.moore@hp.com>
--- Paul Moore <paul.moore@hp.com> wrote:
> On Friday 15 February 2008 6:24:25 pm Casey Schaufler wrote:
> > From: Casey Schaufler <casey@schaufler-ca.com>
> >
> > Smack uses CIPSO labeling, but allows for unlabeled packets
> > by specifying an "ambient" label that is applied to incoming
> > unlabeled packets. Because the other end of the connection
> > may dislike IP options, and ssh is one know application that
> > behaves thus, it is prudent to respond in kind. This patch
> > changes the network labeling behavior such that an outgoing
> > packet that would be given a CIPSO label that matches the
> > ambient label is left unlabeled. An "unlbl" domain is added
> > and the netlabel defaulting mechanism invoked rather than
> > assuming that everything is CIPSO. Locking has been added
> > around changes to the ambient label as the mechanisms used
> > to do so are more involved.
> >
> > Cleaned up some issues noted in review.
> > Make smk_cipso_doi() static.
> > Create a hook for the new security_secctx_to_secid()
> > using existing underlying code.
> > Fill in audit data for netlbl domain calls.
> > Collapse unnecessary multiple assignments.
> >
> > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>
> Looks good to me, thanks for making those changes.
>
> Acked-by: Paul Moore <paul.moore@hp.com>
Thank you for the insights.
Casey Schaufler
casey@schaufler-ca.com
^ permalink raw reply
* Re: [RFC] Best method to control a "transmit-only" mode on fiber NICs (specifically sky2)
From: Stephen Hemminger @ 2008-02-16 6:24 UTC (permalink / raw)
To: Kyle Moffett; +Cc: Linux NetDev, LKML
In-Reply-To: <f73f7ab80802151941x5daf34e3scd0f42a05291db51@mail.gmail.com>
Kyle Moffett wrote:
> Hi,
>
> The company I'm working for has an unusual fiber NIC configuration
> that we use for one of our network appliances. We connect only a
> single fiber from the TX port on one NIC to the RX port on another
> NIC, providing a physically-one-way path for enhanced security.
> Unfortunately this doesn't work with most NIC drivers, as even with
> auto-negotiation off they look for link probe pulses before they
> consider the link "up" and are willing to send packets. We have been
> able to use Myricom 10GigE NICs with a custom firmware image. More
> recently we have patched the sky2 driver to turn on the FIB_FORCE_LNK
> flag in the PHY control register; this seems to work on the
> Marvell-chipset boards we have here.
>
> What would be the preferred way to control this "force link" flag?
> Right now we are accessing it using ethtool; we have added an
> additional "duplex" mode: "DUPLEX_TXONLY", with a value of 2. When
> you specify a speed and turn off autonegotiation ("./patched-ethtool
> -s eth2 speed 1000 autoneg off duplex txonly"), it will turn on the
> specified bit in the PHY control register and the link will
> automatically come up. We also have one related bug-fix^Wdirty hack
> for sky2 to reset the PHY a second time during netif-up after enabling
> interrupts; otherwise the immediate "link up" interrupt gets lost.
> Once I get approval from the company I will patch the post itself for
> review.
>
> I look forward to your comments and suggestions
>
> Cheers,
> Kyle Moffett
>
For the second problem, you could just change the code to check the
status immediately,
by calling the same code the irq does. It might need some refactoring
but should only be minor surgery.
I was thinking about doing that anyway for the forced link (no
negotiation case).
^ permalink raw reply
* Re: RESEND, HTB(?) softlockup, vanilla 2.6.24
From: Jarek Poplawski @ 2008-02-16 8:00 UTC (permalink / raw)
To: Denys Fedoryshchenko; +Cc: netdev
In-Reply-To: <20080213081318.M90354@visp.net.lb>
Denys Fedoryshchenko wrote, On 02/13/2008 09:13 AM:
> It is very difficult to reproduce, happened after running about 1month. No
> changes done in classes at time of crash.
>
> Kernel 2.6.24 vanilla
Hi,
I could be wrong, but IMHO this looks like stack was overridden here,
so my proposal is to try this:
CONFIG_DEBUG_STACKOVERFLOW=y
But, if you're not very interested in reproducing this, you could also
try to turn off some other debugging, especially lockdep.
Regards,
Jarek P.
...
> Feb 10 15:53:22 SHAPER [ 8271.778915] BUG: NMI Watchdog detected LOCKUP
> Feb 10 15:53:22 SHAPER on CPU1, eip c01f0e5d, registers:
...
> Feb 10 15:53:22 SHAPER [ 8271.779307] Pid: 0, comm: swapper Not tainted
> (2.6.24-build-0021 #26)
> Feb 10 15:53:22 SHAPER [ 8271.779327] EIP: 0060:[<c01f0e5d>] EFLAGS: 00000082
> CPU: 1
> Feb 10 15:53:22 SHAPER [ 8271.779349] EIP is at __rb_rotate_right+0x5/0x50
> Feb 10 15:53:22 SHAPER [ 8271.779366] EAX: f76494a4 EBX: f76494a4 ECX:
> f76494a4 EDX: c1ff5f80
> Feb 10 15:53:22 SHAPER [ 8271.779386] ESI: f76494a4 EDI: c1ff5f80 EBP:
> 00000000 ESP: f7c29c70
> Feb 10 15:53:22 SHAPER [ 8271.779406] DS: 007b ES: 007b FS: 00d8 GS: 0000
> SS: 0068
> Feb 10 15:53:22 SHAPER [ 8271.779425] Process swapper (pid: 0, ti=f7c28000
> task=f7c20a60 task.ti=f7c28000)
> Feb 10 15:53:22 SHAPER
> Feb 10 15:53:22 SHAPER [ 8271.779446] Stack:
> Feb 10 15:53:22 SHAPER f76494a4
> Feb 10 15:53:22 SHAPER f76494a4
> Feb 10 15:53:22 SHAPER f76494a4
> Feb 10 15:53:22 SHAPER c01f0ef4
> Feb 10 15:53:22 SHAPER c1ff5f80
> Feb 10 15:53:22 SHAPER f76494a4
> Feb 10 15:53:22 SHAPER f76494a8
> Feb 10 15:53:22 SHAPER c1ff5f78
> Feb 10 15:53:22 SHAPER
> Feb 10 15:53:22 SHAPER [ 8271.779493]
> Feb 10 15:53:22 SHAPER [ 8271.779307] Pid: 0, comm: swapper Not tainted
> (2.6.24-build-0021 #26)
> Feb 10 15:53:22 SHAPER [ 8271.779327] EIP: 0060:[<c01f0e5d>] EFLAGS: 00000082
> CPU: 1
> Feb 10 15:53:22 SHAPER [ 8271.779349] EIP is at __rb_rotate_right+0x5/0x50
> Feb 10 15:53:22 SHAPER [ 8271.779366] EAX: f76494a4 EBX: f76494a4 ECX:
> f76494a4 EDX: c1ff5f80
> Feb 10 15:53:22 SHAPER [ 8271.779386] ESI: f76494a4 EDI: c1ff5f80 EBP:
> 00000000 ESP: f7c29c70
> Feb 10 15:53:22 SHAPER [ 8271.779406] DS: 007b ES: 007b FS: 00d8 GS: 0000
> SS: 0068
> Feb 10 15:53:22 SHAPER [ 8271.779425] Process swapper (pid: 0, ti=f7c28000
> task=f7c20a60 task.ti=f7c28000)
> Feb 10 15:53:22 SHAPER
> Feb 10 15:53:22 SHAPER [ 8271.779446] Stack:
> Feb 10 15:53:22 SHAPER f76494a4
> Feb 10 15:53:22 SHAPER f76494a4
> Feb 10 15:53:22 SHAPER f76494a4
...
^ permalink raw reply
* Re: forcedeth: MAC-address reversed on resume from suspend
From: Adolfo R. Brandes @ 2008-02-16 9:41 UTC (permalink / raw)
To: David Miller
Cc: B.Steinbrink, bunk, andi, richie, linux-kernel, aabdulla, jgarzik,
netdev
In-Reply-To: <20080107.232355.228915298.davem@davemloft.net>
On Jan 8, 2008 5:23 AM, David Miller <davem@davemloft.net> wrote:
> I am going to push this patch upstream, it is correct and we
> have a positive test case that failed before, so overall it's
> a net improvement even if we still don't exactly know why
> Adolfo's case still fails.
Sorry it took so long, but I got around to testing the patch properly,
i.e. same kernel on CK804 and MCP51, and it worked beautifully!
Thanks!
Adolfo
^ permalink raw reply
* Re: include/linux/pcounter.h
From: Eric Dumazet @ 2008-02-16 10:07 UTC (permalink / raw)
To: Andrew Morton; +Cc: Herbert Xu, David S. Miller, netdev, linux-kernel
In-Reply-To: <20080215193711.2e3d41f3.akpm@linux-foundation.org>
Andrew Morton a écrit :
> - First up, why was this added at all? We have percpu_counter.h which
> has several years development invested in it. afaict it would suit the
> present applications of pcounters.
>
> If some deficiency in percpu_counters has been identified, is it
> possible to correct that deficiency rather than implementing a whole new
> counter type? That would be much better.
>
> - The comments in pcounter.h appear to indicate that there is a
> performance advantage (and we infer that the advantage is when the
> statically-allocated flavour of pcounters is used). When compared with
> percpu_counters the number of data-reference indirections is the same as
> with percpu_counters, so no advantage there.
>
> And, bizarrely, because of a quite inappropriate abstraction toy, both
> flavours of pcounters add an indirect function call which I believe is
> significantly more expensive than a plain old pointer indirection.
>
> So it's quite possible that DEFINE_PCOUNTER-style counters consume more
> memory and are slower than percpu_counters. They will surely be much
> slower on the read side. More below.
>
> If we really want to put some helper wrappers around
> DEFINE_PER_CPU(s32) then I'd suggest that we should do that as a
> standalone thing and not attempt to graft the same interface onto two
> quite different types of storage (DEFINE_PER_CPU and alloc_per_cpu)
>
> - The comment "2)" in pcounter.h (which overflows 80 cols and probably
> wasn't passed through checkpatch) indicates that some other
> implementation (presumably plain old DEFINE_PER_CPU) will use
> NR_CPUS*(32+sizeof(void *)) bytes of storage. But DEFINE_PCOUNTER will
> use as much memory as DEFINE_PER_CPU(s32) and both pcounter_alloc()-style
> pcounters and percpu_counters use
> num_possible_cpus()*sizeof(s32)+epsilon.
>
> - The CONFIG_SMP=n stubs in pcounter.h are cheesy and are vulnerable to
> several well-known compilation risks which I always forget. Should be
> converted to regular static inlines.
>
> - the comment in lib/pcounter.c needlessly exceeds 80 cols.
>
> - pcounter_dyn_add() will spew a
> use-of-smp_processor_id()-in-preemptible-code warning if used in places
> where one could reasonably use it. The interface could do with a bit of
> a rethink. Or at least some justification and documentation.
>
> - pcounter_getval() will be disastrously inefficient if
> num_possible_cpus() is much greater than num_online_cpus(). It should
> use for_each_online_cpu() (as does percpu_counter), and implement a CPU
> hotplug notifier (as does percpu_counter).
>
> It will remain grossly inefficient at high CPU counts, unlike
> percpu_counters, which solved this problem by doing a batched spill into
> a central counter at add/sub time.
>
> The danger here is that someone will actually use this interface in new
> code. Six months later (when it's too late to fix it) the big-NUMA guys
> come up and say "whaa, when our user does <this> it disabled interrupts
> for N milliseconds".
>
> - pcounter_getval() can return incorrect negative numbers. This can
> cause caller malfunctions in very rare situations because callers just
> don't expect the things which they're counting to go negative.
>
> We experienced this during the evolution of percpu_counter. See
> percpu_counter_sum_positive() and friends.
>
> - pcounter_alloc() should return -ENOMEM on allocation error, not "1".
>
> - pcounter_free() perhaps shouldn't test for (self->per_cpu_values !=
> NULL), because callers shouldn't be calling it if pcounter_alloc() failed
> (arguable).
>
>
>
> afaict the whole implementation can and should be removed and replaced with
> percpu_counters. I don't think there's much point in its ability to manage
> DEFINE_PER_CPU counters: pcounter_getval() remains grossly inefficient (and
> can return negative values) and quite a bit of new code will need to be put
> in place to address that.
>
> But perhaps there are plans to evolve it into something further in the
> future, I don't know. But I would suggest that the people who have worked
> upon percpu_counters (principally Gautham, Peter Z, clameter and me) be
> involved in that work.
>
Andrew, pcounter is a temporary abstraction.
It is temporaty because it will vanish as soon as Christoph Clameter (or
somebody else) provides real cheap per cpu counter implementation.
At time I introduced it in network tree (locally, not meant to invade kernel
land and makes you unhappy :) ), the goals were :
Some counters (total sockets count) were a single integer, that were doing
ping-pong between cpus (SMP/NUMA). As they are basically lazy values (as we
dont really need to read their value), using plain atomic_t was overkill.
Using a plain percpu_counters was expensive (NR_CPUS*(32+sizeof(void *))
instead of num_possible_cpus()*4).
Using 'online' instead of 'possible' stuff is not really needed for a
temporary thing.
- We dont care of read sides. We want really fast write side. Real fast.
Read side is when you do a "cat /proc/net/sockstat".
That is ... once in a while...
Now when we allocate a new socket, code to increment the "socket count" is :
c03a74a8 <tcp_pcounter_add>:
c03a74a8: b8 90 26 5f c0 mov $0xc05f2690,%eax
c03a74ad: 64 8b 0d 10 f1 5e c0 mov %fs:0xc05ef110,%ecx
c03a74b4: 01 14 01 add %edx,(%ecx,%eax,1)
c03a74b7: c3 ret
That is 4 instructions. I could be two in the future, thanks to current work
on fs/gs based percpu variables.
Current percpu_counters implementation is more expensive :
c021467b <__percpu_counter_add>:
c021467b: 55 push %ebp
c021467c: 57 push %edi
c021467d: 89 c7 mov %eax,%edi
c021467f: 56 push %esi
c0214680: 53 push %ebx
c0214681: 83 ec 04 sub $0x4,%esp
c0214684: 8b 40 14 mov 0x14(%eax),%eax
c0214687: 64 8b 1d 08 f0 5e c0 mov %fs:0xc05ef008,%ebx
c021468e: 8b 6c 24 18 mov 0x18(%esp),%ebp
c0214692: f7 d0 not %eax
c0214694: 8b 1c 98 mov (%eax,%ebx,4),%ebx
c0214697: 89 1c 24 mov %ebx,(%esp)
c021469a: 8b 03 mov (%ebx),%eax
c021469c: 89 c3 mov %eax,%ebx
c021469e: 89 c6 mov %eax,%esi
c02146a0: c1 fe 1f sar $0x1f,%esi
c02146a3: 89 e8 mov %ebp,%eax
c02146a5: 01 d3 add %edx,%ebx
c02146a7: 11 ce adc %ecx,%esi
c02146a9: 99 cltd
c02146aa: 39 d6 cmp %edx,%esi
c02146ac: 7f 15 jg c02146c3
<__percpu_counter_add+0x48>
c02146ae: 7c 04 jl c02146b4
<__percpu_counter_add+0x39>
c02146b0: 39 eb cmp %ebp,%ebx
c02146b2: 73 0f jae c02146c3
<__percpu_counter_add+0x48>
c02146b4: f7 dd neg %ebp
c02146b6: 89 e8 mov %ebp,%eax
c02146b8: 99 cltd
c02146b9: 39 d6 cmp %edx,%esi
c02146bb: 7f 20 jg c02146dd
<__percpu_counter_add+0x62>
c02146bd: 7c 04 jl c02146c3
<__percpu_counter_add+0x48>
c02146bf: 39 eb cmp %ebp,%ebx
c02146c1: 77 1a ja c02146dd
<__percpu_counter_add+0x62>
c02146c3: 89 f8 mov %edi,%eax
c02146c5: e8 04 cc 1f 00 call c04112ce <_spin_lock>
c02146ca: 01 5f 04 add %ebx,0x4(%edi)
c02146cd: 11 77 08 adc %esi,0x8(%edi)
c02146d0: 8b 04 24 mov (%esp),%eax
c02146d3: c7 00 00 00 00 00 movl $0x0,(%eax)
c02146d9: fe 07 incb (%edi)
c02146db: eb 05 jmp c02146e2
<__percpu_counter_add+0x67>
c02146dd: 8b 04 24 mov (%esp),%eax
c02146e0: 89 18 mov %ebx,(%eax)
c02146e2: 58 pop %eax
c02146e3: 5b pop %ebx
c02146e4: 5e pop %esi
c02146e5: 5f pop %edi
c02146e6: 5d pop %ebp
c02146e7: c3 ret
Once it is better, just make pcounter vanish.
It is even clearly stated at the top of include/linux/pcounter.h
/*
* Using a dynamic percpu 'int' variable has a cost :
* 1) Extra dereference
* Current per_cpu_ptr() implementation uses an array per 'percpu variable'.
* 2) memory cost of NR_CPUS*(32+sizeof(void *)) instead of num_possible_cpus()*4
*
* This pcounter implementation is an abstraction to be able to use
* either a static or a dynamic per cpu variable.
* One dynamic per cpu variable gets a fast & cheap implementation, we can
* change pcounter implementation too.
*/
We all agree.
Thank you
^ permalink raw reply
* Re: RESEND, HTB(?) softlockup, vanilla 2.6.24
From: Denys Fedoryshchenko @ 2008-02-16 10:25 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: netdev
In-Reply-To: <47B69824.4030405@gmail.com>
Thanks, i will try it.
You think lockdep can be buggy?
On Sat, 16 Feb 2008 09:00:36 +0100, Jarek Poplawski wrote
> Denys Fedoryshchenko wrote, On 02/13/2008 09:13 AM:
>
> > It is very difficult to reproduce, happened after running about 1month.
No
> > changes done in classes at time of crash.
> >
> > Kernel 2.6.24 vanilla
>
> Hi,
>
> I could be wrong, but IMHO this looks like stack was overridden here,
> so my proposal is to try this:
>
> CONFIG_DEBUG_STACKOVERFLOW=y
>
> But, if you're not very interested in reproducing this, you could
> also try to turn off some other debugging, especially lockdep.
>
> Regards,
> Jarek P.
>
> ....
>
> > Feb 10 15:53:22 SHAPER [ 8271.778915] BUG: NMI Watchdog detected LOCKUP
> > Feb 10 15:53:22 SHAPER on CPU1, eip c01f0e5d, registers:
>
> ....
>
> > Feb 10 15:53:22 SHAPER [ 8271.779307] Pid: 0, comm: swapper Not tainted
> > (2.6.24-build-0021 #26)
> > Feb 10 15:53:22 SHAPER [ 8271.779327] EIP: 0060:[<c01f0e5d>] EFLAGS:
00000082
> > CPU: 1
> > Feb 10 15:53:22 SHAPER [ 8271.779349] EIP is at __rb_rotate_right+0x5/0x50
> > Feb 10 15:53:22 SHAPER [ 8271.779366] EAX: f76494a4 EBX: f76494a4 ECX:
> > f76494a4 EDX: c1ff5f80
> > Feb 10 15:53:22 SHAPER [ 8271.779386] ESI: f76494a4 EDI: c1ff5f80 EBP:
> > 00000000 ESP: f7c29c70
> > Feb 10 15:53:22 SHAPER [ 8271.779406] DS: 007b ES: 007b FS: 00d8 GS:
0000
> > SS: 0068
> > Feb 10 15:53:22 SHAPER [ 8271.779425] Process swapper (pid: 0,
ti=f7c28000
> > task=f7c20a60 task.ti=f7c28000)
> > Feb 10 15:53:22 SHAPER
> > Feb 10 15:53:22 SHAPER [ 8271.779446] Stack:
> > Feb 10 15:53:22 SHAPER f76494a4
> > Feb 10 15:53:22 SHAPER f76494a4
> > Feb 10 15:53:22 SHAPER f76494a4
> > Feb 10 15:53:22 SHAPER c01f0ef4
> > Feb 10 15:53:22 SHAPER c1ff5f80
> > Feb 10 15:53:22 SHAPER f76494a4
> > Feb 10 15:53:22 SHAPER f76494a8
> > Feb 10 15:53:22 SHAPER c1ff5f78
> > Feb 10 15:53:22 SHAPER
> > Feb 10 15:53:22 SHAPER [ 8271.779493]
> > Feb 10 15:53:22 SHAPER [ 8271.779307] Pid: 0, comm: swapper Not tainted
> > (2.6.24-build-0021 #26)
> > Feb 10 15:53:22 SHAPER [ 8271.779327] EIP: 0060:[<c01f0e5d>] EFLAGS:
00000082
> > CPU: 1
> > Feb 10 15:53:22 SHAPER [ 8271.779349] EIP is at __rb_rotate_right+0x5/0x50
> > Feb 10 15:53:22 SHAPER [ 8271.779366] EAX: f76494a4 EBX: f76494a4 ECX:
> > f76494a4 EDX: c1ff5f80
> > Feb 10 15:53:22 SHAPER [ 8271.779386] ESI: f76494a4 EDI: c1ff5f80 EBP:
> > 00000000 ESP: f7c29c70
> > Feb 10 15:53:22 SHAPER [ 8271.779406] DS: 007b ES: 007b FS: 00d8 GS:
0000
> > SS: 0068
> > Feb 10 15:53:22 SHAPER [ 8271.779425] Process swapper (pid: 0,
ti=f7c28000
> > task=f7c20a60 task.ti=f7c28000)
> > Feb 10 15:53:22 SHAPER
> > Feb 10 15:53:22 SHAPER [ 8271.779446] Stack:
> > Feb 10 15:53:22 SHAPER f76494a4
> > Feb 10 15:53:22 SHAPER f76494a4
> > Feb 10 15:53:22 SHAPER f76494a4
>
> ....
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Denys Fedoryshchenko
Technical Manager
Virtual ISP S.A.L.
^ permalink raw reply
* Re: include/linux/pcounter.h
From: Andrew Morton @ 2008-02-16 10:50 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Herbert Xu, David S. Miller, netdev, linux-kernel
In-Reply-To: <47B6B5E1.90705@cosmosbay.com>
On Sat, 16 Feb 2008 11:07:29 +0100 Eric Dumazet <dada1@cosmosbay.com> wrote:
>
> Andrew, pcounter is a temporary abstraction.
It's buggy! Main problems are a) possible return of negative numbers b)
some of the API can't be from preemptible code c) excessive interrupt-off
time on some machines if used from irq-disabled sections.
> It is temporaty because it will vanish as soon as Christoph Clameter (or
> somebody else) provides real cheap per cpu counter implementation.
numbers?
most of percpu_counter_add() is only executed once per FBC_BATCH calls.
> At time I introduced it in network tree (locally, not meant to invade kernel
> land and makes you unhappy :) ), the goals were :
Well maybe as a temporary networking-only thing OK, based upon
performance-tested results. But I don't think the present code is suitable
as part of the kernel-wide toolkit.
> Some counters (total sockets count) were a single integer, that were doing
> ping-pong between cpus (SMP/NUMA). As they are basically lazy values (as we
> dont really need to read their value), using plain atomic_t was overkill.
>
> Using a plain percpu_counters was expensive (NR_CPUS*(32+sizeof(void *))
> instead of num_possible_cpus()*4).
No, percpu_counters use alloc_percpu(), which is O(num_possible_cpus), not
O(NR_CPUS).
> Using 'online' instead of 'possible' stuff is not really needed for a
> temporary thing.
This was put in ./lib/!
> - We dont care of read sides.
Well the present single caller in networking might not care. But this was
put in ./lib/ and was exported to modules. That is an invitation to all
kernel developers to use it in new code. Which may result in truly awful
performance on high-cpu-count machines.
> We want really fast write side. Real fast.
eh? It's called on a per-connection basis, not on a per-packet basis?
> Read side is when you do a "cat /proc/net/sockstat".
> That is ... once in a while...
For the current single caller. But it's in ./lib/.
And there's always someone out there who does whatever we don't expect them
to do.
> Now when we allocate a new socket, code to increment the "socket count" is :
>
> c03a74a8 <tcp_pcounter_add>:
> c03a74a8: b8 90 26 5f c0 mov $0xc05f2690,%eax
> c03a74ad: 64 8b 0d 10 f1 5e c0 mov %fs:0xc05ef110,%ecx
> c03a74b4: 01 14 01 add %edx,(%ecx,%eax,1)
> c03a74b7: c3 ret
I can't find that code. I suspect that's the DEFINE_PER_CPU flavour, which
isn't used anywhere afaict. Plus this omits the local_irq_save/restore (or
preempt_disable/enable) and the indirect function call, which can be
expensive.
> That is 4 instructions. I could be two in the future, thanks to current work
> on fs/gs based percpu variables.
>
> Current percpu_counters implementation is more expensive :
>
> c021467b <__percpu_counter_add>:
> c021467b: 55 push %ebp
> c021467c: 57 push %edi
> c021467d: 89 c7 mov %eax,%edi
> c021467f: 56 push %esi
> c0214680: 53 push %ebx
> c0214681: 83 ec 04 sub $0x4,%esp
> c0214684: 8b 40 14 mov 0x14(%eax),%eax
> c0214687: 64 8b 1d 08 f0 5e c0 mov %fs:0xc05ef008,%ebx
> c021468e: 8b 6c 24 18 mov 0x18(%esp),%ebp
> c0214692: f7 d0 not %eax
> c0214694: 8b 1c 98 mov (%eax,%ebx,4),%ebx
> c0214697: 89 1c 24 mov %ebx,(%esp)
> c021469a: 8b 03 mov (%ebx),%eax
> c021469c: 89 c3 mov %eax,%ebx
> c021469e: 89 c6 mov %eax,%esi
> c02146a0: c1 fe 1f sar $0x1f,%esi
> c02146a3: 89 e8 mov %ebp,%eax
> c02146a5: 01 d3 add %edx,%ebx
> c02146a7: 11 ce adc %ecx,%esi
> c02146a9: 99 cltd
> c02146aa: 39 d6 cmp %edx,%esi
> c02146ac: 7f 15 jg c02146c3
> <__percpu_counter_add+0x48>
> c02146ae: 7c 04 jl c02146b4
One of the above two branches is taken ((FBC_BATCH-1)/FBC_BATCH)ths of the
time.
> <__percpu_counter_add+0x39>
> c02146b0: 39 eb cmp %ebp,%ebx
> c02146b2: 73 0f jae c02146c3
> <__percpu_counter_add+0x48>
> c02146b4: f7 dd neg %ebp
> c02146b6: 89 e8 mov %ebp,%eax
> c02146b8: 99 cltd
> c02146b9: 39 d6 cmp %edx,%esi
> c02146bb: 7f 20 jg c02146dd
> <__percpu_counter_add+0x62>
> c02146bd: 7c 04 jl c02146c3
> <__percpu_counter_add+0x48>
> c02146bf: 39 eb cmp %ebp,%ebx
> c02146c1: 77 1a ja c02146dd
> <__percpu_counter_add+0x62>
> c02146c3: 89 f8 mov %edi,%eax
> c02146c5: e8 04 cc 1f 00 call c04112ce <_spin_lock>
> c02146ca: 01 5f 04 add %ebx,0x4(%edi)
> c02146cd: 11 77 08 adc %esi,0x8(%edi)
> c02146d0: 8b 04 24 mov (%esp),%eax
> c02146d3: c7 00 00 00 00 00 movl $0x0,(%eax)
> c02146d9: fe 07 incb (%edi)
> c02146db: eb 05 jmp c02146e2
> <__percpu_counter_add+0x67>
> c02146dd: 8b 04 24 mov (%esp),%eax
> c02146e0: 89 18 mov %ebx,(%eax)
> c02146e2: 58 pop %eax
> c02146e3: 5b pop %ebx
> c02146e4: 5e pop %esi
> c02146e5: 5f pop %edi
> c02146e6: 5d pop %ebp
> c02146e7: c3 ret
>
>
> Once it is better, just make pcounter vanish.
Some of the stuff in there is from the __percpu_disguise() thing which we
probably can live without.
But I'd be surprised if benchmarking reveals that the pcounter code is
justifiable in its present networking application or indeed in any future
ones.
> It is even clearly stated at the top of include/linux/pcounter.h
>
> /*
> * Using a dynamic percpu 'int' variable has a cost :
> * 1) Extra dereference
> * Current per_cpu_ptr() implementation uses an array per 'percpu variable'.
> * 2) memory cost of NR_CPUS*(32+sizeof(void *)) instead of num_possible_cpus()*4
> *
> * This pcounter implementation is an abstraction to be able to use
> * either a static or a dynamic per cpu variable.
> * One dynamic per cpu variable gets a fast & cheap implementation, we can
> * change pcounter implementation too.
> */
>
>
> We all agree.
>
No we don't. That comment is afaict wrong about the memory consumption and
the abstraction *isn't useful*.
Why do we want some abstraction which makes alloc_percpu() storage and
DEFINE_PERCPU storage "look the same"? What use is there in that? One is
per-object storage and one is singleton storage - they're quite different
things and they are used in quite different situations and they are
basically never interchangeable. Yet we add this pretend-they're-the-same
wrapper around them which costs us an indirect function call on the
fastpath.
^ permalink raw reply
* [resend] [PATCH] IPv4: Reset scope when changing address
From: Bjørn Mork @ 2008-02-16 11:45 UTC (permalink / raw)
To: netdev
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=us-ascii, Size: 1687 bytes --]
<#part type=text/plain nofile=yes>
Any comments on this? Apparently introduced in 2.1.68, so there's not
much hurry. But I'd still like to hear whether that analysis is correct
or not...
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="==-=-="
The bug did bite at least one user, who did have to resort to rebooting
the system after an "ifconfig eth0 127.0.0.1" typo.
Deleting the address and adding a new is a less intrusive workaround.
But I still beleive this is a bug that should be fixed. Some way or
another.
Another possibility would be to remove the scope mangling based on
address. This will always be incomplete (are 127/8 the only address
space with host scope requirements?)
Bjørn
<#part type=text/x-diff filename="devinet-rt_scope-fix.patch" disposition=inline nofile=yes>
We set the scope to RT_SCOPE_HOST if an IPv4 interface is configured
with a loopback address (127/8). The scope is never reset, and will
remain set to RT_SCOPE_HOST after changing the address. This patch
resets the scope if the address is changed again, to restore normal
functionality.
Signed-off-by: Bjorn Mork <bjorn@mork.no>
--==-=-=
devinet.c | 1 +
1 file changed, 1 insertion(+)
--- linux-2.6.24-rc8.orig/net/ipv4/devinet.c 2008-01-16 05:22:48.000000000 +0100
+++ linux-2.6.24-rc8/net/ipv4/devinet.c 2008-01-23 19:17:30.000000000 +0100
@@ -753,6 +753,7 @@
inet_del_ifa(in_dev, ifap, 0);
ifa->ifa_broadcast = 0;
ifa->ifa_anycast = 0;
+ ifa->ifa_scope = 0;
}
ifa->ifa_address = ifa->ifa_local = sin->sin_addr.s_addr;
--==-=-=
Content-Type: text/plain; charset=iso-8859-1
--==-=-=--
^ permalink raw reply
* Re: include/linux/pcounter.h
From: Eric Dumazet @ 2008-02-16 12:03 UTC (permalink / raw)
To: Andrew Morton; +Cc: Herbert Xu, David S. Miller, netdev, linux-kernel
In-Reply-To: <20080216025051.751b4a86.akpm@linux-foundation.org>
Andrew Morton a écrit :
> On Sat, 16 Feb 2008 11:07:29 +0100 Eric Dumazet <dada1@cosmosbay.com> wrote:
>
>> Andrew, pcounter is a temporary abstraction.
>
> It's buggy! Main problems are a) possible return of negative numbers b)
> some of the API can't be from preemptible code c) excessive interrupt-off
> time on some machines if used from irq-disabled sections.
a) We dont care of possibly off values when reading /proc/net/sockstat
Same arguments apply for percpu_counters.
b) It is called from network parts where preemption is disabled.
net/ipv4/inet_timewait_sock.c:94:
sock_prot_inuse_add(sk->sk_prot, -1);
net/ipv4/inet_hashtables.c:291: sock_prot_inuse_add(sk->sk_prot, 1);
net/ipv4/inet_hashtables.c:335: sock_prot_inuse_add(sk->sk_prot, 1);
net/ipv4/inet_hashtables.c:357: sock_prot_inuse_add(sk->sk_prot, 1);
net/ipv4/inet_hashtables.c:390: sock_prot_inuse_add(sk->sk_prot, -1);
net/ipv4/raw.c:95: sock_prot_inuse_add(sk->sk_prot, 1);
net/ipv4/raw.c:104: sock_prot_inuse_add(sk->sk_prot, -1);
net/ipv4/udp.c:235: sock_prot_inuse_add(sk->sk_prot, 1);
net/ipv6/ipv6_sockglue.c:271:
sock_prot_inuse_add(sk->sk_prot, -1);
net/ipv6/ipv6_sockglue.c:272:
sock_prot_inuse_add(&tcp_prot, 1);
net/ipv6/ipv6_sockglue.c:285:
sock_prot_inuse_add(sk->sk_prot, -1);
net/ipv6/ipv6_sockglue.c:286:
sock_prot_inuse_add(prot, 1);
net/ipv6/inet6_hashtables.c:46: sock_prot_inuse_add(sk->sk_prot, 1);
net/ipv6/inet6_hashtables.c:207: sock_prot_inuse_add(sk->sk_prot, 1);
c) No need to play with irq games here.
>
>> It is temporaty because it will vanish as soon as Christoph Clameter (or
>> somebody else) provides real cheap per cpu counter implementation.
>
> numbers?
>
> most of percpu_counter_add() is only executed once per FBC_BATCH calls.
>
>> At time I introduced it in network tree (locally, not meant to invade kernel
>> land and makes you unhappy :) ), the goals were :
>
> Well maybe as a temporary networking-only thing OK, based upon
> performance-tested results. But I don't think the present code is suitable
> as part of the kernel-wide toolkit.
>
>> Some counters (total sockets count) were a single integer, that were doing
>> ping-pong between cpus (SMP/NUMA). As they are basically lazy values (as we
>> dont really need to read their value), using plain atomic_t was overkill.
>>
>> Using a plain percpu_counters was expensive (NR_CPUS*(32+sizeof(void *))
>> instead of num_possible_cpus()*4).
>
> No, percpu_counters use alloc_percpu(), which is O(num_possible_cpus), not
> O(NR_CPUS).
You are playing with words.
The array was exacly sizeof(void *) * NR_CPUS (at least when pcounter were
added in network tree, commit b3242151906372f30f57feaa43b4cac96a23edb1 was
done only ten days ago). Then it needed 32 bytes per possible cpu (maybe less
now with SLUB)
>
>> Using 'online' instead of 'possible' stuff is not really needed for a
>> temporary thing.
>
> This was put in ./lib/!
>
>> - We dont care of read sides.
>
> Well the present single caller in networking might not care. But this was
> put in ./lib/ and was exported to modules. That is an invitation to all
> kernel developers to use it in new code. Which may result in truly awful
> performance on high-cpu-count machines.
>
>> We want really fast write side. Real fast.
>
> eh? It's called on a per-connection basis, not on a per-packet basis?
Yes, per connection basis. Some workloads want to open/close more than 1000
sockets per second.
You are right we also need to reduce all atomic inc/dec done per packet :)
A pcounter/perc_cpu for the device refcounter would be a very nice win, but as
number of devices in the system might be very big, David said no to a percpu
conversion. We will reconsider this when new percpu stuff can go in, so that
the memory cost will be minimal (4 bytes per cpu per device)
>
>> Read side is when you do a "cat /proc/net/sockstat".
>> That is ... once in a while...
>
> For the current single caller. But it's in ./lib/.
>
> And there's always someone out there who does whatever we don't expect them
> to do.
>
>> Now when we allocate a new socket, code to increment the "socket count" is :
>>
>> c03a74a8 <tcp_pcounter_add>:
>> c03a74a8: b8 90 26 5f c0 mov $0xc05f2690,%eax
>> c03a74ad: 64 8b 0d 10 f1 5e c0 mov %fs:0xc05ef110,%ecx
>> c03a74b4: 01 14 01 add %edx,(%ecx,%eax,1)
>> c03a74b7: c3 ret
>
> I can't find that code. I suspect that's the DEFINE_PER_CPU flavour, which
> isn't used anywhere afaict. Plus this omits the local_irq_save/restore (or
> preempt_disable/enable) and the indirect function call, which can be
> expensive.
Please do : nm vmlinux | grep tcp_pcounter_add
c03a74a8 t tcp_pcounter_add
It should be there, even if its a static function
>
>> That is 4 instructions. I could be two in the future, thanks to current work
>> on fs/gs based percpu variables.
>>
>> Current percpu_counters implementation is more expensive :
>>
>> c021467b <__percpu_counter_add>:
>> c021467b: 55 push %ebp
>> c021467c: 57 push %edi
>> c021467d: 89 c7 mov %eax,%edi
>> c021467f: 56 push %esi
>> c0214680: 53 push %ebx
>> c0214681: 83 ec 04 sub $0x4,%esp
>> c0214684: 8b 40 14 mov 0x14(%eax),%eax
>> c0214687: 64 8b 1d 08 f0 5e c0 mov %fs:0xc05ef008,%ebx
>> c021468e: 8b 6c 24 18 mov 0x18(%esp),%ebp
>> c0214692: f7 d0 not %eax
>> c0214694: 8b 1c 98 mov (%eax,%ebx,4),%ebx
>> c0214697: 89 1c 24 mov %ebx,(%esp)
>> c021469a: 8b 03 mov (%ebx),%eax
>> c021469c: 89 c3 mov %eax,%ebx
>> c021469e: 89 c6 mov %eax,%esi
>> c02146a0: c1 fe 1f sar $0x1f,%esi
>> c02146a3: 89 e8 mov %ebp,%eax
>> c02146a5: 01 d3 add %edx,%ebx
>> c02146a7: 11 ce adc %ecx,%esi
>> c02146a9: 99 cltd
>> c02146aa: 39 d6 cmp %edx,%esi
>> c02146ac: 7f 15 jg c02146c3
>> <__percpu_counter_add+0x48>
>> c02146ae: 7c 04 jl c02146b4
>
> One of the above two branches is taken ((FBC_BATCH-1)/FBC_BATCH)ths of the
> time.
>
>
>> <__percpu_counter_add+0x39>
>> c02146b0: 39 eb cmp %ebp,%ebx
>> c02146b2: 73 0f jae c02146c3
>> <__percpu_counter_add+0x48>
>> c02146b4: f7 dd neg %ebp
>> c02146b6: 89 e8 mov %ebp,%eax
>> c02146b8: 99 cltd
>> c02146b9: 39 d6 cmp %edx,%esi
>> c02146bb: 7f 20 jg c02146dd
>> <__percpu_counter_add+0x62>
>> c02146bd: 7c 04 jl c02146c3
>> <__percpu_counter_add+0x48>
>> c02146bf: 39 eb cmp %ebp,%ebx
>> c02146c1: 77 1a ja c02146dd
>> <__percpu_counter_add+0x62>
>> c02146c3: 89 f8 mov %edi,%eax
>> c02146c5: e8 04 cc 1f 00 call c04112ce <_spin_lock>
>> c02146ca: 01 5f 04 add %ebx,0x4(%edi)
>> c02146cd: 11 77 08 adc %esi,0x8(%edi)
>> c02146d0: 8b 04 24 mov (%esp),%eax
>> c02146d3: c7 00 00 00 00 00 movl $0x0,(%eax)
>> c02146d9: fe 07 incb (%edi)
>> c02146db: eb 05 jmp c02146e2
>> <__percpu_counter_add+0x67>
>> c02146dd: 8b 04 24 mov (%esp),%eax
>> c02146e0: 89 18 mov %ebx,(%eax)
>> c02146e2: 58 pop %eax
>> c02146e3: 5b pop %ebx
>> c02146e4: 5e pop %esi
>> c02146e5: 5f pop %edi
>> c02146e6: 5d pop %ebp
>> c02146e7: c3 ret
>>
>>
>> Once it is better, just make pcounter vanish.
>
> Some of the stuff in there is from the __percpu_disguise() thing which we
> probably can live without.
>
> But I'd be surprised if benchmarking reveals that the pcounter code is
> justifiable in its present networking application or indeed in any future
> ones.
I have no benchmarks, but real workloads where it matters, and where userland
eats icache/dcache all the time.
>
>> It is even clearly stated at the top of include/linux/pcounter.h
>>
>> /*
>> * Using a dynamic percpu 'int' variable has a cost :
>> * 1) Extra dereference
>> * Current per_cpu_ptr() implementation uses an array per 'percpu variable'.
>> * 2) memory cost of NR_CPUS*(32+sizeof(void *)) instead of num_possible_cpus()*4
>> *
>> * This pcounter implementation is an abstraction to be able to use
>> * either a static or a dynamic per cpu variable.
>> * One dynamic per cpu variable gets a fast & cheap implementation, we can
>> * change pcounter implementation too.
>> */
>>
>>
>> We all agree.
>>
>
> No we don't. That comment is afaict wrong about the memory consumption and
> the abstraction *isn't useful*.
Fact is that we need percpu 32bits counters, and we need to have pointers to them.
Current percpu_counters cannot cope that.
>
> Why do we want some abstraction which makes alloc_percpu() storage and
> DEFINE_PERCPU storage "look the same"? What use is there in that? One is
> per-object storage and one is singleton storage - they're quite different
> things and they are used in quite different situations and they are
> basically never interchangeable. Yet we add this pretend-they're-the-same
> wrapper around them which costs us an indirect function call on the
> fastpath.
I believe all 'pcounter' are in fact statically allocated 'one per struct
proto' to track inuse count. (search for DEFINE_PROTO_INUSE() uses)
# find net include|xargs grep -n DEFINE_PROTO_INUSE
net/dccp/ipv6.c:1105:DEFINE_PROTO_INUSE(dccp_v6)
net/dccp/ipv4.c:920:DEFINE_PROTO_INUSE(dccp_v4)
net/ipv6/udp.c:1001:DEFINE_PROTO_INUSE(udpv6)
net/ipv6/udplite.c:43:DEFINE_PROTO_INUSE(udplitev6)
net/ipv6/raw.c:1187:DEFINE_PROTO_INUSE(rawv6)
net/ipv6/tcp_ipv6.c:2108:DEFINE_PROTO_INUSE(tcpv6)
net/ipv4/udp.c:1477:DEFINE_PROTO_INUSE(udp)
net/ipv4/udplite.c:47:DEFINE_PROTO_INUSE(udplite)
net/ipv4/tcp_ipv4.c:2406:DEFINE_PROTO_INUSE(tcp)
net/ipv4/raw.c:828:DEFINE_PROTO_INUSE(raw)
net/sctp/socket.c:6461:DEFINE_PROTO_INUSE(sctp)
net/sctp/socket.c:6495:DEFINE_PROTO_INUSE(sctpv6)
include/net/sock.h:624:# define DEFINE_PROTO_INUSE(NAME) DEFINE_PCOUNTER(NAME)
include/net/sock.h:644:# define DEFINE_PROTO_INUSE(NAME)
So pcounter_alloc()/pcounter_free() could just be deleted from pcounter.h
indirect functions calls are everywhere in kernel, network, fs, everywhere.
As soon we can put in 'struct pcounter' the address of a percpu variable, we
wont need anymore pointers to the pcounter_add()/getval() function.
mov 0(pcounter),%eax # get the address of a percpuvar
add %edx,fs:%eax
This just need the percpu work done by SGI guys to be finished.
^ permalink raw reply
* [Patch]net/xfrm/xfrm_policy.c: replace timer with delayed_work
From: WANG Cong @ 2008-02-16 15:55 UTC (permalink / raw)
To: Linux Kernel Mailing List; +Cc: Herbert Xu, David Miller, Andrew Morton, netdev
As suggested by Herbert, using workqueue is better than timer
for net/xfrm/xfrm_policy.c, so replace them with delayed_work.
Note that, this patch is not fully tested, just compile and
run as a whole on an Intel Core Duo matchine. So should be
in -mm first.
Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David Miller <davem@davemloft.net>
---
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index ac72116..53f4794 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -430,7 +430,7 @@ struct xfrm_policy
/* This lock only affects elements except for entry. */
rwlock_t lock;
atomic_t refcnt;
- struct timer_list timer;
+ struct delayed_work work;
u32 priority;
u32 index;
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 47219f9..58066f0 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -126,9 +126,10 @@ static inline unsigned long make_jiffies(long secs)
return secs*HZ;
}
-static void xfrm_policy_timer(unsigned long data)
+static void xfrm_policy_worker(struct work_struct *w)
{
- struct xfrm_policy *xp = (struct xfrm_policy*)data;
+ struct xfrm_policy *xp =
+ container_of((struct delayed_work *)w, struct xfrm_policy, work);
unsigned long now = get_seconds();
long next = LONG_MAX;
int warn = 0;
@@ -181,7 +182,7 @@ static void xfrm_policy_timer(unsigned long data)
if (warn)
km_policy_expired(xp, dir, 0, 0);
if (next != LONG_MAX &&
- !mod_timer(&xp->timer, jiffies + make_jiffies(next)))
+ !schedule_delayed_work(&xp->work, make_jiffies(next)))
xfrm_pol_hold(xp);
out:
@@ -208,12 +209,11 @@ struct xfrm_policy *xfrm_policy_alloc(gfp_t gfp)
policy = kzalloc(sizeof(struct xfrm_policy), gfp);
if (policy) {
+ INIT_DELAYED_WORK(&policy->work, xfrm_policy_worker);
INIT_HLIST_NODE(&policy->bydst);
INIT_HLIST_NODE(&policy->byidx);
rwlock_init(&policy->lock);
atomic_set(&policy->refcnt, 1);
- setup_timer(&policy->timer, xfrm_policy_timer,
- (unsigned long)policy);
}
return policy;
}
@@ -227,7 +227,7 @@ void xfrm_policy_destroy(struct xfrm_policy *policy)
BUG_ON(policy->bundles);
- if (del_timer(&policy->timer))
+ if (cancel_delayed_work(&policy->work))
BUG();
security_xfrm_policy_free(policy);
@@ -244,7 +244,7 @@ static void xfrm_policy_gc_kill(struct xfrm_policy *policy)
dst_free(dst);
}
- if (del_timer(&policy->timer))
+ if (cancel_delayed_work(&policy->work))
atomic_dec(&policy->refcnt);
if (atomic_read(&policy->refcnt) > 1)
@@ -566,7 +566,7 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
hlist_add_head(&policy->byidx, xfrm_policy_byidx+idx_hash(policy->index));
policy->curlft.add_time = get_seconds();
policy->curlft.use_time = 0;
- if (!mod_timer(&policy->timer, jiffies + HZ))
+ if (!schedule_delayed_work(&policy->work, HZ))
xfrm_pol_hold(policy);
write_unlock_bh(&xfrm_policy_lock);
^ permalink raw reply related
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