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.
> "\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)
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.
> +}
> +/*
> +* 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?
> *
> * 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?
> +
> */
> 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?
> *
> * 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?
> 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.
> 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.
> #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?
> 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.
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
