On 2/27/07, Moni Levy <[EMAIL PROTECTED]> wrote: > On 2/27/07, Roland Dreier <[EMAIL PROTECTED]> wrote: > > > I did a short code review of the ipoib code concentrating on > > > partitioning support and I mentioned that the asynchronous events > > > handler in the ipoib code does not take the port number reported in > > > the event record into consideration. The effect of that is that all of > > > the ib# devices related to that specific HCA are flushed when it seems > > > to me that only the relevant port one should be. Is that done on > > > purpose, or am I missing something ? > > > > I don't think there's any particular reason the code is that way > > except for the oversight never being corrected. But it looks trivial > > to fix, like the patch below. Does that look right to you? > > > > > p.s. I'm working on a patch that should solve another issue caused by > > > PKEY reordering & ipoib behavior and the above issue further > > > complicates things for me. > > > > Why not fix the issue first then? > > > > commit a27cbe878203076247c1b5287f5ab59ed143b560 > > Author: Roland Dreier <[EMAIL PROTECTED]> > > Date: Tue Feb 27 07:37:49 2007 -0800 > > > > IPoIB: Only handle async events for one port > > > > An asynchronous event carries the port number that the event occurred > > on, so there's no reason for an IPoIB interface to process an event > > associated with a different local HCA port. > > > > Signed-off-by: Roland Dreier <[EMAIL PROTECTED]> > > > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_verbs.c > > b/drivers/infiniband/ulp/ipoib/ipoib_verbs.c > > index 3cb551b..7f3ec20 100644 > > --- a/drivers/infiniband/ulp/ipoib/ipoib_verbs.c > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_verbs.c > > @@ -259,12 +259,13 @@ void ipoib_event(struct ib_event_handler *handler, > > struct ipoib_dev_priv *priv = > > container_of(handler, struct ipoib_dev_priv, event_handler); > > > > - if (record->event == IB_EVENT_PORT_ERR || > > - record->event == IB_EVENT_PKEY_CHANGE || > > - record->event == IB_EVENT_PORT_ACTIVE || > > - record->event == IB_EVENT_LID_CHANGE || > > - record->event == IB_EVENT_SM_CHANGE || > > - record->event == IB_EVENT_CLIENT_REREGISTER) { > > + if ((record->event == IB_EVENT_PORT_ERR || > > + record->event == IB_EVENT_PKEY_CHANGE || > > + record->event == IB_EVENT_PORT_ACTIVE || > > + record->event == IB_EVENT_LID_CHANGE || > > + record->event == IB_EVENT_SM_CHANGE || > > + record->event == IB_EVENT_CLIENT_REREGISTER) && > > + record->element.port_num == priv->port) { > > ipoib_dbg(priv, "Port state change event\n"); > > queue_work(ipoib_workqueue, &priv->flush_task); > > } > > > > That's exactly what I intended to post.
On a second thought based on the fact that on a two port HCA we'll have a 50% miss on the events being delivered, I would move the new condition to be evaluated first. I apologize if this is too much of micro optimization. What do you think ? --Moni > > --Moni > _______________________________________________ openib-general mailing list openib-general@openib.org http://openib.org/mailman/listinfo/openib-general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general