[openib-general] Re: [RFC] [PATCH 1/2] multicast support for multiple users

[Sean Hefty]

Roland Dreier wrote:
>  > +static void queue_join(struct mcast_member *member)
>  > +{
>  > +	struct mcast_group *group = member->group;
>  > +	unsigned long flags;
>  > +
>  > +	spin_lock_irqsave(&group->lock, flags);
>  > +	list_add(&member->list, &group->pending_list);
>  > +	if (group->state == MCAST_IDLE) {
>  > +		group->state = MCAST_BUSY;
>  > +		spin_unlock_irqrestore(&group->lock, flags);
>  > +		atomic_inc(&group->refcount);
>  > +		queue_work(mcast_wq, &group->work);
>  > +	} else
>  > +		spin_unlock_irqrestore(&group->lock, flags);
>  > +}
> 
> should the atomic_inc() be outside the lock here?  It seems that
> leaves a window for things to go bad.  It might be simpler to just do
> something like:

The mcast_member already holds a reference on the group, or we couldn't have 
acquired the spin_lock.  The second reference is taken because we're queuing a 
work item that will access the group from a callback.  That said, your changes 
look cleaner, so I'll update the code.

>  > +static void adjust_membership(struct mcast_group *group, u8 join_state, int inc)
>  > +{
>  > +	int i;
>  > +
>  > +	for (i = 0; i < 3; i++, join_state >>= 1)
>  > +		if (join_state & 0x1)
>  > +			group->members[i] += inc;
>  > +}
>  > +
>  > +static u8 get_leave_state(struct mcast_group *group)
>  > +{
>  > +	u8 leave_state = 0;
>  > +	int i;
>  > +
>  > +	for (i = 0; i < 3; i++)
>  > +		if (!group->members[i])
>  > +			leave_state |= (0x1 << i);
>  > +
>  > +	return leave_state & group->rec.join_state;
>  > +}
> 
> These look rather magical -- perhaps a comment to make them understandable?

I'll add some comments.  Basically, a multicast group has 3 types of members. 
These calls track the number of members of each type.

>  > +	case MCAST_JOINING:
>  > +		list_del_init(&member->list);
>  > +		break;
> 
> Why not just list_del()?  I don't see anywhere that this list_head is
> used after this.

I think you're correct.  list_del_init() is needed elsewhere, but I think this 
can be just list_del().

Thanks for the feedback.

- Sean