These two patches fix memory alignment issues on arm that I've been
slowly working on for a while now.
The totemipaddress struct patch is pretty straighforward. I did not run
into that in flatiron, since the unaligned access is occuring in a
function only present in trunk, totemip_is_mcast() ... specifically the
line:
addr = ntohl(*(int32_t*)ip_addr->addr);
aligned(4) in the struct fixes the problem also but I used 8 since I saw
alot of other structs in the project were aligned(8) and I assumed
that's better preparation for any 64 bit issues in the future.
The dispatch_buffer patch is a bigger deal obviously, so that patch was
basically made to be minimally intrusive and let me run corosync
successfully. The primary test of "working" was using testcpg with
non-multiple of 4 sized messages and a quick run of corosync with
pacemaker and watching "crm status" to show the single node online. I
haven't yet tested 2 nodes..... I'd rather end the year thinking I got
all working, so I'll wait until tomorrow to discover any endian issues
with message contents. At least it runs with no bus errors for now:)
One idea for a real patch would be to just extend this patch to actually
write nulls to the padding byte locations and let it all be done with
control_buffer->read,write index fiddling. But there must be something
more elegant.
I know arm is obviously not major target arch for corosync, meaning this
could be considered a big change for not much gain. But hopefully
there's some desire to deal with this since there could potentially be
alignment issues on another arch some day.
I did debate patching pacemaker to add padding to it's messages so they
are aligned, but it seemed more proper to deal with it in corosync.
To summarize my findings: The alignment issue occurs when a
"non-multiple of 4" sized message is written to dispatch_buffer, and the
next message is written directly after it and there is a subsequent
access to some portion of that message.
Even if all the structs and whatnot that comprise the message contents
are aligned, all that alignment work with message contents is wasted
when the message itself is stored on a odd address, breaking alignment
for direct access to probably most everything inside the message.
Thanks
Greg Walton
diff --git a/include/corosync/totem/totemip.h
b/include/corosync/totem/totemip.h
index e7f1d18..d9af0e0 100644
--- a/include/corosync/totem/totemip.h
+++ b/include/corosync/totem/totemip.h
@@ -59,10 +59,10 @@ void totemip_nosigpipe(int s);
#define TOTEM_IP_ADDRESS
struct totem_ip_address
{
- unsigned int nodeid;
- unsigned short family;
- unsigned char addr[TOTEMIP_ADDRLEN];
-} __attribute__((packed));
+ unsigned int nodeid __attribute__((aligned(8)));
+ unsigned short family __attribute__((aligned(8)));
+ unsigned char addr[TOTEMIP_ADDRLEN] __attribute__((aligned(8)));
+} __attribute__((aligned(8)));
extern int totemip_equal(const struct totem_ip_address *addr1,
diff --git a/exec/coroipcs.c b/exec/coroipcs.c
index 0ca22d3..d53e4fb 100644
--- a/exec/coroipcs.c
+++ b/exec/coroipcs.c
@@ -1236,11 +1236,28 @@ static int shared_mem_dispatch_bytes_left (const
struct conn_info *conn_info)
static void memcpy_dwrap (struct conn_info *conn_info, void *msg,
unsigned int len)
{
unsigned int write_idx;
+ unsigned int padding;
write_idx = conn_info->control_buffer->write;
+ /*
+ * Calculate padding that would be needed to make the message size a
+ * multiple of 8 bytes
+ */
+ padding = 8 - (len % 8);
+ if ( padding == 8 ) {
+ padding=0;
+ }
+
+ /*
+ * Write the message normally, and just pretend we wrote the extra
padding bytes
+ * by increasing control_buffer->write by the padding amount
+ *
+ * FIXME: Padding byte memory locations should be zeroed out in a real
patch, but
+ * but for this proof of concept the aim is to be minimally
intrusive.
+ */
memcpy (&conn_info->dispatch_buffer[write_idx], msg, len);
- conn_info->control_buffer->write = (write_idx + len) %
conn_info->dispatch_size;
+ conn_info->control_buffer->write = (write_idx + len + padding) %
conn_info->dispatch_size;
}
static void msg_send (void *conn, const struct iovec *iov, unsigned int
iov_len,
diff --git a/lib/coroipcc.c b/lib/coroipcc.c
index bbeb95f..ab24b71 100644
--- a/lib/coroipcc.c
+++ b/lib/coroipcc.c
@@ -910,6 +910,7 @@ coroipcc_dispatch_put (hdb_handle_t handle)
cs_error_t res;
char *addr;
unsigned int read_idx;
+ unsigned int padding;
res = hdb_error_to_cs (hdb_handle_get_always (&ipc_hdb, handle, (void
**)&ipc_instance));
if (res != CS_OK) {
@@ -931,8 +932,22 @@ retry_ipc_sem_wait:
read_idx = ipc_instance->control_buffer->read;
header = (coroipc_response_header_t *) &addr[read_idx];
+
+ /*
+ * Calculate padding that would be needed to make the message size a
+ * multiple of 8 bytes
+ */
+ padding = 8 - (header->size % 8);
+ if ( padding == 8 ) {
+ padding=0;
+ }
+
+ /*
+ * coroipcs.c memcpy_dwrap() will have pretended to write padding bytes
to
+ * this message, so here we pretend to have read them.
+ */
ipc_instance->control_buffer->read =
- (read_idx + header->size) % ipc_instance->dispatch_size;
+ (read_idx + header->size + padding) %
ipc_instance->dispatch_size;
/*
* Put from dispatch get and also from this call's get
*/
_______________________________________________
Openais mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/openais
