[ofa-general] Re: [PATCH] osm: QoS: selecting PathRecord according to QoS policy
Yevgeny Kliteynik
kliteyn at dev.mellanox.co.il
Thu Sep 6 04:42:58 PDT 2007
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
>
More information about the general
mailing list