[ofa-general] Re: [PATCH] opensm/osm_multicast.c: bug with joining/leaving mcast group

[Yevgeny Kliteynik]

Hi Sasha,

Sasha Khapyorsky wrote:
> Hi Yevgeny,
> 
> On 16:36 Mon 10 Nov     , Yevgeny Kliteynik wrote:
>> I think there's a bug in the osm_mgrp_add/remove_port functions.
>> If some mcast group member has JoinState 0x1 (full member),
>> and then new join from the same port received with JoinState
>> 0x2 (non member), OpenSM will reduce number of full members
>> of this group, which eventually might cause group deletion.
> 
> Right, isn't this how things should work? When full member updates it
> state to non member the number of full members are reduced, and then
> last full member leaves the MC group is deleted (o15-0.2-1.9).

I thought so too, but turns out that it's wrong:

o15-0.1.11: If SA supports UD multicast, then if an endport joins a
multicast group as specified in o15-0.1.10:, SA shall replace the
endport’s current MCMemberRecord:JoinState component with the logical
OR of the MCMemberRecord:JoinState component with the endport’s current
MCMemberRecord:JoinState component if the endport had joined this
multicast group before.

So the full member doesn't update its state to non-member, but rather
adds additional bit to the JoinState (the non-member).

-- Yevgeny

> Sasha
> 
>> Similar problem (only in logically opposite direction) happens
>> when port tries to partially leave mcast group.
>>
>> This patch should fix it.
>>
>> Signed-off-by: Yevgeny Kliteynik <kliteyn at dev.mellanox.co.il>
>> ---
>>  opensm/opensm/osm_multicast.c |   33 +++++++++++----------------------
>>  1 files changed, 11 insertions(+), 22 deletions(-)
>>
>> diff --git a/opensm/opensm/osm_multicast.c b/opensm/opensm/osm_multicast.c
>> index d62d585..350fd22 100644
>> --- a/opensm/opensm/osm_multicast.c
>> +++ b/opensm/opensm/osm_multicast.c
>> @@ -172,17 +172,11 @@ osm_mcm_port_t *osm_mgrp_add_port(IN osm_subn_t *subn, osm_log_t *log,
>>  		p_mgrp->last_change_id++;
>>  	}
>>
>> -	if ((join_state ^ prev_join_state) & IB_JOIN_STATE_FULL) {
>> -		if (join_state & IB_JOIN_STATE_FULL) {
>> -			if (++p_mgrp->full_members == 1) {
>> -				mgrp_send_notice(subn, log, p_mgrp, 66);
>> -				p_mgrp->to_be_deleted = 0;
>> -			}
>> -		} else if (--p_mgrp->full_members == 0) {
>> -			mgrp_send_notice(subn, log, p_mgrp, 67);
>> -			if (!p_mgrp->well_known)
>> -				p_mgrp->to_be_deleted = 1;
>> -		}
>> +	if ((join_state & IB_JOIN_STATE_FULL) &&
>> +	    !(prev_join_state & IB_JOIN_STATE_FULL) &&
>> +	    (++p_mgrp->full_members == 1)) {
>> +		mgrp_send_notice(subn, log, p_mgrp, 66);
>> +		p_mgrp->to_be_deleted = 0;
>>  	}
>>
>>  	return (p_mcm_port);
>> @@ -224,17 +218,12 @@ int osm_mgrp_remove_port(osm_subn_t *subn, osm_log_t *log, osm_mgrp_t *mgrp,
>>
>>  	/* no more full members so the group will be deleted after re-route
>>  	   but only if it is not a well known group */
>> -	if ((port_join_state ^ new_join_state) & IB_JOIN_STATE_FULL) {
>> -		if (port_join_state & IB_JOIN_STATE_FULL) {
>> -			if (--mgrp->full_members == 0) {
>> -				mgrp_send_notice(subn, log, mgrp, 67);
>> -				if (!mgrp->well_known)
>> -					mgrp->to_be_deleted = 1;
>> -			}
>> -		} else if (++mgrp->full_members == 1) {
>> -			mgrp_send_notice(subn, log, mgrp, 66);
>> -			mgrp->to_be_deleted = 0;
>> -		}
>> +	if ((port_join_state & IB_JOIN_STATE_FULL) &&
>> +	    !(new_join_state & IB_JOIN_STATE_FULL) &&
>> +	    (--mgrp->full_members == 0)) {
>> +		mgrp_send_notice(subn, log, mgrp, 67);
>> +		if (!mgrp->well_known)
>> +			mgrp->to_be_deleted = 1;
>>  	}
>>
>>  	return ret;
>> -- 
>> 1.5.1.4
>>
>