Hi,
here comes a slightly modified version of the patches, see comments
inline.
bye,
Sumit
On Wed, Dec 05, 2012 at 12:50:11PM -0500, Simo Sorce wrote:
On Wed, 2012-12-05 at 18:23 +0100, Sumit Bose wrote:
> 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
>
I started to change it but didn't like the resulting code, because it
turned out to be complex and hard to follow. Since the original struct
only user 8 or 12 bytes per group (depending if your are on 32bits or on
64bits) I would prefer to keep the old struct for this round of patches.
> >
> >
> > 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.
I guess I cannot see how you can have an entry without domain_id, maybe
I missed how that can happen.
For the local domain the domain_id is added by ipa-adtrust-install, if
it wasn't run before there is no domain_id for the local domain.
> > 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.
ok, do we have a ticket open as well ?
I added a fix with patch 0012.
> > > 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.
ok
we talked on irc and found that code works efficient here.
> > > 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.
No I think my solution was clever, but as much as I like it on paper,
the numbers do not warrant wasting time on it right now.
I added the new test to patch 0011.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel