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

Linux: Removing threadpool_open and threadpool_close along with SM states #416

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

Conversation

nishudeshu
Copy link
Contributor

Removing Threadpool Open and Close and SM states.

@nishudeshu nishudeshu changed the title Specs for removing threadpool open and close and SM states Linux removing threadpool_open and threadpool_close along with SM states Dec 1, 2024
@nishudeshu
Copy link
Contributor Author

nishudeshu commented Dec 1, 2024

/azp run #Resolved

Copy link

azure-pipelines bot commented Dec 1, 2024

Azure Pipelines successfully started running 1 pipeline(s).

#Resolved

@nishudeshu nishudeshu changed the title Linux removing threadpool_open and threadpool_close along with SM states Linux: Removing threadpool_open and threadpool_close along with SM states Dec 1, 2024
@nishudeshu nishudeshu enabled auto-merge (squash) December 1, 2024 17:44
@dcristoloveanu
Copy link
Member

dcristoloveanu commented Dec 2, 2024

#include "c_pal/sm.h"

This is not needed any longer #Resolved


Refers to: linux/src/threadpool_linux.c:32 in a679d72. [](commit_id = a679d72, deletion_comment = False)

@nishudeshu
Copy link
Contributor Author

#include "c_pal/sm.h"

Removed.


In reply to: 2512829554


Refers to: linux/src/threadpool_linux.c:32 in a679d72. [](commit_id = a679d72, deletion_comment = False)

Copy link

No pipelines are associated with this pull request.

wake_by_address_single(threadpool->task_array[current_index].pending_work_item_count_ptr);
/* Codes_SRS_THREADPOOL_LINUX_05_046: [ threadpool_work_func shall release the shared SRW lock by calling srw_lock_release_exclusive. ]*/
(void)interlocked_decrement(threadpool->task_array[current_index].pending_work_item_count_ptr);
wake_by_address_single(threadpool->task_array[current_index].pending_work_item_count_ptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're only waiting up if the value is zero, then you need to have an if on the decrement as to only wait on zero, it makes it more efficient.

if (interlocked_decrement(...) == 0)
{
wake_by_address_single(...);
}


srw_lock_release_exclusive(threadpool_ptr->srw_lock);
wake_by_address_single(task_item->pending_work_item_count_ptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

wake_by_address_single(task_item->pending_work_item_count_ptr);

I'm a little confused why we do a wake by address here. The only thing that's waiting for this is in the destroy function.

@nishudeshu
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jebrando
Copy link
Contributor

jebrando commented Dec 2, 2024

int threadpool_open(THANDLE(THREADPOOL) threadpool)

Maybe add a comment saying that the open and close will go away soon.


Refers to: linux/src/threadpool_linux.c:446 in a679d72. [](commit_id = a679d72, deletion_comment = False)

Copy link
Contributor

@jebrando jebrando left a comment

Choose a reason for hiding this comment

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

:shipit:

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