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
_______________________________________________
openib-general mailing list
[email protected]
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
