Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
Hi. I didn't want to leave this hanging and it stayed in my head so I thought I'd better just finish it and test it. I tried out this patch and it got rid of all three unaligned acces errors I was seeing with process connectors and the patch is indeed much smaller. I ran our container daemon program in debug mode to be sure the forks and exits still worked right when the patch was applied and all seemed well. I applied this patch to x86_64 as well as a sanity check and it seems working fine. Things look good to me. I'm ok if you prefer this patch, or the one already in -mm. Signed-off-by: Erik Jacobson <[EMAIL PROTECTED]> --- cn_proc.c | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) --- linux.orig/drivers/connector/cn_proc.c 2006-12-12 23:03:31.0 -0600 +++ linux/drivers/connector/cn_proc.c 2006-12-12 23:06:34.243535000 -0600 @@ -28,6 +28,7 @@ #include #include #include +#include #include @@ -60,7 +61,7 @@ ev = (struct proc_event*)msg->data; get_seq(>seq, >cpu); ktime_get_ts(); /* get high res monotonic timestamp */ - ev->timestamp_ns = timespec_to_ns(); + put_unaligned(timespec_to_ns(), (__u64 *) >timestamp_ns); ev->what = PROC_EVENT_FORK; ev->event_data.fork.parent_pid = task->real_parent->pid; ev->event_data.fork.parent_tgid = task->real_parent->tgid; @@ -88,7 +89,7 @@ ev = (struct proc_event*)msg->data; get_seq(>seq, >cpu); ktime_get_ts(); /* get high res monotonic timestamp */ - ev->timestamp_ns = timespec_to_ns(); + put_unaligned(timespec_to_ns(), (__u64 *) >timestamp_ns); ev->what = PROC_EVENT_EXEC; ev->event_data.exec.process_pid = task->pid; ev->event_data.exec.process_tgid = task->tgid; @@ -124,7 +125,7 @@ return; get_seq(>seq, >cpu); ktime_get_ts(); /* get high res monotonic timestamp */ - ev->timestamp_ns = timespec_to_ns(); + put_unaligned(timespec_to_ns(), (__u64 *) >timestamp_ns); memcpy(>id, _proc_event_id, sizeof(msg->id)); msg->ack = 0; /* not used */ @@ -146,7 +147,7 @@ ev = (struct proc_event*)msg->data; get_seq(>seq, >cpu); ktime_get_ts(); /* get high res monotonic timestamp */ - ev->timestamp_ns = timespec_to_ns(); + put_unaligned(timespec_to_ns(), (__u64 *) >timestamp_ns); ev->what = PROC_EVENT_EXIT; ev->event_data.exit.process_pid = task->pid; ev->event_data.exit.process_tgid = task->tgid; @@ -181,7 +182,7 @@ ev = (struct proc_event*)msg->data; msg->seq = rcvd_seq; ktime_get_ts(); /* get high res monotonic timestamp */ - ev->timestamp_ns = timespec_to_ns(); + put_unaligned(timespec_to_ns(), (__u64 *) >timestamp_ns); ev->cpu = -1; ev->what = PROC_EVENT_NONE; ev->event_data.ack.err = err; - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
> But it's rather a lot of churn for such a thing. Did you consider simply > using > put_unaligned() against the specific offending field(s)? Hi. This was not considered. I wanted to give you some quick feedback, so I tried your suggestion in the fork path. It seemed to fix the problem as well. put_unaligned(timespec_to_ns(), (__u64 *) >timestamp_ns); Is what I tried. I'm on vacation tomorrow but on Thursday, if you like, I can whip up a patch that does this and test it more thoroughly. Is this the direction you prefer? What I did just now was really quick and dirty to see if it has a shot or not but it looks like put_unaligned will fix it too. -- Erik Jacobson - Linux System Software - SGI - Eagan, Minnesota - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
On Tue, 12 Dec 2006 20:31:32 -0600 Erik Jacobson <[EMAIL PROTECTED]> wrote: > > But it's rather a lot of churn for such a thing. Did you consider simply > > using > > put_unaligned() against the specific offending field(s)? > > Hi. This was not considered. > > I wanted to give you some quick feedback, so I tried your suggestion in the > fork path. It seemed to fix the problem as well. OK. > put_unaligned(timespec_to_ns(), (__u64 *) >timestamp_ns); > > Is what I tried. > > I'm on vacation tomorrow but on Thursday, if you like, I can whip up > a patch that does this and test it more thoroughly. Is this the > direction you prefer? What I did just now was really quick and dirty > to see if it has a shot or not but it looks like put_unaligned will > fix it too. > Well it's a one-liner and it makes it very clear what's going on. So unless there's some undiscovered downside, yes, I think it's a good way to go. It'll be an easier patch for the -stable guys to swallow too. There's no particular hurry on it. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
On Tue, 12 Dec 2006 11:54:11 -0600 Erik Jacobson <[EMAIL PROTECTED]> wrote: > Hi Andrew. > > There was some discussion on this patch but I believe we've agreed > on the first version I sent. This was ACKed by Matt Helsley. > > Would you consider taking this in to -mm? > > I've included my original patch email at the bottom. > I'm a bit late to the party. > cn_proc.c | 94 > +++--- But it's rather a lot of churn for such a thing. Did you consider simply using put_unaligned() against the specific offending field(s)? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
Hi Andrew.
There was some discussion on this patch but I believe we've agreed
on the first version I sent. This was ACKed by Matt Helsley.
Would you consider taking this in to -mm?
I've included my original patch email at the bottom.
On Mon, Dec 11, 2006 at 03:52:46PM -0800, Matt Helsley wrote:
(snipped up)
> On Thu, 2006-12-07 at 17:22 -0600, Erik Jacobson wrote:
> > Signed-off-by: Erik Jacobson <[EMAIL PROTECTED]>
> Acked-by: Matt Helsley <[EMAIL PROTECTED]>
> > ---
> > cn_proc.c | 94
> > +++---
Original patch email:
Date: Thu, 7 Dec 2006 17:22:13 -0600
From: Erik Jacobson <[EMAIL PROTECTED]>
To: linux-kernel@vger.kernel.org
Cc: [EMAIL PROTECTED], [EMAIL PROTECTED]
Subject: [PATCH] connector: Some fixes for ia64 unaligned access errors
On ia64, the various functions that make up cn_proc.c cause kernel
unaligned access errors.
If you are using these, for example, to get notification about
all tasks forking and exiting, you get multiple unaligned access errors
per process.
Here, we just adjust how the variables are declared and use memcopy to
avoid the error messages.
Signed-off-by: Erik Jacobson <[EMAIL PROTECTED]>
---
cn_proc.c | 94 +++---
1 file changed, 47 insertions(+), 47 deletions(-)
--- linux.orig/drivers/connector/cn_proc.c 2006-11-29 15:57:37.0
-0600
+++ linux/drivers/connector/cn_proc.c 2006-12-07 16:50:03.195035791 -0600
@@ -49,7 +49,7 @@
void proc_fork_connector(struct task_struct *task)
{
struct cn_msg *msg;
- struct proc_event *ev;
+ struct proc_event ev;
__u8 buffer[CN_PROC_MSG_SIZE];
struct timespec ts;
@@ -57,19 +57,19 @@
return;
msg = (struct cn_msg*)buffer;
- ev = (struct proc_event*)msg->data;
- get_seq(>seq, >cpu);
+ get_seq(>seq, );
ktime_get_ts(); /* get high res monotonic timestamp */
- ev->timestamp_ns = timespec_to_ns();
- ev->what = PROC_EVENT_FORK;
- ev->event_data.fork.parent_pid = task->real_parent->pid;
- ev->event_data.fork.parent_tgid = task->real_parent->tgid;
- ev->event_data.fork.child_pid = task->pid;
- ev->event_data.fork.child_tgid = task->tgid;
+ ev.timestamp_ns = timespec_to_ns();
+ ev.what = PROC_EVENT_FORK;
+ ev.event_data.fork.parent_pid = task->real_parent->pid;
+ ev.event_data.fork.parent_tgid = task->real_parent->tgid;
+ ev.event_data.fork.child_pid = task->pid;
+ ev.event_data.fork.child_tgid = task->tgid;
memcpy(>id, _proc_event_id, sizeof(msg->id));
msg->ack = 0; /* not used */
- msg->len = sizeof(*ev);
+ msg->len = sizeof(ev);
+ memcpy(msg->data, , sizeof(ev));
/* If cn_netlink_send() failed, the data is not sent */
cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
}
@@ -77,7 +77,7 @@
void proc_exec_connector(struct task_struct *task)
{
struct cn_msg *msg;
- struct proc_event *ev;
+ struct proc_event ev;
struct timespec ts;
__u8 buffer[CN_PROC_MSG_SIZE];
@@ -85,24 +85,24 @@
return;
msg = (struct cn_msg*)buffer;
- ev = (struct proc_event*)msg->data;
- get_seq(>seq, >cpu);
+ get_seq(>seq, );
ktime_get_ts(); /* get high res monotonic timestamp */
- ev->timestamp_ns = timespec_to_ns();
- ev->what = PROC_EVENT_EXEC;
- ev->event_data.exec.process_pid = task->pid;
- ev->event_data.exec.process_tgid = task->tgid;
+ ev.timestamp_ns = timespec_to_ns();
+ ev.what = PROC_EVENT_EXEC;
+ ev.event_data.exec.process_pid = task->pid;
+ ev.event_data.exec.process_tgid = task->tgid;
memcpy(>id, _proc_event_id, sizeof(msg->id));
msg->ack = 0; /* not used */
- msg->len = sizeof(*ev);
+ msg->len = sizeof(ev);
+ memcpy(msg->data, , sizeof(ev));
cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
}
void proc_id_connector(struct task_struct *task, int which_id)
{
struct cn_msg *msg;
- struct proc_event *ev;
+ struct proc_event ev;
__u8 buffer[CN_PROC_MSG_SIZE];
struct timespec ts;
@@ -110,32 +110,32 @@
return;
msg = (struct cn_msg*)buffer;
- ev = (struct proc_event*)msg->data;
- ev->what = which_id;
- ev->event_data.id.process_pid = task->pid;
- ev->event_data.id.process_tgid = task->tgid;
+ ev.what = which_id;
+ ev.event_data.id.process_pid = task->pid;
+ ev.event_data.id.process_tgid = task->tgid;
if (which_id == PROC_EVENT_UID) {
- ev->event_data.id.r.ruid = task->uid;
- ev->event_data.id.e.euid = task->euid;
+ ev.event_data.id.r.ruid = task->uid;
+ ev.event_data.id.e.euid = task->euid;
} else if (which_id == PROC_EVENT_GID) {
-
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
Hi Andrew.
There was some discussion on this patch but I believe we've agreed
on the first version I sent. This was ACKed by Matt Helsley.
Would you consider taking this in to -mm?
I've included my original patch email at the bottom.
On Mon, Dec 11, 2006 at 03:52:46PM -0800, Matt Helsley wrote:
(snipped up)
On Thu, 2006-12-07 at 17:22 -0600, Erik Jacobson wrote:
Signed-off-by: Erik Jacobson [EMAIL PROTECTED]
Acked-by: Matt Helsley [EMAIL PROTECTED]
---
cn_proc.c | 94
+++---
Original patch email:
Date: Thu, 7 Dec 2006 17:22:13 -0600
From: Erik Jacobson [EMAIL PROTECTED]
To: linux-kernel@vger.kernel.org
Cc: [EMAIL PROTECTED], [EMAIL PROTECTED]
Subject: [PATCH] connector: Some fixes for ia64 unaligned access errors
On ia64, the various functions that make up cn_proc.c cause kernel
unaligned access errors.
If you are using these, for example, to get notification about
all tasks forking and exiting, you get multiple unaligned access errors
per process.
Here, we just adjust how the variables are declared and use memcopy to
avoid the error messages.
Signed-off-by: Erik Jacobson [EMAIL PROTECTED]
---
cn_proc.c | 94 +++---
1 file changed, 47 insertions(+), 47 deletions(-)
--- linux.orig/drivers/connector/cn_proc.c 2006-11-29 15:57:37.0
-0600
+++ linux/drivers/connector/cn_proc.c 2006-12-07 16:50:03.195035791 -0600
@@ -49,7 +49,7 @@
void proc_fork_connector(struct task_struct *task)
{
struct cn_msg *msg;
- struct proc_event *ev;
+ struct proc_event ev;
__u8 buffer[CN_PROC_MSG_SIZE];
struct timespec ts;
@@ -57,19 +57,19 @@
return;
msg = (struct cn_msg*)buffer;
- ev = (struct proc_event*)msg-data;
- get_seq(msg-seq, ev-cpu);
+ get_seq(msg-seq, ev.cpu);
ktime_get_ts(ts); /* get high res monotonic timestamp */
- ev-timestamp_ns = timespec_to_ns(ts);
- ev-what = PROC_EVENT_FORK;
- ev-event_data.fork.parent_pid = task-real_parent-pid;
- ev-event_data.fork.parent_tgid = task-real_parent-tgid;
- ev-event_data.fork.child_pid = task-pid;
- ev-event_data.fork.child_tgid = task-tgid;
+ ev.timestamp_ns = timespec_to_ns(ts);
+ ev.what = PROC_EVENT_FORK;
+ ev.event_data.fork.parent_pid = task-real_parent-pid;
+ ev.event_data.fork.parent_tgid = task-real_parent-tgid;
+ ev.event_data.fork.child_pid = task-pid;
+ ev.event_data.fork.child_tgid = task-tgid;
memcpy(msg-id, cn_proc_event_id, sizeof(msg-id));
msg-ack = 0; /* not used */
- msg-len = sizeof(*ev);
+ msg-len = sizeof(ev);
+ memcpy(msg-data, ev, sizeof(ev));
/* If cn_netlink_send() failed, the data is not sent */
cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
}
@@ -77,7 +77,7 @@
void proc_exec_connector(struct task_struct *task)
{
struct cn_msg *msg;
- struct proc_event *ev;
+ struct proc_event ev;
struct timespec ts;
__u8 buffer[CN_PROC_MSG_SIZE];
@@ -85,24 +85,24 @@
return;
msg = (struct cn_msg*)buffer;
- ev = (struct proc_event*)msg-data;
- get_seq(msg-seq, ev-cpu);
+ get_seq(msg-seq, ev.cpu);
ktime_get_ts(ts); /* get high res monotonic timestamp */
- ev-timestamp_ns = timespec_to_ns(ts);
- ev-what = PROC_EVENT_EXEC;
- ev-event_data.exec.process_pid = task-pid;
- ev-event_data.exec.process_tgid = task-tgid;
+ ev.timestamp_ns = timespec_to_ns(ts);
+ ev.what = PROC_EVENT_EXEC;
+ ev.event_data.exec.process_pid = task-pid;
+ ev.event_data.exec.process_tgid = task-tgid;
memcpy(msg-id, cn_proc_event_id, sizeof(msg-id));
msg-ack = 0; /* not used */
- msg-len = sizeof(*ev);
+ msg-len = sizeof(ev);
+ memcpy(msg-data, ev, sizeof(ev));
cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
}
void proc_id_connector(struct task_struct *task, int which_id)
{
struct cn_msg *msg;
- struct proc_event *ev;
+ struct proc_event ev;
__u8 buffer[CN_PROC_MSG_SIZE];
struct timespec ts;
@@ -110,32 +110,32 @@
return;
msg = (struct cn_msg*)buffer;
- ev = (struct proc_event*)msg-data;
- ev-what = which_id;
- ev-event_data.id.process_pid = task-pid;
- ev-event_data.id.process_tgid = task-tgid;
+ ev.what = which_id;
+ ev.event_data.id.process_pid = task-pid;
+ ev.event_data.id.process_tgid = task-tgid;
if (which_id == PROC_EVENT_UID) {
- ev-event_data.id.r.ruid = task-uid;
- ev-event_data.id.e.euid = task-euid;
+ ev.event_data.id.r.ruid = task-uid;
+ ev.event_data.id.e.euid = task-euid;
} else if (which_id == PROC_EVENT_GID) {
- ev-event_data.id.r.rgid = task-gid;
-
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
On Tue, 12 Dec 2006 11:54:11 -0600 Erik Jacobson [EMAIL PROTECTED] wrote: Hi Andrew. There was some discussion on this patch but I believe we've agreed on the first version I sent. This was ACKed by Matt Helsley. Would you consider taking this in to -mm? I've included my original patch email at the bottom. I'm a bit late to the party. cn_proc.c | 94 +++--- But it's rather a lot of churn for such a thing. Did you consider simply using put_unaligned() against the specific offending field(s)? - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
On Tue, 12 Dec 2006 20:31:32 -0600 Erik Jacobson [EMAIL PROTECTED] wrote: But it's rather a lot of churn for such a thing. Did you consider simply using put_unaligned() against the specific offending field(s)? Hi. This was not considered. I wanted to give you some quick feedback, so I tried your suggestion in the fork path. It seemed to fix the problem as well. OK. put_unaligned(timespec_to_ns(ts), (__u64 *) ev-timestamp_ns); Is what I tried. I'm on vacation tomorrow but on Thursday, if you like, I can whip up a patch that does this and test it more thoroughly. Is this the direction you prefer? What I did just now was really quick and dirty to see if it has a shot or not but it looks like put_unaligned will fix it too. Well it's a one-liner and it makes it very clear what's going on. So unless there's some undiscovered downside, yes, I think it's a good way to go. It'll be an easier patch for the -stable guys to swallow too. There's no particular hurry on it. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
But it's rather a lot of churn for such a thing. Did you consider simply using put_unaligned() against the specific offending field(s)? Hi. This was not considered. I wanted to give you some quick feedback, so I tried your suggestion in the fork path. It seemed to fix the problem as well. put_unaligned(timespec_to_ns(ts), (__u64 *) ev-timestamp_ns); Is what I tried. I'm on vacation tomorrow but on Thursday, if you like, I can whip up a patch that does this and test it more thoroughly. Is this the direction you prefer? What I did just now was really quick and dirty to see if it has a shot or not but it looks like put_unaligned will fix it too. -- Erik Jacobson - Linux System Software - SGI - Eagan, Minnesota - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
Hi. I didn't want to leave this hanging and it stayed in my head so I thought I'd better just finish it and test it. I tried out this patch and it got rid of all three unaligned acces errors I was seeing with process connectors and the patch is indeed much smaller. I ran our container daemon program in debug mode to be sure the forks and exits still worked right when the patch was applied and all seemed well. I applied this patch to x86_64 as well as a sanity check and it seems working fine. Things look good to me. I'm ok if you prefer this patch, or the one already in -mm. Signed-off-by: Erik Jacobson [EMAIL PROTECTED] --- cn_proc.c | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) --- linux.orig/drivers/connector/cn_proc.c 2006-12-12 23:03:31.0 -0600 +++ linux/drivers/connector/cn_proc.c 2006-12-12 23:06:34.243535000 -0600 @@ -28,6 +28,7 @@ #include linux/init.h #include linux/connector.h #include asm/atomic.h +#include asm/unaligned.h #include linux/cn_proc.h @@ -60,7 +61,7 @@ ev = (struct proc_event*)msg-data; get_seq(msg-seq, ev-cpu); ktime_get_ts(ts); /* get high res monotonic timestamp */ - ev-timestamp_ns = timespec_to_ns(ts); + put_unaligned(timespec_to_ns(ts), (__u64 *) ev-timestamp_ns); ev-what = PROC_EVENT_FORK; ev-event_data.fork.parent_pid = task-real_parent-pid; ev-event_data.fork.parent_tgid = task-real_parent-tgid; @@ -88,7 +89,7 @@ ev = (struct proc_event*)msg-data; get_seq(msg-seq, ev-cpu); ktime_get_ts(ts); /* get high res monotonic timestamp */ - ev-timestamp_ns = timespec_to_ns(ts); + put_unaligned(timespec_to_ns(ts), (__u64 *) ev-timestamp_ns); ev-what = PROC_EVENT_EXEC; ev-event_data.exec.process_pid = task-pid; ev-event_data.exec.process_tgid = task-tgid; @@ -124,7 +125,7 @@ return; get_seq(msg-seq, ev-cpu); ktime_get_ts(ts); /* get high res monotonic timestamp */ - ev-timestamp_ns = timespec_to_ns(ts); + put_unaligned(timespec_to_ns(ts), (__u64 *) ev-timestamp_ns); memcpy(msg-id, cn_proc_event_id, sizeof(msg-id)); msg-ack = 0; /* not used */ @@ -146,7 +147,7 @@ ev = (struct proc_event*)msg-data; get_seq(msg-seq, ev-cpu); ktime_get_ts(ts); /* get high res monotonic timestamp */ - ev-timestamp_ns = timespec_to_ns(ts); + put_unaligned(timespec_to_ns(ts), (__u64 *) ev-timestamp_ns); ev-what = PROC_EVENT_EXIT; ev-event_data.exit.process_pid = task-pid; ev-event_data.exit.process_tgid = task-tgid; @@ -181,7 +182,7 @@ ev = (struct proc_event*)msg-data; msg-seq = rcvd_seq; ktime_get_ts(ts); /* get high res monotonic timestamp */ - ev-timestamp_ns = timespec_to_ns(ts); + put_unaligned(timespec_to_ns(ts), (__u64 *) ev-timestamp_ns); ev-cpu = -1; ev-what = PROC_EVENT_NONE; ev-event_data.ack.err = err; - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
From: Matt Helsley <[EMAIL PROTECTED]> Date: Mon, 11 Dec 2006 19:09:16 -0800 > Hmm, that GCC assumption conflicts with the prototypes of memcpy() I've > seen. When GCC expands __builtin_memcpy() internally it looks at the types of the arguments, and what it knows about their guarenteed alignment. memcpy()'s declaration of the first argument as "void *" has zero influence upon any of this. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
On Mon, 2006-12-11 at 17:50 -0800, David Miller wrote: > From: Pete Zaitcev <[EMAIL PROTECTED]> > Date: Mon, 11 Dec 2006 17:29:07 -0800 > > > On Mon, 11 Dec 2006 15:52:47 -0800, Matt Helsley <[EMAIL PROTECTED]> wrote: > > > > > I'm shocked memcpy() introduces 8-byte stores that violate architecture > > > alignment rules. Is there any chance this a bug in ia64's memcpy() > > > implementation? I've tried to read it but since I'm not familiar with > > > ia64 asm I can't make out significant parts of it in > > > arch/ia64/lib/memcpy.S. > > > > The arch/ia64/lib/memcpy.S is probably fine, it must be gcc doing > > an inline substitution of a well-known function. > > > > A commenter on my blog mentioned seeing the same thing in the past. > > (http://zaitcev.livejournal.com/107185.html?thread=128945#t128945) > > > > It's possible that applying (void *) cast to the first argument of memcpy > > would disrupt this optimization. But since we have a well understood > > patch by Erik, which only adds a penalty of 32 bytes of stack waste > > and 32 bytes of memcpy, I thought it best not to bother with heaping > > workarounds. > > Yes GCC can assume the object is aligned because of the type > of the argument to memcpy(). Hmm, that GCC assumption conflicts with the prototypes of memcpy() I've seen. Does the code really check the type or just the size argument? If the latter then I don't think assuming alignment is correct -- we could be copying a non-nul-terminated string that happens to be a power of 2 in length. Cheers, -Matt Helsley - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] connector: Some fixes for ia64 unaligned access errors
Pete Zaitcev wrote on Monday, December 11, 2006 5:29 PM
> On Mon, 11 Dec 2006 15:52:47 -0800, Matt Helsley <[EMAIL PROTECTED]> wrote:
>
> > I'm shocked memcpy() introduces 8-byte stores that violate architecture
> > alignment rules. Is there any chance this a bug in ia64's memcpy()
> > implementation? I've tried to read it but since I'm not familiar with
> > ia64 asm I can't make out significant parts of it in
> > arch/ia64/lib/memcpy.S.
>
> The arch/ia64/lib/memcpy.S is probably fine, it must be gcc doing
> an inline substitution of a well-known function.
arch/ia64/lib/memcpy.S is fine because it does alignment check at the very
beginning of the function and depends on the alignment of dst/src alignment,
it does different thing. The unaligned access is coming from gcc inlined
version of memcpy.
But looking deeply, memory allocation for proc_event in proc_for_connector
doesn't looked correct at all:
In drivers/connector/cn_proc.c:
#define CN_PROC_MSG_SIZE (sizeof(struct cn_msg) + sizeof(struct proc_event))
void proc_fork_connector(struct task_struct *task)
{
struct cn_msg *msg;
struct proc_event *ev;
__u8 buffer[CN_PROC_MSG_SIZE];
You can't do that because gcc assumes struct proc_event aligns on certain
boundary. Doing fancy hand crafting like that breaks code generated by gcc.
- Ken
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
From: Pete Zaitcev <[EMAIL PROTECTED]> Date: Mon, 11 Dec 2006 17:29:07 -0800 > On Mon, 11 Dec 2006 15:52:47 -0800, Matt Helsley <[EMAIL PROTECTED]> wrote: > > > I'm shocked memcpy() introduces 8-byte stores that violate architecture > > alignment rules. Is there any chance this a bug in ia64's memcpy() > > implementation? I've tried to read it but since I'm not familiar with > > ia64 asm I can't make out significant parts of it in > > arch/ia64/lib/memcpy.S. > > The arch/ia64/lib/memcpy.S is probably fine, it must be gcc doing > an inline substitution of a well-known function. > > A commenter on my blog mentioned seeing the same thing in the past. > (http://zaitcev.livejournal.com/107185.html?thread=128945#t128945) > > It's possible that applying (void *) cast to the first argument of memcpy > would disrupt this optimization. But since we have a well understood > patch by Erik, which only adds a penalty of 32 bytes of stack waste > and 32 bytes of memcpy, I thought it best not to bother with heaping > workarounds. Yes GCC can assume the object is aligned because of the type of the argument to memcpy(). I tried myself some games with adding a "packed" attribute to the pointer declaration (trying to tell it that "the thing pointed to" might be unaligned), but to no avail. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
On Mon, 11 Dec 2006 15:52:47 -0800, Matt Helsley <[EMAIL PROTECTED]> wrote: > I'm shocked memcpy() introduces 8-byte stores that violate architecture > alignment rules. Is there any chance this a bug in ia64's memcpy() > implementation? I've tried to read it but since I'm not familiar with > ia64 asm I can't make out significant parts of it in > arch/ia64/lib/memcpy.S. The arch/ia64/lib/memcpy.S is probably fine, it must be gcc doing an inline substitution of a well-known function. A commenter on my blog mentioned seeing the same thing in the past. (http://zaitcev.livejournal.com/107185.html?thread=128945#t128945) It's possible that applying (void *) cast to the first argument of memcpy would disrupt this optimization. But since we have a well understood patch by Erik, which only adds a penalty of 32 bytes of stack waste and 32 bytes of memcpy, I thought it best not to bother with heaping workarounds. -- Pete - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
On Thu, 2006-12-07 at 17:22 -0600, Erik Jacobson wrote:
> On ia64, the various functions that make up cn_proc.c cause kernel
> unaligned access errors.
>
> If you are using these, for example, to get notification about
> all tasks forking and exiting, you get multiple unaligned access errors
> per process.
>
> Here, we just adjust how the variables are declared and use memcopy to
> avoid the error messages.
>
> Signed-off-by: Erik Jacobson <[EMAIL PROTECTED]>
Acked-by: Matt Helsley <[EMAIL PROTECTED]>
> ---
>
> cn_proc.c | 94
> +++---
> 1 file changed, 47 insertions(+), 47 deletions(-)
> --- linux.orig/drivers/connector/cn_proc.c2006-11-29 15:57:37.0
> -0600
> +++ linux/drivers/connector/cn_proc.c 2006-12-07 16:50:03.195035791 -0600
> @@ -49,7 +49,7 @@
> void proc_fork_connector(struct task_struct *task)
> {
> struct cn_msg *msg;
> - struct proc_event *ev;
> + struct proc_event ev;
> __u8 buffer[CN_PROC_MSG_SIZE];
> struct timespec ts;
>
> @@ -57,19 +57,19 @@
> return;
>
> msg = (struct cn_msg*)buffer;
> - ev = (struct proc_event*)msg->data;
> - get_seq(>seq, >cpu);
> + get_seq(>seq, );
> ktime_get_ts(); /* get high res monotonic timestamp */
> - ev->timestamp_ns = timespec_to_ns();
> - ev->what = PROC_EVENT_FORK;
> - ev->event_data.fork.parent_pid = task->real_parent->pid;
> - ev->event_data.fork.parent_tgid = task->real_parent->tgid;
> - ev->event_data.fork.child_pid = task->pid;
> - ev->event_data.fork.child_tgid = task->tgid;
> + ev.timestamp_ns = timespec_to_ns();
> + ev.what = PROC_EVENT_FORK;
> + ev.event_data.fork.parent_pid = task->real_parent->pid;
> + ev.event_data.fork.parent_tgid = task->real_parent->tgid;
> + ev.event_data.fork.child_pid = task->pid;
> + ev.event_data.fork.child_tgid = task->tgid;
>
> memcpy(>id, _proc_event_id, sizeof(msg->id));
> msg->ack = 0; /* not used */
> - msg->len = sizeof(*ev);
> + msg->len = sizeof(ev);
> + memcpy(msg->data, , sizeof(ev));
> /* If cn_netlink_send() failed, the data is not sent */
> cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
> }
> @@ -77,7 +77,7 @@
> void proc_exec_connector(struct task_struct *task)
> {
> struct cn_msg *msg;
> - struct proc_event *ev;
> + struct proc_event ev;
> struct timespec ts;
> __u8 buffer[CN_PROC_MSG_SIZE];
>
> @@ -85,24 +85,24 @@
> return;
>
> msg = (struct cn_msg*)buffer;
> - ev = (struct proc_event*)msg->data;
> - get_seq(>seq, >cpu);
> + get_seq(>seq, );
> ktime_get_ts(); /* get high res monotonic timestamp */
> - ev->timestamp_ns = timespec_to_ns();
> - ev->what = PROC_EVENT_EXEC;
> - ev->event_data.exec.process_pid = task->pid;
> - ev->event_data.exec.process_tgid = task->tgid;
> + ev.timestamp_ns = timespec_to_ns();
> + ev.what = PROC_EVENT_EXEC;
> + ev.event_data.exec.process_pid = task->pid;
> + ev.event_data.exec.process_tgid = task->tgid;
>
> memcpy(>id, _proc_event_id, sizeof(msg->id));
> msg->ack = 0; /* not used */
> - msg->len = sizeof(*ev);
> + msg->len = sizeof(ev);
> + memcpy(msg->data, , sizeof(ev));
> cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
> }
>
> void proc_id_connector(struct task_struct *task, int which_id)
> {
> struct cn_msg *msg;
> - struct proc_event *ev;
> + struct proc_event ev;
> __u8 buffer[CN_PROC_MSG_SIZE];
> struct timespec ts;
>
> @@ -110,32 +110,32 @@
> return;
>
> msg = (struct cn_msg*)buffer;
> - ev = (struct proc_event*)msg->data;
> - ev->what = which_id;
> - ev->event_data.id.process_pid = task->pid;
> - ev->event_data.id.process_tgid = task->tgid;
> + ev.what = which_id;
> + ev.event_data.id.process_pid = task->pid;
> + ev.event_data.id.process_tgid = task->tgid;
> if (which_id == PROC_EVENT_UID) {
> - ev->event_data.id.r.ruid = task->uid;
> - ev->event_data.id.e.euid = task->euid;
> + ev.event_data.id.r.ruid = task->uid;
> + ev.event_data.id.e.euid = task->euid;
> } else if (which_id == PROC_EVENT_GID) {
> - ev->event_data.id.r.rgid = task->gid;
> - ev->event_data.id.e.egid = task->egid;
> + ev.event_data.id.r.rgid = task->gid;
> + ev.event_data.id.e.egid = task->egid;
> } else
> return;
> - get_seq(>seq, >cpu);
> + get_seq(>seq, );
> ktime_get_ts(); /* get high res monotonic timestamp */
> - ev->timestamp_ns = timespec_to_ns();
> + ev.timestamp_ns = timespec_to_ns();
>
> memcpy(>id, _proc_event_id, sizeof(msg->id));
> msg->ack = 0; /* not used */
> - msg->len = sizeof(*ev);
> + msg->len = sizeof(ev);
> + memcpy(msg->data, , sizeof(ev));
>
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
On Sat, 2006-12-09 at 18:34 -0800, Pete Zaitcev wrote: > On Sat, 9 Dec 2006 15:09:13 -0600, Erik Jacobson <[EMAIL PROTECTED]> wrote: > > > > Please try to declare u64 timestamp_ns, then copy it into the *ev > > > instead of copying whole *ev. This ought to fix the problem if > > > buffer[] ends aligned to 32 bits or better. > > > > So I took this suggestion for a spin and met with the same result. > > The unaligned access messages are still produced. > > I see. And I see you went a few steps forward with dignosing it: > > > dbg fork after timespec_to_ns call, b4 memcpy > > kernel unaligned access to 0xe03076b6fbe4, ip=0xa001004f1480 > > dbg fork after memcpy, b4 other ev settings... > > > a001004f1470 [MMI] ld8 r40=[r14] > > a001004f1476 ld8 r38=[r38] > > a001004f147c nop.i 0x0;; > > a001004f1480 [MIB] st8 [r39]=r40 > > a001004f1486 nop.i 0x0 > > a001004f148c br.call.sptk.many > > b0=a001000a36c0 ;; I'm not very familiar with ia64 asm but it looks like its loading and storying 8 bytes at a time for the memcpy(). > It seems rather strange that memcpy gets optimized this way. I could > not have foreseen it. Still, it was worth a try, even if putting 32 > extra bytes on stack and running memcpy on them does not seem too > onerous, for a fork(). Thanks for doing it, and let's go with your > original patch then... if Matt Helsley does not mind. OK, I'll ack the original. > Thank you, > -- Pete I'm shocked memcpy() introduces 8-byte stores that violate architecture alignment rules. Is there any chance this a bug in ia64's memcpy() implementation? I've tried to read it but since I'm not familiar with ia64 asm I can't make out significant parts of it in arch/ia64/lib/memcpy.S. Cheers, -Matt Helsley - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
On Sat, 2006-12-09 at 18:34 -0800, Pete Zaitcev wrote: On Sat, 9 Dec 2006 15:09:13 -0600, Erik Jacobson [EMAIL PROTECTED] wrote: Please try to declare u64 timestamp_ns, then copy it into the *ev instead of copying whole *ev. This ought to fix the problem if buffer[] ends aligned to 32 bits or better. So I took this suggestion for a spin and met with the same result. The unaligned access messages are still produced. I see. And I see you went a few steps forward with dignosing it: dbg fork after timespec_to_ns call, b4 memcpy kernel unaligned access to 0xe03076b6fbe4, ip=0xa001004f1480 dbg fork after memcpy, b4 other ev settings... a001004f1470 proc_fork_connector+0x1f0 [MMI] ld8 r40=[r14] a001004f1476 proc_fork_connector+0x1f6 ld8 r38=[r38] a001004f147c proc_fork_connector+0x1fc nop.i 0x0;; a001004f1480 proc_fork_connector+0x200 [MIB] st8 [r39]=r40 a001004f1486 proc_fork_connector+0x206 nop.i 0x0 a001004f148c proc_fork_connector+0x20c br.call.sptk.many b0=a001000a36c0 printk;; I'm not very familiar with ia64 asm but it looks like its loading and storying 8 bytes at a time for the memcpy(). It seems rather strange that memcpy gets optimized this way. I could not have foreseen it. Still, it was worth a try, even if putting 32 extra bytes on stack and running memcpy on them does not seem too onerous, for a fork(). Thanks for doing it, and let's go with your original patch then... if Matt Helsley does not mind. OK, I'll ack the original. Thank you, -- Pete I'm shocked memcpy() introduces 8-byte stores that violate architecture alignment rules. Is there any chance this a bug in ia64's memcpy() implementation? I've tried to read it but since I'm not familiar with ia64 asm I can't make out significant parts of it in arch/ia64/lib/memcpy.S. snip Cheers, -Matt Helsley - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
On Thu, 2006-12-07 at 17:22 -0600, Erik Jacobson wrote:
On ia64, the various functions that make up cn_proc.c cause kernel
unaligned access errors.
If you are using these, for example, to get notification about
all tasks forking and exiting, you get multiple unaligned access errors
per process.
Here, we just adjust how the variables are declared and use memcopy to
avoid the error messages.
Signed-off-by: Erik Jacobson [EMAIL PROTECTED]
Acked-by: Matt Helsley [EMAIL PROTECTED]
---
cn_proc.c | 94
+++---
1 file changed, 47 insertions(+), 47 deletions(-)
--- linux.orig/drivers/connector/cn_proc.c2006-11-29 15:57:37.0
-0600
+++ linux/drivers/connector/cn_proc.c 2006-12-07 16:50:03.195035791 -0600
@@ -49,7 +49,7 @@
void proc_fork_connector(struct task_struct *task)
{
struct cn_msg *msg;
- struct proc_event *ev;
+ struct proc_event ev;
__u8 buffer[CN_PROC_MSG_SIZE];
struct timespec ts;
@@ -57,19 +57,19 @@
return;
msg = (struct cn_msg*)buffer;
- ev = (struct proc_event*)msg-data;
- get_seq(msg-seq, ev-cpu);
+ get_seq(msg-seq, ev.cpu);
ktime_get_ts(ts); /* get high res monotonic timestamp */
- ev-timestamp_ns = timespec_to_ns(ts);
- ev-what = PROC_EVENT_FORK;
- ev-event_data.fork.parent_pid = task-real_parent-pid;
- ev-event_data.fork.parent_tgid = task-real_parent-tgid;
- ev-event_data.fork.child_pid = task-pid;
- ev-event_data.fork.child_tgid = task-tgid;
+ ev.timestamp_ns = timespec_to_ns(ts);
+ ev.what = PROC_EVENT_FORK;
+ ev.event_data.fork.parent_pid = task-real_parent-pid;
+ ev.event_data.fork.parent_tgid = task-real_parent-tgid;
+ ev.event_data.fork.child_pid = task-pid;
+ ev.event_data.fork.child_tgid = task-tgid;
memcpy(msg-id, cn_proc_event_id, sizeof(msg-id));
msg-ack = 0; /* not used */
- msg-len = sizeof(*ev);
+ msg-len = sizeof(ev);
+ memcpy(msg-data, ev, sizeof(ev));
/* If cn_netlink_send() failed, the data is not sent */
cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
}
@@ -77,7 +77,7 @@
void proc_exec_connector(struct task_struct *task)
{
struct cn_msg *msg;
- struct proc_event *ev;
+ struct proc_event ev;
struct timespec ts;
__u8 buffer[CN_PROC_MSG_SIZE];
@@ -85,24 +85,24 @@
return;
msg = (struct cn_msg*)buffer;
- ev = (struct proc_event*)msg-data;
- get_seq(msg-seq, ev-cpu);
+ get_seq(msg-seq, ev.cpu);
ktime_get_ts(ts); /* get high res monotonic timestamp */
- ev-timestamp_ns = timespec_to_ns(ts);
- ev-what = PROC_EVENT_EXEC;
- ev-event_data.exec.process_pid = task-pid;
- ev-event_data.exec.process_tgid = task-tgid;
+ ev.timestamp_ns = timespec_to_ns(ts);
+ ev.what = PROC_EVENT_EXEC;
+ ev.event_data.exec.process_pid = task-pid;
+ ev.event_data.exec.process_tgid = task-tgid;
memcpy(msg-id, cn_proc_event_id, sizeof(msg-id));
msg-ack = 0; /* not used */
- msg-len = sizeof(*ev);
+ msg-len = sizeof(ev);
+ memcpy(msg-data, ev, sizeof(ev));
cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
}
void proc_id_connector(struct task_struct *task, int which_id)
{
struct cn_msg *msg;
- struct proc_event *ev;
+ struct proc_event ev;
__u8 buffer[CN_PROC_MSG_SIZE];
struct timespec ts;
@@ -110,32 +110,32 @@
return;
msg = (struct cn_msg*)buffer;
- ev = (struct proc_event*)msg-data;
- ev-what = which_id;
- ev-event_data.id.process_pid = task-pid;
- ev-event_data.id.process_tgid = task-tgid;
+ ev.what = which_id;
+ ev.event_data.id.process_pid = task-pid;
+ ev.event_data.id.process_tgid = task-tgid;
if (which_id == PROC_EVENT_UID) {
- ev-event_data.id.r.ruid = task-uid;
- ev-event_data.id.e.euid = task-euid;
+ ev.event_data.id.r.ruid = task-uid;
+ ev.event_data.id.e.euid = task-euid;
} else if (which_id == PROC_EVENT_GID) {
- ev-event_data.id.r.rgid = task-gid;
- ev-event_data.id.e.egid = task-egid;
+ ev.event_data.id.r.rgid = task-gid;
+ ev.event_data.id.e.egid = task-egid;
} else
return;
- get_seq(msg-seq, ev-cpu);
+ get_seq(msg-seq, ev.cpu);
ktime_get_ts(ts); /* get high res monotonic timestamp */
- ev-timestamp_ns = timespec_to_ns(ts);
+ ev.timestamp_ns = timespec_to_ns(ts);
memcpy(msg-id, cn_proc_event_id, sizeof(msg-id));
msg-ack = 0; /* not used */
- msg-len = sizeof(*ev);
+ msg-len = sizeof(ev);
+ memcpy(msg-data, ev, sizeof(ev));
cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
}
void proc_exit_connector(struct task_struct *task)
{
struct cn_msg *msg;
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
On Mon, 11 Dec 2006 15:52:47 -0800, Matt Helsley [EMAIL PROTECTED] wrote: I'm shocked memcpy() introduces 8-byte stores that violate architecture alignment rules. Is there any chance this a bug in ia64's memcpy() implementation? I've tried to read it but since I'm not familiar with ia64 asm I can't make out significant parts of it in arch/ia64/lib/memcpy.S. The arch/ia64/lib/memcpy.S is probably fine, it must be gcc doing an inline substitution of a well-known function. A commenter on my blog mentioned seeing the same thing in the past. (http://zaitcev.livejournal.com/107185.html?thread=128945#t128945) It's possible that applying (void *) cast to the first argument of memcpy would disrupt this optimization. But since we have a well understood patch by Erik, which only adds a penalty of 32 bytes of stack waste and 32 bytes of memcpy, I thought it best not to bother with heaping workarounds. -- Pete - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
From: Pete Zaitcev [EMAIL PROTECTED] Date: Mon, 11 Dec 2006 17:29:07 -0800 On Mon, 11 Dec 2006 15:52:47 -0800, Matt Helsley [EMAIL PROTECTED] wrote: I'm shocked memcpy() introduces 8-byte stores that violate architecture alignment rules. Is there any chance this a bug in ia64's memcpy() implementation? I've tried to read it but since I'm not familiar with ia64 asm I can't make out significant parts of it in arch/ia64/lib/memcpy.S. The arch/ia64/lib/memcpy.S is probably fine, it must be gcc doing an inline substitution of a well-known function. A commenter on my blog mentioned seeing the same thing in the past. (http://zaitcev.livejournal.com/107185.html?thread=128945#t128945) It's possible that applying (void *) cast to the first argument of memcpy would disrupt this optimization. But since we have a well understood patch by Erik, which only adds a penalty of 32 bytes of stack waste and 32 bytes of memcpy, I thought it best not to bother with heaping workarounds. Yes GCC can assume the object is aligned because of the type of the argument to memcpy(). I tried myself some games with adding a packed attribute to the pointer declaration (trying to tell it that the thing pointed to might be unaligned), but to no avail. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] connector: Some fixes for ia64 unaligned access errors
Pete Zaitcev wrote on Monday, December 11, 2006 5:29 PM
On Mon, 11 Dec 2006 15:52:47 -0800, Matt Helsley [EMAIL PROTECTED] wrote:
I'm shocked memcpy() introduces 8-byte stores that violate architecture
alignment rules. Is there any chance this a bug in ia64's memcpy()
implementation? I've tried to read it but since I'm not familiar with
ia64 asm I can't make out significant parts of it in
arch/ia64/lib/memcpy.S.
The arch/ia64/lib/memcpy.S is probably fine, it must be gcc doing
an inline substitution of a well-known function.
arch/ia64/lib/memcpy.S is fine because it does alignment check at the very
beginning of the function and depends on the alignment of dst/src alignment,
it does different thing. The unaligned access is coming from gcc inlined
version of memcpy.
But looking deeply, memory allocation for proc_event in proc_for_connector
doesn't looked correct at all:
In drivers/connector/cn_proc.c:
#define CN_PROC_MSG_SIZE (sizeof(struct cn_msg) + sizeof(struct proc_event))
void proc_fork_connector(struct task_struct *task)
{
struct cn_msg *msg;
struct proc_event *ev;
__u8 buffer[CN_PROC_MSG_SIZE];
You can't do that because gcc assumes struct proc_event aligns on certain
boundary. Doing fancy hand crafting like that breaks code generated by gcc.
- Ken
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
On Mon, 2006-12-11 at 17:50 -0800, David Miller wrote: From: Pete Zaitcev [EMAIL PROTECTED] Date: Mon, 11 Dec 2006 17:29:07 -0800 On Mon, 11 Dec 2006 15:52:47 -0800, Matt Helsley [EMAIL PROTECTED] wrote: I'm shocked memcpy() introduces 8-byte stores that violate architecture alignment rules. Is there any chance this a bug in ia64's memcpy() implementation? I've tried to read it but since I'm not familiar with ia64 asm I can't make out significant parts of it in arch/ia64/lib/memcpy.S. The arch/ia64/lib/memcpy.S is probably fine, it must be gcc doing an inline substitution of a well-known function. A commenter on my blog mentioned seeing the same thing in the past. (http://zaitcev.livejournal.com/107185.html?thread=128945#t128945) It's possible that applying (void *) cast to the first argument of memcpy would disrupt this optimization. But since we have a well understood patch by Erik, which only adds a penalty of 32 bytes of stack waste and 32 bytes of memcpy, I thought it best not to bother with heaping workarounds. Yes GCC can assume the object is aligned because of the type of the argument to memcpy(). Hmm, that GCC assumption conflicts with the prototypes of memcpy() I've seen. Does the code really check the type or just the size argument? If the latter then I don't think assuming alignment is correct -- we could be copying a non-nul-terminated string that happens to be a power of 2 in length. Cheers, -Matt Helsley - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
From: Matt Helsley [EMAIL PROTECTED] Date: Mon, 11 Dec 2006 19:09:16 -0800 Hmm, that GCC assumption conflicts with the prototypes of memcpy() I've seen. When GCC expands __builtin_memcpy() internally it looks at the types of the arguments, and what it knows about their guarenteed alignment. memcpy()'s declaration of the first argument as void * has zero influence upon any of this. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
On Sat, 9 Dec 2006 15:09:13 -0600, Erik Jacobson <[EMAIL PROTECTED]> wrote:
> > Please try to declare u64 timestamp_ns, then copy it into the *ev
> > instead of copying whole *ev. This ought to fix the problem if
> > buffer[] ends aligned to 32 bits or better.
>
> So I took this suggestion for a spin and met with the same result.
> The unaligned access messages are still produced.
I see. And I see you went a few steps forward with dignosing it:
> dbg fork after timespec_to_ns call, b4 memcpy
> kernel unaligned access to 0xe03076b6fbe4, ip=0xa001004f1480
> dbg fork after memcpy, b4 other ev settings...
> a001004f1470 [MMI] ld8 r40=[r14]
> a001004f1476 ld8 r38=[r38]
> a001004f147c nop.i 0x0;;
> a001004f1480 [MIB] st8 [r39]=r40
> a001004f1486 nop.i 0x0
> a001004f148c br.call.sptk.many
> b0=a001000a36c0 ;;
It seems rather strange that memcpy gets optimized this way. I could
not have foreseen it. Still, it was worth a try, even if putting 32
extra bytes on stack and running memcpy on them does not seem too
onerous, for a fork(). Thanks for doing it, and let's go with your
original patch then... if Matt Helsley does not mind.
Thank you,
-- Pete
P.S. This syntax seems to cry for being expressed in diff -u:
>ktime_get_ts(); /* get high res monotonic timestamp */
>//ev->timestamp_ns = timespec_to_ns(); removed
>printk("dbg fork before timestamp_to_ns call\n");
>timestamp_ns = timespec_to_ns(); //added
>printk("dbg fork after timespec_to_ns call, b4 memcpy\n");
>memcpy(>timestamp_ns, _ns, sizeof(ev->timestamp_ns)); //added
>printk("dbg fork after memcpy, b4 other ev settings...\n");
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
> > Here, we just adjust how the variables are declared and use memcopy to
> > avoid the error messages.
> > - ev->timestamp_ns = timespec_to_ns();
> > + ev.timestamp_ns = timespec_to_ns();
> Please try to declare u64 timestamp_ns, then copy it into the *ev
> instead of copying whole *ev. This ought to fix the problem if
> buffer[] ends aligned to 32 bits or better.
So I took this suggestion for a spin and met with the same result.
Please confirm I understood what you were asking.
The unaligned access messages are still produced.
Here is how I changed the function (declared timestamp_ns in the
top of the function, and used memcpy to copy it in place).
The case of fork is provided below but I did it to all the appropriate
functions. See the removed and added comments to see what changed.
void proc_fork_connector(struct task_struct *task)
{
struct cn_msg *msg;
struct proc_event *ev;
__u8 buffer[CN_PROC_MSG_SIZE];
struct timespec ts;
u64 timestamp_ns; // added
if (atomic_read(_event_num_listeners) < 1)
return;
msg = (struct cn_msg*)buffer;
ev = (struct proc_event*)msg->data;
get_seq(>seq, >cpu);
ktime_get_ts(); /* get high res monotonic timestamp */
//ev->timestamp_ns = timespec_to_ns(); removed
printk("dbg fork before timestamp_to_ns call\n");
timestamp_ns = timespec_to_ns(); //added
printk("dbg fork after timespec_to_ns call, b4 memcpy\n");
memcpy(>timestamp_ns, _ns, sizeof(ev->timestamp_ns)); //added
printk("dbg fork after memcpy, b4 other ev settings...\n");
ev->what = PROC_EVENT_FORK;
ev->event_data.fork.parent_pid = task->real_parent->pid;
ev->event_data.fork.parent_tgid = task->real_parent->tgid;
ev->event_data.fork.child_pid = task->pid;
ev->event_data.fork.child_tgid = task->tgid;
memcpy(>id, _proc_event_id, sizeof(msg->id));
msg->ack = 0; /* not used */
msg->len = sizeof(*ev);
/* If cn_netlink_send() failed, the data is not sent */
cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
}
Here is the output from this kernel when the container program tracking
tasks is running. You can see the unaligned access messages along with
my debug statements for the case of fork. The TASK and EXIT messages are
from the container program running in debug mode in the background of the
same tty.
[EMAIL PROTECTED] ~]# /bin/true
dbg fork before timestamp_to_ns call
dbg fork after timespec_to_ns call, b4 memcpy
kernel unaligned access to 0xe03076b6fbe4, ip=0xa001004f1480
dbg fork after memcpy, b4 other ev settings...
FORK parent 3389 (bash) child 3418 (bash)
kernel unaligned access to 0xe030732dfe24, ip=0xa001004f17c1
kernel unaligned access to 0xe030732dfe04, ip=0xa001004f1241
[EMAIL PROTECTED] ~]# EXIT pid 3418 (No-Info)
I had built this kernel with KDB and I checked the IPs.
ip 0xa001004f1480 is in proc_fork_connector, between the two
printk calls. That should make it the memcpy. That jives with the
console output too.
0xa001004f17c1 is proc_exec_connector
0xa001004f1241 is proc_exit_connector
I'm not able to read the disassembly well, but I've included the full
disassembly of proc_fork_connector below in case that's helpful.
I could try some memory dumps if needed. I really don't have lots of
experience fixing kernel unaligned access issues so if there are other
things I should try that are better than my original patch, I'm all ears.
vmlinux: file format elf64-ia64-little
Disassembly of section .text:
proc_fork_connector():
a001004f1280 [MMB] alloc r36=ar.pfs,9,6,0
a001004f1286 adds r12=-80,r12
a001004f128c nop.b 0x0
a001004f1290 [MMI] adds r20=3200,r13
a001004f1296 addl r2=-1981124,r1
a001004f129c mov r35=b0;;
a001004f12a0 [MMI] ld4.acq r14=[r2]
a001004f12a6 adds r19=40,r20
a001004f12ac adds r34=40,r12;;
a001004f12b0 [MIB] nop.m 0x0
a001004f12b6 cmp4.lt p7,p6=0,r14
a001004f12bc(p06) br.cond.dpnt.few
a001004f1570
a001004f12c0 [MMI] ld4 r15=[r19];;
a001004f12c6 adds r8=1,r15
a001004f12cc nop.i 0x0
a001004f12d0 [MMI] nop.m 0x0;;
a001004f12d6 st4 [r19]=r8
a001004f12dc nop.i 0x0;;
a001004f12e0 [MMI] mov r3=-65496
a001004f12e6 mov r14=-35804
a001004f12ec adds r40=48,r12
a001004f12f0 [MII] adds r30=20,r20
a001004f12f6 adds r28=64,r12;;
a001004f12fc nop.i 0x0
a001004f1300 [MMI] ld8 r2=[r3]
a001004f1306 nop.m 0x0
a001004f130c nop.i 0x0;;
a001004f1310 [MMI] add r31=r14,r2;;
a001004f1316 ld4 r38=[r31]
a001004f131c nop.i 0x0;;
a001004f1320 [MMI] adds r39=1,r38
a001004f1326 st4 [r40]=r38
a001004f132c
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
Here, we just adjust how the variables are declared and use memcopy to
avoid the error messages.
- ev-timestamp_ns = timespec_to_ns(ts);
+ ev.timestamp_ns = timespec_to_ns(ts);
Please try to declare u64 timestamp_ns, then copy it into the *ev
instead of copying whole *ev. This ought to fix the problem if
buffer[] ends aligned to 32 bits or better.
So I took this suggestion for a spin and met with the same result.
Please confirm I understood what you were asking.
The unaligned access messages are still produced.
Here is how I changed the function (declared timestamp_ns in the
top of the function, and used memcpy to copy it in place).
The case of fork is provided below but I did it to all the appropriate
functions. See the removed and added comments to see what changed.
void proc_fork_connector(struct task_struct *task)
{
struct cn_msg *msg;
struct proc_event *ev;
__u8 buffer[CN_PROC_MSG_SIZE];
struct timespec ts;
u64 timestamp_ns; // added
if (atomic_read(proc_event_num_listeners) 1)
return;
msg = (struct cn_msg*)buffer;
ev = (struct proc_event*)msg-data;
get_seq(msg-seq, ev-cpu);
ktime_get_ts(ts); /* get high res monotonic timestamp */
//ev-timestamp_ns = timespec_to_ns(ts); removed
printk(dbg fork before timestamp_to_ns call\n);
timestamp_ns = timespec_to_ns(ts); //added
printk(dbg fork after timespec_to_ns call, b4 memcpy\n);
memcpy(ev-timestamp_ns, timestamp_ns, sizeof(ev-timestamp_ns)); //added
printk(dbg fork after memcpy, b4 other ev settings...\n);
ev-what = PROC_EVENT_FORK;
ev-event_data.fork.parent_pid = task-real_parent-pid;
ev-event_data.fork.parent_tgid = task-real_parent-tgid;
ev-event_data.fork.child_pid = task-pid;
ev-event_data.fork.child_tgid = task-tgid;
memcpy(msg-id, cn_proc_event_id, sizeof(msg-id));
msg-ack = 0; /* not used */
msg-len = sizeof(*ev);
/* If cn_netlink_send() failed, the data is not sent */
cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
}
Here is the output from this kernel when the container program tracking
tasks is running. You can see the unaligned access messages along with
my debug statements for the case of fork. The TASK and EXIT messages are
from the container program running in debug mode in the background of the
same tty.
[EMAIL PROTECTED] ~]# /bin/true
dbg fork before timestamp_to_ns call
dbg fork after timespec_to_ns call, b4 memcpy
kernel unaligned access to 0xe03076b6fbe4, ip=0xa001004f1480
dbg fork after memcpy, b4 other ev settings...
FORK parent 3389 (bash) child 3418 (bash)
kernel unaligned access to 0xe030732dfe24, ip=0xa001004f17c1
kernel unaligned access to 0xe030732dfe04, ip=0xa001004f1241
[EMAIL PROTECTED] ~]# EXIT pid 3418 (No-Info)
I had built this kernel with KDB and I checked the IPs.
ip 0xa001004f1480 is in proc_fork_connector, between the two
printk calls. That should make it the memcpy. That jives with the
console output too.
0xa001004f17c1 is proc_exec_connector
0xa001004f1241 is proc_exit_connector
I'm not able to read the disassembly well, but I've included the full
disassembly of proc_fork_connector below in case that's helpful.
I could try some memory dumps if needed. I really don't have lots of
experience fixing kernel unaligned access issues so if there are other
things I should try that are better than my original patch, I'm all ears.
vmlinux: file format elf64-ia64-little
Disassembly of section .text:
proc_fork_connector():
a001004f1280 proc_fork_connector [MMB] alloc r36=ar.pfs,9,6,0
a001004f1286 proc_fork_connector+0x6 adds r12=-80,r12
a001004f128c proc_fork_connector+0xc nop.b 0x0
a001004f1290 proc_fork_connector+0x10 [MMI] adds r20=3200,r13
a001004f1296 proc_fork_connector+0x16 addl r2=-1981124,r1
a001004f129c proc_fork_connector+0x1c mov r35=b0;;
a001004f12a0 proc_fork_connector+0x20 [MMI] ld4.acq r14=[r2]
a001004f12a6 proc_fork_connector+0x26 adds r19=40,r20
a001004f12ac proc_fork_connector+0x2c adds r34=40,r12;;
a001004f12b0 proc_fork_connector+0x30 [MIB] nop.m 0x0
a001004f12b6 proc_fork_connector+0x36 cmp4.lt p7,p6=0,r14
a001004f12bc proc_fork_connector+0x3c (p06) br.cond.dpnt.few
a001004f1570 proc_fork_connector+0x2f0
a001004f12c0 proc_fork_connector+0x40 [MMI] ld4 r15=[r19];;
a001004f12c6 proc_fork_connector+0x46 adds r8=1,r15
a001004f12cc proc_fork_connector+0x4c nop.i 0x0
a001004f12d0 proc_fork_connector+0x50 [MMI] nop.m 0x0;;
a001004f12d6 proc_fork_connector+0x56 st4 [r19]=r8
a001004f12dc proc_fork_connector+0x5c nop.i 0x0;;
a001004f12e0 proc_fork_connector+0x60 [MMI] mov r3=-65496
a001004f12e6 proc_fork_connector+0x66 mov r14=-35804
a001004f12ec proc_fork_connector+0x6c
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
On Sat, 9 Dec 2006 15:09:13 -0600, Erik Jacobson [EMAIL PROTECTED] wrote: Please try to declare u64 timestamp_ns, then copy it into the *ev instead of copying whole *ev. This ought to fix the problem if buffer[] ends aligned to 32 bits or better. So I took this suggestion for a spin and met with the same result. The unaligned access messages are still produced. I see. And I see you went a few steps forward with dignosing it: dbg fork after timespec_to_ns call, b4 memcpy kernel unaligned access to 0xe03076b6fbe4, ip=0xa001004f1480 dbg fork after memcpy, b4 other ev settings... a001004f1470 proc_fork_connector+0x1f0 [MMI] ld8 r40=[r14] a001004f1476 proc_fork_connector+0x1f6 ld8 r38=[r38] a001004f147c proc_fork_connector+0x1fc nop.i 0x0;; a001004f1480 proc_fork_connector+0x200 [MIB] st8 [r39]=r40 a001004f1486 proc_fork_connector+0x206 nop.i 0x0 a001004f148c proc_fork_connector+0x20c br.call.sptk.many b0=a001000a36c0 printk;; It seems rather strange that memcpy gets optimized this way. I could not have foreseen it. Still, it was worth a try, even if putting 32 extra bytes on stack and running memcpy on them does not seem too onerous, for a fork(). Thanks for doing it, and let's go with your original patch then... if Matt Helsley does not mind. Thank you, -- Pete P.S. This syntax seems to cry for being expressed in diff -u: ktime_get_ts(ts); /* get high res monotonic timestamp */ //ev-timestamp_ns = timespec_to_ns(ts); removed printk(dbg fork before timestamp_to_ns call\n); timestamp_ns = timespec_to_ns(ts); //added printk(dbg fork after timespec_to_ns call, b4 memcpy\n); memcpy(ev-timestamp_ns, timestamp_ns, sizeof(ev-timestamp_ns)); //added printk(dbg fork after memcpy, b4 other ev settings...\n); - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
On Fri, 2006-12-08 at 19:20 -0800, Pete Zaitcev wrote: > On Thu, 7 Dec 2006 17:22:13 -0600, Erik Jacobson <[EMAIL PROTECTED]> wrote: Erik, Thanks for cc'ing me on this patch. > > Here, we just adjust how the variables are declared and use memcopy to > > avoid the error messages. > > - ev->timestamp_ns = timespec_to_ns(); > > + ev.timestamp_ns = timespec_to_ns(); > > Please try to declare u64 timestamp_ns, then copy it into the *ev > instead of copying whole *ev. This ought to fix the problem if > buffer[] ends aligned to 32 bits or better. Yes, I like this suggestion. > Also... Since Linus does not take patches in general off l-k anymore, > you have to find whoever herds the connector and feed the patch through > him/her. > > -- Pete I contributed and continue to follow this particular connector (Evgeniy Polyakov wrote the generic connector code). In the past I've given my patches to Andrew Morton and requested inclusion in -mm -- that may be the best route (Andrew?). I'd be happy to Ack a patch implementing Pete's suggestion. Cheers, -Matt Helsley - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
On Thu, 7 Dec 2006 17:22:13 -0600, Erik Jacobson <[EMAIL PROTECTED]> wrote: > Here, we just adjust how the variables are declared and use memcopy to > avoid the error messages. > - ev->timestamp_ns = timespec_to_ns(); > + ev.timestamp_ns = timespec_to_ns(); Please try to declare u64 timestamp_ns, then copy it into the *ev instead of copying whole *ev. This ought to fix the problem if buffer[] ends aligned to 32 bits or better. Also... Since Linus does not take patches in general off l-k anymore, you have to find whoever herds the connector and feed the patch through him/her. -- Pete - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
On Thu, 7 Dec 2006 17:22:13 -0600, Erik Jacobson [EMAIL PROTECTED] wrote: Here, we just adjust how the variables are declared and use memcopy to avoid the error messages. - ev-timestamp_ns = timespec_to_ns(ts); + ev.timestamp_ns = timespec_to_ns(ts); Please try to declare u64 timestamp_ns, then copy it into the *ev instead of copying whole *ev. This ought to fix the problem if buffer[] ends aligned to 32 bits or better. Also... Since Linus does not take patches in general off l-k anymore, you have to find whoever herds the connector and feed the patch through him/her. -- Pete - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: Some fixes for ia64 unaligned access errors
On Fri, 2006-12-08 at 19:20 -0800, Pete Zaitcev wrote: On Thu, 7 Dec 2006 17:22:13 -0600, Erik Jacobson [EMAIL PROTECTED] wrote: Erik, Thanks for cc'ing me on this patch. Here, we just adjust how the variables are declared and use memcopy to avoid the error messages. - ev-timestamp_ns = timespec_to_ns(ts); + ev.timestamp_ns = timespec_to_ns(ts); Please try to declare u64 timestamp_ns, then copy it into the *ev instead of copying whole *ev. This ought to fix the problem if buffer[] ends aligned to 32 bits or better. Yes, I like this suggestion. Also... Since Linus does not take patches in general off l-k anymore, you have to find whoever herds the connector and feed the patch through him/her. -- Pete I contributed and continue to follow this particular connector (Evgeniy Polyakov wrote the generic connector code). In the past I've given my patches to Andrew Morton and requested inclusion in -mm -- that may be the best route (Andrew?). I'd be happy to Ack a patch implementing Pete's suggestion. Cheers, -Matt Helsley - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
