[openib-general] [RFC] IB/ipoib: Asynchronous events delivered without port parameter.

[Moni Levy]

On 2/27/07, Moni Levy <monil at voltaire.com> wrote:
> On 2/27/07, Roland Dreier <rdreier at cisco.com> 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 <rolandd at cisco.com>
> > 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 <rolandd at cisco.com>
> >
> > 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
>