On (11/09/13 17:55), Simo Sorce wrote:
On Wed, 2013-09-11 at 21:21 +0200, Lukas Slebodnik wrote:
> On (11/09/13 12:50), Simo Sorce wrote:
> >On Wed, 2013-09-11 at 17:39 +0200, Lukas Slebodnik wrote:
> >> On (11/09/13 16:30), Michal Židek wrote:
> >> >On 09/11/2013 04:16 PM, Simo Sorce wrote:
> >> >>On Wed, 2013-09-11 at 15:58 +0200, Lukas Slebodnik wrote:
> >> >>>On (11/09/13 09:39), Simo Sorce wrote:
> >> >>>>On Wed, 2013-09-11 at 12:24 +0200, Lukas Slebodnik wrote:
> >> >>>>>ehlo,
> >> >>>>>
> >> >>>>>Two patches are attached.
> >> >>>>>
> >> >>>>>The 1st patch reverts commit
4662725ffef62b3b2502481438effa7c8fef9f80.
> >> >>>>>"mmap_cache: Skip records which doesn't have
same hash"
> >> >>>>> --next patch lokks nicer with recerted commit
4662725ffe
> >> >>>>>
> >> >>>>>
> >> >>>>>The 2nd patch changes behaviour of record chaining.
> >> >>>>>
> >> >>>>>LS
> >> >>>>
> >> >>>>NACK, you cannot just change the record structure w/o
bumping the whole
> >> >>>>mmap MAJOR version number.
> >> >>>>
> >> >>>>Aside:
> >> >>>>
> >> >>>>If we are not I am thinking whether we should do some other
> >> >>>>optimizations to keep the size down.
> >> >>>>
> >> >>>>For example, hashes are generally relatively small
quantities as we do
> >> >>>>not allow huge tables. If we can limit tables size to a max
of 65k
> >> >>>>elements we could change the hash type from uint32_t to
uint16_t, and
> >> >>>>end up using the same size we are using now.
> >> >>>>We already limit the total mmap size to 4G as rel_prt_t is a
uint32_t,
> >> >>>>and in practice we'll never allow maps with a very large
size as it eats
> >> >>>>into the memory available to 32 bit processes.
> >> >>>>So perhaps 65k for the hash tables is an acceptable limit.
(I think we
> >> >>>>currently have a 100k limit which is probably a little bit
overblown as
> >> >>>>a default anyway.
> >> >>>>
> >> >>>>Simo.
> >> >>>>
> >> >>>I would rather reduce "uint64_t expire" into
"uint32_t expire_offset"
> >> >>>
> >> >>>expire_offset will be time since creation of mmap cache.
> >> >>>and uint64_t "creation_time" will be stored in
"struct sss_mc_ctx"
> >> >>
> >> >>Uhmm, I do agree that 4billion seconds offset is probably ok, but
then
> >> >>it means the information is split in 2 different places, and this
will
> >> >>make clients more complicated, as clients will have to keep track of
the
> >> >>creation time + expiration offset now ...
> >> >>
> >> >>>Do you agree?
> >> >>
> >> >>I am not 100% convinced yet, although I do agree that it would save
> >> >>quite some space.
> >> >>
> >> >>>Do I need to increment SSS_MC_MAJOR_VNO or only
SSS_MC_MINOR_VNO?
> >> >>
> >> >>Major, you are completely changing the structure, an older client
cannot
> >> >>be allowed to use the new map unless it explicitly know the layout
> >> >>changed.
> >> >
> >> >Well, if we already have to change the major version, I would go with
> >> >the two 32 bit next1 and nex2 members. If we really want to keep the
> >> >current size of the record (btw. why? We have to change the major ver.
> >> >anyway) then I like the two 16 bit pointers next1 and next2 more than
> >> >the 32 bit time offset (the memcache is just another cache layer, so it
> >> >does not have to be "gigantic"... 16 bit pointers are just
fine IMO).
> >> >
> >> I don't like 16 bit next pointers.
> >>
> >> Default value of hash table size is 100000 (for passwd)
> >> It is 53% greater value than UINT_16_MAX.
> >> If we decided to enlarge this default value in future, we would be still
> >> limited by UINT_16_MAX.
> >>
> >> I think it is not good idea to reduce size of next pointers.
> >
> >I am thinking we might want to change the way we create the way we
> >handle record size to see if we can make a change that will be more
> >future proof.
> >
> >If we stored the size of the records in the header and always calculate
> >the position of b2 based on that information then we could introduce a
> >variable size record structure. This would allow in future to increase
> >the size of the structure for optional parameters w/o necessarily having
> >to bump up the major version.
> >
> >However I want to have at the very least 64bit alignemtn always
> >preserverd, so from 32 bytes we want to grow in 8 bytes steps, which
> >means adding a uint32_t padding before b2.
> totally agree.
>
> SSS_MC_MAJOR_VNO was increased.
> padding was added. sizeof(struct sss_mc_ctx) will be 40
>
> Updated patches are attached.
It looks ok to me.
However, just to be clear we are planning on pushing this to 1.12 only
right ?
Branch 1.11 has not been created yet. (only tag)
Are we fine to with patches in 1.11 (and maybe 1.10) ?
LS
I am not hugely comfortable with pushing it to existing releases.
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