Re: [openib-general] [RFC] IB/ipoib: Asynchronous events delivered without port parameter.
On 2/27/07, Roland Dreier <[EMAIL PROTECTED]> wrote: > > 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 ? > > That wouldn't really be correct since element.port_num isn't valid > unless we already know it's a port-related event. You're perfectly right, sorry. > > And it's not worth worrying about this since it's not remotely a hot path. Ok. --Moni > > - R. > ___ 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
Re: [openib-general] [RFC] IB/ipoib: Asynchronous events delivered without port parameter.
> 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 ? That wouldn't really be correct since element.port_num isn't valid unless we already know it's a port-related event. And it's not worth worrying about this since it's not remotely a hot path. - R. ___ 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
Re: [openib-general] [RFC] IB/ipoib: Asynchronous events delivered without port parameter.
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
Re: [openib-general] [RFC] IB/ipoib: Asynchronous events delivered without port parameter.
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.
--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
Re: [openib-general] [RFC] IB/ipoib: Asynchronous events delivered without port parameter.
> Quoting Roland Dreier <[EMAIL PROTECTED]>:
> Subject: Re: [RFC] IB/ipoib: Asynchronous events delivered without port
> parameter.
>
> >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);
> }
Looks good.
--
MST
___
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
Re: [openib-general] [RFC] IB/ipoib: Asynchronous events delivered without port parameter.
>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);
}
___
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
Re: [openib-general] [RFC] IB/ipoib: Asynchronous events delivered without port parameter.
> Quoting Moni Levy <[EMAIL PROTECTED]>: > Subject: [RFC] IB/ipoib: Asynchronous events delivered without port parameter. > > Hello, > 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 ? > > Thanks, > Moni > > 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. If true, why is this a problem? -- MST ___ 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
[openib-general] [RFC] IB/ipoib: Asynchronous events delivered without port parameter.
Hello, 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 ? Thanks, Moni 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. ___ 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
