[ofa-general] [PATCH 11/10] libibmad:infiniband-diags: deprecate madrpc_set_[retries|timeout] WAS: [PATCH 1/10] libibmad: Clean up "new" interface
Sasha Khapyorsky
sashak at voltaire.com
Sat Feb 28 23:26:31 PST 2009
On 14:34 Fri 20 Feb , Ira Weiny wrote:
> On Fri, 20 Feb 2009 13:24:35 -0500
> Hal Rosenstock <hal.rosenstock at gmail.com> wrote:
>
> > On Fri, Feb 20, 2009 at 8:41 AM, Hal Rosenstock
> > <hal.rosenstock at gmail.com> wrote:
> > > On Thu, Feb 19, 2009 at 10:05 PM, Ira Weiny <weiny2 at llnl.gov> wrote:
> > >> >From 2774b4ab4608e25bdc365bca3a94c7d51ee19372 Mon Sep 17 00:00:00 2001
> > >> From: Ira Weiny <weiny2 at llnl.gov>
> > >> Date: Wed, 18 Feb 2009 16:37:36 -0800
> > >> Subject: [PATCH] libibmad: Clean up "new" interface
> > >>
> > >> type all "void *ibmad_port" and "void *srcport" with struct ibmad_port *
> > >> Create new mad_rpc_portid(struct ibmad_port *srcport) function
> > >> which mirrors madrpc_portid(void)
> > >> Mark all "old" functions with __attribute__ ((deprecated))
> > >>
> > >> Signed-off-by: Ira Weiny <weiny2 at llnl.gov>
> > >> ---
> > >> libibmad/include/infiniband/mad.h | 139 ++++++++++++++++++++++---------------
> > >> libibmad/src/gs.c | 19 +++---
> > >> libibmad/src/libibmad.map | 1 +
> > >> libibmad/src/resolve.c | 10 ++-
> > >> libibmad/src/rpc.c | 29 ++++----
> > >> libibmad/src/sa.c | 4 +-
> > >> libibmad/src/smp.c | 4 +-
> > >> 7 files changed, 118 insertions(+), 88 deletions(-)
> > >>
> > >> diff --git a/libibmad/include/infiniband/mad.h b/libibmad/include/infiniband/mad.h
> > >> index 1aaaa1b..80e38be 100644
> > >> --- a/libibmad/include/infiniband/mad.h
> > >> +++ b/libibmad/include/infiniband/mad.h
> > >> @@ -724,100 +724,125 @@ static inline int mad_is_vendor_range2(int mgmt)
> > >> }
> > >>
> > >> /* rpc.c */
> > >> -MAD_EXPORT int madrpc_portid(void);
> > >> -MAD_EXPORT int madrpc_set_retries(int retries);
> > >> -MAD_EXPORT int madrpc_set_timeout(int timeout);
> >
> > retries and timeouts could also be made per ibmad_port struct basis
> > rather than one for all clients. Those two APIs would be deprecated in
> > favor of new ones (mad_rpc_set_retries/timeout).
> >
>
> Patch below. (To be applied after the others.)
>
>
> >From d12b291041bdfe0d3bddecb7a71ee769a601fd83 Mon Sep 17 00:00:00 2001
> From: Ira Weiny <weiny2 at llnl.gov>
> Date: Fri, 20 Feb 2009 14:30:52 -0800
> Subject: [PATCH] libibmad:infiniband-diags: deprecate madrpc_set_[retries|timeout]
>
> replace with mad_rpc_set_[retries|timeout] which are per ibmad_port
> object
> Update all diags with new functions
>
> Signed-off-by: Ira Weiny <weiny2 at llnl.gov>
> ---
> infiniband-diags/src/ibaddr.c | 1 +
> infiniband-diags/src/ibdiag_common.c | 1 -
> infiniband-diags/src/ibping.c | 1 +
> infiniband-diags/src/ibportstate.c | 1 +
> infiniband-diags/src/ibroute.c | 1 +
> infiniband-diags/src/ibsendtrap.c | 1 +
> infiniband-diags/src/ibsysstat.c | 1 +
> infiniband-diags/src/ibtracert.c | 1 +
> infiniband-diags/src/perfquery.c | 1 +
> infiniband-diags/src/saquery.c | 1 +
> infiniband-diags/src/sminfo.c | 1 +
> infiniband-diags/src/smpquery.c | 1 +
> infiniband-diags/src/vendstat.c | 1 +
> libibmad/include/infiniband/mad.h | 6 ++++--
> libibmad/src/libibmad.map | 2 ++
> libibmad/src/mad_internal.h | 2 ++
> libibmad/src/rpc.c | 29 ++++++++++++++++++++---------
> 17 files changed, 40 insertions(+), 12 deletions(-)
>
> diff --git a/infiniband-diags/src/ibaddr.c b/infiniband-diags/src/ibaddr.c
> index bb22be9..e782b36 100644
> --- a/infiniband-diags/src/ibaddr.c
> +++ b/infiniband-diags/src/ibaddr.c
> @@ -142,6 +142,7 @@ int main(int argc, char **argv)
> srcport = mad_rpc_open_port(ibd_ca, ibd_ca_port, mgmt_classes, 3);
> if (!srcport)
> IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
> + mad_rpc_set_timeout(ibd_timeout, srcport);
>
> if (argc) {
> if (ib_resolve_portid_str_via(&portid, argv[0], ibd_dest_type,
> diff --git a/infiniband-diags/src/ibdiag_common.c b/infiniband-diags/src/ibdiag_common.c
> index 609df69..38d6cd3 100644
> --- a/infiniband-diags/src/ibdiag_common.c
> +++ b/infiniband-diags/src/ibdiag_common.c
> @@ -175,7 +175,6 @@ static int process_opt(int ch, char *optarg)
> break;
> case 't':
> val = strtoul(optarg, 0, 0);
> - madrpc_set_timeout(val);
> ibd_timeout = val;
> break;
> case 's':
> diff --git a/infiniband-diags/src/ibping.c b/infiniband-diags/src/ibping.c
> index 901079f..28e3a64 100644
> --- a/infiniband-diags/src/ibping.c
> +++ b/infiniband-diags/src/ibping.c
> @@ -213,6 +213,7 @@ int main(int argc, char **argv)
> srcport = mad_rpc_open_port(ibd_ca, ibd_ca_port, mgmt_classes, 3);
> if (!srcport)
> IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
> + mad_rpc_set_timeout(ibd_timeout, srcport);
>
> if (server) {
> if (mad_register_server_via(ping_class, 0, 0, oui, srcport) < 0)
> diff --git a/infiniband-diags/src/ibportstate.c b/infiniband-diags/src/ibportstate.c
> index 65c9ca1..deaad51 100644
> --- a/infiniband-diags/src/ibportstate.c
> +++ b/infiniband-diags/src/ibportstate.c
> @@ -228,6 +228,7 @@ int main(int argc, char **argv)
> srcport = mad_rpc_open_port(ibd_ca, ibd_ca_port, mgmt_classes, 3);
> if (!srcport)
> IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
> + mad_rpc_set_timeout(ibd_timeout, srcport);
>
> if (ib_resolve_portid_str_via(&portid, argv[0], ibd_dest_type,
> ibd_sm_id, srcport) < 0)
> diff --git a/infiniband-diags/src/ibroute.c b/infiniband-diags/src/ibroute.c
> index 60bfdd8..07eddc4 100644
> --- a/infiniband-diags/src/ibroute.c
> +++ b/infiniband-diags/src/ibroute.c
> @@ -410,6 +410,7 @@ int main(int argc, char **argv)
> srcport = mad_rpc_open_port(ibd_ca, ibd_ca_port, mgmt_classes, 3);
> if (!srcport)
> IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
> + mad_rpc_set_timeout(ibd_timeout, srcport);
>
> if (!argc) {
> if (ib_resolve_self_via(&portid, 0, 0, srcport) < 0)
> diff --git a/infiniband-diags/src/ibsendtrap.c b/infiniband-diags/src/ibsendtrap.c
> index 75120f0..916b537 100644
> --- a/infiniband-diags/src/ibsendtrap.c
> +++ b/infiniband-diags/src/ibsendtrap.c
> @@ -143,6 +143,7 @@ int main(int argc, char **argv)
> srcport = mad_rpc_open_port(ibd_ca, ibd_ca_port, mgmt_classes, 2);
> if (!srcport)
> IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
> + mad_rpc_set_timeout(ibd_timeout, srcport);
>
> rc = send_trap(trap_name);
> mad_rpc_close_port(srcport);
> diff --git a/infiniband-diags/src/ibsysstat.c b/infiniband-diags/src/ibsysstat.c
> index d7daa37..7e668e8 100644
> --- a/infiniband-diags/src/ibsysstat.c
> +++ b/infiniband-diags/src/ibsysstat.c
> @@ -339,6 +339,7 @@ int main(int argc, char **argv)
> srcport = mad_rpc_open_port(ibd_ca, ibd_ca_port, mgmt_classes, 3);
> if (!srcport)
> IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
> + mad_rpc_set_timeout(ibd_timeout, srcport);
>
> if (server) {
> if (mad_register_server_via(sysstat_class, 1, 0, oui, srcport) < 0)
> diff --git a/infiniband-diags/src/ibtracert.c b/infiniband-diags/src/ibtracert.c
> index 1965aa0..87b5b17 100644
> --- a/infiniband-diags/src/ibtracert.c
> +++ b/infiniband-diags/src/ibtracert.c
> @@ -753,6 +753,7 @@ int main(int argc, char **argv)
> srcport = mad_rpc_open_port(ibd_ca, ibd_ca_port, mgmt_classes, 3);
> if (!srcport)
> IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
> + mad_rpc_set_timeout(ibd_timeout, srcport);
>
> node_name_map = open_node_name_map(node_name_map_file);
>
> diff --git a/infiniband-diags/src/perfquery.c b/infiniband-diags/src/perfquery.c
> index 2f104b8..3d89cc7 100644
> --- a/infiniband-diags/src/perfquery.c
> +++ b/infiniband-diags/src/perfquery.c
> @@ -389,6 +389,7 @@ int main(int argc, char **argv)
> srcport = mad_rpc_open_port(ibd_ca, ibd_ca_port, mgmt_classes, 4);
> if (!srcport)
> IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
> + mad_rpc_set_timeout(ibd_timeout, srcport);
>
> if (argc) {
> if (ib_resolve_portid_str_via(&portid, argv[0], ibd_dest_type,
> diff --git a/infiniband-diags/src/saquery.c b/infiniband-diags/src/saquery.c
> index e6cbe50..43eff85 100644
> --- a/infiniband-diags/src/saquery.c
> +++ b/infiniband-diags/src/saquery.c
> @@ -1323,6 +1323,7 @@ static bind_handle_t get_bind_handle(void)
> srcport = mad_rpc_open_port(ibd_ca, ibd_ca_port, mgmt_classes, 2);
> if (!srcport)
> IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
> + mad_rpc_set_timeout(ibd_timeout, srcport);
>
> ib_resolve_smlid_via(&handle.dport, ibd_timeout, srcport);
> if (!handle.dport.lid)
> diff --git a/infiniband-diags/src/sminfo.c b/infiniband-diags/src/sminfo.c
> index ebf6a47..0caa3f3 100644
> --- a/infiniband-diags/src/sminfo.c
> +++ b/infiniband-diags/src/sminfo.c
> @@ -118,6 +118,7 @@ int main(int argc, char **argv)
> srcport = mad_rpc_open_port(ibd_ca, ibd_ca_port, mgmt_classes, 3);
> if (!srcport)
> IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
> + mad_rpc_set_timeout(ibd_timeout, srcport);
>
> if (argc) {
> if (ib_resolve_portid_str_via(&portid, argv[0], ibd_dest_type,
> diff --git a/infiniband-diags/src/smpquery.c b/infiniband-diags/src/smpquery.c
> index 2ed1e65..dc6b685 100644
> --- a/infiniband-diags/src/smpquery.c
> +++ b/infiniband-diags/src/smpquery.c
> @@ -455,6 +455,7 @@ int main(int argc, char **argv)
> srcport = mad_rpc_open_port(ibd_ca, ibd_ca_port, mgmt_classes, 3);
> if (!srcport)
> IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
> + mad_rpc_set_timeout(ibd_timeout, srcport);
>
> node_name_map = open_node_name_map(node_name_map_file);
>
> diff --git a/infiniband-diags/src/vendstat.c b/infiniband-diags/src/vendstat.c
> index d001a01..1c1c08f 100644
> --- a/infiniband-diags/src/vendstat.c
> +++ b/infiniband-diags/src/vendstat.c
> @@ -157,6 +157,7 @@ int main(int argc, char **argv)
> srcport = mad_rpc_open_port(ibd_ca, ibd_ca_port, mgmt_classes, 4);
> if (!srcport)
> IBERROR("Failed to open '%s' port '%d'", ibd_ca, ibd_ca_port);
> + mad_rpc_set_timeout(ibd_timeout, srcport);
Now you need to duplicate this single call over all tools. For me it
looks like an overkill. Probably it would be simpler to just read global
ibd_timeout variable in rpc.c?
>
> if (argc) {
> if (ib_resolve_portid_str_via(&portid, argv[0], ibd_dest_type,
> diff --git a/libibmad/include/infiniband/mad.h b/libibmad/include/infiniband/mad.h
> index 5cf135e..cbd3049 100644
> --- a/libibmad/include/infiniband/mad.h
> +++ b/libibmad/include/infiniband/mad.h
> @@ -693,8 +693,6 @@ MAD_EXPORT int mad_build_pkt(void *umad, ib_rpc_t * rpc, ib_portid_t * dport,
>
> /* New interface */
> MAD_EXPORT void madrpc_show_errors(int set);
> -MAD_EXPORT int madrpc_set_retries(int retries);
> -MAD_EXPORT int madrpc_set_timeout(int timeout);
> MAD_EXPORT struct ibmad_port *mad_rpc_open_port(char *dev_name, int dev_port, int *mgmt_classes,
> int num_classes);
> MAD_EXPORT void mad_rpc_close_port(struct ibmad_port *srcport);
> @@ -703,6 +701,8 @@ MAD_EXPORT void *mad_rpc(const struct ibmad_port *srcport, ib_rpc_t * rpc, ib_po
> MAD_EXPORT void *mad_rpc_rmpp(const struct ibmad_port *srcport, ib_rpc_t * rpc, ib_portid_t * dport,
> ib_rmpp_hdr_t * rmpp, void *data);
> MAD_EXPORT int mad_rpc_portid(struct ibmad_port *srcport);
> +MAD_EXPORT int mad_rpc_set_retries(int retries, struct ibmad_port *srcport);
> +MAD_EXPORT int mad_rpc_set_timeout(int timeout_ms, struct ibmad_port *srcport);
>
> /* register.c */
> MAD_EXPORT int mad_register_port_client(int port_id, int mgmt,
> @@ -761,6 +761,8 @@ static inline int mad_is_vendor_range2(int mgmt)
> }
>
> /* rpc.c */
> +MAD_EXPORT int madrpc_set_retries(int retries) __attribute__ ((deprecated));
> +MAD_EXPORT int madrpc_set_timeout(int timeout) __attribute__ ((deprecated));
> MAD_EXPORT int madrpc_portid(void) __attribute__ ((deprecated));
> void *madrpc(ib_rpc_t * rpc, ib_portid_t * dport, void *payload, void *rcvdata)
> __attribute__ ((deprecated));
> diff --git a/libibmad/src/libibmad.map b/libibmad/src/libibmad.map
> index 0412027..f231485 100644
> --- a/libibmad/src/libibmad.map
> +++ b/libibmad/src/libibmad.map
> @@ -80,6 +80,8 @@ IBMAD_1.3 {
> madrpc_save_mad;
> madrpc_set_retries;
> madrpc_set_timeout;
> + mad_rpc_set_retries;
> + mad_rpc_set_timeout;
> madrpc_show_errors;
> ib_path_query;
> sa_call;
> diff --git a/libibmad/src/mad_internal.h b/libibmad/src/mad_internal.h
> index 9afe7a9..3991cc3 100644
> --- a/libibmad/src/mad_internal.h
> +++ b/libibmad/src/mad_internal.h
> @@ -39,6 +39,8 @@
> struct ibmad_port {
> int port_id; /* file descriptor returned by umad_open() */
> int class_agents[MAX_CLASS]; /* class2agent mapper */
> + int retries;
> + int timeout_ms;
> };
>
> #endif /* _MAD_INTERNAL_H_ */
> diff --git a/libibmad/src/rpc.c b/libibmad/src/rpc.c
> index 210f0c2..229020d 100644
> --- a/libibmad/src/rpc.c
> +++ b/libibmad/src/rpc.c
> @@ -49,7 +49,7 @@ int ibdebug;
>
> static int mad_portid = -1;
> static int iberrs;
> -
> + int timeout;
Typo?
> static int madrpc_retries = MAD_DEF_RETRIES;
> static int def_madrpc_timeout = MAD_DEF_TIMEOUT_MS;
> static void *save_mad;
> @@ -85,9 +85,17 @@ int madrpc_set_timeout(int timeout)
> return 0;
> }
>
> -int madrpc_def_timeout(void)
> +int mad_rpc_set_retries(int retries, struct ibmad_port *srcport)
> +{
> + if (retries > 0)
> + srcport->retries = retries;
> + return srcport->retries;
> +}
> +
> +int mad_rpc_set_timeout(int timeout_ms, struct ibmad_port *srcport)
> {
> - return def_madrpc_timeout;
> + srcport->timeout_ms = timeout_ms;
> + return 0;
> }
>
> int madrpc_portid(void)
> @@ -102,14 +110,14 @@ int mad_rpc_portid(struct ibmad_port *srcport)
>
> static int
> _do_madrpc(int port_id, void *sndbuf, void *rcvbuf, int agentid, int len,
> - int timeout)
> + int timeout, const struct ibmad_port *srcport)
> {
> uint32_t trid; /* only low 32 bits */
> - int retries;
> + int retries, max_retries;
> int length, status;
>
> if (!timeout)
> - timeout = def_madrpc_timeout;
> + timeout = srcport ? srcport->timeout_ms : def_madrpc_timeout;
Now you have three timeouts - one in rpc struct, another is per port and
default one. Isn't it too much?
>
> if (ibdebug > 1) {
> IBWARN(">>> sending: len %d pktsz %zu", len, umad_size() + len);
> @@ -125,7 +133,8 @@ _do_madrpc(int port_id, void *sndbuf, void *rcvbuf, int agentid, int len,
> trid =
> (uint32_t) mad_get_field64(umad_get_mad(sndbuf), 0, IB_MAD_TRID_F);
>
> - for (retries = 0; retries < madrpc_retries; retries++) {
> + max_retries = srcport ? srcport->retries : madrpc_retries;
> + for (retries = 0; retries < max_retries; retries++) {
Same with retries - it is hard for me to believe that any multithreaded
application will try to setup different retry values per port, for
different threads, "on the fly".... (rpc.c with all its limited
functionality will not be sufficient for such flexibility level anyway
:)).
Sasha
> if (retries) {
> ERRS("retry %d (timeout %d ms)", retries, timeout);
> }
> @@ -178,7 +187,7 @@ void *mad_rpc(const struct ibmad_port *port, ib_rpc_t * rpc, ib_portid_t * dport
>
> if ((len = _do_madrpc(port->port_id, sndbuf, rcvbuf,
> port->class_agents[rpc->mgtclass],
> - len, rpc->timeout)) < 0) {
> + len, rpc->timeout, port)) < 0) {
> IBWARN("_do_madrpc failed; dport (%s)", portid2str(dport));
> return 0;
> }
> @@ -217,7 +226,7 @@ void *mad_rpc_rmpp(const struct ibmad_port *port, ib_rpc_t * rpc, ib_portid_t *
>
> if ((len = _do_madrpc(port->port_id, sndbuf, rcvbuf,
> port->class_agents[rpc->mgtclass],
> - len, rpc->timeout)) < 0) {
> + len, rpc->timeout, port)) < 0) {
> IBWARN("_do_madrpc failed; dport (%s)", portid2str(dport));
> return 0;
> }
> @@ -356,6 +365,8 @@ struct ibmad_port *mad_rpc_open_port(char *dev_name, int dev_port,
> }
>
> p->port_id = port_id;
> + p->retries = MAD_DEF_RETRIES;
> + p->timeout_ms = MAD_DEF_TIMEOUT_MS;
> return p;
> }
>
> --
> 1.5.4.5
>
>
> _______________________________________________
> general mailing list
> general at lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
>
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
More information about the general
mailing list