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

[Roland Dreier]

 > +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:

	spin_lock_irqsave(&group->lock, flags);
	list_add(&member->list, &group->pending_list);
	if (group->state == MCAST_IDLE) {
		group->state = MCAST_BUSY;
		atomic_inc(&group->refcount);
		queue_work(mcast_wq, &group->work);
	}
	spin_unlock_irqrestore(&group->lock, flags);

 > +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?


 > +	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.