[ofa-general] Re: [PATCH v3] osm: QoS - parsing port names
Yevgeny Kliteynik
kliteyn at dev.mellanox.co.il
Sun Oct 21 01:00:47 PDT 2007
Sasha Khapyorsky wrote:
> On 16:11 Tue 16 Oct , Yevgeny Kliteynik wrote:
>> Added node-by-name hash to the QoS policy object and
>> as port names are parsed they use this hash to locate
>> that actual port that the name refers to.
>> For now I prefer to keep this hash local, so it's part
>> of QoS policy object.
>> When the same parser will be used for partitions too,
>> this hash will be moved to be part of the subnet object.
>>
>> V3 changes (vs. V2):
>> - node-by-name instead of ca-by-name
>> - removed any constraints on the format of node name
>>
>> Signed-off-by: Yevgeny Kliteynik <kliteyn at dev.mellanox.co.il>
>> ---
>> opensm/include/opensm/osm_qos_policy.h | 3 +-
>> opensm/opensm/osm_qos_parser.y | 64 ++++++++++++++++++++++++++------
>> opensm/opensm/osm_qos_policy.c | 38 ++++++++++++++++---
>> 3 files changed, 86 insertions(+), 19 deletions(-)
>>
>> diff --git a/opensm/include/opensm/osm_qos_policy.h b/opensm/include/opensm/osm_qos_policy.h
>> index 30c2e6d..61fc325 100644
>> --- a/opensm/include/opensm/osm_qos_policy.h
>> +++ b/opensm/include/opensm/osm_qos_policy.h
>> @@ -49,6 +49,7 @@
>>
>> #include <iba/ib_types.h>
>> #include <complib/cl_list.h>
>> +#include <opensm/st.h>
>> #include <opensm/osm_port.h>
>> #include <opensm/osm_partition.h>
>> #include <opensm/osm_sa_path_record.h>
>> @@ -72,7 +73,6 @@ typedef struct _osm_qos_port_t {
>> 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 (.../.../...) */
>> uint8_t node_types; /* node types bitmask */
>> cl_qmap_t port_map;
>> } osm_qos_port_group_t;
>> @@ -148,6 +148,7 @@ typedef struct _osm_qos_policy_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_subn_t *p_subn; /* osm subnet object */
>> + st_table * p_node_hash; /* node by name hash */
>> } osm_qos_policy_t;
>>
>> /***************************************************/
>> diff --git a/opensm/opensm/osm_qos_parser.y b/opensm/opensm/osm_qos_parser.y
>> index d2917d3..5a6e0c9 100644
>> --- a/opensm/opensm/osm_qos_parser.y
>> +++ b/opensm/opensm/osm_qos_parser.y
>> @@ -245,7 +245,8 @@ qos_policy_entry: port_groups_section
>> * use: our SRP storage targets
>> * port-guid: 0x1000000000000001,0x1000000000000002
>> * ...
>> - * port-name: vs1/HCA-1/P1
>> + * port-name: vs1 HCA-1/P1
>> + * port-name: node_and_HCA_name/P2
>
> Maybe node_desc is cleaner instead of node_and_HCA_name.
>
>> * ...
>> * pkey: 0x00FF-0x0FFF
>> * ...
>> @@ -602,21 +603,60 @@ port_group_use_start: TK_USE {
>>
>> port_group_port_name: port_group_port_name_start string_list {
>> /* 'port-name' in 'port-group' - any num of instances */
>> - cl_list_iterator_t list_iterator;
>> - char * tmp_str;
>> -
>> - list_iterator = cl_list_head(&tmp_parser_struct.str_list);
>> - while( list_iterator != cl_list_end(&tmp_parser_struct.str_list) )
>> + cl_list_iterator_t list_iterator;
>> + osm_node_t * p_node;
>> + osm_physp_t * p_physp;
>> + unsigned port_num;
>> + char * tmp_str;
>> + char * port_str;
>> +
>> + /* parsing port name strings */
>> + for (list_iterator = cl_list_head(&tmp_parser_struct.str_list);
>> + list_iterator != cl_list_end(&tmp_parser_struct.str_list);
>> + list_iterator = cl_list_next(list_iterator))
>> {
>> tmp_str = (char*)cl_list_obj(list_iterator);
>> + if (tmp_str)
>> + {
>> + /* last slash in port name string is a separator
>> + between node name and port number */
>> + port_str = strrchr(tmp_str, '/');
>> + if (!port_str || (strlen(port_str) < 3) ||
>
> If port number is not specified it could be nice wildcarding - all
> ports for this node. There is no wild card expansion with multiple ports
> mapping in this patch, so this comment is just idea for future use, no
> need to change yet.
I prefer to have an 'explicit' wildcarding: ..../P* or ..../P[2-8]
>> + (port_str[1] != 'p' && port_str[1] != 'P')) {
>> + yyerror("illegal port name");
>> + free(tmp_str);
>> + cl_list_remove_all(&tmp_parser_struct.str_list);
>> + return 1;
>> + }
>>
>> - /*
>> - * TODO: parse port name strings
>> - */
>> + if (!(port_num = strtoul(&port_str[2],NULL,0))) {
>> + yyerror("illegal port number in port name");
>> + free(tmp_str);
>> + cl_list_remove_all(&tmp_parser_struct.str_list);
>> + return 1;
>> + }
>>
>> - if (tmp_str)
>> - cl_list_insert_tail(&p_current_port_group->port_name_list,tmp_str);
>> - list_iterator = cl_list_next(list_iterator);
>> + /* separate node name from port number */
>> + port_str[0] = '\0';
>> +
>> + if (st_lookup(p_qos_policy->p_node_hash,
>> + (st_data_t)tmp_str,
>> + (st_data_t*)&p_node))
>> + {
>> + /* we found the node, now get the right port */
>> + p_physp = osm_node_get_physp_ptr(p_node, port_num);
>> + if (!p_physp) {
>> + yyerror("port number out of range in port name");
>> + free(tmp_str);
>> + cl_list_remove_all(&tmp_parser_struct.str_list);
>> + return 1;
>> + }
>> + /* we found the port, now add it to guid table */
>> + __parser_add_port_to_port_map(&p_current_port_group->port_map,
>> + p_physp);
>> + }
>> + free(tmp_str);
>> + }
>> }
>> cl_list_remove_all(&tmp_parser_struct.str_list);
>> }
>> diff --git a/opensm/opensm/osm_qos_policy.c b/opensm/opensm/osm_qos_policy.c
>> index 51dd7b9..1207295 100644
>> --- a/opensm/opensm/osm_qos_policy.c
>> +++ b/opensm/opensm/osm_qos_policy.c
>> @@ -59,6 +59,33 @@
>> /***************************************************
>> ***************************************************/
>>
>> +static void
>> +__build_nodebyname_hash(osm_qos_policy_t * p_qos_policy)
>> +{
>> + osm_node_t * p_node;
>> + cl_qmap_t * p_node_guid_tbl = &p_qos_policy->p_subn->node_guid_tbl;
>> +
>> + p_qos_policy->p_node_hash = st_init_strtable();
>> + CL_ASSERT(p_qos_policy->p_node_hash);
>> +
>> + if (!p_node_guid_tbl || !cl_qmap_count(p_node_guid_tbl))
>> + return;
>> +
>> + for (p_node = (osm_node_t *) cl_qmap_head(p_node_guid_tbl);
>> + p_node != (osm_node_t *) cl_qmap_end(p_node_guid_tbl);
>> + p_node = (osm_node_t *) cl_qmap_next(&p_node->map_item)) {
>> + if (!st_lookup(p_qos_policy->p_node_hash,
>> + (st_data_t)p_node->print_desc,
>> + (st_data_t*)&p_node))
>> + st_insert(p_qos_policy->p_node_hash,
>> + (st_data_t)p_node->print_desc,
>> + (st_data_t)p_node);
>
> st_lookup() is not needed? st_insert() replace entry if it exists. In
> case of identical node_desc last will appear.
Whether the first or the last appearance will remain in the hash,
it's bad either way, but if the value in the hash will be replaced,
it will create memory leak, since the previous value won't be freed.
-- Yevgeny
> Sasha
>
>> + }
>> +}
>> +
>> +/***************************************************
>> + ***************************************************/
>> +
>> static boolean_t
>> __is_num_in_range_arr(uint64_t ** range_arr,
>> unsigned range_arr_len, uint64_t num)
>> @@ -127,8 +154,6 @@ osm_qos_port_group_t *osm_qos_policy_port_group_create()
>> return NULL;
>>
>> memset(p, 0, sizeof(osm_qos_port_group_t));
>> -
>> - cl_list_init(&p->port_name_list, 10);
>> cl_qmap_init(&p->port_map);
>>
>> return p;
>> @@ -150,10 +175,6 @@ void osm_qos_policy_port_group_destroy(osm_qos_port_group_t * p)
>> if (p->use)
>> free(p->use);
>>
>> - cl_list_apply_func(&p->port_name_list, __free_single_element, NULL);
>> - cl_list_remove_all(&p->port_name_list);
>> - cl_list_destroy(&p->port_name_list);
>> -
>> p_port = (osm_qos_port_t *) cl_qmap_head(&p->port_map);
>> while (p_port != (osm_qos_port_t *) cl_qmap_end(&p->port_map))
>> {
>> @@ -423,6 +444,8 @@ osm_qos_policy_t * osm_qos_policy_create(osm_subn_t * p_subn)
>> cl_list_init(&p_qos_policy->qos_match_rules, 10);
>>
>> p_qos_policy->p_subn = p_subn;
>> + __build_nodebyname_hash(p_qos_policy);
>> +
>> return p_qos_policy;
>> }
>>
>> @@ -495,6 +518,9 @@ void osm_qos_policy_destroy(osm_qos_policy_t * p_qos_policy)
>> cl_list_remove_all(&p_qos_policy->qos_match_rules);
>> cl_list_destroy(&p_qos_policy->qos_match_rules);
>>
>> + if (p_qos_policy->p_node_hash)
>> + st_free_table(p_qos_policy->p_node_hash);
>> +
>> free(p_qos_policy);
>>
>> p_qos_policy = NULL;
>> --
>> 1.5.1.4
>>
>
More information about the general
mailing list