Hi Sasha, Sasha Khapyorsky wrote:
Hi Yevgeny!On 15:03 Mon 20 Aug , Yevgeny Kliteynik wrote:Adding QoS fields to PathRecord. Signed-off-by: Yevgeny Kliteynik <[EMAIL PROTECTED]>Some comments are below.--- infiniband-diags/src/saquery.c | 8 +- opensm/include/iba/ib_types.h | 158 +++++++++++++++++++++++++++---- opensm/opensm/osm_helper.c | 8 +- opensm/opensm/osm_sa_multipath_record.c | 2 +- opensm/opensm/osm_sa_path_record.c | 4 +- opensm/osmtest/osmtest.c | 2 +- 6 files changed, 154 insertions(+), 28 deletions(-) diff --git a/infiniband-diags/src/saquery.c b/infiniband-diags/src/saquery.c index 522399e..f085854 100644 --- a/infiniband-diags/src/saquery.c +++ b/infiniband-diags/src/saquery.c @@ -188,7 +188,7 @@ static void print_path_record(ib_path_rec_t *p_pr) { printf("PathRecord dump:\n" - "\t\tresv0...................0x%016" PRIx64 "\n" + "\t\tservice_id..............0x%016" PRIx64 "\n" "\t\tdgid....................0x%016" PRIx64 " : " "0x%016" PRIx64 "\n" "\t\tsgid....................0x%016" PRIx64 " : " @@ -199,7 +199,7 @@ print_path_record(ib_path_rec_t *p_pr) "\t\ttclass..................0x%X\n" "\t\tnum_path_revers.........0x%X\n" "\t\tpkey....................0x%X\n" - "\t\tsl......................0x%X\n" + "\t\tqos_class_sl............0x%X\n"I think it is more useful to have separate printouts for sl and qos_class.
Agree
"\t\tmtu.....................0x%X\n" "\t\trate....................0x%X\n" "\t\tpkt_life................0x%X\n" @@ -207,7 +207,7 @@ print_path_record(ib_path_rec_t *p_pr) "\t\tresv2...................0x%X\n" "\t\tresv3...................0x%X\n" "", - *(uint64_t*)p_pr->resv0, + cl_ntoh64( p_pr->service_id ), cl_ntoh64( p_pr->dgid.unicast.prefix ), cl_ntoh64( p_pr->dgid.unicast.interface_id ), cl_ntoh64( p_pr->sgid.unicast.prefix ), @@ -218,7 +218,7 @@ print_path_record(ib_path_rec_t *p_pr) p_pr->tclass, p_pr->num_path, cl_ntoh16( p_pr->pkey ), - cl_ntoh16( p_pr->sl ), + cl_ntoh16( p_pr->qos_class_sl ), p_pr->mtu, p_pr->rate, p_pr->pkt_life, diff --git a/opensm/include/iba/ib_types.h b/opensm/include/iba/ib_types.h index 490e3fc..f96893a 100644 --- a/opensm/include/iba/ib_types.h +++ b/opensm/include/iba/ib_types.h @@ -1647,6 +1647,28 @@ static inline boolean_t OSM_API ib_class_is_rmpp(IN const uint8_t class_code) #define IB_SMINFO_STATE_MASTER 3 /**********/+/****d* IBA Base: Constants/IB_PATH_REC_SL_MASK+* NAME +* IB_PATH_REC_SL_MASK +* +* DESCRIPTION +* Mask for the sl field for path record +* +* SOURCE +*/ +#define IB_PATH_REC_SL_MASK 0x000F + +/****d* IBA Base: Constants/IB_PATH_REC_QOS_CLASS_MASK +* NAME +* IB_PATH_REC_QOS_CLASS_MASK +* +* DESCRIPTION +* Mask for the QoS class field for path record +* +* SOURCE +*/ +#define IB_PATH_REC_QOS_CLASS_MASK 0xFFF0 + /****d* IBA Base: Constants/IB_PATH_REC_SELECTOR_MASK * NAME * IB_PATH_REC_SELECTOR_MASK @@ -2286,7 +2308,7 @@ ib_gid_get_guid(IN const ib_gid_t * const p_gid) */ #include <complib/cl_packon.h> typedef struct _ib_path_rec { - uint8_t resv0[8]; + ib_net64_t service_id; ib_gid_t dgid; ib_gid_t sgid; ib_net16_t dlid; @@ -2295,7 +2317,7 @@ typedef struct _ib_path_rec { uint8_t tclass; uint8_t num_path; ib_net16_t pkey; - ib_net16_t sl; + ib_net16_t qos_class_sl; uint8_t mtu; uint8_t rate; uint8_t pkt_life; @@ -2306,8 +2328,8 @@ typedef struct _ib_path_rec { #include <complib/cl_packoff.h> /* * FIELDS -* resv0 -* Reserved bytes. +* service_id +* Service ID for QoS. * * dgid * GID of destination port. @@ -2335,11 +2357,8 @@ typedef struct _ib_path_rec { * pkey * Partition key (P_Key) to use on this path. * -* resv1 -* Reserved byte. -* -* sl -* Service level to use on this path. +* qos_class_sl +* QoS class and service level to use on this path. * * mtu * MTU and MTU selector fields to use on this path @@ -2360,6 +2379,7 @@ typedef struct _ib_path_rec { *********//* Path Record Component Masks */+#define IB_PR_COMPMASK_SERVICEID (CL_HTON64(((uint64_t)1)<<1)) #define IB_PR_COMPMASK_DGID (CL_HTON64(((uint64_t)1)<<2)) #define IB_PR_COMPMASK_SGID (CL_HTON64(((uint64_t)1)<<3)) #define IB_PR_COMPMASK_DLID (CL_HTON64(((uint64_t)1)<<4)) @@ -2372,7 +2392,7 @@ typedef struct _ib_path_rec { #define IB_PR_COMPMASK_REVERSIBLE (CL_HTON64(((uint64_t)1)<<11)) #define IB_PR_COMPMASK_NUMBPATH (CL_HTON64(((uint64_t)1)<<12)) #define IB_PR_COMPMASK_PKEY (CL_HTON64(((uint64_t)1)<<13)) -#define IB_PR_COMPMASK_RESV1 (CL_HTON64(((uint64_t)1)<<14)) +#define IB_PR_COMPMASK_QOS_CLASS (CL_HTON64(((uint64_t)1)<<14)) #define IB_PR_COMPMASK_SL (CL_HTON64(((uint64_t)1)<<15)) #define IB_PR_COMPMASK_MTUSELEC (CL_HTON64(((uint64_t)1)<<16)) #define IB_PR_COMPMASK_MTU (CL_HTON64(((uint64_t)1)<<17)) @@ -2630,6 +2650,7 @@ ib_path_rec_init_local(IN ib_path_rec_t * const p_rec, IN uint8_t num_path, IN ib_net16_t pkey, IN uint8_t sl, + IN uint16_t qos_class, IN uint8_t mtu_selector, IN uint8_t mtu, IN uint8_t rate_selector, @@ -2643,8 +2664,8 @@ ib_path_rec_init_local(IN ib_path_rec_t * const p_rec, p_rec->slid = slid; p_rec->num_path = num_path; p_rec->pkey = pkey; - /* Lower 4 bits of path rec's SL are reserved. */ - p_rec->sl = cl_ntoh16(sl); + p_rec->qos_class_sl = cl_hton16( (sl & IB_PATH_REC_SL_MASK) | + (qos_class << 4) ); p_rec->mtu = (uint8_t) ((mtu & IB_PATH_REC_BASE_MASK) | (uint8_t) (mtu_selector << 6)); p_rec->rate = (uint8_t) ((rate & IB_PATH_REC_BASE_MASK) | @@ -2656,8 +2677,8 @@ ib_path_rec_init_local(IN ib_path_rec_t * const p_rec, /* Clear global routing fields for local path records */ p_rec->hop_flow_raw = 0; p_rec->tclass = 0; + p_rec->service_id = 0;- *((uint64_t *) p_rec->resv0) = 0;*((uint32_t *) p_rec->resv2) = 0; *((uint16_t *) p_rec->resv2 + 2) = 0; } @@ -2687,6 +2708,9 @@ ib_path_rec_init_local(IN ib_path_rec_t * const p_rec, * pkey * [in] Partition key (P_Key) to use on this path. * +* qos_class +* [in] QoS class to use on this path. Lower 12-bits are valid. +* * sl * [in] Service level to use on this path. Lower 4-bits are valid. * @@ -2750,6 +2774,41 @@ ib_path_rec_num_path(IN const ib_path_rec_t * const p_rec) * ib_path_rec_t *********/+/****f* IBA Base: Types/ib_path_rec_set_sl+* NAME +* ib_path_rec_set_sl +* +* DESCRIPTION +* Set path service level. +* +* SYNOPSIS +*/ +static inline void OSM_API +ib_path_rec_set_sl( + IN ib_path_rec_t* const p_rec, + IN const uint8_t sl ) +{ + p_rec->qos_class_sl = cl_hton16( ( cl_ntoh16(p_rec->qos_class_sl) & + IB_PATH_REC_QOS_CLASS_MASK ) | + ( sl & IB_PATH_REC_SL_MASK) );You could do something like: (p_rec->qos_class_sl & CL_HTON16(IB_PATH_REC_QOS_CLASS_MASK)) | cl_hton16(sl & IB_PATH_REC_SL_MASK)
Sure, why not
CL_HTON16(IB_PATH_REC_QOS_CLASS_MASK) is calculated at compile time, so one cl_hton16() is saved.+} +/* +* PARAMETERS +* p_rec +* [in] Pointer to the path record object. +* +* sl +* [in] Service level to set. +* +* RETURN VALUES +* None +* +* NOTES +* +* SEE ALSO +* ib_path_rec_t +*********/ + /****f* IBA Base: Types/ib_path_rec_sl * NAME * ib_path_rec_sl @@ -2762,7 +2821,7 @@ ib_path_rec_num_path(IN const ib_path_rec_t * const p_rec) static inline uint8_t OSM_API ib_path_rec_sl(IN const ib_path_rec_t * const p_rec) { - return ((uint8_t) ((cl_ntoh16(p_rec->sl)) & 0xF)); + return ((uint8_t) ((cl_ntoh16(p_rec->qos_class_sl)) & IB_PATH_REC_SL_MASK)); }/*@@ -2779,6 +2838,70 @@ ib_path_rec_sl(IN const ib_path_rec_t * const p_rec) * ib_path_rec_t *********/+/****f* IBA Base: Types/ib_path_rec_set_qos_class+* NAME +* ib_path_rec_set_qos_class +* +* DESCRIPTION +* Set path QoS class. +* +* SYNOPSIS +*/ +static inline void OSM_API +ib_path_rec_set_qos_class( + IN ib_path_rec_t* const p_rec, + IN const uint16_t qos_class ) +{ + p_rec->qos_class_sl = cl_hton16( ( cl_ntoh16(p_rec->qos_class_sl) & + IB_PATH_REC_SL_MASK ) | + ( qos_class << 4) );Similar to above.
Right
+} +/* +* PARAMETERS +* p_rec +* [in] Pointer to the path record object. +* +* qos_class +* [in] QoS class to set. +* +* RETURN VALUES +* None +* +* NOTES +* +* SEE ALSO +* ib_path_rec_t +*********/ + +/****f* IBA Base: Types/ib_path_rec_qos_class +* NAME +* ib_path_rec_qos_class +* +* DESCRIPTION +* Get QoS class. +* +* SYNOPSIS +*/ +static inline uint16_t OSM_API +ib_path_rec_qos_class( + IN const ib_path_rec_t* const p_rec ) +{ + return (cl_ntoh16( p_rec->qos_class_sl ) >> 4); +} +/* +* PARAMETERS +* p_rec +* [in] Pointer to the path record object. +* +* RETURN VALUES +* QoS class of the path record. +* +* NOTES +* +* SEE ALSO +* ib_path_rec_t +*********/ + /****f* IBA Base: Types/ib_path_rec_mtu * NAME * ib_path_rec_mtu @@ -2940,7 +3063,7 @@ ib_path_rec_pkt_life(IN const ib_path_rec_t * const p_rec) * [in] Pointer to the path record object. * * RETURN VALUES -* Encoded path pkt_life = 4.096 ?sec * 2 ** PacketLifeTime. +* Encoded path pkt_life = 4.096 ??sec * 2 ** PacketLifeTime.It is not ascii. What is the change here?
I think my diff tool has troubles handling unusual chars. There's a 'ยต' character there - it wasn't changed. I guess it would be better to replace it with "micro" in the future - less elegant, but also less error prone.
* * NOTES * @@ -2988,7 +3111,8 @@ ib_path_rec_pkt_life_sel(IN const ib_path_rec_t * const p_rec) * DESCRIPTION * Get flow label. * -* SYNOPSIS +* SYNOPSIS p_rec->tclass = 0;What do you mean?
Beats me...
+ */ static inline uint32_t OSM_API ib_path_rec_flow_lbl(IN const ib_path_rec_t * const p_rec) @@ -5999,7 +6123,7 @@ ib_multipath_rec_pkt_life(IN const ib_multipath_rec_t * const p_rec) * [in] Pointer to the multipath record object. * * RETURN VALUES -* Encoded multipath pkt_life = 4.096 ?sec * 2 ** PacketLifeTime. +* Encoded multipath pkt_life = 4.096 ??sec * 2 ** PacketLifeTime.ascii?
Same as above
* * NOTES * diff --git a/opensm/opensm/osm_helper.c b/opensm/opensm/osm_helper.c index c428823..04e52a8 100644 --- a/opensm/opensm/osm_helper.c +++ b/opensm/opensm/osm_helper.c @@ -1044,7 +1044,7 @@ osm_dump_path_record(IN osm_log_t * const p_log, if (osm_log_is_active(p_log, log_level)) { osm_log(p_log, log_level, "PathRecord dump:\n" - "\t\t\t\tresv0...................0x%016" PRIx64 "\n" + "\t\t\t\tservice_id..............0x%016" PRIx64 "\n" "\t\t\t\tdgid....................0x%016" PRIx64 " : " "0x%016" PRIx64 "\n" "\t\t\t\tsgid....................0x%016" PRIx64 " : " @@ -1055,6 +1055,7 @@ osm_dump_path_record(IN osm_log_t * const p_log, "\t\t\t\ttclass..................0x%X\n" "\t\t\t\tnum_path_revers.........0x%X\n" "\t\t\t\tpkey....................0x%X\n" + "\t\t\t\tqos_class...............0x%X\n" "\t\t\t\tsl......................0x%X\n" "\t\t\t\tmtu.....................0x%X\n" "\t\t\t\trate....................0x%X\n" @@ -1063,7 +1064,7 @@ osm_dump_path_record(IN osm_log_t * const p_log, "\t\t\t\tresv2...................0x%X\n" "\t\t\t\tresv3...................0x%X\n" "", - *(uint64_t *) p_pr->resv0, + cl_ntoh64(p_pr->service_id), cl_ntoh64(p_pr->dgid.unicast.prefix), cl_ntoh64(p_pr->dgid.unicast.interface_id), cl_ntoh64(p_pr->sgid.unicast.prefix), @@ -1074,7 +1075,8 @@ osm_dump_path_record(IN osm_log_t * const p_log, p_pr->tclass, p_pr->num_path, cl_ntoh16(p_pr->pkey), - cl_ntoh16(p_pr->sl), + ib_path_rec_qos_class(p_pr), + ib_path_rec_sl(p_pr), p_pr->mtu, p_pr->rate, p_pr->pkt_life, diff --git a/opensm/opensm/osm_sa_multipath_record.c b/opensm/opensm/osm_sa_multipath_record.c index b3f42e2..3c79de7 100644 --- a/opensm/opensm/osm_sa_multipath_record.c +++ b/opensm/opensm/osm_sa_multipath_record.c @@ -724,7 +724,7 @@ __osm_mpr_rcv_build_pr(IN osm_mpr_rcv_t * const p_rcv, p_pr->hop_flow_raw &= cl_hton32(1 << 31);p_pr->pkey = p_parms->pkey;- p_pr->sl = cl_hton16(p_parms->sl); + ib_path_rec_set_sl(p_pr, p_parms->sl);Now when ib_path_rec_set_sl() sets only low 4 bits (and all 16 as before), shouldn't upper 12 bits (qos_class) be initialized here?
Right
p_pr->mtu = (uint8_t) (p_parms->mtu | 0x80); p_pr->rate = (uint8_t) (p_parms->rate | 0x80);diff --git a/opensm/opensm/osm_sa_path_record.c b/opensm/opensm/osm_sa_path_record.cindex 602fd2a..e8b24d0 100644 --- a/opensm/opensm/osm_sa_path_record.c +++ b/opensm/opensm/osm_sa_path_record.c @@ -740,7 +740,7 @@ __osm_pr_rcv_build_pr(IN osm_pr_rcv_t * const p_rcv, #endifp_pr->pkey = p_parms->pkey;- p_pr->sl = cl_hton16(p_parms->sl); + ib_path_rec_set_sl(p_pr, p_parms->sl);Ditto.
OK
p_pr->mtu = (uint8_t) (p_parms->mtu | 0x80); p_pr->rate = (uint8_t) (p_parms->rate | 0x80);@@ -1968,7 +1968,7 @@ void osm_pr_rcv_process(IN void *context, IN void *data)mcmember_rec. sl_flow_hop, &sl, &flow_label, &hop_limit); - p_pr_item->path_rec.sl = cl_hton16(sl); + ib_path_rec_set_sl(&p_pr_item->path_rec, sl);Ditto.
OK
#ifndef ROUTER_EXP p_pr_item->path_rec.hop_flow_raw = cl_hton32(hop_limit) | (flow_label << 8); diff --git a/opensm/osmtest/osmtest.c b/opensm/osmtest/osmtest.c index 36cb825..5bf11ea 100644 --- a/opensm/osmtest/osmtest.c +++ b/opensm/osmtest/osmtest.c @@ -1918,7 +1918,7 @@ osmtest_write_path_info(IN osmtest_t * const p_osmt, cl_ntoh64(p_rec->sgid.unicast.interface_id), cl_ntoh16(p_rec->dlid), cl_ntoh16(p_rec->slid), cl_ntoh32(p_rec->hop_flow_raw), p_rec->tclass, - p_rec->num_path, cl_ntoh16(p_rec->pkey), p_rec->sl, + p_rec->num_path, cl_ntoh16(p_rec->pkey), ib_path_rec_sl(p_rec),Why to not print qos_class too?
Good idea.
p_rec->mtu, p_rec->rate, p_rec->pkt_life, p_rec->preference);--1.5.1.4Also it would be nice if newly added code will be formatted with opensm/osm_indent. Thanks.
To tell you the truth, I'm not a big fan of this script.
Sometimes it does stuff it shouldn't. For instance,
I run it on osm_qos_policy.c, and this is what it does:
Before:
static osm_qos_match_rule_t *
__qos_policy_get_match_rule_by_pr(
const osm_pr_rcv_t * p_rcv,
const ib_path_rec_t * p_pr,
const osm_physp_t * p_src_physp,
const osm_physp_t * p_dest_physp,
ib_net64_t comp_mask)
After:
static osm_qos_match_rule_t *__qos_policy_get_match_rule_by_pr(const
osm_pr_rcv_t *
p_rcv,
const
ib_path_rec_t *
p_pr,
const osm_physp_t
* p_src_physp,
const osm_physp_t
* p_dest_physp,
ib_net64_t
comp_mask)
Correct me if I'm wrong, but I think that the 'before' example
is closer to our coding style than the 'after'.
But you're right in general.
It affects only patch 3/7 - adding QoS policy c & h files.
I'll repost this patch with rest of the fixed patches.
-- Yevgeny
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
