[ofa-general] Re: [PATCH] osm: QoS: selecting PathRecord according to QoS policy
Sasha Khapyorsky
sashak at voltaire.com
Wed Sep 5 07:50:10 PDT 2007
Hi Yevgeny,
On 10:53 Wed 05 Sep , Yevgeny Kliteynik wrote:
> >> ib_net16_t dest_lid;
> >> + uint8_t i;
> >> + uint8_t vl;
> >> + ib_slvl_table_t *p_slvl_tbl = NULL;
> >> + boolean_t valid_sls[IB_MAX_NUM_VLS];
> > Use here uint16_t sl_mask instead of array - flow will be simpler.
>
> No, it won't.
> It will save three lines in the end when checking whether there is
> a valid sl that doesn't lead to VL15,
It saves loop, not just three lines :)
> but it will compilcate a bit
> rest of the related code, because I still need to read port's SL2VL
> table values one by one and mark them in the array (or bitmap) one
> by one.
Right, but since (!sl_mask) check is cheap you are able to stop PR
generation at the moment when no valid SLs exist. Just look at the
patch (against recent PR code):
diff --git a/opensm/opensm/osm_sa_path_record.c b/opensm/opensm/osm_sa_path_record.c
index edfa15f..1c6532b 100644
--- a/opensm/opensm/osm_sa_path_record.c
+++ b/opensm/opensm/osm_sa_path_record.c
@@ -253,16 +253,12 @@ __osm_pr_rcv_get_path_parms(IN osm_pr_rcv_t * const p_rcv,
uint8_t in_port_num;
ib_net16_t dest_lid;
uint8_t i;
- uint8_t vl;
ib_slvl_table_t *p_slvl_tbl = NULL;
- boolean_t valid_sls[IB_MAX_NUM_VLS];
- boolean_t sl2vl_valid_path;
- uint8_t first_valid_sl;
+ uint16_t sl_mask = 0xffff;
osm_qos_level_t *p_qos_level = NULL;
OSM_LOG_ENTER(p_rcv->p_log, __osm_pr_rcv_get_path_parms);
- memset(valid_sls, TRUE, IB_MAX_NUM_VLS);
dest_lid = cl_hton16(dest_lid_ho);
p_dest_physp = p_dest_port->p_physp;
@@ -328,12 +324,18 @@ __osm_pr_rcv_get_path_parms(IN osm_pr_rcv_t * const p_rcv,
p_slvl_tbl = osm_physp_get_slvl_tbl(p_physp, 0);
/* update valid SLs that still exist on this route */
- for (i = 0; i < IB_MAX_NUM_VLS; i++) {
- if (valid_sls[i]) {
- vl = ib_slvl_table_get(p_slvl_tbl, i);
- if (vl == IB_DROP_VL)
- valid_sls[i] = FALSE;
- }
+ for (i = 0; i < IB_MAX_NUM_VLS; i++)
+ if (sl_mask & (1 << i) &&
+ ib_slvl_table_get(p_slvl_tbl, i) == IB_DROP_VL)
+ sl_mask &= ~(1 << i);
+
+ if (!sl_mask) {
+ if (osm_log_is_active(p_rcv->p_log, OSM_LOG_DEBUG))
+ osm_log(p_rcv->p_log, OSM_LOG_DEBUG,
+ "__osm_pr_rcv_get_path_parms: "
+ "All the SLs lead to VL15 on this path\n");
+ status = IB_NOT_FOUND;
+ goto Exit;
}
}
@@ -456,12 +458,18 @@ __osm_pr_rcv_get_path_parms(IN osm_pr_rcv_t * const p_rcv,
* Check SL2VL table of the switch and update valid SLs
*/
p_slvl_tbl = osm_physp_get_slvl_tbl(p_physp, in_port_num);
- for (i = 0; i < IB_MAX_NUM_VLS; i++) {
- if (valid_sls[i]) {
- vl = ib_slvl_table_get(p_slvl_tbl, i);
- if (vl == IB_DROP_VL)
- valid_sls[i] = FALSE;
- }
+ for (i = 0; i < IB_MAX_NUM_VLS; i++)
+ if (sl_mask & (1 << i) &&
+ ib_slvl_table_get(p_slvl_tbl, i) == IB_DROP_VL)
+ sl_mask &= ~(1 << i);
+ if (!sl_mask) {
+ if (osm_log_is_active(p_rcv->p_log, OSM_LOG_DEBUG))
+ osm_log(p_rcv->p_log, OSM_LOG_DEBUG,
+ "__osm_pr_rcv_get_path_parms: "
+ "All the SLs lead to VL15 "
+ "on this path\n");
+ status = IB_NOT_FOUND;
+ goto Exit;
}
}
}
@@ -483,31 +491,6 @@ __osm_pr_rcv_get_path_parms(IN osm_pr_rcv_t * const p_rcv,
"Path min MTU = %u, min rate = %u\n",
mtu, rate);
- if (!p_rcv->p_subn->opt.no_qos) {
- /*
- * check whether there is some SL
- * that won't lead to VL15 eventually
- */
- sl2vl_valid_path = FALSE;
- for (i = 0; i < IB_MAX_NUM_VLS; i++) {
- if (valid_sls[i]) {
- sl2vl_valid_path = TRUE;
- first_valid_sl = i;
- break;
- }
- }
-
- if (!sl2vl_valid_path) {
- if (osm_log_is_active(p_rcv->p_log, OSM_LOG_DEBUG)) {
- osm_log(p_rcv->p_log, OSM_LOG_DEBUG,
- "__osm_pr_rcv_get_path_parms: "
- "All the SLs lead to VL15 on this path\n");
- }
- status = IB_NOT_FOUND;
- goto Exit;
- }
- }
-
if (!p_rcv->p_subn->opt.no_qos && p_rcv->p_subn->p_qos_policy) {
/* Get QoS Level object according to the path request */
osm_qos_policy_get_qos_level_by_pr(p_rcv->p_subn->p_qos_policy,
@@ -542,11 +525,11 @@ __osm_pr_rcv_get_path_parms(IN osm_pr_rcv_t * const p_rcv,
pkt_life = p_qos_level->pkt_life;
if (p_qos_level->sl_set) {
- if (!valid_sls[p_qos_level->sl]) {
+ sl = p_qos_level->sl;
+ if (!(sl_mask & ( 1 << sl))) {
status = IB_NOT_FOUND;
goto Exit;
}
- sl = p_qos_level->sl;
}
if (osm_log_is_active(p_rcv->p_log, OSM_LOG_DEBUG))
@@ -830,12 +813,16 @@ __osm_pr_rcv_get_path_parms(IN osm_pr_rcv_t * const p_rcv,
sl = osm_get_lash_sl(p_rcv->p_subn->p_osm,
p_src_port, p_dest_port);
} else if (!p_rcv->p_subn->opt.no_qos) {
- sl = first_valid_sl;
+ for (i = 0; i < IB_MAX_NUM_VLS; i++)
+ if (sl_mask&(1 << i)) {
+ sl = i;
+ break;
+ }
}
else
sl = OSM_DEFAULT_SL;
- if (!p_rcv->p_subn->opt.no_qos && !valid_sls[sl]) {
+ if (!p_rcv->p_subn->opt.no_qos && !(sl_mask & (1 << sl))) {
osm_log(p_rcv->p_log, OSM_LOG_ERROR,
"__osm_pr_rcv_get_path_parms: ERR 1F23: "
"Selected SL (%u) leads to VL15\n", p_prtn->sl);
> >> + /*
> >> + * set Pkey for this path record request
> >> + */
> >> +
> >> + if ((comp_mask & IB_PR_COMPMASK_RAWTRAFFIC) &&
> >> + (cl_ntoh32(p_pr->hop_flow_raw) & (1 << 31)))
> > No extra () was needed - this generates confused diff lines.
>
> No sure what you mean here by "confused diff lines".
I mean those extra lines in the patch where the only differences are
formatting or cosmetic stuff like extra braces. If you have a reason to
make such changes just send it as separate patch.
> I agree that the extra () are not *needed*, but isn't
>
> if ((comp_mask & IB_PR_COMPMASK_RAWTRAFFIC) &&
> (cl_ntoh32(p_pr->hop_flow_raw) & (1 << 31)))
>
> is more readable than
>
> if (comp_mask & IB_PR_COMPMASK_RAWTRAFFIC &&
> cl_ntoh32(p_pr->hop_flow_raw) & 1 << 31)
>
> ?
No. It requires 2+ seconds to make sure that some braces are just
"extra" ones.
BTW the second is incorrect - should be (1 << 31), those '()' were
needed.
Sasha
More information about the general
mailing list