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

fix the hang caused by pthread_cancel #564

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

Conversation

lijing0010
Copy link
Contributor

@lijing0010 lijing0010 commented Jul 30, 2020

Signed-off-by: Jing Li jing.b.li@intel.com

Description

pthread_cancel is an async call, need to join() before next move, otherwise it may crash.

Issue

If init/deinit for about 150 times with default input parameter, will crash at set_thread_affinity()

Author(s)

Performance impact

  • quality
  • memory
  • speed
  • 8 bit
  • 10 bit
  • N/A

Merge method

  • Allow the maintainer to squash and merge when PR is ready to create a 1-commit to the master branch. The maintainer will be able to fix typos / combine commit messages to create a more readable 1-commit message or use whatever is stated in the 'Description' section
  • I will clean up my commits and the maintainer shall use 'rebase and merge' to the master branch

Signed-off-by: Jing Li <jing.b.li@intel.com>
@1480c1
Copy link
Member

1480c1 commented Jul 30, 2020

@lijing0010 you need to put an x in between one of the boxes at the bottom

pthread_cancel(*((pthread_t*)threadHandle));
pthread_join(*((pthread_t*)threadHandle), NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

       int pthread_create(pthread_t *thread, const pthread_attr_t *attr,
                          void *(*start_routine) (void *), void *arg); 
 On success, pthread_create() returns 0; on error, it returns an error number, and the contents of *thread are undefined.

from: https://man7.org/linux/man-pages/man3/pthread_create.3.html.
Since *thread is undefined, do we even need to call pthread_cancel and pthread_join in this case?

Copy link
Contributor

@intelmark intelmark Jul 30, 2020

Choose a reason for hiding this comment

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

In either case the two levels of return code checking can be condensed (i.e. ret != 0) is not needed.

if (ret == EPERM) is sufficient.

    if (ret != 0) {
        if (ret == EPERM) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

       int pthread_create(pthread_t *thread, const pthread_attr_t *attr,
                          void *(*start_routine) (void *), void *arg); 
 On success, pthread_create() returns 0; on error, it returns an error number, and the contents of *thread are undefined.

from: https://man7.org/linux/man-pages/man3/pthread_create.3.html.
Since *thread is undefined, do we even need to call pthread_cancel and pthread_join in this case?

good question:), actually I don't think it's a good idea to turn set the thread policy to real time, it will drain the CPU. Of course it's good for encoding, and can have a little better encoder performance. But in real use case where you have other components like decoders/demuxers etc, doing so will not end up well

Signed-off-by: Jun Tian <jun.tian@intel.com>
@tianjunwork
Copy link
Contributor

tianjunwork commented Aug 11, 2020

Hi @lijing0010 , before we remove setting thread priority code, how about making this clean up first? Could you review? Thank you.

@tianjunwork tianjunwork added the Clean up A cleaner implementation or improved functionality label Aug 11, 2020
Co-authored-by: Christopher Degawa <ccom@randomderp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean up A cleaner implementation or improved functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants