[openib-general] [PATCH] for review to timeout send MADs

Wed Oct 6 17:41:29 PDT 2004

    Sean> A workqueue is used to schedule delayed processing of MAD
    Sean> timeouts.  The scheduling delay of the timeout thread is
    Sean> adjusted when a send completes or is canceled.  If anyone
    Sean> sees an issue with my usage of the workqueue, just let me
    Sean> know.

It seems you are using the system-wide keventd queue.  This isn't
necessarily a problem per se, but it would probably be better to use a
MAD-layer private workqueue (I suggested a single-threaded workqueue
per MAD "port" earlier).  This avoids two problems.  First, keventd is
subject to arbitrary delays because it is used as a dumping ground for
any code that needs to sleep.  Second, if the MAD layer has its own
workqueue, then it can be used for completion processing as well; as
it stands it seems sort of funny to create a kthread to do some work
and run other work from a workqueue.

A few low-level comments too:

                rbuf = list_entry(&port_priv->recv_posted_mad_list[qpn],
-                                struct ib_mad_recv_buf,
-                                list);
-               rbuf = (struct ib_mad_recv_buf *)rbuf->list.next;
+                                 struct ib_mad_recv_buf,
+                                 list);
+               rbuf = (struct ib_mad_recv_buf*)rbuf->list.next;

I don't understand what's going on here; can this not be written as:

                rbuf = list_entry(port_priv->recv_posted_mad_list[qpn].next,
                                  struct ib_mad_recv_buf, list);

(By the way the cast should be written with spaces as:
        (struct ib_mad_recv_buf *) rbuf->list.next)

This patch seems whitespace-challenged in other places too:

-               recv = container_of(mad_priv_hdr, struct ib_mad_private, header);
+               recv = container_of(mad_priv_hdr,struct ib_mad_private,header);

and has extra empty lines places like here:

+               if (time_after(mad_agent_priv->timeout, mad_send_wr->timeout)) {
+
+                       mad_agent_priv->timeout = mad_send_wr->timeout;

and here:

+       if (list_empty(&mad_agent_priv->wait_list)) {
+               cancel_delayed_work(&mad_agent_priv->work);
+       } else {
+
+               mad_send_wr = list_entry(mad_agent_priv->wait_list.next,