Bug#609371: linux-image-2.6.37-trunk-sparc64: module scsi_mod: Unknown relocation: 36

After applying David's "remove align" patch, I got it to boot on x86_64
with the following two patches. I thought just adding the "align" to the
structure declaration would work, but it still failed on the syscall for
init_module. By removing the double "declaration" of event_exit_##sname,
removed this problem.

I'll test this on x86 32bit and PPC 64. If it works there, I'll push all
of them out for 38. Should these go to 37 stable too?

-- Steve



--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

* Steven Rostedt (rostedt@goodmis.org) wrote:
> After applying David's "remove align" patch, I got it to boot on x86_64
> with the following two patches. I thought just adding the "align" to the
> structure declaration would work, but it still failed on the syscall for
> init_module. By removing the double "declaration" of event_exit_##sname,
> removed this problem.
> 
> I'll test this on x86 32bit and PPC 64. If it works there, I'll push all
> of them out for 38. Should these go to 37 stable too?

Please hold before adding these patches into git. They don't seem to address the
underlying problem correctly. See the latest exchanges between David Miller and
myself for more info.

We need to come up with something better than "it boots" as an explanation for
the fix.

Thanks,

Mathieu

> 
> -- Steve
> 
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index cacc27a..0e3bce6 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -142,8 +142,6 @@ extern struct trace_event_functions exit_syscall_print_funcs;
>  #define SYSCALL_TRACE_EXIT_EVENT(sname)					\
>  	static struct syscall_metadata					\
>  	__attribute__((__aligned__(4))) __syscall_meta_##sname;		\
> -	static struct ftrace_event_call					\
> -	__attribute__((__aligned__(4))) event_exit_##sname;		\
>  	static struct ftrace_event_call __used				\
>  	  __attribute__((__aligned__(4)))				\
>  	  __attribute__((section("_ftrace_events")))			\
> 
> 
> Index: linux-trace.git/include/linux/ftrace_event.h
> ===================================================================
> --- linux-trace.git.orig/include/linux/ftrace_event.h
> +++ linux-trace.git/include/linux/ftrace_event.h
> @@ -194,7 +194,7 @@ struct ftrace_event_call {
>  	int				perf_refcount;
>  	struct hlist_head __percpu	*perf_events;
>  #endif
> -};
> +} __attribute__((__aligned__(32)));
>  
>  #define PERF_MAX_TRACE_SIZE	2048
>  
> 
>

On Wed, 2011-01-19 at 11:15 -0500, Mathieu Desnoyers wrote:
> * Steven Rostedt (rostedt@goodmis.org) wrote:
> > After applying David's "remove align" patch, I got it to boot on x86_64
> > with the following two patches. I thought just adding the "align" to the
> > structure declaration would work, but it still failed on the syscall for
> > init_module. By removing the double "declaration" of event_exit_##sname,
> > removed this problem.
> > 
> > I'll test this on x86 32bit and PPC 64. If it works there, I'll push all
> > of them out for 38. Should these go to 37 stable too?
> 
> Please hold before adding these patches into git. They don't seem to address the
> underlying problem correctly. See the latest exchanges between David Miller and
> myself for more info.
> 
> We need to come up with something better than "it boots" as an explanation for
> the fix.

Yes, I agree that we should solve this issue correctly. But if there is
a work around to the problem, we could implement that if the real
solution is not in our grasp yet.

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

* Steven Rostedt (rostedt@goodmis.org) wrote:
> On Wed, 2011-01-19 at 11:15 -0500, Mathieu Desnoyers wrote:
> > * Steven Rostedt (rostedt@goodmis.org) wrote:
> > > After applying David's "remove align" patch, I got it to boot on x86_64
> > > with the following two patches. I thought just adding the "align" to the
> > > structure declaration would work, but it still failed on the syscall for
> > > init_module. By removing the double "declaration" of event_exit_##sname,
> > > removed this problem.
> > > 
> > > I'll test this on x86 32bit and PPC 64. If it works there, I'll push all
> > > of them out for 38. Should these go to 37 stable too?
> > 
> > Please hold before adding these patches into git. They don't seem to address the
> > underlying problem correctly. See the latest exchanges between David Miller and
> > myself for more info.
> > 
> > We need to come up with something better than "it boots" as an explanation for
> > the fix.
> 
> Yes, I agree that we should solve this issue correctly. But if there is
> a work around to the problem, we could implement that if the real
> solution is not in our grasp yet.

A known working workaround (used in tracepoints for a few years) is to align the
type declaration on 32 bytes. It wastes space, but works. With this solution,
you should remove all the per-variable alignment attributes.

Now what I'm discussing with David Miller is if creating a

  __long_packed_aligned

and using it for *both* type and variable alignment would be more palatable (it
also works, and is more compact).

David proposed a solution with an array of pointers (extra indirection) which I
don't really like for 3 reasons I exposed in my reply to him.

So it's not that the solution is not in our grasp yet, it's more that we have to
choose the right one.

Thanks,

Mathieu

From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date: Wed, 19 Jan 2011 13:20:53 -0500

> Now what I'm discussing with David Miller is if creating a
> 
>   __long_packed_aligned
> 
> and using it for *both* type and variable alignment would be more palatable (it
> also works, and is more compact).

As I mentioned in another reply, we should not be using packed.

Packed has other implications, which makes it use byte-at-a-time accesses
for all parts of a structure when you tag it with 'packed'.  GCC doesn't
try to be clever and see that actually such accesses are safe.

If plain "__long_aligned" works and, since you're tagging it to the structure
definition, it only specifies a minimum-alignment, then I'm fine with using
that to fix this.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

* David Miller (davem@davemloft.net) wrote:
> From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Date: Wed, 19 Jan 2011 13:20:53 -0500
> 
> > Now what I'm discussing with David Miller is if creating a
> > 
> >   __long_packed_aligned
> > 
> > and using it for *both* type and variable alignment would be more palatable (it
> > also works, and is more compact).
> 
> As I mentioned in another reply, we should not be using packed.
> 
> Packed has other implications, which makes it use byte-at-a-time accesses
> for all parts of a structure when you tag it with 'packed'.  GCC doesn't
> try to be clever and see that actually such accesses are safe.

Please see my explanation about the difference between "packed" and "packed,
aligned(4 or 8)" in the other thread. I share your concern about ugly code
generated by "packed", but my tests with a sparc32 gcc show that using both
packed and aligned attributes generate nice code.

> If plain "__long_aligned" works and, since you're tagging it to the structure
> definition, it only specifies a minimum-alignment, then I'm fine with using
> that to fix this.

I'd prefer to add the "packed" to ensure that gcc never decides for some odd
reason to use an alignment larger than the one we specify in the linker script.

Thanks,

Mathieu

From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date: Wed, 19 Jan 2011 17:15:38 -0500

> * David Miller (davem@davemloft.net) wrote:
>> If plain "__long_aligned" works and, since you're tagging it to the structure
>> definition, it only specifies a minimum-alignment, then I'm fine with using
>> that to fix this.
> 
> I'd prefer to add the "packed" to ensure that gcc never decides for some odd
> reason to use an alignment larger than the one we specify in the linker script.

Agreed.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html