Sasha Khapyorsky wrote:
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 :)
So now you see it yourself that it didn't save any loop :)
You had to do this loop in the end anyway to get any valid SL.
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.
Agree
> 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;
- }
- }
Here's the loop that you saved
-
- 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;
+ }
And here's the loop that you've added.
Anyway, I agree that this implementation is better - additional
check for sl_mask might save some runtime (and it's also more
"elegant" code :))
I'll integrate it in the next version of this patch
-- Yevgeny
}
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
_______________________________________________
general mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
