[Openais] Re: recent segfault

Steven Dake sdake at mvista.com
Wed Feb 2 11:09:21 PST 2005


On Wed, 2005-02-02 at 09:37, Mark Haverkamp wrote:
> On Tue, 2005-02-01 at 14:51 -0700, Steven Dake wrote:
> [...] 
> > 
> > good idea Mark.  The patch should be pretty easy to develop.  I'm
> > looking at the sort queue in use bug now.  If you want to work up a
> > patch for the continuation bit idea that would be cool.
> 
> Here is a patch.  It seem to be running OK, but its hard to cause the
> condition that this fixes.  So take a close look.  Also, I noticed that
> the re-assembly loops are essentially duplicate code.  Would you mind if
> I look at collapsing those loops into one?
> 

Mark

I'll give it a run with my patch to fix unrecovered messages during
recovery.  Please feel free to rewrite/improve the assembly code.  I'm
sure it can be improved.  Only thing to keep in mind is that a message
from a local processor is different from a remote processor.

Thanks
-steve

> Mark.
> 
> 
> ===== exec/totempg.c 1.6 vs edited =====
> --- 1.6/exec/totempg.c	2005-02-01 12:39:32 -08:00
> +++ edited/exec/totempg.c	2005-02-02 08:30:12 -08:00
> @@ -99,9 +99,21 @@
>  	short type;
>  };
>  
> +/*
> + * totempg_mcast structure
> + *
> + * header:				Identify the mcast.
> + * fragmented:			Set if this message continues into next message
> + * continuation:		Set if this message is a continuation from last message
> + * msg_count			Indicates how many packed messages are contained
> + * 						in the mcast.
> + * Also, the size of each packed message and the messages themselves are
> + * appended to the end of this structure when sent.
> + */
>  struct totempg_mcast {
>  	struct totempg_mcast_header header;
> -	short fragmented; /* This message continues into next message */
> +	unsigned char fragmented; 
> +	unsigned char continuation; 
>  	short msg_count;
>  	/*
>  	 * short msg_len[msg_count];
> @@ -114,7 +126,8 @@
>  /*
>   * Maximum packet size for totem pg messages
>   */
> -#define TOTEMPG_PACKET_SIZE (TOTEMSRP_PACKET_SIZE_MAX - sizeof (struct totempg_mcast))
> +#define TOTEMPG_PACKET_SIZE (TOTEMSRP_PACKET_SIZE_MAX - \
> +				sizeof (struct totempg_mcast))
>  
>  /*
>   * Local variables used for packing small messages
> @@ -147,9 +160,19 @@
>  struct assembly *assembly_list[16]; // MAX PROCESSORS TODO
>  int assembly_list_entries = 0;
>  
> +/*
> + * Staging buffer for packed messages.  Messages are staged in this buffer
> + * before sending.  Multiple messages may fit which cuts down on the 
> + * number of mcasts sent.  If a message doesn't completely fit, then 
> + * the mcast header has a fragment bit set that says that there are more 
> + * data to follow.  fragment_size is an index into the buffer.  It indicates
> + * the size of message data and where to place new message data.  
> + * fragment_contuation indicates whether the first packed message in 
> + * the buffer is a continuation of a previously packed fragment.
> + */
>  static unsigned char fragmentation_data[TOTEMPG_PACKET_SIZE];
> -
>  int fragment_size = 0;
> +int fragment_continuation = 0;
>  
>  static struct iovec iov_delv;
>  
> @@ -238,11 +261,14 @@
>  	assert (assembly);
>  
>  	/*
> -	 * Assemble the header into one block of data
> -	 * Assemble the packet contents into one block of data to simplify delivery
> +	 * Assemble the header into one block of data and
> +	 * assemble the packet contents into one block of data to simplify delivery
>  	 */
>  	if (iov_len == 1) {
> -		/* message originated from external processor - 1 iovec for full msg */
> +		/* 
> +		 * This message originated from external processor 
> +		 * because there is only one iovec for the full msg.
> +		 */
>  		char *data;
>  		int datasize;
>  
> @@ -263,7 +289,10 @@
>  		memcpy (&assembly->data[assembly->index], &data[datasize],
>  			iovec[0].iov_len - datasize);
>  	} else {
> -		/* message originated from local processor  - <1 iovec for full msg */
> +		/* 
> +		 * The message originated from local processor  
> +		 * becasue there is greater than one iovec for then full msg.
> +		 */
>  		h_index = 0;
>  		for (i = 0; i < 2; i++) {
>  			memcpy (&header[h_index], iovec[i].iov_base, iovec[i].iov_len);
> @@ -284,7 +313,6 @@
>  	}
>  
>  	if (endian_conversion_required) {
> -		mcast->fragmented = swab16 (mcast->fragmented);
>  		mcast->msg_count = swab16 (mcast->msg_count);
>  		for (i = 0; i < mcast->msg_count; i++) {
>  			msg_lens[i] = swab16 (msg_lens[i]);
> @@ -309,8 +337,19 @@
>  		iov_delv.iov_base = &assembly->data[0];
>  		iov_delv.iov_len = assembly->index + msg_lens[0];
>  		for  (i = 0; i < mcast->msg_count; i++) {
> -			assembly->index += msg_lens[i];
>  //printf ("app deliver\n");
> +			/*
> +			 * If the first packed message is a continuation
> +			 * of a previous message, but the assembly buffer
> +			 * is empty, then we need to discard it since we can't
> +			 * assemble a complete message.
> +			 */
> +			if (mcast->continuation && (assembly->index == 0)) {
> +				assembly->index += msg_lens[i];
> +				mcast->continuation = 0;
> +				continue;
> +			}
> +			assembly->index += msg_lens[i];
>  			app_deliver_fn (source_addr, &iov_delv, 1,
>  				endian_conversion_required);
>  			iov_delv.iov_base = &assembly->data[assembly->index];
> @@ -326,6 +365,17 @@
>  		iov_delv.iov_base = &assembly->data[0];
>  		iov_delv.iov_len = assembly->index + msg_lens[0];
>  		for  (i = 0; i < mcast->msg_count - 1; i++) {
> +			/*
> +			 * If the first packed message is a continuation
> +			 * of a previous message, but the assembly buffer
> +			 * is empty, then we need to discard it since we can't
> +			 * assemble a complete message.
> +			 */
> +			if (mcast->continuation && (assembly->index == 0)) {
> +				assembly->index += msg_lens[i];
> +				mcast->continuation = 0;
> +				continue;
> +			}
>  			assembly->index += msg_lens[i];
>  //printf ("app deliver\n");
>  			app_deliver_fn (source_addr, &iov_delv, 1,
> @@ -381,6 +431,14 @@
>  		return (0);
>  	}
>  	mcast.fragmented = 0;
> +
> +	/*
> +	 * Was the first message in this buffer a continuation of a
> +	 * fragmented message?
> +	 */
> +	mcast.continuation = fragment_continuation;
> +	fragment_continuation = 0;
> +
>  	mcast.msg_count = mcast_packed_msg_count;
>  
>  	iovecs[0].iov_base = &mcast;
> @@ -468,6 +526,7 @@
>  
>  	for (i = 0; i < iov_len; ) {
>  		mcast.fragmented = 0;
> +		mcast.continuation = fragment_continuation;
>  		copy_len = iovec[i].iov_len - copy_base;
>  
>  		/*
> @@ -498,11 +557,15 @@
>  
>  			/*
>  			 * if we're not on the last iovec or the iovec is too large to
> -			 * fit, then indicate a fragment.
> +			 * fit, then indicate a fragment. This also means that the next
> +			 * message will have the continuation of this one.
>  			 */
>  			if ((i < (iov_len - 1)) || 
>  					((copy_base + copy_len) < iovec[i].iov_len)) {
>  				mcast.fragmented = 1;
> +				fragment_continuation = 1;
> +			} else {
> +				fragment_continuation = 0;
>  			}
>  
>  			/*




More information about the Openais mailing list