These work for me on my arm arch. I did a full rebuild with make distclean,
autogen.sh etc. after i went through git101 this morning.
By work i mean builds + runs on a single node with pacemaker enabled, no
alignment errors and crmd and all the rest is able to talk to
the pacemaker process. I'll find out later if it keeps running in a real
cluster.
Thanks for the all the effort on this, I'm pretty obsessed with running
pacemaker on these arm devices :)
I appreciate your help in submitting the other little patches too.
Thanks
Greg
On 03-Jan-11 18:32, Steven Dake wrote:
> Thanks for the patches - They aren't quite right, with comments below.
> I will mail patches to the ml following this email. Would you mind
> testing them? I only have 386 and x86_64 arches to test with here.
>
> Thanks!
>
>
> On 12/31/2010 01:39 PM, Greg Walton wrote:
>> 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);
>>
> another way to address this is to use memcpy to copy ip_addr->addr.
> memcpy is designed to handle unaligned accesses. The method proposed by
> changing the packing breaks onwire compatibility and can't be merged.
>
>> 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;
>> + }
>> +
> The logic of this patch looks correct but the implementation isn't very
> tidy :) Could you try my patch instead?
>
> Also thanks for the debug work and making some initial stab at the
> patches. We really need contributions like yours to improve the code base.
>
> Regards
> -steve
>
>> + /*
>> + * 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
_______________________________________________
Openais mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/openais
