Thanks for looking into this... and going easy on me about the patches:)
I'll test the new ones some night this week and report back.
Thanks again,
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
