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
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.
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"
Do you agree? Do I need to increment SSS_MC_MAJOR_VNO or only SSS_MC_MINOR_VNO?
LS
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.
Note that this means that until a long lived process is restarted after the upgrade that changes this structure it will fail to use the cache consistently (as it should, or it would just read garbage).
Simo.
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).
Note that this means that until a long lived process is restarted after the upgrade that changes this structure it will fail to use the cache consistently (as it should, or it would just read garbage).
Simo.
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.
LS
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.
Simo.
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.
LS
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 ?
I am not hugely comfortable with pushing it to existing releases.
Simo.
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 ?
I am not hugely comfortable with pushing it to existing releases.
Simo.
The main problem is that existing releases could crash in some rare situations There is a report in bugzilla one week old. https://bugzilla.redhat.com/show_bug.cgi?id=997406#c12
User reported crash and he had latest sssd 1-10 with all mmap_cache patches; but it didn't help. sssd_nss crashed.
We had a long discussion about solution (revalidation of chains), but we did not find any simple "back-compatible" fix.
How do we want to fix it in older branches If we don't want to backport patch from previous mail.
LS
On Thu, Sep 12, 2013 at 09:05:07AM +0200, Lukas Slebodnik wrote:
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 ?
I am not hugely comfortable with pushing it to existing releases.
Can you describe why? Are you afraid of regressions? (Genuine questions, sorry, I still don't know the memcache code well)
Would it help to have a memcache unit test?
Simo.
The main problem is that existing releases could crash in some rare situations There is a report in bugzilla one week old. https://bugzilla.redhat.com/show_bug.cgi?id=997406#c12
User reported crash and he had latest sssd 1-10 with all mmap_cache patches; but it didn't help. sssd_nss crashed.
We had a long discussion about solution (revalidation of chains), but we did not find any simple "back-compatible" fix.
How do we want to fix it in older branches If we don't want to backport patch from previous mail.
Right, I'd like to backport a fix for the above problem to all existing branches if possible.
On Thu, 2013-09-12 at 11:26 +0200, Jakub Hrozek wrote:
On Thu, Sep 12, 2013 at 09:05:07AM +0200, Lukas Slebodnik wrote:
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 ?
I am not hugely comfortable with pushing it to existing releases.
Can you describe why? Are you afraid of regressions? (Genuine questions, sorry, I still don't know the memcache code well)
Yes.
Would it help to have a memcache unit test?
No, we are talking about old client trying to read new mmpa file with incompatible format and failing. A unit test will not do because we have always the new code in there.
But for the future we should probably have a sample file we dro pin place and make sure client libraries properly ignore any format they do not understand. Both an older format no more supported and a newer format not yet supported (basically just new major version and then all zeros instead of content beyond the header.
The main problem is that existing releases could crash in some rare situations There is a report in bugzilla one week old. https://bugzilla.redhat.com/show_bug.cgi?id=997406#c12
User reported crash and he had latest sssd 1-10 with all mmap_cache patches; but it didn't help. sssd_nss crashed.
We had a long discussion about solution (revalidation of chains), but we did not find any simple "back-compatible" fix.
How do we want to fix it in older branches If we don't want to backport patch from previous mail.
Right, I'd like to backport a fix for the above problem to all existing branches if possible.
I know, but we need some regression testing and not rush it.
Simo.
On Thu, 2013-09-12 at 09:05 +0200, Lukas Slebodnik wrote:
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 ?
I am not hugely comfortable with pushing it to existing releases.
Simo.
The main problem is that existing releases could crash in some rare situations There is a report in bugzilla one week old. https://bugzilla.redhat.com/show_bug.cgi?id=997406#c12
User reported crash and he had latest sssd 1-10 with all mmap_cache patches; but it didn't help. sssd_nss crashed.
We had a long discussion about solution (revalidation of chains), but we did not find any simple "back-compatible" fix.
How do we want to fix it in older branches If we don't want to backport patch from previous mail.
The problem is that a database format change is not to be taken lightly as it kills completely the use of the mmap cache until all long lived processes are restarted. If we can detect the problem I would rather nuke the caches. Deletions are rare, they happen only when a user disappear IIRC, so if we have a simple method of detecting when a deletion may cause issue we can simply reset the mmap cache then it would have less dramatic effects I think. But I am open to other ideas.
Simo.
On Thu, Sep 12, 2013 at 09:40:57AM -0400, Simo Sorce wrote:
On Thu, 2013-09-12 at 09:05 +0200, Lukas Slebodnik wrote:
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 ?
I am not hugely comfortable with pushing it to existing releases.
Simo.
The main problem is that existing releases could crash in some rare situations There is a report in bugzilla one week old. https://bugzilla.redhat.com/show_bug.cgi?id=997406#c12
User reported crash and he had latest sssd 1-10 with all mmap_cache patches; but it didn't help. sssd_nss crashed.
We had a long discussion about solution (revalidation of chains), but we did not find any simple "back-compatible" fix.
How do we want to fix it in older branches If we don't want to backport patch from previous mail.
The problem is that a database format change is not to be taken lightly as it kills completely the use of the mmap cache until all long lived processes are restarted. If we can detect the problem I would rather nuke the caches. Deletions are rare, they happen only when a user disappear IIRC, so if we have a simple method of detecting when a deletion may cause issue we can simply reset the mmap cache then it would have less dramatic effects I think. But I am open to other ideas.
Simo.
Hm, perhaps for this problem we could hack up something like Michal's previous patch to detect the problem and cp the corrupt database for dissection?
On Thu, 2013-09-12 at 16:51 +0200, Jakub Hrozek wrote:
On Thu, Sep 12, 2013 at 09:40:57AM -0400, Simo Sorce wrote:
On Thu, 2013-09-12 at 09:05 +0200, Lukas Slebodnik wrote:
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 ?
I am not hugely comfortable with pushing it to existing releases.
Simo.
The main problem is that existing releases could crash in some rare situations There is a report in bugzilla one week old. https://bugzilla.redhat.com/show_bug.cgi?id=997406#c12
User reported crash and he had latest sssd 1-10 with all mmap_cache patches; but it didn't help. sssd_nss crashed.
We had a long discussion about solution (revalidation of chains), but we did not find any simple "back-compatible" fix.
How do we want to fix it in older branches If we don't want to backport patch from previous mail.
The problem is that a database format change is not to be taken lightly as it kills completely the use of the mmap cache until all long lived processes are restarted. If we can detect the problem I would rather nuke the caches. Deletions are rare, they happen only when a user disappear IIRC, so if we have a simple method of detecting when a deletion may cause issue we can simply reset the mmap cache then it would have less dramatic effects I think. But I am open to other ideas.
Simo.
Hm, perhaps for this problem we could hack up something like Michal's previous patch to detect the problem and cp the corrupt database for dissection?
As long as we can reliably detect the problem without returning wrong answers :)
The best would be to detect on the server side that the operation of removing the record will probably cause issues and just reset the whole ccache, a bit drastic indeed but better than having a corrupt chain.
Simo.
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@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Thu, Sep 12, 2013 at 06:35:05PM +0200, Lukas Slebodnik wrote:
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 would prefer to have Lukas' patch in RHEL-7.
With upstream 1.11 and Fedora packages, I'd be fine with the workaround if there are concerns about backwards compatibility.
On 09/12/2013 12:40 PM, Jakub Hrozek wrote:
On Thu, Sep 12, 2013 at 06:35:05PM +0200, Lukas Slebodnik wrote:
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 would prefer to have Lukas' patch in RHEL-7.
With upstream 1.11 and Fedora packages, I'd be fine with the workaround if there are concerns about backwards compatibility.
I know that we want to backward compatible and changes that cause people to reboot their servers are not best but in reality do you think there are a lot of people who do updates to productions servers and not reboot after update just in case while there are in the maintenance window?
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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 ?
I am not hugely comfortable with pushing it to existing releases.
Simo.
I found litle mistakes in function sss_mc_is_valid_rec. I used wrong hash for finding next record in chain, and thus function sss_mc_is_valid_rec considered record for invalid in some cases.
Here is an interdiff.
diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c index 7e33b70..af23e23 100644 --- a/src/responder/nss/nsssrv_mmap_cache.c +++ b/src/responder/nss/nsssrv_mmap_cache.c @@ -513,7 +513,7 @@ static bool sss_mc_is_valid_rec(struct sss_mc_ctx *mcc, struct sss_mc_rec *rec) slot = mcc->hash_table[rec->hash1]; while (slot != MC_INVALID_VAL32 && self != rec) { self = MC_SLOT_TO_PTR(mcc->data_table, slot, struct sss_mc_rec); - slot = sss_mc_next_slot_with_hash(self, self->hash1); + slot = sss_mc_next_slot_with_hash(self, rec->hash1); } if (self != rec) { return false; @@ -524,7 +524,7 @@ static bool sss_mc_is_valid_rec(struct sss_mc_ctx *mcc, struct sss_mc_rec *rec) slot = mcc->hash_table[rec->hash2]; while (slot != MC_INVALID_VAL32 && self != rec) { self = MC_SLOT_TO_PTR(mcc->data_table, slot, struct sss_mc_rec); - slot = sss_mc_next_slot_with_hash(self, self->hash2); + slot = sss_mc_next_slot_with_hash(self, rec->hash2); } if (self != rec) { return false;
Patches are attached.
LS
On Mon, Sep 16, 2013 at 06:22:48PM +0200, Lukas Slebodnik wrote:
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 ?
I am not hugely comfortable with pushing it to existing releases.
Simo.
I found litle mistakes in function sss_mc_is_valid_rec. I used wrong hash for finding next record in chain, and thus function sss_mc_is_valid_rec considered record for invalid in some cases.
Here is an interdiff.
diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c index 7e33b70..af23e23 100644 --- a/src/responder/nss/nsssrv_mmap_cache.c +++ b/src/responder/nss/nsssrv_mmap_cache.c @@ -513,7 +513,7 @@ static bool sss_mc_is_valid_rec(struct sss_mc_ctx *mcc, struct sss_mc_rec *rec) slot = mcc->hash_table[rec->hash1]; while (slot != MC_INVALID_VAL32 && self != rec) { self = MC_SLOT_TO_PTR(mcc->data_table, slot, struct sss_mc_rec);
slot = sss_mc_next_slot_with_hash(self, self->hash1);
slot = sss_mc_next_slot_with_hash(self, rec->hash1); } if (self != rec) { return false;@@ -524,7 +524,7 @@ static bool sss_mc_is_valid_rec(struct sss_mc_ctx *mcc, struct sss_mc_rec *rec) slot = mcc->hash_table[rec->hash2]; while (slot != MC_INVALID_VAL32 && self != rec) { self = MC_SLOT_TO_PTR(mcc->data_table, slot, struct sss_mc_rec);
slot = sss_mc_next_slot_with_hash(self, self->hash2);
slot = sss_mc_next_slot_with_hash(self, rec->hash2); } if (self != rec) { return false;Patches are attached.
LS
I think the current patches are ok so ACK from me. I'm not the biggest memcache expert around, so I'd like to get a second opinion before pushing these to master (and Michal's patches to stable branches)
On Sun, Sep 22, 2013 at 05:44:57PM +0200, Jakub Hrozek wrote:
On Mon, Sep 16, 2013 at 06:22:48PM +0200, Lukas Slebodnik wrote:
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 ?
I am not hugely comfortable with pushing it to existing releases.
Simo.
I found litle mistakes in function sss_mc_is_valid_rec. I used wrong hash for finding next record in chain, and thus function sss_mc_is_valid_rec considered record for invalid in some cases.
Here is an interdiff.
diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c index 7e33b70..af23e23 100644 --- a/src/responder/nss/nsssrv_mmap_cache.c +++ b/src/responder/nss/nsssrv_mmap_cache.c @@ -513,7 +513,7 @@ static bool sss_mc_is_valid_rec(struct sss_mc_ctx *mcc, struct sss_mc_rec *rec) slot = mcc->hash_table[rec->hash1]; while (slot != MC_INVALID_VAL32 && self != rec) { self = MC_SLOT_TO_PTR(mcc->data_table, slot, struct sss_mc_rec);
slot = sss_mc_next_slot_with_hash(self, self->hash1);
slot = sss_mc_next_slot_with_hash(self, rec->hash1); } if (self != rec) { return false;@@ -524,7 +524,7 @@ static bool sss_mc_is_valid_rec(struct sss_mc_ctx *mcc, struct sss_mc_rec *rec) slot = mcc->hash_table[rec->hash2]; while (slot != MC_INVALID_VAL32 && self != rec) { self = MC_SLOT_TO_PTR(mcc->data_table, slot, struct sss_mc_rec);
slot = sss_mc_next_slot_with_hash(self, self->hash2);
slot = sss_mc_next_slot_with_hash(self, rec->hash2); } if (self != rec) { return false;Patches are attached.
LS
I think the current patches are ok so ACK from me. I'm not the biggest memcache expert around, so I'd like to get a second opinion before pushing these to master (and Michal's patches to stable branches)
Pushed both patches to master.
sssd-devel@lists.fedorahosted.org
-
Dmitri Pal -
Jakub Hrozek -
Lukas Slebodnik -
Michal Židek -
Simo Sorce