GNOME Bugzilla – Bug 726456
Unaligned accesses in metatree code
Last modified: 2018-09-21 17:40:07 UTC
Forwarded from Debian-BTS: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=596054 by Michael Cree The gvfs metatree code has misaligned accesses to 64 bit data. On the Alpha architecture this causes traps to the kernel to complete the memory access (i.e. very inefficient) and pollutes the kernel logs with messages. While not a show stopper it is nevertheless annoying, particularly as the gvfs library is linked in a number of other packages (such as nautilus and evince) and they are all generating unaligned trap messages in the kernel log. I attach a patch that "fixes" the problem by providing a means to tell the compiler (provided it is gcc) that the access is misaligned at the specific points in the code, thus the compiler generates code that can access the datum without causing unaligned traps to the kernel. The patch is really just to illustrate where the problem occurs -- a better approach is probably to fix the "Journal Entry" in the code to contain the 64 bit data (the field "mtime" in the structure) on 64 bit boundaries. I noted a comment in the code that said the mtime field must be 32 bit aligned but didn't give any reasons why, so I decided not to risk modifying the alignment. patch attached by Michael Cree
Created attachment 272060 [details] [review] gvfs-fix-unaligned-64bit-accesses-patch.txt
I can confirm this problem on OpenBSD/loongson (mips64el). However, loongson is a strict-alignment platform, so this causes a SIGBUS [1], which is even more problematic than a slow unaligned access. gvfs-fix-unaligned-64bit-accesses-patch.txt works for me. [1]: (gdb) run Starting program: /usr/local/bin/thunar Program received signal SIGBUS, Bus error. meta_journal_iterate (journal=0x4d80b100, path=Variable "path" is not available. ) at metatree.c:1233 1233 mtime = GUINT64_FROM_BE (entry->mtime); (gdb) bt
+ Trace 233390
(In reply to comment #1) > Created an attachment (id=272060) [details] > gvfs-fix-unaligned-64bit-accesses-patch.txt Thanks for the patch. (In reply to comment #2) > I can confirm this problem on OpenBSD/loongson (mips64el). > However, loongson is a strict-alignment platform, so this > causes a SIGBUS [1], which is even more problematic than a > slow unaligned access. > > gvfs-fix-unaligned-64bit-accesses-patch.txt works for me. > > [1]: (gdb) run > Starting program: /usr/local/bin/thunar > > Program received signal SIGBUS, Bus error. > meta_journal_iterate (journal=0x4d80b100, path=Variable "path" is not > available. > ) at metatree.c:1233 > 1233 mtime = GUINT64_FROM_BE (entry->mtime); > (gdb) bt > Does the attached patch solve it for you?
*** Bug 658794 has been marked as a duplicate of this bug. ***
Comment on attachment 272060 [details] [review] gvfs-fix-unaligned-64bit-accesses-patch.txt I'd like to commit the patch to fix it, but it is quite magic for me. So would be good to apply it only for mentioned platforms for sure. Would it be enough to add ifdefs for: __alpha__ (#0), __mips__ (#2), __sparc__ (658794#0)?
(In reply to comment #3) > (In reply to comment #1) > > Created an attachment (id=272060) [details] [review] [details] > > gvfs-fix-unaligned-64bit-accesses-patch.txt > > Thanks for the patch. > > (In reply to comment #2) > > I can confirm this problem on OpenBSD/loongson (mips64el). > > However, loongson is a strict-alignment platform, so this > > causes a SIGBUS [1], which is even more problematic than a > > slow unaligned access. > > > > gvfs-fix-unaligned-64bit-accesses-patch.txt works for me. > > > > [1]: (gdb) run > > Starting program: /usr/local/bin/thunar > > > > Program received signal SIGBUS, Bus error. > > meta_journal_iterate (journal=0x4d80b100, path=Variable "path" is not > > available. > > ) at metatree.c:1233 > > 1233 mtime = GUINT64_FROM_BE (entry->mtime); > > (gdb) bt > > > > Does the attached patch solve it for you? Yes: (In reply to comment #2) > gvfs-fix-unaligned-64bit-accesses-patch.txt works for me.
Thanks for answer and I'm sorry I missed your note in #2...
Created attachment 274590 [details] [review] metadata: fix misaligned accesses I have rebased original patch to master and added ifdefs for mentioned platforms.
Review of attachment 274590 [details] [review]: Looks OK, although I can't guarantee that it would work! It would make sense to commit with --author='Michael Cree <mcree@orcon.net.nz>' and remove the "Author is..." line from the commit message. ::: metadata/metatree.c @@ +141,3 @@ + * that may be misaligned. This causes no loss of inefficiency on + * architectures where alignment is not an issue provided we are compiled + * with compiler optimisation turned on. Now that you've got ifdefs for the specific architectures, this last bit of the comment is probably unnecessary/irrelevant.
Created attachment 275052 [details] [review] metadata: fix misaligned accesses (In reply to comment #9) > Review of attachment 274590 [details] [review]: > > Looks OK, although I can't guarantee that it would work! Thanks for the review. > It would make sense to commit with --author='Michael Cree <mcree@orcon.net.nz>' Fixed, don't know about this option. > and remove the "Author is..." line from the commit message. > > ::: metadata/metatree.c > @@ +141,3 @@ > + * that may be misaligned. This causes no loss of inefficiency on > + * architectures where alignment is not an issue provided we are compiled > + * with compiler optimisation turned on. > > Now that you've got ifdefs for the specific architectures, this last bit of the > comment is probably unnecessary/irrelevant. You are right, it isn't necessary, so cut it out...
As the reporter of https://bugzilla.gnome.org/show_bug.cgi?id=658794 (duplicate) I have checked now the proposed patch on sparc/gentoo linux an can confirm that it works for: gvfs-1.6.7 (one hunk must be applied manually) gcc version 4.4.5 (Gentoo 4.4.5 p1.3, pie-0.4.5) sparc64 sun4u TI UltraSparc IIIi (Jalapeno) GNU/Linux Thanks to @althaser for this smart solution!
I'm the original author of the patch. There was a comment that the patch seemed to be "magic". The explanation is straightforward: In C, the keyword "packed" when used with a structure tells the compiler not to place any padding between fields in the structure that might have otherwise been inserted for efficiency or correct alignment of fields. It might also mean the start of the structure may not be correctly aligned as the structure could also occur in an array, and with "packed" there is no inter-element packing of the array to achieve alignment, but I now can't remember if that is the case or not in ANSI C. I see that I used a gcc extension to force a packed structure rather than the ANSI C "packed" keyword; I suspect the gcc construct was stronger than the ANSI C keyword and forced the compiler to think the whole structure might be misaligned. So to get the compiler to drop the assumption of correct alignment we embed the access of entry->mtime into a packed structure. Without the assumption of (64bit) alignment the compiler is therefore forced into issuing a less efficient sequence of CPU instructions, but one that can safely perform a misaligned access without kernel traps or bus errors. I suspect a better solution is to design the mtime field to always be 64 bit aligned as it is a 64 bit quantity and, thus, would achieve greater efficiency and correctness on all architectures by default without any "magic" such as that described above. I don't know the code well enough to know whether that is (easily) feasible or not.
Comment on attachment 275052 [details] [review] metadata: fix misaligned accesses commit d17e4dbfef0b9859b7d8a149b931cf685f9b4e15
(In reply to comment #12) > I'm the original author of the patch. There was a comment that the patch > seemed to be "magic". The explanation is straightforward: In C, the keyword > "packed" when used with a structure tells the compiler not to place any padding > between fields in the structure that might have otherwise been inserted for > efficiency or correct alignment of fields. It might also mean the start of the > structure may not be correctly aligned as the structure could also occur in an > array, and with "packed" there is no inter-element packing of the array to > achieve alignment, but I now can't remember if that is the case or not in ANSI > C. I see that I used a gcc extension to force a packed structure rather than > the ANSI C "packed" keyword; I suspect the gcc construct was stronger than the > ANSI C keyword and forced the compiler to think the whole structure might be > misaligned. > > So to get the compiler to drop the assumption of correct alignment we embed the > access of entry->mtime into a packed structure. Without the assumption of > (64bit) alignment the compiler is therefore forced into issuing a less > efficient sequence of CPU instructions, but one that can safely perform a > misaligned access without kernel traps or bus errors. Thanks for the patch and explanation. > I suspect a better solution is to design the mtime field to always be 64 bit > aligned as it is a 64 bit quantity and, thus, would achieve greater efficiency > and correctness on all architectures by default without any "magic" such as > that described above. I don't know the code well enough to know whether that > is (easily) feasible or not. The patch was committed, however let the bug open with this idea, how to fix it better.
Since we know the data will be 32bit aligned already, wouldn't it be better to simply declare the struct una_u64 with __attribute(__aligned(4)) to tell the compiler so, and then just use it unconditionally? The compiler will know better than the short list of #ifdefs wether that alignement needs any special treatment. And then just merge the endian conversion into the helper function: static inline guint64 read_mtime(void *src) { const struct una_u64 *ptr = (const struct una_u64 *) p; return GUINT64_FROM_BE(ptr->x); } reducing the calls to mtime = read_mtime (&entry->mtime);
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gvfs/issues/227.