On 2011/05/12, at 17:25, Steven Dake wrote:
> On 04/18/2011 08:07 PM, Zane Bitter wrote:
>>
>> On 2011/04/18, at 16:17, Steven Dake wrote:
>>
>>> On 04/16/2011 02:11 PM, Zane Bitter wrote:
>>>> Signed-off-by: Zane Bitter <zane.bit...@gmail.com>
>>>> ---
>>>> exec/Makefile.am | 2 -
>>>> exec/totembuf.c | 212
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> exec/totembuf.h | 46 ++++++++++++
>>>> exec/totemudp.c | 11 ++-
>>>> 4 files changed, 268 insertions(+), 3 deletions(-)
>>>> create mode 100644 exec/totembuf.c
>>>> create mode 100644 exec/totembuf.h
>>>>
>>>> diff --git a/exec/Makefile.am b/exec/Makefile.am
>>>> index 39a7213..8ee1e07 100644
>>>> --- a/exec/Makefile.am
>>>> +++ b/exec/Makefile.am
>>>> @@ -37,7 +37,7 @@ INCLUDES = -I$(top_builddir)/include
>>>> -I$(top_srcdir)/include $(nss_CFLAGS) $(rd
>>>>
>>>> TOTEM_SRC = coropoll.c totemip.c totemnet.c totemudp.c \
>>>> totemudpu.c totemrrp.c totemsrp.c totemmrp.c \
>>>> - totempg.c crypto.c wthread.c tsafe.c
>>>> + totempg.c totembuf.c crypto.c wthread.c tsafe.c
>>>> if BUILD_RDMA
>>>> TOTEM_SRC += totemiba.c
>>>> endif
>>>> diff --git a/exec/totembuf.c b/exec/totembuf.c
>>>> new file mode 100644
>>>> index 0000000..2e4895d
>>>> --- /dev/null
>>>> +++ b/exec/totembuf.c
>>>> @@ -0,0 +1,212 @@
>>>> +/*
>>>> + * Copyright (c) 2011 Zane Bitter (zane.bit...@gmail.com)
>>>> + *
>>>> + * All rights reserved.
>>>> + *
>>>> + * This software licensed under BSD license, the text of which follows:
>>>> + *
>>>> + * Redistribution and use in source and binary forms, with or without
>>>> + * modification, are permitted provided that the following conditions are
>>>> met:
>>>> + *
>>>> + * - Redistributions of source code must retain the above copyright
>>>> notice,
>>>> + * this list of conditions and the following disclaimer.
>>>> + * - Redistributions in binary form must reproduce the above copyright
>>>> notice,
>>>> + * this list of conditions and the following disclaimer in the
>>>> documentation
>>>> + * and/or other materials provided with the distribution.
>>>> + * - Neither the name of the MontaVista Software, Inc. nor the names of
>>>> its
>>>> + * contributors may be used to endorse or promote products derived from
>>>> this
>>>> + * software without specific prior written permission.
>>>> + *
>>>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>>>> "AS IS"
>>>> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
>>>> THE
>>>> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
>>>> PURPOSE
>>>> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS
>>>> BE
>>>> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
>>>> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
>>>> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
>>>> BUSINESS
>>>> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
>>>> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
>>>> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
>>>> + * THE POSSIBILITY OF SUCH DAMAGE.
>>>> + */
>>>> +
>>>> +#include <config.h>
>>>> +#include <corosync/list.h>
>>>> +
>>>> +#include <stddef.h>
>>>> +#include <stdlib.h>
>>>> +#include <stdint.h>
>>>> +#include <assert.h>
>>>> +#include <pthread.h>
>>>> +
>>>> +#include "totembuf.h"
>>>> +
>>>> +
>>>> +#define MAX_FREE_LIST_LENGTH 500
>>>> +
>>>> +#define TOTEMBUF_FROM_BUFFER(BUF) ((struct totembuf *)((char *)(BUF) -
>>>> offsetof(struct totembuf, buffer)))
>>>> +
>>>> +#define TOTEMBUF_MAGIC 0xdeadbeefu
>>>> +
>>>> +
>>>> +struct totembuf_list {
>>>> + pthread_key_t key;
>>>> + size_t size;
>>>> +};
>>>> +
>>>> +struct totembuf_list_head {
>>>> + struct list_head list_free;
>>>> + const struct totembuf_list *owner;
>>>> + unsigned int length;
>>>> +};
>>>> +
>>>> +struct totembuf {
>>>> + uint32_t magic;
>>>> + struct list_head list_free;
>>>> + const struct totembuf_list *owner;
>>>> + const struct totembuf_list_head *head;
>>>> + int reference;
>>>> + char buffer[0];
>>>> +};
>>>> +
>>>> +
>>>> +static void totembuf_free_list_destroy (void *list_head);
>>>> +
>>>> +
>>>> +struct totembuf_list *totembuf_list_init (size_t buffer_size)
>>>> +{
>>>> + struct totembuf_list *free_list = malloc (sizeof (struct
>>>> totembuf_list));
>>>> + if (!free_list) {
>>>> + return NULL;
>>>> + }
>>>> +
>>>> + pthread_key_create (&free_list->key, &totembuf_free_list_destroy);
>>>> + free_list->size = buffer_size;
>>>> +
>>>> + return (free_list);
>>>> +}
>>>> +
>>>> +static struct totembuf_list_head *totembuf_free_list_get (
>>>> + const struct totembuf_list *free_list)
>>>> +{
>>>> + struct totembuf_list_head *head;
>>>> +
>>>> + /* Get the Thread-Local free list head */
>>>> + head = pthread_getspecific (free_list->key);
>>>> +
>>>> + if (!head) {
>>>> + head = malloc (sizeof (*head));
>>>> + if (head) {
>>>> + int result;
>>>> +
>>>> + list_init (&head->list_free);
>>>> + head->length = 0;
>>>> + head->owner = free_list;
>>>> + result = pthread_setspecific (free_list->key, head);
>>>> + if (result) {
>>>> + free (head);
>>>> + return (NULL);
>>>> + }
>>>> + }
>>>> + }
>>>> +
>>>> + return (head);
>>>> +}
>>>> +
>>>> +static void totembuf_free_list_destroy (void *list_head)
>>>> +{
>>>> + struct totembuf_list_head *head = list_head;
>>>> + struct list_head *list = head->list_free.next;
>>>> +
>>>> + assert (head->owner != NULL);
>>>> + pthread_setspecific (head->owner->key, NULL);
>>>> +
>>>> + while (list != &head->list_free) {
>>>> + struct totembuf *block = list_entry (list, struct totembuf,
>>>> list_free);
>>>> + list = list->next;
>>>> +
>>>> + free (block);
>>>> + }
>>>> +
>>>> + list_init (&head->list_free);
>>>> + free (head);
>>>> +}
>>>> +
>>>> +void *totembuf_alloc (const struct totembuf_list *free_list)
>>>> +{
>>>> + struct totembuf *block;
>>>> + struct totembuf_list_head *head;
>>>> +
>>>> + head = totembuf_free_list_get (free_list);
>>>> + if (list_empty (&head->list_free)) {
>>>> + block = malloc (sizeof (struct totembuf) + free_list->size);
>>>> + if (block == NULL) {
>>>> + return (NULL);
>>>> + }
>>>> +
>>>> + block->magic = TOTEMBUF_MAGIC;
>>>> +
>>>> + list_init (&block->list_free);
>>>> + block->owner = free_list;
>>>> + block->head = head;
>>>> + } else {
>>>> + block = list_entry (head->list_free.next, struct totembuf,
>>>> list_free);
>>>> + list_del (&block->list_free);
>>>
>>> This needs a list_init after the list_del. list_del does not put the
>>> list header in a "fresh" state and will result in corruption of the free
>>> list.
>>
>> Well spotted, thanks. I didn't notice anything that looked like corruption
>> when I was testing, I suspect because the list_add doesn't look at the
>> contents of the new entry when you re-add it to the free list. But I totally
>> agree that it's much better to have it in a defined state when it's floating
>> around allocated.
>>>
>
> I found it via valgrind when running in iba mode.
I think it's an interaction with the other error that we discussed in the iba
code, in which buffers allocated by totemsrp_alloc() were not marked as being
retained outside of the iba. That could definitely cause corruption of the free
list of a very similar kind to what I assume you're seeing here.
_______________________________________________
Openais mailing list
Openais@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/openais
