[ofa-general] Re: [opensm patch][1/2] fix qos config parsing bugs
Al Chu
chu11 at llnl.gov
Thu Nov 13 09:20:02 PST 2008
Hey Sasha,
On Thu, 2008-11-13 at 02:24 +0200, Sasha Khapyorsky wrote:
> Hi Al,
>
> On 15:57 Tue 11 Nov , Al Chu wrote:
> >
> > Sorry, I may have not explained it well. Lets say I do this in the
> > config file.
> >
> > qos_vlarb_high FOOBAR
> > # qos_ca_vlarb_high BLAH
> > qos_swe_vlarb_high XYZZY
> >
> > I currently expect qos_ca_vlarb_high to use the value of FOOBAR because
> > I commented out the field. But it uses OSM_DEFAULT_QOS_HIGH_LIMIT
> > instead. The reason is because qos_build_config() checks for NULL to
> > use default vs. non-default values.
> >
> > p = opt->vlarb_high ? opt->vlarb_high : dflt->vlarb_high;
> >
> > Under the above situation where I've commented out veral fields, opt-
> > >vlarb_high is always non-NULL b/c it was set to
> > OSM_DEFAULT_QOS_HIGH_LIMIT. Thus OSM_DEFAULT_QOS_HIGH_LIMIT is used
> > instead of FOOBAR.
> >
> > > > 2)
> > > >
> > > > In qos_build_config() we load the high_limit like this:
> > > >
> > > > cfg->vl_high_limit = (uint8_t) opt->high_limit;
> > > >
> > > > So there is no way to tell the qos_ca, qos_swe, qos_rtr, etc. high_limit
> > > > options to "go back to" the default high_limit. It just assumes that
> > > > whatever is input (or was set by default) is what you should use.
> > >
> > > Right. What is a limitation here? That an user cannot set this to
> > > "no value"? But she/he can just skip it.
> >
> > Similar to the above issue, lets say I want to do:
> >
> > qos_high_limit 8
> > # qos_ca_high_limit 15
> > # qos_swe_high_limit 15
> >
> > I want qos_ca_high_limit and qos_swe_high_limit to use whatever I set in
> > qos_high_limit. But the code doesn't allow for this.
> >
> > >
> > > > 3)
> > > >
> > > > Some fields like qos_vlarb_high are assumed to be correctly set and can
> > > > segfault opensm.
> > >
> > > qos_build_config() assumes that valid parameters are used. And we are
> > > using this this way (I hope :)) (finally it is not library API).
> >
> > I think the issue is the osm_subnet.c code did not properly check all
> > inputs, and subsequently some inputs used in qos_build_config() were
> > bad. I think
> >
> > qos_vlarb_high (null)
> >
> > was something I tried that opensm seg-faulted on.
>
> Ok. I see now.
>
> Probably it will be simpler just to generate a valid qos parameter sets
> right after parser (in verification time)?
Ahh, I see what you did. It's much cleaner this way.
> Like in your modified (and
> rebased against recent patches) patch below?
Patch looks good to me.
Thanks,
Al
>
> Sasha
>
>
> >From a973a8a1ea6c805cf07965d86731ae04510266ce Mon Sep 17 00:00:00 2001
> From: Al Chu <chu11 at llnl.gov>
> Date: Mon, 10 Nov 2008 13:41:04 -0800
> Subject: [PATCH] fix qos config parsing bugs
>
> Signed-off-by: Albert Chu <chu11 at llnl.gov>
> Signed-off-by: Sasha Khapyorsky <sashak at voltaire.com>
> ---
> opensm/include/opensm/osm_subnet.h | 12 +-
> opensm/opensm/osm_qos.c | 6 +-
> opensm/opensm/osm_subnet.c | 298 ++++++++++++++++++++---------------
> 3 files changed, 181 insertions(+), 135 deletions(-)
>
> diff --git a/opensm/include/opensm/osm_subnet.h b/opensm/include/opensm/osm_subnet.h
> index a16cbce..2bcd232 100644
> --- a/opensm/include/opensm/osm_subnet.h
> +++ b/opensm/include/opensm/osm_subnet.h
> @@ -100,7 +100,7 @@ struct osm_qos_policy;
> */
> typedef struct osm_qos_options {
> unsigned max_vls;
> - unsigned high_limit;
> + int high_limit;
> char *vlarb_high;
> char *vlarb_low;
> char *sl2vl;
> @@ -109,20 +109,20 @@ typedef struct osm_qos_options {
> * FIELDS
> *
> * max_vls
> -* The number of maximum VLs on the Subnet
> +* The number of maximum VLs on the Subnet (0 == use default)
> *
> * high_limit
> * The limit of High Priority component of VL Arbitration
> -* table (IBA 7.6.9)
> +* table (IBA 7.6.9) (-1 == use default)
> *
> * vlarb_high
> -* High priority VL Arbitration table template.
> +* High priority VL Arbitration table template. (NULL == use default)
> *
> * vlarb_low
> -* Low priority VL Arbitration table template.
> +* Low priority VL Arbitration table template. (NULL == use default)
> *
> * sl2vl
> -* SL2VL Mapping table (IBA 7.6.6) template.
> +* SL2VL Mapping table (IBA 7.6.6) template. (NULL == use default)
> *
> *********/
>
> diff --git a/opensm/opensm/osm_qos.c b/opensm/opensm/osm_qos.c
> index 1679ae0..b451c25 100644
> --- a/opensm/opensm/osm_qos.c
> +++ b/opensm/opensm/osm_qos.c
> @@ -382,7 +382,11 @@ static void qos_build_config(struct qos_config *cfg,
> memset(cfg, 0, sizeof(*cfg));
>
> cfg->max_vls = opt->max_vls > 0 ? opt->max_vls : dflt->max_vls;
> - cfg->vl_high_limit = (uint8_t) opt->high_limit;
> +
> + if (opt->high_limit >= 0)
> + cfg->vl_high_limit = (uint8_t) opt->high_limit;
> + else
> + cfg->vl_high_limit = (uint8_t) dflt->high_limit;
>
> p = opt->vlarb_high ? opt->vlarb_high : dflt->vlarb_high;
> for (i = 0; i < 2 * IB_NUM_VL_ARB_ELEMENTS_IN_BLOCK; i++) {
> diff --git a/opensm/opensm/osm_subnet.c b/opensm/opensm/osm_subnet.c
> index 8569043..1c9777e 100644
> --- a/opensm/opensm/osm_subnet.c
> +++ b/opensm/opensm/osm_subnet.c
> @@ -370,6 +370,15 @@ static void subn_set_default_qos_options(IN osm_qos_options_t * opt)
> opt->sl2vl = OSM_DEFAULT_QOS_SL2VL;
> }
>
> +static void subn_init_qos_options(IN osm_qos_options_t * opt)
> +{
> + opt->max_vls = 0;
> + opt->high_limit = -1;
> + opt->vlarb_high = NULL;
> + opt->vlarb_low = NULL;
> + opt->sl2vl = NULL;
> +}
> +
> /**********************************************************************
> **********************************************************************/
> void osm_subn_set_default_opt(IN osm_subn_opt_t * const p_opt)
> @@ -457,11 +466,11 @@ void osm_subn_set_default_opt(IN osm_subn_opt_t * const p_opt)
> p_opt->no_clients_rereg = FALSE;
> p_opt->prefix_routes_file = OSM_DEFAULT_PREFIX_ROUTES_FILE;
> p_opt->consolidate_ipv6_snm_req = FALSE;
> - subn_set_default_qos_options(&p_opt->qos_options);
> - subn_set_default_qos_options(&p_opt->qos_ca_options);
> - subn_set_default_qos_options(&p_opt->qos_sw0_options);
> - subn_set_default_qos_options(&p_opt->qos_swe_options);
> - subn_set_default_qos_options(&p_opt->qos_rtr_options);
> + subn_init_qos_options(&p_opt->qos_options);
> + subn_init_qos_options(&p_opt->qos_ca_options);
> + subn_init_qos_options(&p_opt->qos_sw0_options);
> + subn_init_qos_options(&p_opt->qos_swe_options);
> + subn_init_qos_options(&p_opt->qos_rtr_options);
> }
>
> /**********************************************************************
> @@ -526,6 +535,21 @@ opts_unpack_uint32(IN char *p_req_key,
> /**********************************************************************
> **********************************************************************/
> static void
> +opts_unpack_int32(IN char *p_req_key,
> + IN char *p_key, IN char *p_val_str, IN int32_t * p_val)
> +{
> + if (!strcmp(p_req_key, p_key)) {
> + int32_t val = strtol(p_val_str, NULL, 0);
> + if (val != *p_val) {
> + log_config_value(p_key, "%d", val);
> + *p_val = val;
> + }
> + }
> +}
> +
> +/**********************************************************************
> + **********************************************************************/
> +static void
> opts_unpack_uint16(IN char *p_req_key,
> IN char *p_key, IN char *p_val_str, IN uint16_t * p_val)
> {
> @@ -651,7 +675,7 @@ subn_parse_qos_options(IN const char *prefix,
> snprintf(name, sizeof(name), "%s_max_vls", prefix);
> opts_unpack_uint32(name, p_key, p_val_str, &opt->max_vls);
> snprintf(name, sizeof(name), "%s_high_limit", prefix);
> - opts_unpack_uint32(name, p_key, p_val_str, &opt->high_limit);
> + opts_unpack_int32(name, p_key, p_val_str, &opt->high_limit);
> snprintf(name, sizeof(name), "%s_vlarb_high", prefix);
> opts_unpack_charp(name, p_key, p_val_str, &opt->vlarb_high);
> snprintf(name, sizeof(name), "%s_vlarb_low", prefix);
> @@ -786,138 +810,142 @@ osm_parse_prefix_routes_file(IN osm_subn_t * const p_subn)
>
> /**********************************************************************
> **********************************************************************/
> -
> -static void subn_verify_max_vls(unsigned *max_vls, const char *prefix)
> +static void subn_verify_max_vls(unsigned *max_vls, const char *prefix, unsigned dflt)
> {
> - if (*max_vls > 15) {
> - log_report(" Invalid Cached Option:%s_max_vls=%u:"
> - "Using Default:%u\n",
> - prefix, *max_vls, OSM_DEFAULT_QOS_MAX_VLS);
> - *max_vls = OSM_DEFAULT_QOS_MAX_VLS;
> + if (!(*max_vls) || *max_vls > 15) {
> + log_report(" Invalid Cached Option: %s_max_vls=%u: "
> + "Using Default = %u\n", prefix, *max_vls, dflt);
> + *max_vls = dflt;
> }
> }
>
> -static void subn_verify_high_limit(unsigned *high_limit, const char *prefix)
> +static void subn_verify_high_limit(int *high_limit, const char *prefix, int dflt)
> {
> - if (*high_limit > 255) {
> - log_report(" Invalid Cached Option:%s_high_limit=%u:"
> - "Using Default:%u\n",
> - prefix, *high_limit, OSM_DEFAULT_QOS_HIGH_LIMIT);
> - *high_limit = OSM_DEFAULT_QOS_HIGH_LIMIT;
> + if (*high_limit < 0 || *high_limit > 255) {
> + log_report(" Invalid Cached Option: %s_high_limit=%d: "
> + "Using Default: %d\n", prefix, *high_limit, dflt);
> + *high_limit = dflt;
> }
> }
>
> -static void subn_verify_vlarb(char *vlarb, const char *prefix,
> - const char *suffix)
> +static void subn_verify_vlarb(char **vlarb, const char *prefix,
> + const char *suffix, char *dflt)
> {
> - if (vlarb) {
> - char *str, *tok, *end, *ptr;
> - int count = 0;
> -
> - str = strdup(vlarb);
> -
> - tok = strtok_r(str, ",\n", &ptr);
> - while (tok) {
> - char *vl_str, *weight_str;
> -
> - vl_str = tok;
> - weight_str = strchr(tok, ':');
> -
> - if (weight_str) {
> - long vl, weight;
> -
> - *weight_str = '\0';
> - weight_str++;
> -
> - vl = strtol(vl_str, &end, 0);
> -
> - if (*end)
> - log_report(" Warning: Cached Option "
> - "%s_vlarb_%s:vl=%s "
> - "improperly formatted\n",
> - prefix, suffix, vl_str);
> - else if (vl < 0 || vl > 14)
> - log_report(" Warning: Cached Option "
> - "%s_vlarb_%s:vl=%ld out "
> - "of range\n",
> - prefix, suffix, vl);
> -
> - weight = strtol(weight_str, &end, 0);
> -
> - if (*end)
> - log_report(" Warning: Cached Option "
> - "%s_vlarb_%s:weight=%s "
> - "improperly formatted\n",
> - prefix, suffix, weight_str);
> - else if (weight < 0 || weight > 255)
> - log_report(" Warning: Cached Option "
> - "%s_vlarb_%s:weight=%ld "
> - "out of range\n",
> - prefix, suffix, weight);
> - } else
> - log_report(" Warning: Cached Option "
> - "%s_vlarb_%s:vl:weight=%s "
> - "improperly formatted\n",
> - prefix, suffix, tok);
> + char *str, *tok, *end, *ptr;
> + int count = 0;
> +
> + if (*vlarb == NULL) {
> + log_report(" Invalid Cached Option: %s_vlarb_%s: "
> + "Using Default\n", prefix, suffix);
> + *vlarb = dflt;
> + return;
> + }
>
> - count++;
> - tok = strtok_r(NULL, ",\n", &ptr);
> - }
> + str = strdup(*vlarb);
> +
> + tok = strtok_r(str, ",\n", &ptr);
> + while (tok) {
> + char *vl_str, *weight_str;
>
> - if (count > 64)
> - log_report(" Warning: Cached Option %s_vlarb_%s: "
> - "> 64 listed: excess vl:weight pairs "
> - "will be dropped\n", prefix, suffix);
> + vl_str = tok;
> + weight_str = strchr(tok, ':');
>
> - free(str);
> + if (weight_str) {
> + long vl, weight;
> +
> + *weight_str = '\0';
> + weight_str++;
> +
> + vl = strtol(vl_str, &end, 0);
> +
> + if (*end)
> + log_report(" Warning: Cached Option "
> + "%s_vlarb_%s:vl=%s"
> + " improperly formatted\n",
> + prefix, suffix, vl_str);
> + else if (vl < 0 || vl > 14)
> + log_report(" Warning: Cached Option "
> + "%s_vlarb_%s:vl=%ld out of range\n",
> + prefix, suffix, vl);
> +
> + weight = strtol(weight_str, &end, 0);
> +
> + if (*end)
> + log_report(" Warning: Cached Option "
> + "%s_vlarb_%s:weight=%s "
> + "improperly formatted\n",
> + prefix, suffix, weight_str);
> + else if (weight < 0 || weight > 255)
> + log_report(" Warning: Cached Option "
> + "%s_vlarb_%s:weight=%ld "
> + "out of range\n",
> + prefix, suffix, weight);
> + } else
> + log_report(" Warning: Cached Option "
> + "%s_vlarb_%s:vl:weight=%s "
> + "improperly formatted\n",
> + prefix, suffix, tok);
> +
> + count++;
> + tok = strtok_r(NULL, ",\n", &ptr);
> }
> +
> + if (count > 64)
> + log_report(" Warning: Cached Option %s_vlarb_%s: > 64 listed:"
> + " excess vl:weight pairs will be dropped\n",
> + prefix, suffix);
> +
> + free(str);
> }
>
> -static void subn_verify_sl2vl(char *sl2vl, const char *prefix)
> +static void subn_verify_sl2vl(char **sl2vl, const char *prefix, char *dflt)
> {
> - if (sl2vl) {
> - char *str, *tok, *end, *ptr;
> - int count = 0;
> + char *str, *tok, *end, *ptr;
> + int count = 0;
> +
> + if (*sl2vl == NULL) {
> + log_report(" Invalid Cached Option: %s_sl2vl: Using Default\n",
> + prefix);
> + *sl2vl = dflt;
> + return;
> + }
>
> - str = strdup(sl2vl);
> + str = strdup(*sl2vl);
>
> - tok = strtok_r(str, ",\n", &ptr);
> - while (tok) {
> - long vl = strtol(tok, &end, 0);
> + tok = strtok_r(str, ",\n", &ptr);
> + while (tok) {
> + long vl = strtol(tok, &end, 0);
>
> - if (*end)
> - log_report(" Warning: Cached Option %s_sl2vl:"
> - "vl=%s improperly formatted\n",
> - prefix, tok);
> - else if (vl < 0 || vl > 15)
> - log_report(" Warning: Cached Option %s_sl2vl:"
> - "vl=%ld out of range\n",
> - prefix, vl);
> -
> - count++;
> - tok = strtok_r(NULL, ",\n", &ptr);
> - }
> + if (*end)
> + log_report(" Warning: Cached Option %s_sl2vl:vl=%s "
> + "improperly formatted\n", prefix, tok);
> + else if (vl < 0 || vl > 15)
> + log_report(" Warning: Cached Option %s_sl2vl:vl=%ld "
> + "out of range\n", prefix, vl);
>
> - if (count < 16)
> - log_report(" Warning: Cached Option %s_sl2vl: < 16 VLs "
> - "listed\n", prefix);
> + count++;
> + tok = strtok_r(NULL, ",\n", &ptr);
> + }
>
> - if (count > 16)
> - log_report(" Warning: Cached Option %s_sl2vl: "
> - "> 16 listed: excess VLs will be dropped\n",
> - prefix);
> + if (count < 16)
> + log_report(" Warning: Cached Option %s_sl2vl: < 16 VLs "
> + "listed\n", prefix);
>
> - free(str);
> - }
> + if (count > 16)
> + log_report(" Warning: Cached Option %s_sl2vl: > 16 listed: "
> + "excess VLs will be dropped\n", prefix);
> +
> + free(str);
> }
>
> -static void subn_verify_qos_set(osm_qos_options_t *set, const char *prefix)
> +static void subn_verify_qos_set(osm_qos_options_t *set, const char *prefix,
> + osm_qos_options_t *dflt)
> {
> - subn_verify_max_vls(&set->max_vls, prefix);
> - subn_verify_high_limit(&set->high_limit, prefix);
> - subn_verify_vlarb(set->vlarb_low, prefix, "low");
> - subn_verify_vlarb(set->vlarb_high, prefix, "high");
> - subn_verify_sl2vl(set->sl2vl, prefix);
> + subn_verify_max_vls(&set->max_vls, prefix, dflt->max_vls);
> + subn_verify_high_limit(&set->high_limit, prefix, dflt->high_limit);
> + subn_verify_vlarb(&set->vlarb_low, prefix, "low", dflt->vlarb_low);
> + subn_verify_vlarb(&set->vlarb_high, prefix, "high", dflt->vlarb_high);
> + subn_verify_sl2vl(&set->sl2vl, prefix, dflt->sl2vl);
> }
>
> static void subn_verify_conf_file(IN osm_subn_opt_t * const p_opts)
> @@ -957,11 +985,24 @@ static void subn_verify_conf_file(IN osm_subn_opt_t * const p_opts)
> }
>
> if (p_opts->qos) {
> - subn_verify_qos_set(&p_opts->qos_options, "qos");
> - subn_verify_qos_set(&p_opts->qos_ca_options, "qos_ca");
> - subn_verify_qos_set(&p_opts->qos_sw0_options, "qos_sw0");
> - subn_verify_qos_set(&p_opts->qos_swe_options, "qos_swe");
> - subn_verify_qos_set(&p_opts->qos_rtr_options, "qos_rtr");
> + osm_qos_options_t dflt;
> +
> + /* the default options in qos_options must be correct.
> + * every other one need not be, b/c those will default
> + * back to whatever is in qos_options.
> + */
> +
> + subn_set_default_qos_options(&dflt);
> +
> + subn_verify_qos_set(&p_opts->qos_options, "qos", &dflt);
> + subn_verify_qos_set(&p_opts->qos_ca_options, "qos_ca",
> + &p_opts->qos_options);
> + subn_verify_qos_set(&p_opts->qos_sw0_options, "qos_sw0",
> + &p_opts->qos_options);
> + subn_verify_qos_set(&p_opts->qos_swe_options, "qos_swe",
> + &p_opts->qos_options);
> + subn_verify_qos_set(&p_opts->qos_rtr_options, "qos_rtr",
> + &p_opts->qos_options);
> }
>
> #ifdef ENABLE_OSM_PERF_MGR
> @@ -1267,30 +1308,31 @@ int osm_subn_rescan_conf_files(IN osm_subn_t * const p_subn)
> return -1;
> }
>
> + subn_init_qos_options(&p_subn->opt.qos_options);
> + subn_init_qos_options(&p_subn->opt.qos_ca_options);
> + subn_init_qos_options(&p_subn->opt.qos_sw0_options);
> + subn_init_qos_options(&p_subn->opt.qos_swe_options);
> + subn_init_qos_options(&p_subn->opt.qos_rtr_options);
> +
> while (fgets(line, 1023, opts_file) != NULL) {
> /* get the first token */
> p_key = strtok_r(line, " \t\n", &p_last);
> if (p_key) {
> p_val = strtok_r(NULL, " \t\n", &p_last);
>
> - subn_parse_qos_options("qos",
> - p_key, p_val,
> + subn_parse_qos_options("qos", p_key, p_val,
> &p_subn->opt.qos_options);
>
> - subn_parse_qos_options("qos_ca",
> - p_key, p_val,
> + subn_parse_qos_options("qos_ca", p_key, p_val,
> &p_subn->opt.qos_ca_options);
>
> - subn_parse_qos_options("qos_sw0",
> - p_key, p_val,
> + subn_parse_qos_options("qos_sw0", p_key, p_val,
> &p_subn->opt.qos_sw0_options);
>
> - subn_parse_qos_options("qos_swe",
> - p_key, p_val,
> + subn_parse_qos_options("qos_swe", p_key, p_val,
> &p_subn->opt.qos_swe_options);
>
> - subn_parse_qos_options("qos_rtr",
> - p_key, p_val,
> + subn_parse_qos_options("qos_rtr", p_key, p_val,
> &p_subn->opt.qos_rtr_options);
>
> }
--
Albert Chu
chu11 at llnl.gov
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory
More information about the general
mailing list