[Openais] [patch] Unnecessary `else' in assembly code of totempg

Steven Dake sdake at redhat.com
Tue Feb 17 11:51:25 PST 2009


On Tue, 2009-02-17 at 12:26 +0900, Masatake YAMATO wrote:
> > On Sun, 2009-02-15 at 02:05 +0900, Masatake YAMATO wrote:
> > > Hi,
> > > 
> > > Could you merge this patch if it is correct?
> > > 
> > 
> > The optimizer benefits from the else on some archs/compilers and it
> > doesn't cause any harm so I am inclined to leave it the way it is.
> 
> I think the 'else' causes harm.
> 
> Look at the local variable `start'.
> 
> `start' is initialized to 0.
> Then `start' is set to 1 in "if (throw_away_mode == THROW_AWAY_ACTIVE) {}" block.
> 
> The value of `start' is refereed in "if (throw_away_mode ==
> THROW_AWAY_INACTIVE) {}" block.
> 
> If the "else" is there, start is always 0 in 
> "if (throw_away_mode == THROW_AWAY_INACTIVE) {}".
> I could not understand why `start' is set to 1 in 
> "if (throw_away_mode == THROW_AWAY_ACTIVE) {}"
> 
> Masatake YAMATO
> 

The start is an artifact of bad code which has no effect because the
else is present.  If we were to remove else, we should also remove start
entirely.  Removing start entirely isn't a bad idea at all since it
needlessly complicates the code with no benefit.

The way it is supposed to work is if throw away mode is active, no
messages are delivered.  If throw away mode is inactive, all messages
are delivered.  

The reason start is in there is to throw away the first fragment of a
transition from active to inactive.  I don't think this is necessary and
would produce incorrect results. (but this is kind of how the code
worked before).

Regards
-steve

> 
>     static void totempg_deliver_fn (
> 	    unsigned int nodeid,
> 	    struct iovec *iovec,
> 	    int iov_len,
> 	    int endian_conversion_required)
>     {
> 	    struct totempg_mcast *mcast;
> 	    unsigned short *msg_lens;
> 	    int i;
> 	    struct assembly *assembly;
> 	    char header[FRAME_SIZE_MAX];
> 	    int h_index;
> 	    int a_i = 0;
> 	    int msg_count;
> 	    int continuation;
> 	    int start;
> 
> 	    /*
> 	     * Make sure that if this message is a continuation, that it
> 	     * matches the sequence number of the previous fragment.
> 	     * Also, 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. Likewise, if this message isn't a 
> 	     * continuation and the assembly buffer is empty, we have to discard
> 	     * the continued message.
> 	     */
> 	    start = 0;
> 
> 	    if (throw_away_mode == THROW_AWAY_ACTIVE) {
> 		     /* Throw away the first msg block */
> 		    if (mcast->fragmented == 0 || mcast->fragmented == 1) {
> 			    throw_away_mode = THROW_AWAY_INACTIVE;
> 
> 			    assembly->index += msg_lens[0];
> 			    iov_delv.iov_base = &assembly->data[assembly->index];
> 			    iov_delv.iov_len = msg_lens[1];
> 			    start = 1;
> 		    }
> 	    } else 
> 	    if (throw_away_mode == THROW_AWAY_INACTIVE) {
> 		    if (continuation == assembly->last_frag_num) {
> 			    assembly->last_frag_num = mcast->fragmented;
> 			    for  (i = start; i < msg_count; i++) {



More information about the Openais mailing list