[ofa-general] Re: [PATCH 3/7 V2] osm: QoS policy C & H files

[Sasha Khapyorsky]

On 14:17 Thu 23 Aug     , Yevgeny Kliteynik wrote:
> >>>> +	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 each partition by name search is linear over all configured by
OpenSM partitions (it is "parser-only" stuff in PM), when partition is
resolved port guid will be matched using cl_map anyway.

BTW partition name is optional as far as I remember. Not sure this is
an issue however search by pkey value will be faster.

> > 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.

Generic case would be: 30, 1, 2, 16, 17, 23, etc. :)

>  cl_map implemented as a binary tree, right?

Yes.

>  And doing binary search

Binary search is not implemented yet... Currently it is linear.

>  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.

Right. But why do you think it is not typical? (we will not request
from user to re-burn GUIDs for each desired QoS configuration :))

> 
>  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.

Thanks.

> >>>> +	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.

Then we will have two QoS managers in parallel, I don't like this too
much. OTOH the integration should be easy enough (low level stuff is
ready there), so I will not mind if it will be done in the week after
feature freeze.

> >>>> +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?

I think it _is_ useful. I'm trying to say is that we cannot from one
side to provide some functionality and from another expect that nobody
will use it :).

> > 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?

Why not? It is permitted and of course will be useful.

Also as I stated above even for one partition by name search you will
need to scan all configured partitions (so probably using pkey instead
of partition name is better).

>  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

Maybe with fast search, but not how it is now.

>  and
>  partition lists are better,

Absolutely not, even with one partition. There are always two parts:
Partition search (slow by name or fast by pkey) and guid search in
partition (which is cl_map).

Sasha