Issue 6010 - 389 ds ignores nsslapd-maxdescriptors #6027
Conversation
| :expectedresults: | ||
| 1. Success | ||
| 2. Value of SYSTEMD_LIMIT is returned | ||
| 2. Success |
There was a problem hiding this comment.
Validation GitHub CI test fails here (the repeated number).
|
Good catch @droideck ! test_conn_limits sets a small value for the max number of file descriptors and try to fill up the connection table, so it is quite probable that it need to be adapted ... |
|
I had a patch for the two issues mentioned here, but didn't get a chance to push till now. |
| @@ -122,7 +122,8 @@ connection_table_new(int table_size) | |||
| ct->list_num = config_get_num_listeners(); | |||
| ct->num_active = (int *)slapi_ch_calloc(1, ct->list_num * sizeof(int)); | |||
| ct->size = table_size - (table_size % ct->list_num); | |||
| ct->list_size = ct->size/ct->list_num; | |||
| /* Account for first slot of each list being used as head (c_next). */ | |||
| ct->list_size = (ct->size/ct->list_num)+1; | |||
There was a problem hiding this comment.
There was a bug when we were mapping the connection table lists to the freelist, as slot 0 of each ct list is used as head of the linked list it was skipped, resulting in each list being 1 less than it should be.
This became apparent when the freelist offset is used to determine if the server has room for a new connection.
| @@ -831,7 +830,7 @@ accept_thread(void *vports) | |||
|
|
|||
| while (!g_get_shutdown()) { | |||
| /* Do we need to accept new connections? */ | |||
| int accept_new_connections = ((ct->size - g_get_current_conn_count()) > slapdFrontendConfig->reservedescriptors); | |||
| int accept_new_connections = (ct->size > ct->conn_next_offset); | |||
There was a problem hiding this comment.
IMHO a better way for the accept_thread to know if there is "room" for a new connection is to use the freelist offset. A downside of using the freelist offset is that it is protected by a lock, but I didn't want to lock and unlock in the accept_thread loop.
Apart from the accept_thread, the only other user of the freelist offset are the connection table list threads, where they move connections back onto the freelist, decrementing the offset. So in a worse case scenario the offset can be reduced by a max of 1, meaning a single connection slot could be unused, IMHO this is better than adding the overhead of locking/unlocking here. WDYT
There was a problem hiding this comment.
IIUC I agree with your proposal. However a side effect when connection table is close to be full is that we may erroneously believe the CT is full AND wait for 250ms before accepting a new connection.
In case we "believe" there is no room, (ct->size <= ct->conn_next_offset), we may double check the value of conn_next_offset (holding the lock) before sleeping. Most of the time we will not have to double check, so it will benefit from the fix and the lock payload is only when CT is almost full.
| @@ -2083,7 +2082,7 @@ unfurl_banners(Connection_Table *ct, daemon_ports_t *ports, PRFileDesc **n_tcps, | |||
| char addrbuf[256]; | |||
| int isfirsttime = 1; | |||
|
|
|||
| if (ct->size <= slapdFrontendConfig->reservedescriptors) { | |||
| if (ct->size > (slapdFrontendConfig->maxdescriptors - slapdFrontendConfig->reservedescriptors)) { | |||
There was a problem hiding this comment.
This code is hit on startup, so we can only use the static descriptor values
There was a problem hiding this comment.
Looks good, and the tests pass!
If @progier389 also satisfied with the new changes, I think we are good to go!
Bug description: During server startup the connection table size is assumed to be lower than or equal to the number of configured reserve file descriptors. This prevents the server from starting whem the number of reserve descriptors is high. Fix description: Change the check to make sure the connection table size is not greater than (max descriptors - reserve descriptors). Also, the number of reserve descriptors is used to determine if the server can accept a new connection. This has been changed to compare the connection table size against the current number of connections. Relates: 389ds#6010 Reviewed by:
jchapma
force-pushed
the
ds-ignore-max-desc
branch
from
9e6c307 to
830fbba
Compare
Bug description: During server startup the connection table size is assumed to be lower than or equal to the number of configured reserve file descriptors. This prevents the server from starting whem the number of reserve descriptors is high. Fix description: Change the check to make sure the connection table size is not greater than (max descriptors - reserve descriptors). Also, the number of reserve descriptors is used to determine if the server can accept a new connection. This has been changed to compare the connection table size against the current number of connections. Relates: #6010 Reviewed by: @progier389, @droideck, @tbordaz (Thank you)
Bug description: During server startup the connection table size is assumed to be lower than or equal to the number of configured reserve file descriptors. This prevents the server from starting whem the number of reserve descriptors is high. Fix description: Change the check to make sure the connection table size is not greater than (max descriptors - reserve descriptors). Also, the number of reserve descriptors is used to determine if the server can accept a new connection. This has been changed to compare the connection table size against the current number of connections. Relates: #6010 Reviewed by: @progier389, @droideck, @tbordaz (Thank you)
Bug description: During server startup the connection table size is assumed to be lower than or equal to the number of configured reserve file descriptors. This prevents the server from starting whem the number of reserve descriptors is high. Fix description: Change the check to make sure the connection table size is not greater than (max descriptors - reserve descriptors). Also, the number of reserve descriptors is used to determine if the server can accept a new connection. This has been changed to compare the connection table size against the current number of connections. Relates: #6010 Reviewed by: @progier389, @droideck, @tbordaz (Thank you)
Bug description: During server startup the connection table size is assumed to be lower than or equal to the number of configured reserve file descriptors. This prevents the server from starting whem the number of reserve descriptors is high. Fix description: Change the check to make sure the connection table size is not greater than (max descriptors - reserve descriptors). Also, the number of reserve descriptors is used to determine if the server can accept a new connection. This has been changed to compare the connection table size against the current number of connections. Relates: #6010 Reviewed by: @progier389, @droideck, @tbordaz (Thank you)
Bug description: During server startup the connection table size is assumed to be lower than or equal to the number of configured reserve file descriptors. This prevents the server from starting whem the number of reserve descriptors is high.
Fix description: Change the check to make sure the connection table size is not greater than (max descriptors - reserve descriptors).
Also, the number of reserve descriptors is used to determine if the server can accept a new connection. This has been changed to compare the connection table size against the current number of connections.
Relates: #6010
Reviewed by: