[SSSD] [PATCHES] Improve group handling in the PAC responder

Sumit Bose sbose at redhat.com
Wed Dec 5 17:23:58 UTC 2012


On Wed, Dec 05, 2012 at 09:04:46AM -0500, Simo Sorce wrote:
> On Wed, 2012-12-05 at 13:36 +0100, Sumit Bose wrote:
> > Hi,
> > 
> > this series of patches fixes https://fedorahosted.org/sssd/ticket/1666
> > and https://fedorahosted.org/sssd/ticket/1672. Details can be found in
> > the commit messages. Just as overview:
> > 
> > 0001-0006 add new structs and call to make handling groups from
> > different domains easier.
> 
> Sorry Sumit but I do not really agree with patch 1, it is extremely
> wasteful of space, and ugly.
> 
> I think you should create a structure for the container and not for each
> element, where you have:
> 
> container {
>     domain1 {
>         domain_info
>         array_of_gids
>     }
>     domain2 { ...
>     ...
> }

ok, I'll change it accordingly

> 
> 
> Patch 4 sounds fishy, I distinctly recall we do not touch domain_id
> because that is the domain identifier. It can't change because if it
> 'changes' it means it is a different domain. I am not even sure you can
> ever match that for updates because IIRC domains are searched by
> domain_id. So where is my reasoning wrong ? Tentatively nack if I am
> correct.

no, as said in the commit message, it is only changed if it is not
already set and that it just covers the case where ipa-adtrust-install
is called on the server and the local domain gets a SID.

> 
> 
> Patch 5 is a step in the right direction but it sounds wrong that you
> search the user in the db and just skip saving if it exist. The user
> info you have in the PAC may be updated and contain information that
> differs from what's in the cache. At the very least you need to test
> that nothing needs to change if you do not want to unconditionally
> overwrite.

yes, there is still a todo in the original code for this, but since it
is not related to the missing groups I haven't touched it yet.

> 
> > 0007 and 0008 add the evaluation of the group memberships from the
> > domain the user belongs to (#1666).
> 
> The domain sid retrieval looks extremely inefficient, we have a neat
> optimization where you can look up the domain once and then just go rid
> by rid and you are going back to build a full sid, which will then be
> turned into a string and matched once again to find the domain.
> I think this should be improved to use the fact you already have the
> sids split out with a modicum of caching.
> You can uses alexander's patch that just landed in freeipa to do binary
> sid comparison w/o converting to string, and a simple array in the
> function to keep a list of all dom_sid -> domaiin structs found so far,
> they wont be many just a handful in most cases so a linear search there
> is also fine.

I cannot follow your comment, I'll ping you on irc about it.

> 
> > 0009 just changes an error code
> 
> ACK
> 
> > 0010 fixes #1672 by removing duplicated groups
> 
> I think using a hash table like function is fine, but I wonder if we can
> save a lot of cycles here by using a smarter hash algorithm, namely just
> truncate the gid and use the lower N bits. This would be orders of
> magnitude more efficient and you pretty much are guaranteed to not have
> many collisions because gids are unique and tend to all start from a
> common base so if you take the least N significant bits you basically
> have an automatic index already built in.
> 
> Building a static table on the stack like the following and only
> allocating (using talloc) only on collisions would also save tons of
> allocations.
> 
> 
> struct gid_el {
>     gid_t gid;
>     struct gid_el *next;
> } gid_table[1024] = { 0 };
> 
> the 'hash/index' is (gid & 0x2FF)

Currently I get to following timing data for get_gids_from_pac():

Running suite(s): PAC responder
Testcase [0], number of groups [1], duration [0s 10us]
Testcase [1], number of groups [10], duration [0s 44us]
Testcase [2], number of groups [100], duration [0s 197us]
Testcase [3], number of groups [1000], duration [0s 2307us]
Testcase [4], number of groups [10000], duration [0s 15636us]
Testcase [5], number of groups [100000], duration [0s 167523us]
Testcase [6], number of groups [1000000], duration [1s 688983us]

I think this is not too bad and I would prefer to use the common hash.
If you disagree I can implement your suggestion.

bye,
Sumit

> 
> 
> > 0011 adds a new unit test
> 
> 
> Simo.
> 
> -- 
> Simo Sorce * Red Hat, Inc * New York
> 
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



More information about the sssd-devel mailing list