URL: https://github.com/SSSD/sssd/pull/999 Author: alexey-tikhonov Title: #999: Mem-cache fixes/improvements Action: opened
PR body: """ This is set of patches consisting of the following:
(1) fix of https://pagure.io/SSSD/sssd/issue/4160 (2) revival of @mzidek-gh's patch to make size of mem-caches configurable (#390) (3 & 4) slight improvements of (2) (5) additional debug&syslog message to signal user that cache is full and may require change of size This is to address https://github.com/SSSD/sssd/pull/390#issuecomment-371428333 (at least to some extent)
I keep those patches separate to make review easier. But if PR will be accepted then it makes sense to squash (2, 3 & 4) into single patch before merging. """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/999/head:pr999 git checkout pr999
URL: https://github.com/SSSD/sssd/pull/999 Title: #999: Mem-cache fixes/improvements
Label: +Waiting for review
URL: https://github.com/SSSD/sssd/pull/999 Title: #999: Mem-cache fixes/improvements
mzidek-gh commented: """ I checked the code of all the patches and it looks good to me. I have not tested the patches yet. I can do that (probably) tomorrow. """
See the full comment at https://github.com/SSSD/sssd/pull/999#issuecomment-595148224
URL: https://github.com/SSSD/sssd/pull/999 Title: #999: Mem-cache fixes/improvements
mzidek-gh commented: """ @sumit-bose Since I was partially involved in making of this PR, can I borrow your eyes for this code review? """
See the full comment at https://github.com/SSSD/sssd/pull/999#issuecomment-595150915
URL: https://github.com/SSSD/sssd/pull/999 Title: #999: Mem-cache fixes/improvements
sumit-bose commented: """ Hi,
in general I agree with the changes but I think the mem-cache size is not a good config option. The concept of slots is specific to how the cache is currently implemented and although I think this won't change I think the number of expected objects might be a better option. This would also help avoid one integer divisions in the code. For the other `/ 8` I would suggest to round up the data from the config file to the next integer which can be devided by 8 to be on the safe side.
If we have a tool to get statistics from the memory cache and this tool can show the average payload (i.e. how many slot a cached entry is using) we might consider to make the payload configurable as well, e.g. by enhancing the number of expected option so that the value can be `number_of_expected_objects:payload`. But should should be added alter.
bye, Sumit
"""
See the full comment at https://github.com/SSSD/sssd/pull/999#issuecomment-598169722
URL: https://github.com/SSSD/sssd/pull/999 Title: #999: Mem-cache fixes/improvements
alexey-tikhonov commented: """ Hi Sumit,
Thank you for the review.
in general I agree with the changes but I think the mem-cache size is not a good config option. The concept of slots is specific to how the cache is currently implemented
Right, this doesn't seem very neat. Initially I offered Michal (this specific patch is his) to configure size in megabytes, but he convinced me that from user perspective those are the same (more on this later).
For the other `/ 8` I would suggest to round up the data from the config file to the next integer which can be devided by 8 to be on the safe side.
I think this was already implemented in `sss_mmap_cache_init()`: https://github.com/SSSD/sssd/blob/3c10595659698c7d8976bcbed88b3696c222d86e/s...
Please let me know if you see any reason to move (or duplicate?) this protection or you think it is done wrong.
I think the number of expected objects might be a better option.
...
If we have a tool to get statistics from the memory cache and this tool can show the average payload (i.e. how many slot a cached entry is using) we might consider to make the payload configurable as well, e.g. by enhancing the number of expected option so that the value can be `number_of_expected_objects:payload`. But should should be added alter.
I understand your intention but IMO from user point of view this is unjustified over-complication. And most probably will not work well "in the field"...
My reasoning is as follows:
There is just no such thing as "average payload". What I typically meet in my experience, is that, for example, groups mem-cache may contain entries with members amount ranging from 1 to 30..60k(!) (this is typical for integration with very large AD - exactly the case when mem-cache is very important). Of course we can calculate average (or weighted average) size but this doesn't make any sense. It would rather do harm because space will be wasted for small entries and estimation will be wrong anyway. So while I understand your idea - "user knows size of own database in amount of entries => let them configure size of the cache in amount entries" - I don't think this is realistic approach. What I offer instead is perhaps very blunt but IMO should work well: "user got message in system log that mem-cache is overfill => increase size of the cache". This is less than ideal solution and still needs improvements (we should report cache overfill only if we have to erase *non expired* entries) but good in the sense of effort/result ration from my point of view (I mostly mean users here, not developers) From developers perspective, I would rather go in totally different way as opposite to "average payload" (btw, justification of those factors in the comments in the source code is very loose). I would prefer to get rid of fixed slot size completly (I must admit I didn't investigate performance impact yet but my gut feeling is it should be ok). But this would be quite large refactoring and intention of this PR - to resolve specific bug and to provide at least basic means to ajust cache for NSS load...
I hope this makes sense. Please let me know what you think.
"""
See the full comment at https://github.com/SSSD/sssd/pull/999#issuecomment-598694168
URL: https://github.com/SSSD/sssd/pull/999 Title: #999: Mem-cache fixes/improvements
sumit-bose commented: """ ...
For the other `/ 8` I would suggest to round up the data from the config file to the next integer which can be devided by 8 to be on the safe side.
I think this was already implemented in `sss_mmap_cache_init()`:
https://github.com/SSSD/sssd/blob/3c10595659698c7d8976bcbed88b3696c222d86e/s...
Please let me know if you see any reason to move (or duplicate?) this protection or you think it is done wrong.
You are right, I only looked at the patches and wasn't aware that it is already done.
I think the number of expected objects might be a better option. ... If we have a tool to get statistics from the memory cache and this tool can show the average payload (i.e. how many slot a cached entry is using) we might consider to make the payload configurable as well, e.g. by enhancing the number of expected option so that the value can be `number_of_expected_objects:payload`. But should should be added alter.
I understand your intention but IMO from user point of view this is unjustified over-complication. And most probably will not work well "in the field"...
My reasoning is as follows:
There is just no such thing as "average payload". What I typically meet in my experience, is that, for example, groups mem-cache may contain entries with members amount ranging from 1 to 30..60k(!) (this is typical for integration with very large AD - exactly the case when mem-cache is very important). Of course we can calculate average (or weighted average) size but this doesn't make any sense. It would rather do harm because space will be wasted for small entries and estimation will be wrong anyway. So while I understand your idea - "user knows size of own database in amount of entries => let them configure size of the cache in amount entries" - I don't think this is realistic approach. What I offer instead is perhaps very blunt but IMO should work well: "user got message in system log that mem-cache is overfill => increase size of the cache". This is less than ideal solution and still needs improvements (we should report cache overfill only if we have to erase _non expired_ entries) but good in the sense of effort/result ration from my point of view (I mostly mean users here, not developers)
I agree about the payload, the only purpose is to have some relation between the number of slots in the data table and the number of entries in the hash table and I think it is really not needed to make it configurable I was just saying that we can in case there will be a potential use-case. My main concern if the integer division, especially with 3 and 5. It should not do much harm because it is only the size of the hash table but I still think it is better to avoid it and using the number of entries as an option would make it a multiplication in the data table size.
From developers perspective, I would rather go in totally different way as opposite to "average payload" (btw, justification of those factors in the comments in the source code is very loose). I would prefer to get rid of fixed slot size completly (I must admit I didn't investigate performance impact yet but my gut feeling is it should be ok). But this would be quite large refactoring and intention of this PR - to resolve specific bug and to provide at least basic means to ajust cache for NSS load...
Originally the slot size was 32bytes and the fixed slot size together with all the memory alignment was use to make sure the a slot or 2 will fix into a CPU cache line. This was of course broken by extending the slot size to 40. But I think the original idea was good and in the long term it would be good to come back to 32bytes (or maybe 64bytes). I'm not sure if a flexible slot size would have much benefits.
If you prefer the number of slots as a config option I would suggest to make the factor between the data and the hash table size (aka "payload") either constantly 4 or even 1. The latter will of course increase the hash table size but with the positive side effect that there might be less collisions.
bye, Sumit
I hope this makes sense. Please let me know what you think.
"""
See the full comment at https://github.com/SSSD/sssd/pull/999#issuecomment-601231341
URL: https://github.com/SSSD/sssd/pull/999 Author: alexey-tikhonov Title: #999: Mem-cache fixes/improvements Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/999/head:pr999 git checkout pr999
URL: https://github.com/SSSD/sssd/pull/999 Title: #999: Mem-cache fixes/improvements
alexey-tikhonov commented: """
From developers perspective, I would rather go in totally different way as opposite to "average payload" (btw, justification of those factors in the comments in the source code is very loose). I would prefer to get rid of fixed slot size completly (I must admit I didn't investigate performance impact yet but my gut feeling is it should be ok). But this would be quite large refactoring and intention of this PR - to resolve specific bug and to provide at least basic means to ajust cache for NSS load...
Originally the slot size was 32bytes and the fixed slot size together with all the memory alignment was use to make sure the a slot or 2 will fix into a CPU cache line. This was of course broken by extending the slot size to 40.
As it was discussed on IRC, this is more about sizes and alignment of "sss_mc_rec" and "sss_mc_*_data" headers. I still think MC_SLOT_SIZE per se is only implicitly relevant.
But I think the original idea was good and in the long term it would be good to come back to 32bytes (or maybe 64bytes). I'm not sure if a flexible slot size would have much benefits.
Benefits would be: (1) code simplification; (2) less memory waste. But I agree performance impact should be measured.
But this perhaps means it is better to configure size of cache in megabytes, not in slots (to avoid change of user visible semantic later)...
If you prefer the number of slots as a config option I would suggest to make the factor between the data and the hash table size (aka "payload") either constantly 4 or even 1. The latter will of course increase the hash table size but with the positive side effect that there might be less collisions.
I changed the patch to use constant factor and opted to 2 (because every entry takes 2 slots at the very least).
Please, take a look at this version. """
See the full comment at https://github.com/SSSD/sssd/pull/999#issuecomment-601770502
URL: https://github.com/SSSD/sssd/pull/999 Title: #999: Mem-cache fixes/improvements
sumit-bose commented: """ Hi,
thanks for the latest version. I agree with the changes. But while testing I realized that I see the mem cache full log message not the at the first time the cache was full. I think this is caused by an older issue where `mcc->next-slot` is not updated while the cache is filled for the first time. I'd like to suggest to include a patch similar to the following in your patch set:
``` diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c index 18f0c6c..d639cd0 100644 --- a/src/responder/nss/nsssrv_mmap_cache.c +++ b/src/responder/nss/nsssrv_mmap_cache.c @@ -441,6 +441,7 @@ static errno_t sss_mc_find_free_slots(struct sss_mc_ctx *mcc, } if (cur == t) { /* ok found num_slots consecutive free bits */ + mcc->next_slot = cur; *free_slot = cur - num_slots; return EOK; }
```
bye, Sumit """
See the full comment at https://github.com/SSSD/sssd/pull/999#issuecomment-603881327
URL: https://github.com/SSSD/sssd/pull/999 Title: #999: Mem-cache fixes/improvements
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/999 Title: #999: Mem-cache fixes/improvements
Label: -Waiting for review
URL: https://github.com/SSSD/sssd/pull/999 Title: #999: Mem-cache fixes/improvements
alexey-tikhonov commented: """
But while testing I realized that I see the mem cache full log message not the at the first time the cache was full. I think this is caused by an older issue where `mcc->next-slot` is not updated while the cache is filled for the first time.
Thank you for the great catch.
Well, I wanted to leave this for a separate PR, but ...
You are right that this is a bug in the original code. And while being quite harmless in the original implementation, this old bug indeed creates issue in the context of my patches.
I tend to agree that additional patch proposed by you would fix this issue.
But since we are touching `sss_mc_find_free_slots()` anyway, I am going to incorporate in this patches what I was going to propose as a separate PR. """
See the full comment at https://github.com/SSSD/sssd/pull/999#issuecomment-606083747
URL: https://github.com/SSSD/sssd/pull/999 Title: #999: Mem-cache fixes/improvements
alexey-tikhonov commented: """
But while testing I realized that I see the mem cache full log message not the at the first time the cache was full. I think this is caused by an older issue where `mcc->next-slot` is not updated while the cache is filled for the first time.
...
You are right that this is a bug in the original code. And while being quite harmless in the original implementation, this old bug indeed creates issue in the context of my patches.
This is not that straightforward, actually.
The [doc](https://docs.pagure.org/SSSD.sssd/developers/mmap_cache_1.15.html#how-does-t...) doesn't describe clearly what is actually implemented on that part, so it is hard to say if this was done intentionally or by coincidence, but missed update of `sss_mc_ctx::next_slot` (in case slots were found via explicit lookup of free space) can be treated as a kind of "feature"...
Let me explain.
Initially we start with an empty buffer and fill it consequently without any gaps (actually that's not always the case, but more on this later (*)). ``` |R1|R1|R2|R2|R3|R3|...|Rn|Rn|Rn|0|0|...L slots...|0| ``` Until we get to a situation when we need to store record with sizeof(R(n+1)) > L At this point it doesn't matter if `next_slot` was actually tracking current "tail": in either way we will end up storing R(n+1) in the beginning of the buffer: ``` |R(n+1)|R(n+1)|R(n+1)|R(n+1)|R(n+1)|0|...|Rn|Rn|Rn|0|0|...L slots...|0| ``` Now imagine sizeof(R(n+2)) < L so we store it in a free space at the of the buffer: ``` |R(n+1)|R(n+1)|R(n+1)|R(n+1)|R(n+1)|0|...|Rn|Rn|Rn|R(n+2)|R(n+2)|0|...L2...|0| ``` And let sizeof(R(n+3))>L2. This is the point where it becomes important if `next_slot` was updated on a previous step. If we fix the "bug" and `next_slot` was updated, we end up rewriting "fresh" R(n+1). If `next_slot` was not updated (current implementation), we rewrite "oldest" record after R(n+1) The latter makes more sense (most probably only coincidentally), but frankly speaking implementation is clumsy / inefficient and imo "profit/complexity" trade off is not the best.
At this point I am very tempted to say that we should follow [this comment](https://github.com/SSSD/sssd/blob/f3e89aa02ca9c10ab2ac456ca643b3cf5edb44a0/s...) and completely get rid of attempt to lookup free space. This would result in under utilization of the space in the buffer. For the end of the buffer this should not be a problem as in general sizeof(any single record) << sizeof(mem-cache) Another potential source of the "gap" (*) is update of the record that changed its size. This also should be very rare case.
@sumit-bose, what would you say? """
See the full comment at https://github.com/SSSD/sssd/pull/999#issuecomment-606657210
URL: https://github.com/SSSD/sssd/pull/999 Author: alexey-tikhonov Title: #999: Mem-cache fixes/improvements Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/999/head:pr999 git checkout pr999
URL: https://github.com/SSSD/sssd/pull/999 Author: alexey-tikhonov Title: #999: Mem-cache fixes/improvements Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/999/head:pr999 git checkout pr999
URL: https://github.com/SSSD/sssd/pull/999 Title: #999: Mem-cache fixes/improvements
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/999 Title: #999: Mem-cache fixes/improvements
Label: +Waiting for review
URL: https://github.com/SSSD/sssd/pull/999 Title: #999: Mem-cache fixes/improvements
pbrezina commented: """ @sumit-bose bump """
See the full comment at https://github.com/SSSD/sssd/pull/999#issuecomment-617088005
sssd-devel@lists.fedorahosted.org
-
alexey-tikhonov -
mzidek-gh
-
pbrezina
-
sumit-bose