public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To: David Miller <davem@davemloft.net>
Cc: rostedt@goodmis.org, richm@oldelvet.org.uk,
	609371@bugs.debian.org, ben@decadent.org.uk,
	sparclinux@vger.kernel.org, linux-kernel@vger.kernel.org,
	fweisbec@gmail.com, mingo@redhat.com
Subject: Re: Bug#609371: linux-image-2.6.37-trunk-sparc64: module scsi_mod: Unknown relocation: 36
Date: Wed, 19 Jan 2011 17:13:27 -0500	[thread overview]
Message-ID: <20110119221327.GA23544@Krystal> (raw)
In-Reply-To: <20110119.134047.232915743.davem@davemloft.net>

* David Miller (davem@davemloft.net) wrote:
> From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Date: Wed, 19 Jan 2011 10:33:26 -0500
> 
> > I'm still unsure that __long_long_aligned is needed over __long_aligned though.
> > AFAIK, the only requirement we have for, e.g. tracepoints, is to align on the
> > pointer size (sizeof(long)), so RCU pointer updates are performed atomically.
> > Aligning on the pointer size also allows the architecture to efficiently read
> > the field content. What does aligning on sizeof(long long) bring to us ? Is it
> > that you are concerned about the fact that the "aligned" type attribute, when
> > applied to a structure, is only used as a lower-bound by the compiler ? In that
> > case, we might want to consider using "packed" too:
> 
> My concern is that if there is ever a u64 or similarly "long long"
> typed member in these tracing structures, it will not be aligned
> sufficiently to avoid unaligned access traps on 32-bit systems.

Hrm, I'd like to see what kind of ill-conceived 32-bit architecture would
generate a unaligned access for a 32-bit aligned u64. Do you have examples in
mind ? By definition, the memory accesses should be at most 32-bit, no ? AFAIK,
gcc treats u64 as two distinct reads on all 32-bit architectures.

> If your suggestion defines the lowest possible alignment and GCC will
> do the right thing and "up-align" the structure if necessary, then
> fine.

Well, I must admit that my assumption is that aligning on the "long" size should
be the only alignment required, both on 32-bit and 64-bit. But I'm curious to
see if there are indeed architectures that break this assumption.

Ideally, I'd like to avoid letting gcc up-align a structure, because it is then
hard to know for sure what the alignment value of the section should be (in the
linker script, we can safely choose 32, but it's more a "safe choice" than
anything else). Moreover, I'm not convinced that gcc will choose to up-align the
structure with the exact same alignment values for both the type declaration and
the variable definition (I'm deeply distrusting gcc to do the right thing here).

> If you add "packed" it is going to screw everything up and we'll
> essentially be back to square one.
> 
> On RISC like sparc64, "packed" causes even 16-bit words to be read and
> written a byte at a time.
> 
> Never use "packed" under any circumstances unless absolutely
> unavoidable.

gcc on my sparc64 box (32-bit userland) disagrees with you here ;) Using
gcc (Debian 4.3.3-14) 4.3.3, here is a demonstration that, indeed, "packed"
generates aweful code, but that "packed, aligned(4 or 8)" generates pretty
decent code:

compiling for sparc32:

struct test {
        unsigned long a;
        unsigned long b;
};

Storing to test "a" field in a main() that returns 0, with -O0:

000104f0 <main>:
   104f0:       9d e3 bf 90     save  %sp, -112, %sp
   104f4:       03 00 00 81     sethi  %hi(0x20400), %g1
   104f8:       84 10 63 9c     or  %g1, 0x39c, %g2     ! 2079c <blah>
   104fc:       82 10 20 2a     mov  0x2a, %g1
   10500:       c2 20 80 00     st  %g1, [ %g2 ]
   10504:       82 10 20 00     clr  %g1
   10508:       b0 10 00 01     mov  %g1, %i0
   1050c:       81 e8 00 00     restore 
   10510:       81 c3 e0 08     retl 
   10514:       01 00 00 00     nop 

__attribute__((packed))

000104f0 <main>:
   104f0:       9d e3 bf 90     save  %sp, -112, %sp
   104f4:       03 00 00 81     sethi  %hi(0x20400), %g1
   104f8:       84 10 63 dc     or  %g1, 0x3dc, %g2     ! 207dc <blah>
   104fc:       c2 08 80 00     ldub  [ %g2 ], %g1
   10500:       82 08 60 00     and  %g1, 0, %g1
   10504:       c2 28 80 00     stb  %g1, [ %g2 ]
   10508:       c2 08 a0 01     ldub  [ %g2 + 1 ], %g1
   1050c:       82 08 60 00     and  %g1, 0, %g1
   10510:       c2 28 a0 01     stb  %g1, [ %g2 + 1 ]
   10514:       c2 08 a0 02     ldub  [ %g2 + 2 ], %g1
   10518:       82 08 60 00     and  %g1, 0, %g1
   1051c:       c2 28 a0 02     stb  %g1, [ %g2 + 2 ]
   10520:       c2 08 a0 03     ldub  [ %g2 + 3 ], %g1
   10524:       82 08 60 00     and  %g1, 0, %g1
   10528:       82 10 60 2a     or  %g1, 0x2a, %g1
   1052c:       c2 28 a0 03     stb  %g1, [ %g2 + 3 ]
   10530:       82 10 20 00     clr  %g1
   10534:       b0 10 00 01     mov  %g1, %i0
   10538:       81 e8 00 00     restore 
   1053c:       81 c3 e0 08     retl 
   10540:       01 00 00 00     nop 

__attribute__((packed, aligned(4)))

000104f0 <main>:
   104f0:       9d e3 bf 90     save  %sp, -112, %sp
   104f4:       03 00 00 81     sethi  %hi(0x20400), %g1
   104f8:       84 10 63 9c     or  %g1, 0x39c, %g2     ! 2079c <blah>
   104fc:       82 10 20 2a     mov  0x2a, %g1
   10500:       c2 20 80 00     st  %g1, [ %g2 ]
   10504:       82 10 20 00     clr  %g1
   10508:       b0 10 00 01     mov  %g1, %i0
   1050c:       81 e8 00 00     restore 
   10510:       81 c3 e0 08     retl 
   10514:       01 00 00 00     nop 

__attribute__((packed, aligned(8)))

000104f0 <main>:
   104f0:       9d e3 bf 90     save  %sp, -112, %sp
   104f4:       03 00 00 81     sethi  %hi(0x20400), %g1
   104f8:       84 10 63 a0     or  %g1, 0x3a0, %g2     ! 207a0 <blah>
   104fc:       82 10 20 2a     mov  0x2a, %g1
   10500:       c2 20 80 00     st  %g1, [ %g2 ]
   10504:       82 10 20 00     clr  %g1
   10508:       b0 10 00 01     mov  %g1, %i0
   1050c:       81 e8 00 00     restore 
   10510:       81 c3 e0 08     retl 
   10514:       01 00 00 00     nop 

Now about :

struct test {
        unsigned long long a;
        unsigned long long b;
};

__attribute__((packed, aligned(8)))
(and without attribute)

000104f0 <main>:
   104f0:       9d e3 bf 90     save  %sp, -112, %sp
   104f4:       03 00 00 81     sethi  %hi(0x20400), %g1
   104f8:       82 10 63 a0     or  %g1, 0x3a0, %g1     ! 207a0 <blah>
   104fc:       84 10 20 00     clr  %g2
   10500:       86 10 20 2a     mov  0x2a, %g3
   10504:       c4 38 40 00     std  %g2, [ %g1 ]
   10508:       82 10 20 00     clr  %g1
   1050c:       b0 10 00 01     mov  %g1, %i0
   10510:       81 e8 00 00     restore 
   10514:       81 c3 e0 08     retl 
   10518:       01 00 00 00     nop 
   1051c:       00 00 00 00     illtrap  0

__attribute__((packed, aligned(4)))

000104f0 <main>:
   104f0:       9d e3 bf 90     save  %sp, -112, %sp
   104f4:       03 00 00 81     sethi  %hi(0x20400), %g1
   104f8:       84 10 63 9c     or  %g1, 0x39c, %g2     ! 2079c <blah>
   104fc:       82 10 20 2a     mov  0x2a, %g1
   10500:       c2 20 a0 04     st  %g1, [ %g2 + 4 ]
   10504:       c0 20 80 00     clr  [ %g2 ]
   10508:       82 10 20 00     clr  %g1
   1050c:       b0 10 00 01     mov  %g1, %i0
   10510:       81 e8 00 00     restore 
   10514:       81 c3 e0 08     retl 
   10518:       01 00 00 00     nop 
   1051c:       00 00 00 00     illtrap  0

So the packed, aligned(__alignof__(long)) options does not look that bad.

Mathieu



-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

  parent reply	other threads:[~2011-01-19 22:13 UTC|newest]

Thread overview: 77+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20110113.155700.102679408.davem@davemloft.net>
     [not found] ` <4D302B2F.7030108@oldelvet.org.uk>
     [not found]   ` <4D3074FE.3030707@oldelvet.org.uk>
2011-01-16  5:17     ` Bug#609371: linux-image-2.6.37-trunk-sparc64: module scsi_mod: Unknown relocation: 36 David Miller
2011-01-16 14:17       ` Richard Mortimer
2011-01-16 19:39         ` David Miller
2011-01-17 14:11           ` Steven Rostedt
2011-01-17 14:37             ` Bastian Blank
2011-01-17 19:35             ` Mathieu Desnoyers
2011-01-18  6:36               ` David Miller
2011-01-18  5:34             ` David Miller
2011-01-18  6:00               ` David Miller
2011-01-18  6:08                 ` David Miller
2011-01-18 16:46                   ` Mathieu Desnoyers
2011-01-18 17:33                     ` Steven Rostedt
2011-01-18 18:16                       ` Steven Rostedt
2011-01-18 18:26                         ` Steven Rostedt
2011-01-18 20:13                           ` Mathieu Desnoyers
2011-01-18 20:22                             ` Steven Rostedt
2011-01-19  5:08                               ` Mathieu Desnoyers
2011-01-19  5:16                                 ` David Miller
2011-01-19 15:10                                   ` Mathieu Desnoyers
2011-01-19 16:14                                     ` Sam Ravnborg
2011-01-19 16:18                                       ` Mathieu Desnoyers
2011-01-19  6:32                                 ` David Miller
2011-01-19  7:20                                   ` David Miller
2011-01-19 15:33                                     ` Mathieu Desnoyers
2011-01-19 21:40                                       ` David Miller
2011-01-19 22:00                                         ` Steven Rostedt
2011-01-19 22:09                                           ` David Miller
2011-01-19 22:21                                           ` Mathieu Desnoyers
2011-01-19 22:23                                             ` David Miller
2011-01-19 22:32                                             ` Sam Ravnborg
2011-01-19 22:34                                               ` Mathieu Desnoyers
2011-01-19 22:13                                         ` Mathieu Desnoyers [this message]
2011-01-19 22:21                                           ` David Miller
2011-01-19 22:33                                             ` Mathieu Desnoyers
2011-01-20  0:41                                               ` David Miller
2011-01-21  0:04                                                 ` Mathieu Desnoyers
2011-01-21 18:06                                                   ` Richard Mortimer
2011-01-21 18:52                                                     ` Mathieu Desnoyers
2011-01-21 19:15                                                       ` Mathieu Desnoyers
2011-01-21 20:14                                                       ` Richard Mortimer
2011-01-21 20:40                                                         ` Mathieu Desnoyers
2011-01-21 22:50                                                           ` Richard Mortimer
2011-01-22 18:42                                                             ` Richard Mortimer
2011-01-22 18:53                                                               ` Mathieu Desnoyers
2011-01-19 15:46                                     ` Steven Rostedt
2011-01-19 16:15                                       ` Mathieu Desnoyers
2011-01-19 18:13                                         ` Steven Rostedt
2011-01-19 18:20                                           ` Mathieu Desnoyers
2011-01-19 21:44                                             ` David Miller
2011-01-19 22:15                                               ` Mathieu Desnoyers
2011-01-19 22:22                                                 ` David Miller
2011-01-19 15:11                                   ` Mathieu Desnoyers
2011-01-19 15:27                   ` Richard Mortimer
2011-01-17  6:07       ` David Miller
2011-01-17  9:05         ` Jesper Nilsson
2011-02-01  5:11           ` David Miller
2011-02-01 10:03             ` Jesper Nilsson
2011-01-17 10:22         ` Richard Mortimer
2011-01-17 14:15           ` Steven Rostedt
2011-01-18  6:35             ` David Miller
2011-01-18 17:30               ` Steven Rostedt
2011-01-17 19:46           ` R_SPARC_13 (Re: Bug#609371: linux-image-2.6.37-trunk-sparc64: module scsi_mod: Unknown relocation: 36) Richard Mortimer
2011-01-17 21:02             ` R_SPARC_13 David Miller
2011-01-17 23:34               ` R_SPARC_13 Richard Mortimer
2011-01-18  0:18                 ` R_SPARC_13 David Miller
2011-01-18  0:37                 ` R_SPARC_13 David Miller
2011-01-18  1:28                   ` R_SPARC_13 Richard Mortimer
2011-01-18  6:50                   ` R_SPARC_13 David Miller
2011-01-18 10:52                     ` R_SPARC_13 Richard Mortimer
2011-01-18 13:23                       ` R_SPARC_13 Richard Mortimer
2011-01-18 21:00                         ` R_SPARC_13 David Miller
2011-01-19  4:12                           ` R_SPARC_13 David Miller
2011-01-17 14:39         ` Bug#609371: linux-image-2.6.37-trunk-sparc64: module scsi_mod: Unknown relocation: 36 Bernhard R. Link
2011-01-18  5:24           ` David Miller
2011-01-18  9:26             ` Jesper Nilsson
2011-01-18  6:27           ` David Miller
2011-01-18 17:05             ` Steven Rostedt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20110119221327.GA23544@Krystal \
    --to=mathieu.desnoyers@efficios.com \
    --cc=609371@bugs.debian.org \
    --cc=ben@decadent.org.uk \
    --cc=davem@davemloft.net \
    --cc=fweisbec@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=richm@oldelvet.org.uk \
    --cc=rostedt@goodmis.org \
    --cc=sparclinux@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox