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

[Hal Rosenstock]

On 8/23/07, Sasha Khapyorsky <sashak at voltaire.com> wrote:
> 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.

Isn't this needed ? What about LASH ? How is that supported ? I
thought there were extra options or the like to enable the higher
level QoS functions (manager) ?

-- Hal

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