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

#1792 Centralized the use of pthread_create and thread_setname in the create_thread_with_name function that uses thread_wrapper for compatibility across platforms and with Linux GLIBC<2.12 #1022

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fabiodepin
Copy link

  • Adjusted the thread_setname function for compatibility across platforms and with Linux GLIBC<2.12;
  • Centralized the use of pthread_create and thread_setname in the create_thread_with_name function that uses thread_wrapper for compatibility;

- Adjusted the thread_setname function for compatibility across platforms and with Linux GLIBC<2.12;
- Centralized the use of pthread_create and thread_setname in the create_thread_with_name function that uses thread_wrapper for compatibility;
@dormando
Copy link
Member

dormando commented Apr 5, 2023

Thanks! There was another similar PR in the queue, not sure if you saw it.

I'm a little concerned about having to support systems older than 2010 though.

@fabiodepin
Copy link
Author

I hadn't seen PR #952.

I have now added in this second commit the part that matters regarding FreeBSD compatibility made by @devnexen, along with the changes I proposed.

#elif defined(__FreeBSD__)
pthread_set_name_np(thread, name);
#elif defined(__APPLE__) && defined(__MACH__)
pthread_setname_np(name);
Copy link
Contributor

Choose a reason for hiding this comment

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

does it work as expected on macOS ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes.
On MACOS, the function pthread_setname_np(const char*) sets the internal name of the calling thread to the string value specified by the name argument.
Type is done with the prctl.

Now, if the rest of the source compiled under MACOS, that's another story.

Copy link
Contributor

@devnexen devnexen Apr 6, 2023

Choose a reason for hiding this comment

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

Yes. On MACOS, the function pthread_setname_np(const char*) sets the internal name of the calling thread

Sure https://github.com/apple/darwin-libpthread/blob/main/src/pthread.c#L1140 but what I meant is Linux/FreeBSD allows to specify a specific thread rather than the calling one.

Now, if the rest of the source compiled under MACOS, that's another story.

🙂

Copy link
Member

Choose a reason for hiding this comment

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

It should build on recent MacOS: if it doesn't please report a bug.

Copy link
Author

Choose a reason for hiding this comment

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

Sure https://github.com/apple/darwin-libpthread/blob/main/src/pthread.c#L1140 but what I meant is Linux/FreeBSD allows to specify a specific thread rather than the calling one.

Yes, but as long as you have GLIBC >= 2.12. Por isso o tratamento acima usando prctl ou pthread_setname_np on Linux.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair point even tough glibc 2.12 is already fairly old.

@dormando
Copy link
Member

I'm a little scared to ask but; what exactly are you doing that requires a glibc from before 2010, but also a modern build of memcached?

I've been wanting to move the codebase to C11 (unions, initializers, atomics, etc) but things like this worry me.

@dormando
Copy link
Member

I'm a little scared to ask but; what exactly are you doing that requires a glibc from before 2010, but also a modern build of memcached?

I've been wanting to move the codebase to C11 (unions, initializers, atomics, etc) but things like this worry me.

@fabiodepin can you speak to this? I'm hesitant to open the gate for supporting something so old, but I'm open to hearing about why this is necessary.

I just recently acquired a cheap mac mini so I can validate this change once I've set that up.

@fabiodepin
Copy link
Author

In my case it was needed for use on older Debian servers, which cannot be upgraded to newer versions due to other software running on the server.
But I already have a second glibc that I can use with 'rpath', and other people who need it can use it like that, with a second glibc.

I can't say if more people need to use it or are using it on older systems. But for these cases, it is possible to install a second glibc on the system and use it separately with 'rpath'.

I support the change to C11, I even have some optimization suggestions that I have in notes to do at another time.

  • These changes proposed in this PR should not generate impact and should even facilitate this part related to the use of the setname, as it centralizes its use in a place that can be validated more easily.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants