[ofa-general] Re: [PATCH 3/7 V2] osm: QoS policy C & H files
Hal Rosenstock
hal.rosenstock at gmail.com
Mon Aug 27 09:33:16 PDT 2007
On 8/23/07, Yevgeny Kliteynik <kliteyn at dev.mellanox.co.il> wrote:
> Hi Sasha,
>
> Sasha Khapyorsky wrote:
> > On 11:11 Thu 23 Aug , Yevgeny Kliteynik wrote:
> >>>> +/***************************************************/
> >>>> +
> >>>> +typedef struct osm_qos_port_group_t_ {
> >>>> + char *name; /* single string (this port group name) */
> >>>> + char *use; /* single string (description) */
> >>>> + cl_list_t port_name_list; /* list of port names (.../.../...) */
> >>>> + uint64_t **guid_range_arr; /* array of guid ranges (pair of 64-bit
> >>>> guids) */
> >>>> + unsigned guid_range_len; /* num of guid ranges in the array */
> >>>> + cl_list_t partition_list; /* list of partition names */
> >>>> + boolean_t node_type_ca;
> >>>> + boolean_t node_type_switch;
> >>>> + boolean_t node_type_router;
> >>>> + boolean_t node_type_self;
> >>>> +} osm_qos_port_group_t;
> >>> I see you are using this in "run-time", not just during the parsing.
> >>> Instead of having all this config features you can just resolve port
> >>> guids in parse time and keep it here in cl_map() for fast searches.
> >> By saying "config features", do you mean the four boolean flags?
> >
> > No, I mean everything except name and use.
> >
> >> It looks to me that checking the type of node is as fast as it gets,
> >> and it won't hurt to leave these booleans instead of resolving
> >> all the guids.
> >
> > It could be optimization when types are specified, which is not always
> > the case and then you are going to do linear searches over all lists.
>
> Right, it would be linear scanning of list of partitions.
>
> > And how something like "for each guid in this group" (which is needed
> > for QoS port parameters setup) should be resolved? By matching each guid
> > in the subnet against those lists?
>
> Good point.
>
> >> Moreover, the guids here are stored in range array, which is IMO
> >> better suited for the policy file syntax, because if a user specifies
> >> something like this "0x0-0x0FFF" in guids, it will be only one element
> >> of the array, which is efficient both in memory and in serch time.
> >
> > And what should be there if user specifies ports in the group as:
> > guid1, guid2, guid3, etc. ?
>
> The range array is sorted and "shrinked".
> That is, if a user specifies guids as "30,1,2,3-20,15", eventually you
> would get two elements in the array: 1-20 and 30-30.
> cl_map implemented as a binary tree, right?
> And doing binary search in this kind of array is faster than searching
> a guid in cl_map of all the guids.
> Worst case - you will get the same performance as in case of cl_map if
> *all* the guids are "discreet" and can't be groupped in ranges.
>
> Nevertheless, I agree that there's a problem if there are many partitions
> in the list (although I'm not sure it's a practical case)
>
> I'll work on this.
>
> >> (I probably should mention here that the efficient search in the range
> >> array is not implemented yet, but it would be a simple binary search -
> >> there's a "todo" comment in the search function right now)
> >>
> >>>> +
> >>>> +osm_qos_port_group_t *osm_qos_policy_port_group_create();
> >>>> +void osm_qos_policy_port_group_destroy();
> >>> Would be nice to have function prototypes in one place.
> >>>> +
> >>>> +/***************************************************/
> >>>> +
> >>>> +typedef struct osm_qos_vlarb_scope_t_ {
> >>>> + cl_list_t group_list; /* list of group names (strings) */
> >>>> + cl_list_t across_list; /* list of 'across' group names (strings) */
> >>>> + cl_list_t vlarb_high_list; /* list of num pairs (n:m,...), 32-bit values
> >>>> */
> >>>> + cl_list_t vlarb_low_list; /* list of num pairs (n:m,...), 32-bit values
> >>>> */
> >>> Why cl_list for VLArb? it should be short fixed length arrays?
> >> Right.
> >> Since the actual VLArb setup is not implemented yet, I didn't see
> >> this obvious thing.
> >
> > But it should be implemented. Right?
>
> Sure, but not for OFED 1.3 - we have a feature freeze in 11 days.
Without VLArb configuration, where does this come from ? What is the
QoS support then: just SL ?
-- Hal
>
> >>>> + uint32_t vl_high_limit; /* single integer */
> >>>> + boolean_t vl_high_limit_set;
> >>>> +} osm_qos_vlarb_scope_t;
> >>>> +
> >>>> +osm_qos_vlarb_scope_t *osm_qos_policy_vlarb_scope_create();
> >>>> +void osm_qos_policy_vlarb_scope_destroy();
> >>>> +
> >>>> +/***************************************************/
> >>>> +
> >>>> +typedef struct osm_qos_sl2vl_scope_t_ {
> >>>> + cl_list_t group_list; /* list of strings (port group names) */
> >>>> + boolean_t from[OSM_QOS_POLICY_MAX_PORTS_ON_SWITCH];
> >>>> + boolean_t to[OSM_QOS_POLICY_MAX_PORTS_ON_SWITCH];
> >>>> + cl_list_t across_from_list; /* list of strings (port group names) */
> >>>> + cl_list_t across_to_list; /* list of strings (port group names) */
> >>>> + uint8_t sl2vl_table[16]; /* array of sl2vl values */
> >>>> + boolean_t sl2vl_table_set;
> >>>> +} osm_qos_sl2vl_scope_t;
> >>> This will be used for sl2vl setup? Same as above - why not to generate
> >>> final port guid list just in parse time?
> >>>> +
> >>>> +osm_qos_sl2vl_scope_t *osm_qos_policy_sl2vl_scope_create();
> >>>> +void osm_qos_policy_sl2vl_scope_destroy();
> >>>> +
> >>>> +/***************************************************/
> >>>> +
> >>>> +typedef struct osm_qos_level_t_ {
> >>>> + char *use;
> >>>> + char *name;
> >>>> + uint8_t sl;
> >>>> + boolean_t sl_set;
> >>>> + uint8_t mtu_limit;
> >>>> + boolean_t mtu_limit_set;
> >>>> + uint8_t rate_limit;
> >>>> + boolean_t rate_limit_set;
> >>>> + uint8_t pkt_life;
> >>>> + boolean_t pkt_life_set;
> >>>> + uint64_t **path_bits_range_arr; /* array of bit ranges (real values are
> >>>> 32bits) */
> >>>> + unsigned path_bits_range_len; /* num of bit ranges in the array */
> >>>> + uint64_t **pkey_range_arr; /* array of PKey ranges (real values are
> >>>> 16bits) */
> >>>> + unsigned pkey_range_len;
> >>>> +} osm_qos_level_t;
> >>>> +
> >>>> +osm_qos_level_t *osm_qos_policy_qos_level_create();
> >>>> +void osm_qos_policy_qos_level_destroy();
> >>>> +
> >>>> +boolean_t osm_qos_level_has_pkey(IN const osm_qos_level_t * p_qos_level,
> >>>> + IN ib_net16_t pkey);
> >>>> +
> >>>> +ib_net16_t osm_qos_level_get_shared_pkey(IN const osm_qos_level_t *
> >>>> p_qos_level,
> >>>> + IN const osm_physp_t * p_src_physp,
> >>>> + IN const osm_physp_t * p_dest_physp);
> >>>> +
> >>>> +/***************************************************/
> >>>> +
> >>>> +typedef struct osm_qos_match_rule_t_ {
> >>>> + char *use;
> >>>> + cl_list_t source_list; /* list of strings */
> >>>> + cl_list_t source_group_list; /* list of pointers to relevant port-group
> >>>> */
> >>>> + cl_list_t destination_list; /* list of strings */
> >>>> + cl_list_t destination_group_list; /* list of pointers to relevant
> >>>> port-group */
> >>> I think you should only keep port guids there (mapped for fast searches).
> >> Same as above.
> >> I think that checking node type and then guid range array is
> >> essentially faster than checking guid map.
> >
> > _Only_ for case when the group is specified by type, which is likely
> > will not be typical.
>
> Again, in *worst* case you will get the same performance in searching
> range array as in searching cl_map (which is a binary tree).
>
> >> You might say, of course, that there can be many port groups
> >> in the same match rule, but I don't see this as a practical
> >> example.
> >
> > Of course it could (or you must disable multi groups here, which is
> > not good idea I think and not what was presented in the RFC).
>
> Can you elaborate on this?
> Do you think that having multiple groups is not useful at all?
>
> > Also each group still have this "linear searchable" lists:
> >
> > cl_list_t port_name_list; /* list of port names (.../.../...) */
>
> Port names are not implemented yet :)
> There's a "todo" comment in the code.
> This is the only keyword in the parser that is not implemented - I mean it's parsed,
> but other than creating this list the parser doesn't do anything with it.
> But It definitely won't stay as a pure list.
>
> > uint64_t **guid_range_arr; /* array of guid ranges (pair of 64-bit
>
> This is not just a list, as I've explained before.
>
> > cl_list_t partition_list; /* list of partition names */
>
> I can hardly believe than there will be more than one or two partitions
> in this list. Do you think otherwise?
>
> Anyway, I can extract all the guids and prepare a map. This is certainly the
> easiest implementation. And I still think that on average, guid ranges and
> partition lists are better, but who knows - perhaps someone would want
> to define a bunch of partitions in a single port group and ruin my average... :)
>
> >>>> + char *qos_level_name;
> >>>> + osm_qos_level_t *p_qos_level;
> >>> Why do you need qos_level_name if you keep the pointer to this qos_level
> >>> struct?
> >> In policy file the match rule might appear before the QoS levels,
> >> so matching qos level names to the actual qos levels is done when
> >> the parsing is done.
> >>
> >>>> + uint64_t **service_id_range_arr; /* array of SID ranges (64-bit values)
> >>>> */
> >>>> + unsigned service_id_range_len;
> >>>> + uint64_t **qos_class_range_arr; /* array of QoS Class ranges (real
> >>>> values are 16bits) */
> >>>> + unsigned qos_class_range_len;
> >>>> + uint64_t **pkey_range_arr; /* array of PKey ranges (real values are
> >>>> 16bits) */
> >>>> + unsigned pkey_range_len;
> >>>> +} osm_qos_match_rule_t;
> >>>> +
> >>>> +osm_qos_match_rule_t *osm_qos_policy_match_rule_create();
> >>>> +void osm_qos_policy_match_rule_destroy();
> >>>> +
> >>>> +/***************************************************/
> >>>> +
> >>>> +typedef struct osm_qos_policy_t_ {
> >>>> + cl_list_t port_groups; /* list of osm_qos_port_group_t */
> >>>> + cl_list_t sl2vl_tables; /* list of osm_qos_sl2vl_scope_t */
> >>>> + cl_list_t vlarb_tables; /* list of osm_qos_vlarb_scope_t */
> >>>> + cl_list_t qos_levels; /* list of osm_qos_level_t */
> >>>> + cl_list_t qos_match_rules; /* list of osm_qos_match_rule_t */
> >>>> + osm_qos_level_t *p_default_qos_level; /* default QoS level */
> >>>> +} osm_qos_policy_t;
> >>>> +
> >>>> +void osm_qos_policy_create();
> >>>> +void osm_qos_policy_destroy();
> >>>> +int osm_qos_policy_validate();
> >>>> +
> >>>> +void osm_qos_policy_get_qos_level_by_pr(IN const osm_pr_rcv_t * p_rcv,
> >>>> + IN const ib_path_rec_t * p_pr,
> >>>> + IN const osm_physp_t * p_src_physp,
> >>>> + IN const osm_physp_t * p_dest_physp,
> >>>> + IN ib_net64_t comp_mask,
> >>>> + OUT osm_qos_level_t ** pp_qos_level);
> >>>> +
> >>>> +/***************************************************/
> >>>> +
> >>>> +int osm_qos_parse_policy_file(IN osm_log_t * p_log, IN const char
> >>>> *policy_file);
> >>>> +
> >>>> +/***************************************************/
> >>>> +
> >>>> +#endif /* ifndef OSM_QOS_POLICY_H */
> >>>> diff --git a/opensm/opensm/osm_qos_policy.c
> >>>> b/opensm/opensm/osm_qos_policy.c
> >>>> new file mode 100644
> >>>> index 0000000..bc2aa68
> >>>> --- /dev/null
> >>>> +++ b/opensm/opensm/osm_qos_policy.c
> >>>> @@ -0,0 +1,901 @@
> >>>> +/*
> >>>> + * Copyright (c) 2004-2006 Voltaire, Inc. All rights reserved.
> >>>> + * Copyright (c) 2002-2005 Mellanox Technologies LTD. All rights
> >>>> reserved.
> >>>> + * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
> >>>> + *
> >>>> + * This software is available to you under a choice of one of two
> >>>> + * licenses. You may choose to be licensed under the terms of the GNU
> >>>> + * General Public License (GPL) Version 2, available from the file
> >>>> + * COPYING in the main directory of this source tree, or the
> >>>> + * OpenIB.org BSD license below:
> >>>> + *
> >>>> + * Redistribution and use in source and binary forms, with or
> >>>> + * without modification, are permitted provided that the following
> >>>> + * conditions are met:
> >>>> + *
> >>>> + * - Redistributions of source code must retain the above
> >>>> + * copyright notice, this list of conditions and the following
> >>>> + * disclaimer.
> >>>> + *
> >>>> + * - Redistributions in binary form must reproduce the above
> >>>> + * copyright notice, this list of conditions and the following
> >>>> + * disclaimer in the documentation and/or other materials
> >>>> + * provided with the distribution.
> >>>> + *
> >>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> >>>> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> >>>> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> >>>> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> >>>> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> >>>> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> >>>> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> >>>> + * SOFTWARE.
> >>>> + *
> >>>> + */
> >>>> +
> >>>> +/*
> >>>> + * Abstract:
> >>>> + * OSM QoS Policy functions.
> >>>> + *
> >>>> + * Environment:
> >>>> + * Linux User Mode
> >>>> + *
> >>>> + * Author:
> >>>> + * Yevgeny Kliteynik, Mellanox
> >>>> + */
> >>>> +
> >>>> +#include <opensm/osm_qos_policy.h>
> >>>> +#include <opensm/osm_qos_parser_y.h>
> >>>> +#include <opensm/osm_partition.h>
> >>>> +
> >>>> +extern void yyerror(char *s);
> >>>> +osm_log_t *p_qos_parser_osm_log = NULL;
> >>>> +osm_qos_policy_t *p_qos_policy = NULL;
> >>> Please try to avoid globals - keep it as part of osm_opensm_t or
> >>> osm_subn_t structures.
> >> I thought about it, but didn't want to "condaminate" the osm_opensm_t
> >> or osm_subn_t structures untill the QoS functionality is ready.
> >
> > How globals are better in this sense?
>
> They're not :)
> The difference is that less files are modified.
> But I agree that the QoS policy should be part of osm_opensm_t or osm_subn_t.
>
> -- Yevgeny
>
> > Sasha
> >
>
> _______________________________________________
> general mailing list
> general at lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
>
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
>
More information about the general
mailing list