Issue 5598 - In 2.x, SRCH throughput drops by 10% because of handling… #5604

tbordaz · 2023-01-13T12:36:23Z

… of referral

Bug description:
A part of the fix #5170 append '(objectclass=referral)' to
the original filter (in subtree scope) in order to conform
smart referral support https://www.ietf.org/rfc/rfc3296.txt
This triggers a drop on SRCH throughput (10%).
#5598 limits the case when '(objectclass=referral)' is added
- Most of the time a server does not contain smart referral.
So most of the time it is useless to add that subfilter
- It should not be added for internal searches

Fix description:
A mechanism periodically (each 30s) checks if there are
smart referral entries (referral_check).
When a first smart referral is detected or no more smart
referral is detected it logs a information message.
During a direct subtree search, 'objectclass=referral' is
append at the condition it exists at least a referral

relates: #5598

Reviewed by:

mreynolds389 · 2023-01-13T13:32:39Z

ldap/servers/slapd/backend.c

+int
+slapi_exist_referrals(void)
+{
+    return exist_referrals;


Do we need a lock here? I also recall that "exist_referrals" should be a static uint64_t in this case (something to do safety but I don't remember the details anymore)

Thanks for the review Mark. I think it does not need a lock. The update is periodic so if the caller reads an outdated value it is not a big deal.
Regarding the size, it looks a good point. Indeed the cache line will be in competition between workers, I will do test with cache line alignment.

Performance wise, there is no benefit when making it uint64_t and aligning it on cache size

It's probably not needing a lock, but an atomic to set it with RELEASE and to check the exist with ACQUIRE is correct.

Although using lock or atomic operation as described by @Firstyear is the standard way to set and check flags in different threads, in this specific case I agree with Thierry: it is an overkill.
We do not really care of the timing (as @Firstyear said in another comment we could check if the flag once every few minutes (so if the change stay in the cpu pipe line until next context switch, does not really matter)

progier389 · 2023-01-13T13:59:50Z

I suspect that we also have a regression on search rate result, so why optimizing only the internal search ?
...
Ok forgot that one, I misread the test: the subfilter is not added in internal operation
(which make sense as most plugin do not handle the referrals anyway and anyway they can add the subfilter explicitly if they want the referrals)

progier389 · 2023-01-13T14:01:55Z

Maybe we should rather have one flag per backend:
It is not more costly and it avoid having a referral one one backend impacting all the other backend

mreynolds389 · 2023-01-13T15:30:20Z

Several tests are crashing the server. Not sure if it's related to this PR or not.

progier389 · 2023-01-13T17:35:35Z

Maybe we should rather have one flag per backend:
It is not more costly and it avoid having a referral one one backend impacting all the other backend

About your valid remark about nested suffix: IMHO we should keep things simple in the search request and just look at the current backend flags but in the task setting the flags if we set the flags to true we should also walk the parent suffixes and also set their flag.

Firstyear · 2023-01-16T00:36:23Z

ldap/servers/slapd/backend.c

+    /* loop over the backends to check if a it exists a "smart" referral */
+    filter = "(objectclass=referral)";
+    be = slapi_get_first_backend(&cookie);
+    while (!exist_referral && be) {


The problem with this logic is that if a referral is added then removed, then the check won't run and we'll continue to assume referrals exist.

Not sure to understand your point.
ATM the fix set the flag per server, not per backend. So if a referral exists somewhere, then it stops iterating over the others backends and set the flag.
Now I will rework the fix to make it per backend as @progier389 suggested.

The logic is ok, but I suspect that @Firstyear was confused by the way you named the variables:

exist_referral for the local variable

exist_referrals for the global one
And I also found this choice a bit confusing, IMHO you should have used more different names

Right, it was confusing. I reworked the patch with a per suffix flag. So we no longer have a global 'exist_referrals.
Note that the new patch also suffers from non protected access to the backend flag. I think it is not a big deal even if we miss that flag setting as it will be set again few sec after.

Firstyear · 2023-01-16T00:37:15Z

ldap/servers/slapd/backend.c

+int
+slapi_exist_referrals(void)
+{
+    return exist_referrals;


It's probably not needing a lock, but an atomic to set it with RELEASE and to check the exist with ACQUIRE is correct.

Firstyear · 2023-01-16T00:38:01Z

ldap/servers/slapd/backend.c

+    referral_check_ctx = slapi_eq_repeat_rel(referral_check,
+                                             NULL, (time_t)0,
+                                             /* 30 sec in milliseconds */
+                                             30 * 1000);


I'd even just make this 60 seconds or 5 minutes, I don't think this needs to be checked so frequently.

tbordaz · 2023-02-03T12:49:39Z

@Firstyear , I changed the logic of referral_check to be per backend. If a backend contains a smart referral, it sets a flag. That flag is tested on subtree search (slapi_exist_referrals). Is it addressing your requested changes ?

progier389 · 2023-02-03T14:39:47Z

I wonder if we should not move the check of existing referral for a backend in a separate function
and also call it just after enabling a backend. ( So we will get a more predictive behavior after an import for example )

mreynolds389 · 2023-02-16T00:39:44Z

@Firstyear - Thierry make some changes for you, could you please review them? Thanks!!

Firstyear · 2023-02-16T02:20:02Z

Sorry about the review delay :)

tbordaz · 2023-02-21T13:49:24Z

I wonder if we should not move the check of existing referral for a backend in a separate function and also call it just after enabling a backend. ( So we will get a more predictive behavior after an import for example )

@progier389 , the check of the referral is in function 'slapi_exist_referrals(be)'. Is it what you asked for ?

@tbordaz, No: slapi_exist_referrals check if the flag is set, I was speaking about a function that perform the internal search and set the flag. And this function should also be called when the backend is enabled.

… of referral Bug description: A part of the fix 389ds#5170 append '(objectclass=referral)' to the original filter (in subtree scope) in order to conform smart referral support https://www.ietf.org/rfc/rfc3296.txt This triggers a drop on SRCH throughput (10%). 389ds#5598 limits the case when '(objectclass=referral)' is added - Most of the time a server does not contain smart referral. So most of the time it is useless to add that subfilter - It should not be added for internal searches Fix description: A mechanism periodically (each 30s) checks if there are smart referral entries (referral_check) under each backends. Note that if a smart referral is present in a subsuffix, the parent suffix inherits the referral flag. When a smart referral is detected or no more smart referral is detected it logs a information message. During a direct subtree search, 'objectclass=referral' is append at the condition it exists at least a referral under the backend. relates: 389ds#5598 Reviewed by: Mark Reynolds, Pierre Rogier, William Brown (Thanks)

tbordaz · 2023-02-22T14:05:00Z

I made the change (internal search+flags) in slapi_exist_referral.

… of referral (#5604) Bug description: A part of the fix #5170 append '(objectclass=referral)' to the original filter (in subtree scope) in order to conform smart referral support https://www.ietf.org/rfc/rfc3296.txt This triggers a drop on SRCH throughput (10%). #5598 limits the case when '(objectclass=referral)' is added - Most of the time a server does not contain smart referral. So most of the time it is useless to add that subfilter - It should not be added for internal searches Fix description: A mechanism periodically (each 30s) checks if there are smart referral entries (referral_check) under each backends. Note that if a smart referral is present in a subsuffix, the parent suffix inherits the referral flag. When a smart referral is detected or no more smart referral is detected it logs a information message. During a direct subtree search, 'objectclass=referral' is append at the condition it exists at least a referral under the backend. relates: #5598 Reviewed by: Mark Reynolds, Pierre Rogier, William Brown (Thanks)

tbordaz added the work in progress Work in Progress - can be reviewed, but not ready for merge. label Jan 13, 2023

mreynolds389 reviewed Jan 13, 2023

View reviewed changes

Firstyear requested changes Jan 16, 2023

View reviewed changes

tbordaz force-pushed the issue_5598 branch from 45a4452 to 6107887 Compare January 17, 2023 09:00

tbordaz removed the work in progress Work in Progress - can be reviewed, but not ready for merge. label Jan 18, 2023

tbordaz force-pushed the issue_5598 branch from 6107887 to efeeecb Compare January 19, 2023 13:56

tbordaz force-pushed the issue_5598 branch from efeeecb to 72250b7 Compare February 1, 2023 10:12

Firstyear approved these changes Feb 16, 2023

View reviewed changes

tbordaz force-pushed the issue_5598 branch from 72250b7 to 56a41e0 Compare February 22, 2023 09:39

tbordaz force-pushed the issue_5598 branch from 56a41e0 to b3ac34f Compare February 22, 2023 10:52

tbordaz merged commit a4c4e4e into 389ds:main Feb 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 5598 - In 2.x, SRCH throughput drops by 10% because of handling… #5604

Issue 5598 - In 2.x, SRCH throughput drops by 10% because of handling… #5604

tbordaz commented Jan 13, 2023

mreynolds389 Jan 13, 2023

tbordaz Jan 13, 2023

tbordaz Jan 13, 2023

Firstyear Jan 16, 2023

progier389 Jan 16, 2023

progier389 commented Jan 13, 2023 •

edited

Loading

progier389 commented Jan 13, 2023

mreynolds389 commented Jan 13, 2023

progier389 commented Jan 13, 2023

Firstyear Jan 16, 2023

tbordaz Jan 18, 2023

progier389 Jan 18, 2023

tbordaz Jan 19, 2023

Firstyear Jan 16, 2023

Firstyear Jan 16, 2023

tbordaz commented Feb 3, 2023

progier389 commented Feb 3, 2023

mreynolds389 commented Feb 16, 2023

Firstyear commented Feb 16, 2023

tbordaz commented Feb 21, 2023 •

edited by progier389

Loading

tbordaz commented Feb 22, 2023

Issue 5598 - In 2.x, SRCH throughput drops by 10% because of handling… #5604

Issue 5598 - In 2.x, SRCH throughput drops by 10% because of handling… #5604

Conversation

tbordaz commented Jan 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

progier389 commented Jan 13, 2023 • edited Loading

progier389 commented Jan 13, 2023

mreynolds389 commented Jan 13, 2023

progier389 commented Jan 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbordaz commented Feb 3, 2023

progier389 commented Feb 3, 2023

mreynolds389 commented Feb 16, 2023

Firstyear commented Feb 16, 2023

tbordaz commented Feb 21, 2023 • edited by progier389 Loading

tbordaz commented Feb 22, 2023

progier389 commented Jan 13, 2023 •

edited

Loading

tbordaz commented Feb 21, 2023 •

edited by progier389

Loading