Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use usleep() instead of sched_yield() to yield cpu #13183

Merged
merged 1 commit into from Apr 7, 2024

Conversation

sundb
Copy link
Collaborator

@sundb sundb commented Apr 1, 2024

when the main thread and the module thread are in the same thread, sched_yield() can work well.
when they are both bind to different cpus, sched_yield() will look for the thread with the highest priority, and if the module thread is always the highest priority on a cpu, it will take a long time to let the main thread to reacquire the GIL.

ref https://man7.org/linux/man-pages/man2/sched_yield.2.html

If the calling thread is the only thread in the highest priority
list at that time, it will continue to run after a call to
sched_yield().

@oranagra
Copy link
Member

oranagra commented Apr 1, 2024

if the module thread is always the highest priority on a cpu, it will take a long time to let the main thread to reacquire the GIL.

but the module thread is yielding, so won't it let the lower priority thread get the CPU?

@oranagra
Copy link
Member

oranagra commented Apr 1, 2024

@DvirDukhan @MeirShpilraien i remember some discussion about Redis Search around this topic.

@sundb
Copy link
Collaborator Author

sundb commented Apr 1, 2024

@oranagra from the document, if the module thread and the main thread do not use the same cpu, the module thread will always be the highest priority thread, and sched_yield() will always be the NOP.
i tested in my local pc, when i use taskset -c 0, the blockedclient test passed quickly, otherwise it may timeout.

If the calling thread is the only thread in the highest priority
       list at that time, it will continue to run after a call to
       sched_yield().

Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. approved.
i'd still like to get a reassurance from the search team if possible

@lipzhu
Copy link

lipzhu commented Apr 2, 2024

Suggest using nanosleep instead of usleep.

  1. 4.3BSD, POSIX.1-2001. POSIX.1-2001 declares it obsolete,
    suggesting nanosleep(2) instead.
  2. usleep is removed in POSIX.1-2008.
  3. nanosleep makes the task of resuming a sleep that is interrupted by a signal handler easier.

@tezc
Copy link
Collaborator

tezc commented Apr 3, 2024

@sundb Don't know the surrounding code, so just looking at the change. Are you expecting usleep(0) to have some sort of yield effect? Documentation states it is noop:

If the value of useconds is 0, then the call has no effect

https://pubs.opengroup.org/onlinepubs/7908799/xsh/usleep.html

@sundb
Copy link
Collaborator Author

sundb commented Apr 3, 2024

@tezc haha, there has always been ambiguity about whether or not to use usleep(0) to yield cpu, but it does get used a lot.
IIRC, i've even seen usleep(0) used in the implement of sched_yield().
more safely perhaps we could use usleep(1).

@sundb
Copy link
Collaborator Author

sundb commented Apr 3, 2024

@lipzhu thanks for figue out.
nanosleep() does guarantee more precise timing, but it's also more cumbersome (need more lines of code), and in this case we're just trying to get the thread to yield the cpu, and it feels like usleep(0) is sufficient.

@tezc
Copy link
Collaborator

tezc commented Apr 3, 2024

@sundb I think that behavior is not guaranteed with usleep(0). Though, I'm just trying on my local, usleep(0) goes to sleep indeed. Sleep resolution is 50 microseconds on Linux. On top of that, it spends 4-5 microseconds for rescheduling the thread. So, usleep(0) has ~55 microseconds overhead. Just fyi, if performance is a concern here.

@sundb
Copy link
Collaborator Author

sundb commented Apr 3, 2024

@tezc usleep(0) will be exponentially slower than sched_yield(), but sched_yield() can't satisfied our demand, and it doesn't guarantee that the current thread will go to sleep to let the main thread a chance to get GIL.
Since RM_Yield() is not a frequently api (unlike testing), and it's generally the case that the module is processing a complex task to give the main thread a chance to work on other tasks, i think 50 microseconds isn't going to be a bottleneck.

Copy link
Collaborator

@MeirShpilraien MeirShpilraien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC we also saw the same thing on RediSearch once and also changed to usleep.

@oranagra
Copy link
Member

oranagra commented Apr 4, 2024

@MeirShpilraien i remember an issue with RM_Yield and trying to give up the GIL. calling usleep separately wouldn't have helped (unless maybe you released the mutex before?)

@MeirShpilraien
Copy link
Collaborator

@oranagra I am not talking about RM_Yield, I am talking about some other case where we use sched_yield and it did not work as expected so we change it to usleep:
RediSearch/RediSearch#3628

@oranagra
Copy link
Member

oranagra commented Apr 4, 2024

i see there was a combination of both usleep and yield.
@alonre24 do you remember if that was just an attempt to be conservative or did you get any severe impact from the sleep?

@alonre24
Copy link
Contributor

alonre24 commented Apr 4, 2024

i see there was a combination of both usleep and yield. @alonre24 do you remember if that was just an attempt to be conservative or did you get any severe impact from the sleep?

We saw that if we call usleep(1) after every iteration, it may have an accumulated impact on performance for large indexes with millions of documents. So we added the configuration that allows us to sleep every X iterations as a compromise, given that we could not guarantee that the main thread will get the CPU with sched_yield (or usleep(0)).

@oranagra
Copy link
Member

oranagra commented Apr 4, 2024

ok, in our case we have ctx->next_yield_time = now + 1000000 / server.hz;, so i don't think that's we have that problem of calling sleep too often.

@sundb
Copy link
Collaborator Author

sundb commented Apr 4, 2024

this is a test to verify sched_yield() and usleep() when the main thread and thread bindings are on different CPU.
when the thread is bound to a isolated CPU and has a high priority(i let it run a complex calculation), the main thread
can't grap the CPU forever.

#define _GNU_SOURCE
#include <pthread.h>
#include <stdio.h>
#include <stdlib.h>
#include <sched.h>
#include <unistd.h>

pthread_mutex_t mutex;
int thread_got_lock = 0;

int complexCalculation(void) {
    volatile int result = 0;
    for (int i = 0; i < 10000000; ++i) {
        result += i;
    }
    return result;
}

void *thread_func(void *arg) {
    pthread_mutex_lock(&mutex);
    thread_got_lock = 1;
    while (1) {
        complexCalculation();
        pthread_mutex_unlock(&mutex);
        sched_yield();
        // usleep(0);
        pthread_mutex_lock(&mutex);
    }
    return NULL;
}

int main() {
    int cpu_main = 0; /* bind cup 0 for main thread */
    int cpu_thread = 31; /* bind cup 31 for thread */

    /* set the cpu affinity of the main thread to 0 */
    cpu_set_t cpuset_main;
    CPU_ZERO(&cpuset_main);
    CPU_SET(cpu_main, &cpuset_main);
    if (pthread_setaffinity_np(pthread_self(), sizeof(cpu_set_t), &cpuset_main) != 0) {
        fprintf(stderr, "Error setting main thread affinity\n");
        return 1;
    }

    pthread_t thread;
    if (pthread_create(&thread, NULL, thread_func, NULL) != 0) {
        fprintf(stderr, "Error creating thread\n");
        return 1;
    }
    /* set the cpu affinity of the thread to 31 */
    cpu_set_t cpuset_thread;
    CPU_ZERO(&cpuset_thread);
    CPU_SET(cpu_thread, &cpuset_thread);
    if (pthread_setaffinity_np(thread, sizeof(cpu_set_t), &cpuset_thread) != 0) {
        fprintf(stderr, "Error setting thread affinity\n");
        return 1;
    }

    while (!thread_got_lock) {
        usleep(0);
    }
    printf("thread has acquired the lock\n");
    pthread_mutex_lock(&mutex);

    return 0;
}

@oranagra
Copy link
Member

oranagra commented Apr 7, 2024

what do you mean? that both sched_yield and usleep(0) aren't able to let the main thread grab the mutex?

@sundb
Copy link
Collaborator Author

sundb commented Apr 7, 2024

what do you mean? that both sched_yield and usleep(0) aren't able to let the main thread grab the mutex?

in my tests sched_yield() doesn't let the main thread to get the lock, but usleep(0) does.

@oranagra
Copy link
Member

oranagra commented Apr 7, 2024

ok, then merge away!
see if the top comment needs any refresh.

@sundb
Copy link
Collaborator Author

sundb commented Apr 7, 2024

@oranagra It's okay now.

@sundb sundb merged commit f4481e6 into redis:unstable Apr 7, 2024
14 checks passed
@sundb sundb deleted the yield_with_usleep branch April 7, 2024 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants