[ofa-general] Re: [PATCH 3/3] osm: QoS - parsing port names
Yevgeny Kliteynik
kliteyn at dev.mellanox.co.il
Sun Oct 14 03:24:54 PDT 2007
Sasha Khapyorsky wrote:
> Hi Yevgeny,
>
> On 11:01 Tue 09 Oct , Yevgeny Kliteynik wrote:
>> Added CA-by-name hash to the QoS policy object and
>
> Why it is called "CA"-by-name? In the code below I see that hash is
> created for all nodes (including switches and routers).
In osm_qos_policy.c:
if (p_node->node_info.node_type == IB_NODE_TYPE_CA)
st_insert(p_qos_policy->p_ca_hash,
(st_data_t)p_node->print_desc,
(st_data_t)p_node);
>> 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.
>>
>> 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 | 73 +++++++++++++++++++++++++++-----
>> opensm/opensm/osm_qos_policy.c | 36 +++++++++++++---
>> 3 files changed, 94 insertions(+), 18 deletions(-)
>>
>> diff --git a/opensm/include/opensm/osm_qos_policy.h b/opensm/include/opensm/osm_qos_policy.h
>> index 30c2e6d..5c32896 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_ca_hash; /* hash of CAs by node description */
>> } osm_qos_policy_t;
>>
>> /***************************************************/
>> diff --git a/opensm/opensm/osm_qos_parser.y b/opensm/opensm/osm_qos_parser.y
>> index 2405519..cf342d3 100644
>> --- a/opensm/opensm/osm_qos_parser.y
>> +++ b/opensm/opensm/osm_qos_parser.y
>> @@ -603,23 +603,74 @@ 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 * name_str;
>> + char * tmp_str;
>> + char * host_str;
>> + char * ca_str;
>> + char * port_str;
>> + char * node_desc = (char*)malloc(IB_NODE_DESCRIPTION_SIZE + 1);
>> +
>> + /* 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 && *tmp_str)
>> + {
>> + name_str = tmp_str;
>> + host_str = strtok (name_str,"/");
>> + ca_str = strtok (NULL, "/");
>> + port_str = strtok (NULL, "/");
>> +
>> + if (!host_str || !(*host_str) ||
>> + !ca_str || !(*ca_str) ||
>> + !port_str || !(*port_str) ||
>> + (port_str[0] != 'p' && port_str[0] != 'P')) {
>> + yyerror("illegal port name");
>> + free(tmp_str);
>> + free(node_desc);
>> + cl_list_remove_all(&tmp_parser_struct.str_list);
>> + return 1;
>> + }
>>
>> - /*
>> - * TODO: parse port name strings
>> - */
>> + if (!(port_num = strtoul(&port_str[1],NULL,0))) {
>> + yyerror("illegal port number in port name");
>> + free(tmp_str);
>> + free(node_desc);
>> + 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);
>> + sprintf(node_desc,"%s %s",host_str,ca_str);
>> + free(tmp_str);
>> +
>> + if (st_lookup(p_qos_policy->p_ca_hash,
>> + (st_data_t)node_desc,
>> + (st_data_t*)&p_node))
>
> I am not following this. Hash key is generated as "host_str ca_str", but
> below where hash table is filled NodeDescription string is used. Why
> this should be same?
Because that's how node description is created.
From /etc/init.d/openibd:
# Add node description to sysfs
IBSYSDIR="/sys/class/infiniband"
if [ -d ${IBSYSDIR} ]; then
declare -i hca_id=1
for hca in ${IBSYSDIR}/*
do
if [ -e ${hca}/node_desc ]; then
echo -n "$(hostname -s) HCA-${hca_id}" >> ${hca}/node_desc
fi
let hca_id++
done
fi
>> + {
>> + /* we found the node, now get the right port */
>> + CL_ASSERT(p_node);
>
> Why this CL_ASSERT() needed?
It's not - it was just for debugging. I can remove it.
>> + 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);
>> + free(node_desc);
>> + 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);
>> + }
>> + }
>> }
>> cl_list_remove_all(&tmp_parser_struct.str_list);
>> + free(node_desc);
>> }
>> ;
>>
>> diff --git a/opensm/opensm/osm_qos_policy.c b/opensm/opensm/osm_qos_policy.c
>> index 51dd7b9..0d7235f 100644
>> --- a/opensm/opensm/osm_qos_policy.c
>> +++ b/opensm/opensm/osm_qos_policy.c
>> @@ -59,6 +59,31 @@
>> /***************************************************
>> ***************************************************/
>>
>> +static void
>> +__build_cabyname_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_ca_hash = st_init_strtable();
>> + CL_ASSERT(p_qos_policy->p_ca_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 (p_node->node_info.node_type == IB_NODE_TYPE_CA)
>> + st_insert(p_qos_policy->p_ca_hash,
>> + (st_data_t)p_node->print_desc,
>> + (st_data_t)p_node);
>
> Hmm, why do you think NodeDescription will be unique for each node in a
> fabric?
NodeDescription is a combination of host id and hca number.
If nobody "plays" with these values (and doesn't modify this area
of /etc/init.d/openibd), then NodeDescription will be unique.
But IB spec doesn't require it to be unique. In fact, it doesn't say
anything at all about how this NodeDescription should look.
Moreover, if the device won't be found in /sys/class/infiniband,
or if it won't have /sys/class/infiniband/${hca}/node_desc, I have no
idea what would be the content of NodeDescription.
I am, however, trying to give best value for the money :)
OSM doesn't know what is the host id of a certain hca.
The only thing I can think of right now is that OpenSM can
check the NodeDescription before inserting it to the hash
(which can be done in a consequent patch).
It can check that the description looks like this:
<some string w/o white space><space><HCA-><some number>
-- Yevgeny
> Sasha
>
>> + }
>> +}
>> +
>> +/***************************************************
>> + ***************************************************/
>> +
>> static boolean_t
>> __is_num_in_range_arr(uint64_t ** range_arr,
>> unsigned range_arr_len, uint64_t num)
>> @@ -127,8 +152,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 +173,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 +442,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_cabyname_hash(p_qos_policy);
>> +
>> return p_qos_policy;
>> }
>>
>> @@ -495,6 +516,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_ca_hash)
>> + st_free_table(p_qos_policy->p_ca_hash);
>> +
>> free(p_qos_policy);
>>
>> p_qos_policy = NULL;
>> --
>> 1.5.1.4
>>
>>
>
More information about the general
mailing list