Re: [openib-general] [PATCH] for review to timeout send MADs
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,
___
openib-general mailing list
[EMAIL PROTECTED]
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
RE: [openib-general] [PATCH] for review to timeout send MADs
>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. I am using the system level queue. If we think that using our own MAD queue is better, I will do that. I was thinking more along the lines of a single workqueue for all MAD services, with one per processor, rather than a workqueue per port, however. I was planning on changing completion processing to using work queues in a separate patch. >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); > What happened is that I noticed that list_entry was being used like you saw and went to correct it the way you suggested, but backed out that change (manually) after I saw the problem in another area to avoid combining patches. I missed that I didn't revert to the original code. I've started a separate patch to fix-up this issue, and will make sure that this patch does not modify this area of the code. >This patch seems whitespace-challenged in other places too: I'll go back and update this. Thanks. If you see other places that I missed other than what you mentioned, please let me know. ___ openib-general mailing list [EMAIL PROTECTED] http://openib.org/mailman/listinfo/openib-general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Re: [openib-general] [PATCH] for review to timeout send MADs
> I am using the system level queue. If we think that using our own MAD queue > is better, I will do that. I was thinking more along the lines of a single > workqueue for all MAD services, with one per processor, rather than a > workqueue per port, however. I don't think the system keventd queue is appropriate for completion handling; I definitely think we should have a MAD layer workqueue. I think it's OK to have a single workqueue for the whole MAD layer; it's probably not scalable to systems with a huge number of HCAs, but the right fix for that is more likely to be moving to softirq/timer context anyway. - R. ___ openib-general mailing list [EMAIL PROTECTED] http://openib.org/mailman/listinfo/openib-general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Re: [openib-general] [PATCH] for review to timeout send MADs
On Thu, 2004-10-07 at 10:19, Roland Dreier wrote: > I think it's OK to have a single workqueue for the whole MAD layer; > it's probably > not scalable to systems with a huge number of HCAs, Any idea on where the cutoff for huge is or is this likely to be a matter of experience ? Is it at least 2 ? What about 4 ? Just wondering... -- Hal > but the right fix for that > is more likely to be moving to softirq/timer context anyway. ___ openib-general mailing list [EMAIL PROTECTED] http://openib.org/mailman/listinfo/openib-general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Re: [openib-general] [PATCH] for review to timeout send MADs
> Any idea on where the cutoff for huge is or is this likely to be a > matter of experience ? Is it at least 2 ? What about 4 ? Depends on the number of CPUs and the workload. The single workqueue design starts being inefficient when you start getting idle time because every workqueue thread is asleep waiting for an HCA to do something (like a modify QP firmware command). If there are other HCAs ready to do useful work they won't be able to get to it and throughput will be lower than it would be with a thread per HCA port. - R. ___ openib-general mailing list [EMAIL PROTECTED] http://openib.org/mailman/listinfo/openib-general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
