[openib-general] [RFC] [PATCH 2/7] ib_multicast 2.6.20: add ib_multicast module to track join requests from the same port
Michael S. Tsirkin
mst at mellanox.co.il
Tue Oct 10 16:49:33 PDT 2006
Quoting r. Sean Hefty <sean.hefty at intel.com>:
> Subject: [RFC] [PATCH 2/7] ib_multicast 2.6.20: add ib_multicast module to track join requests from the same port
>
> The IB SA tracks multicast join / leave requests on a per port basis.
> In order to support multiple users of the same multicast group from
> the same port, we need to perform local reference counting on each
> of the nodes.
We do need to do something about multicast support, thanks.
Here are some comments on the patch.
First, on an API cleanliness - it seems with this we get two
ways to join an mcast group - the right refcounted one
and the wrong non-refcounted one. So, why do we keep the old one around?
It would make some sense to keep this API separate if the mcast module would
only be targeted at userspace users - kernel consumers likely
do not share mcast groups, so they could avoid the overhead.
But since this patch seems to move all kernels users to the new API to -
why does this need to be a separate module at all?
E.g. the ipoib change looks very large - but the new and old APIs look
almost exactly the same - so what's going on here?
>
> Add an ib_multicast module to perform reference counting of multicast
> join / leave requests. Modify ib_ipoib to use the multicast module.
>
> Signed-off-by: Sean Hefty <sean.hefty at intel.com>
On the ipoib change - whya re we doing it at all?
ib_ipoib does not actually need the multicast refcounting, does it?
I would be worried about doing major changes in ipoib multicast code, at this
point.
And I'm worried about the extensive use of atomic operations
this patch introduces - both performance and race-condition-wise.
Can't we stick to simple locking? Replacing completion with a single
BUSY bit looks especially scary.
For example:
>
> - mutex_lock(&mcast_mutex);
> + /* Clear the busy flag so we try again */
> + status = test_and_clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
>
> + mutex_lock(&mcast_mutex);
> spin_lock_irq(&priv->lock);
> - mcast->query = NULL;
> -
> - if (test_bit(IPOIB_MCAST_RUN, &priv->flags)) {
> - if (status == -ETIMEDOUT)
> - queue_work(ipoib_workqueue, &priv->mcast_task);
> - else
> - queue_delayed_work(ipoib_workqueue, &priv->mcast_task,
> - mcast->backoff * HZ);
> - } else
> - complete(&mcast->done);
> + if (test_bit(IPOIB_MCAST_RUN, &priv->flags))
> + queue_delayed_work(ipoib_workqueue, &priv->mcast_task,
> + mcast->backoff * HZ);
> spin_unlock_irq(&priv->lock);
> mutex_unlock(&mcast_mutex);
>
> - return;
> + return status;
> }
We used to do complete last thing on mcast object, now you are
touching the object after IPOIB_MCAST_FLAG_BUSY is cleared, apparently.
> static void ipoib_mcast_join(struct net_device *dev, struct ipoib_mcast *mcast,
> @@ -493,15 +488,14 @@ static void ipoib_mcast_join(struct net_
> rec.hop_limit = priv->broadcast->mcmember.hop_limit;
> }
>
> - init_completion(&mcast->done);
> -
> - ret = ib_sa_mcmember_rec_set(&ipoib_sa_client, priv->ca, priv->port,
> - &rec, comp_mask, mcast->backoff * 1000,
> - GFP_ATOMIC, ipoib_mcast_join_complete,
> - mcast, &mcast->query);
> -
> - if (ret < 0) {
> - ipoib_warn(priv, "ib_sa_mcmember_rec_set failed, status %d\n", ret);
> + set_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
> + mcast->mc = ib_join_multicast(priv->ca, priv->port, &rec, comp_mask,
> + GFP_KERNEL, ipoib_mcast_join_complete,
> + mcast);
> + if (IS_ERR(mcast->mc)) {
> + clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags);
> + ret = PTR_ERR(mcast->mc);
> + ipoib_warn(priv, "ib_join_multicast failed, status %d\n", ret);
>
> mcast->backoff *= 2;
> if (mcast->backoff > IPOIB_MAX_BACKOFF_SECONDS)
> @@ -513,8 +507,7 @@ static void ipoib_mcast_join(struct net_
> &priv->mcast_task,
> mcast->backoff * HZ);
> mutex_unlock(&mcast_mutex);
> - } else
> - mcast->query_id = ret;
> + }
> }
>
> void ipoib_mcast_join_task(void *dev_ptr)
> @@ -538,7 +531,7 @@ void ipoib_mcast_join_task(void *dev_ptr
> priv->local_rate = attr.active_speed *
> ib_width_enum_to_int(attr.active_width);
> } else
> - ipoib_warn(priv, "ib_query_port failed\n");
> + ipoib_warn(priv, "ib_query_port failed\n");
> }
>
> if (!priv->broadcast) {
> @@ -565,7 +558,8 @@ void ipoib_mcast_join_task(void *dev_ptr
> }
>
> if (!test_bit(IPOIB_MCAST_FLAG_ATTACHED, &priv->broadcast->flags)) {
> - ipoib_mcast_join(dev, priv->broadcast, 0);
> + if (!test_bit(IPOIB_MCAST_FLAG_BUSY, &priv->broadcast->flags))
> + ipoib_mcast_join(dev, priv->broadcast, 0);
> return;
> }
>
And here (and below) it looks like you assume no one is touching
the object if BUSY is cleared. Hmm.
> @@ -620,26 +614,9 @@ int ipoib_mcast_start_thread(struct net_
> return 0;
> }
>
> -static void wait_for_mcast_join(struct ipoib_dev_priv *priv,
> - struct ipoib_mcast *mcast)
> -{
> - spin_lock_irq(&priv->lock);
> - if (mcast && mcast->query) {
> - ib_sa_cancel_query(mcast->query_id, mcast->query);
> - mcast->query = NULL;
> - spin_unlock_irq(&priv->lock);
> - ipoib_dbg_mcast(priv, "waiting for MGID " IPOIB_GID_FMT "\n",
> - IPOIB_GID_ARG(mcast->mcmember.mgid));
> - wait_for_completion(&mcast->done);
> - }
> - else
> - spin_unlock_irq(&priv->lock);
> -}
> -
> int ipoib_mcast_stop_thread(struct net_device *dev, int flush)
> {
> struct ipoib_dev_priv *priv = netdev_priv(dev);
> - struct ipoib_mcast *mcast;
>
> ipoib_dbg_mcast(priv, "stopping multicast thread\n");
>
> @@ -655,52 +632,27 @@ int ipoib_mcast_stop_thread(struct net_d
> if (flush)
> flush_workqueue(ipoib_workqueue);
>
> - wait_for_mcast_join(priv, priv->broadcast);
> -
> - list_for_each_entry(mcast, &priv->multicast_list, list)
> - wait_for_mcast_join(priv, mcast);
> -
> return 0;
> }
Looks like callbacks could be still running after we do
ipoib_mcast_stop_thread?
>
> static int ipoib_mcast_leave(struct net_device *dev, struct ipoib_mcast *mcast)
> {
> struct ipoib_dev_priv *priv = netdev_priv(dev);
> - struct ib_sa_mcmember_rec rec = {
> - .join_state = 1
> - };
> int ret = 0;
>
> - if (!test_and_clear_bit(IPOIB_MCAST_FLAG_ATTACHED, &mcast->flags))
> - return 0;
> -
> - ipoib_dbg_mcast(priv, "leaving MGID " IPOIB_GID_FMT "\n",
> - IPOIB_GID_ARG(mcast->mcmember.mgid));
> -
> - rec.mgid = mcast->mcmember.mgid;
> - rec.port_gid = priv->local_gid;
> - rec.pkey = cpu_to_be16(priv->pkey);
> + if (test_and_clear_bit(IPOIB_MCAST_FLAG_ATTACHED, &mcast->flags)) {
> + ipoib_dbg_mcast(priv, "leaving MGID " IPOIB_GID_FMT "\n",
> + IPOIB_GID_ARG(mcast->mcmember.mgid));
>
> - /* Remove ourselves from the multicast group */
> - ret = ipoib_mcast_detach(dev, be16_to_cpu(mcast->mcmember.mlid),
> - &mcast->mcmember.mgid);
> - if (ret)
> - ipoib_warn(priv, "ipoib_mcast_detach failed (result = %d)\n", ret);
> + /* Remove ourselves from the multicast group */
> + ret = ipoib_mcast_detach(dev, be16_to_cpu(mcast->mcmember.mlid),
> + &mcast->mcmember.mgid);
> + if (ret)
> + ipoib_warn(priv, "ipoib_mcast_detach failed (result = %d)\n", ret);
> + }
>
> - /*
> - * Just make one shot at leaving and don't wait for a reply;
> - * if we fail, too bad.
> - */
> - ret = ib_sa_mcmember_rec_delete(&ipoib_sa_client, priv->ca, priv->port, &rec,
> - IB_SA_MCMEMBER_REC_MGID |
> - IB_SA_MCMEMBER_REC_PORT_GID |
> - IB_SA_MCMEMBER_REC_PKEY |
> - IB_SA_MCMEMBER_REC_JOIN_STATE,
> - 0, GFP_ATOMIC, NULL,
> - mcast, &mcast->query);
> - if (ret < 0)
> - ipoib_warn(priv, "ib_sa_mcmember_rec_delete failed "
> - "for leave (result = %d)\n", ret);
> + if (test_and_clear_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags))
> + ib_free_multicast(mcast->mc);
>
> return 0;
> }
> @@ -753,7 +705,7 @@ void ipoib_mcast_send(struct net_device
> dev_kfree_skb_any(skb);
> }
>
> - if (mcast->query)
> + if (test_bit(IPOIB_MCAST_FLAG_BUSY, &mcast->flags))
> ipoib_dbg_mcast(priv, "no address vector, "
> "but multicast join already started\n");
> else if (test_bit(IPOIB_MCAST_FLAG_SENDONLY, &mcast->flags))
> @@ -910,7 +862,6 @@ void ipoib_mcast_restart_task(void *dev_
>
> /* We have to cancel outside of the spinlock */
> list_for_each_entry_safe(mcast, tmcast, &remove_list, list) {
> - wait_for_mcast_join(priv, mcast);
> ipoib_mcast_leave(mcast->dev, mcast);
> ipoib_mcast_free(mcast);
> }
What prevents us from exiting while callbacks are in progress?
Basically same applies wherever we used to call wait_for_mcast_join.
> diff --git a/include/rdma/ib_multicast.h b/include/rdma/ib_multicast.h
> new file mode 100755
> index 0000000..423b754
> +/**
> + * ib_join_multicast - Initiates a join request to the specified multicast
> + * group.
> + * @device: Device associated with the multicast group.
> + * @port_num: Port on the specified device to associate with the multicast
> + * group.
> + * @rec: SA multicast member record specifying group attributes.
> + * @comp_mask: Component mask indicating which group attributes of %rec are
> + * valid.
> + * @gfp_mask: GFP mask for memory allocations.
> + * @callback: User callback invoked once the join operation completes.
> + * @context: User specified context stored with the ib_multicast structure.
> + *
> + * This call initiates a multicast join request with the SA for the specified
> + * multicast group. If the join operation is started successfully, it returns
> + * an ib_multicast structure that is used to track the multicast operation.
> + * Users must free this structure by calling ib_free_multicast, even if the
> + * join operation later fails. (The callback status is non-zero.)
> + */
> +struct ib_multicast *ib_join_multicast(struct ib_device *device, u8 port_num,
> + struct ib_sa_mcmember_rec *rec,
> + ib_sa_comp_mask comp_mask, gfp_t gfp_mask,
> + int (*callback)(int status,
> + struct ib_multicast
> + *multicast),
> + void *context);
> +
Is this re-introducing module unload races we had with sa all over again?
I also started to wander why do we need a new API for this at all?
Can't the sa module be fixed to refcount the mcast joins properly for us,
with minor or no API changes?
--
MST
More information about the general
mailing list