[ofa-general] Re: [PATCH] osm: QoS: selecting PathRecord according to QoS policy
Sasha Khapyorsky
sashak at voltaire.com
Thu Sep 6 05:20:22 PDT 2007
On 14:42 Thu 06 Sep , Yevgeny Kliteynik wrote:
> 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.
I did, "my loop" is conditional for one of many cases.
Sasha
>
> >> 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