[ofa-general] Re: [PATCH 1/7] osm: QoS - adding new PathRecord fields
Yevgeny Kliteynik
kliteyn at dev.mellanox.co.il
Tue Aug 21 04:11:59 PDT 2007
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 <kliteyn at dev.mellanox.co.il>
>
> 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.c
>> index 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,
>> #endif
>>
>> p_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.4
>>
>
> Also 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
>
More information about the general
mailing list