Re: [Fwd: Re: [PATCH] libibmad: Fixes for failures when not all ports of HCA are connected]

On 20/03/2013 01:27, Weiny, Ira wrote:

quoted

-----Original Message-----
From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of brendan.doyle-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org
Sent: Thursday, March 14, 2013 9:27 PM
To: Boris Chiu; iweiny-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Pramod Gunjikar
Subject: Re: [Fwd: Re: [PATCH] libibmad: Fixes for failures when not all ports
of HCA are connected]

On 03/15/13 01:23 AM, brendan.doyle-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org wrote:

quoted

So, here is the history...

There is an oracle application used to monitor the health of a node by
calling ib_resolve_portid_str_via(node GUID).

We observed that a call to ib_resolve_portid_str_via() which specified
the use of an unconnected port to issue the query was returning -1
with errno as 0.

Again the library is not documented to set errno upon failure.  While this is unfortunate for your application I don't really want to go down this path.

As far as I can see the library is not documented at all, I can't find any man pages. So
setting errno is not breaking the interface, and I would argue that if it adds value,
which it does in this case, then what is the objection. Additionally I think if the approach
is that errno is not set unless documented in a man page, then we should have some
consistency, a quick grep of errno in libibmad reveals that errno is being set in other
parts of the library, in libibumad too, we see errno set, but again it is not documented
in the umad man pages.

quoted

quoted

The application interpreted this as the query
succeeded but no paths to the specified node were found and assumed
the node was dead. This obviously was not the case, we could not send
the query because the port was down which is very different to the
queried node being down. But the API does not provide a means to
distinguish between these two failure modes. Hence the reason for
updating errno to allow these two failure modes to be distinguished.

There are other, better, ways to tell if a port is up.  umad_get_port for example.

So the application is trying to determine if a remote node is up, I agree that the
application could have been smarter, and not tried to use a local port that is
down to query the SA. That said I think hardening the library against not so
smart applications, can only be a good thing.

quoted

quoted

Now in terms of the detail of the failure and the reason for the other
changes. The call to ib_resolve_portid_str_via() did not specify an
smid, so ib_resolve_portid_str_via() calls ib_resolve_guid_via() which
first tries to resolve the SM LID using the unconnected port:

if (!sm_id) {
                 sm_id = &sm_portid;
                 if (ib_resolve_smlid_via(sm_id, timeout, srcport) < 0)
                         return -1;
         }

The call to this succeeds (return value is not -1) and indicates that
the SM LID is 0, which is of course wrong.

Again this is not wrong.  The fact there is no SM controlling that port is a side effect of you querying a port which is down and the

So I would have to disagree, I think ib_resolve_smlid_via() returning that it
successfully resolved the SM LID and that the SM LID is 0 is wrong. It did
not resolve the SM LID, it couldn't because the port is down, so it should
return a status that indicates it did not resolve the LID, and if it can not
resolve the LID, then ib_resolve_guid_via() should fail. This is just hardening
the library, which can only be a good thing, and avoids unexpected behavior
and misleading results, remote node down , as opposed to we could not
contact the SA to determine if the remote node is down.

unfortunate fact that this library makes generous use of SMP's where it should not.

I won't disagree with that.

quoted

quoted

Even though  port 2 is not
connected, it's SMA is still operational, It receives the get
PORT_INFO SMP (sent as a result of the ib_resolve_smlid_via()) , and
returns it successfully as the value it reads from the adapters
PORT_INFO which happens to be 0. ib_resolve_smlid_via() assumes
everything is OK, it does not bother to check the value of the SM LID
returned. We then try and contact the SM at LID 0!

if ((portid->lid =
              ib_path_query_via(srcport, selfgid, portid->gid, sm_id,
buf)) < 0)
                 return -1;

And this obviously fails, and so ib_resolve_portid_str_via() returns
-1 errno 0, and the app thinks the node is down. You could argue that
we should not specify an unconnected port in the call to
ib_resolve_portid_str_via(), but the changes harden the code, and
allow us to distinguish between no path to the node or could not issue
the SA query.

I feel bad that this code has lead you down this path.  However, these calls were never intended to be used on ports which are down.  The entire umad/mad stack attempts to chose the first active port for just this reason.

Maybe so, but I don't see that as a reason not to harden the library where
we can, so that we can avoid issue like this. I can see that it adds goodness,
I can't see that the proposed changes have any harmful affects, so I'm
struggling to understand why we would not want to incorporate them.

  I believe you should look at issuing SA NodeRecord queries rather than rely on the internals of libibmad to do this functionality.

Yes, the application is being changed to do that,  so we'll avoid this
issue for *this* application, but some other application could run into
the same issue, and some other person could end up spending hours
trying to debug what has happened until eventually realizing where in
libibmad the failure actually was. So again, I see the proposed changes as
hardening the library and adding nothing but goodness, with no harmful
side affects. So again I'm strugling to understand the objection.

Rdgs

Brendan

Sorry,
Ira

quoted

quoted

Rdgs

Brendan




On 03/13/13 08:29 PM, Boris Chiu wrote:

quoted

fyi,

Boris


-------- Original Message --------
Subject: 	Re: [PATCH] libibmad: Fixes for failures when not all ports
of HCA are connected
Date: 	Wed, 13 Mar 2013 12:36:05 -0700
From: 	Ira Weiny <redacted>
To: 	Boris Chiu <redacted>
CC: 	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
References: 	[off-list ref]



Your commit message does not seem to have anything to do with the
patch.  Could you explain how returning errno from these functions
"Fixes for failures when not all ports of HCA are connected"?

Furthermore, I'm reluctant to modify errno in this library.  It is
not documented and in general is poor form.  I realize that the
interface does not currently allow for an alternative.  :-(

More comments below.

On Tue, Mar 12, 2013 at 12:37 PM, Boris Chiu[off-list ref]

wrote:

quoted

quoted

quoted

  From: Brendan Doyle[off-list ref]

  Signed-off-by: Brendan Doyle[off-list ref]
  ---
  src/resolve.c |   22 ++++++++++++++++++----
  src/rpc.c     |    1 +
  src/sa.c      |    2 ++
  3 files changed, 21 insertions(+), 4 deletions(-)

  diff --git a/src/resolve.c b/src/resolve.c  index f866bf4..ab24c79
100644
  --- a/src/resolve.c
  +++ b/src/resolve.c
  @@ -40,6 +40,7 @@
  #include<stdlib.h>
  #include<string.h>
  #include<arpa/inet.h>
  +#include<errno.h>

  #include<infiniband/umad.h>
  #include<infiniband/mad.h>
  @@ -57,10 +58,18 @@ int ib_resolve_smlid_via(ib_portid_t * sm_id,
int  timeout,

          memset(sm_id, 0, sizeof(*sm_id));

  -       if (!smp_query_via(portinfo,&self, IB_ATTR_PORT_INFO, 0, 0,
  srcport))
  +       if (!smp_query_via(portinfo,&self, IB_ATTR_PORT_INFO, 0, 0,
  srcport)) {
  +               if (!errno)
  +                       errno = EIO;
                  return -1;
  +       }

          mad_decode_field(portinfo, IB_PORT_SMLID_F,&lid);
  +       if (lid == 0) {
  +               if (!errno)
  +                       errno = EIO;
  +               return -1;
  +       }

This may not be an error.  A port which is down only requires
PortState and PortPhyState to be valid.

quoted

          mad_decode_field(portinfo, IB_PORT_SMSL_F,&sm_id->sl);

          return ib_portid_set(sm_id, lid, 0, 0);  @@ -95,21 +104,26

@@ int ib_resolve_guid_via(ib_portid_t * portid, uint64_t
  * guid,
                          ib_portid_t * sm_id, int timeout,
                          const struct ibmad_port *srcport)  {
  -       ib_portid_t sm_portid;
  +       ib_portid_t sm_portid = { 0 };
          uint8_t buf[IB_SA_DATA_SIZE] = { 0 };
          ib_portid_t self = { 0 };
          uint64_t selfguid, prefix;
          ibmad_gid_t selfgid;
          uint8_t nodeinfo[64];

  -       if (!sm_id) {
  +       if (!sm_id)
                  sm_id =&sm_portid;  +
  +       if (!sm_id->lid) {
                  if (ib_resolve_smlid_via(sm_id, timeout, srcport)<  0)
                          return -1;
          }

If you want ib_resolve_guid_via to resolve the SM for you, sm_id
should be set to NULL.  I don't see a reason to support an "invalid"
sm_id port id.

Another note is that I have converted the diags to use internal
functions which used umad* and SA queries to resolve GUID's.  This is
more appropriate in the long term.

Ira

quoted

  -       if (!smp_query_via(nodeinfo,&self, IB_ATTR_NODE_INFO, 0, 0,
  srcport))
  +       if (!smp_query_via(nodeinfo,&self, IB_ATTR_NODE_INFO, 0, 0,
  srcport)) {
  +               if (!errno)
  +                       errno = EIO;
                  return -1;
  +       }
          mad_decode_field(nodeinfo, IB_NODE_PORT_GUID_F,&selfguid);
          mad_set_field64(selfgid, 0, IB_GID_PREFIX_F,
IB_DEFAULT_SUBN_PREFIX);
          mad_set_field64(selfgid, 0, IB_GID_GUID_F, selfguid);

diff --git a/src/rpc.c b/src/rpc.c  index 6312d42..cf2b60d 100644
  --- a/src/rpc.c
  +++ b/src/rpc.c
  @@ -178,6 +178,7 @@ _do_madrpc(int port_id, void *sndbuf, void

*rcvbuf, int  agentid, int len,
                                           IB_MAD_TRID_F) != trid);

                  status = umad_status(rcvbuf);
  +               errno = status;
                  if (!status)
                          return length;  /* done */
                  if (status == ENOMEM)  diff --git a/src/sa.c
b/src/sa.c  index 352ed9f..367da2a 100644
  --- a/src/sa.c
  +++ b/src/sa.c
  @@ -38,6 +38,7 @@
  #include<stdio.h>
  #include<stdlib.h>
  #include<string.h>
  +#include<errno.h>

  #include<infiniband/mad.h>
  #include "mad_internal.h"
  @@ -56,6 +57,7 @@ uint8_t *sa_rpc_call(const struct ibmad_port
*ibmad_port,  void *rcvbuf,

          if (portid->lid<= 0) {
                  IBWARN("only lid routes are supported");
  +               errno = EIO;
                  return NULL;
          }

  --
  1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the
body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at
http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html