[ofa-general] Re: [PATCH RFC v2] IB/ipoib: enable IGMP for userpsace multicast IB apps

[Roland Dreier]

 > To support this inter-op for the case where the receiving party resides at
 > the IB side, there is a need to handle IGMP (reports/queries) else the local
 > IP router would not forward multicast traffic towards the IB network.
 > 
 > This patch does a lookup on the database used for multicast reference counting and
 > enhances IPoIB to ignore mulicast group which is already handled by user space, all
 > this under a per device policy flag. That is when the policy flag allows it, IPoIB
 > will not join and attach its QP to a multicast group which has an entry on the database.

I don't really follow this explanation.  OK, I see in the first
paragraph that you want to handle IGMP.  How does the second paragraph
follow?  Why does IGMP mean the kernel IPoIB interface should avoid
joining certain multicast groups?  (Which groups?)

 > 
 > +			/* ignore group which is directly joined by user space */
 > +			if (test_bit(IPOIB_FLAG_ADMIN_UMCAST_ALLOWED, &priv->flags) &&
 > +			    !ib_sa_get_mcmember_rec(priv->ca, priv->port, &mgid, &rec))

I don't follow this.  Why does ib_sa_get_mcmember_rec() returning 0
imply that userspace has already joined the multicast group?

 > +module_param_named(umcast_allowed, ipoib_umcast_allowed, int, 0444);

Not sure I understand why you added the module parameter...

 > +static DEVICE_ATTR(umcast, S_IWUSR | S_IRUGO, show_umcast, set_umcast);

The set_umcast attribute is writable by root anyway so why are there
two ways of setting this?

 > + 	if (!strcmp(buf, "1\n")) {

I don't think this is the most robust way of parsing things.  for
example it will break in a very confusing way if someone uses "echo -n" 
Could you use simple_strtoul() or something like that to handle
leading/trailing whitespace etc?

 - R.