Page MenuHomePhabricator

Minor code cleanup

Authored by tlwilmar on Jan 8 2018, 12:20 PM.

Diff Detail


Event Timeline

tlwilmar created this revision.Jan 8 2018, 12:20 PM
This revision is now accepted and ready to land.Jan 9 2018, 5:03 AM
protze.joachim added inline comments.
5537 ↗(On Diff #128968)

Is there a reason for not putting this into the __kmp_free_implicit_task function?

protze.joachim added inline comments.Jan 9 2018, 5:38 AM
990 ↗(On Diff #128968)

Moving above task=NULL here makes the data race visible. My suggestion:

kmp_taskdata_t *task = NULL;
<atomic_swap>(task, thread->th.th_current_task);
if (task && task->td_dephash) {

Whatever "atomic swap" function would be applicable.

AndreyChurbanov added inline comments.Jan 9 2018, 6:04 AM
5537 ↗(On Diff #128968)

There are cases where the thread structure is cleaned to be re-used, then the zeroing makes sense.
And in other cases where the thread structure has its memory freed there is no need to zero the contents.

So to me it looks logical to have the assignment here.

990 ↗(On Diff #128968)

Sorry, I haven't got what is the data race you are talking about.

protze.joachim added inline comments.Jan 9 2018, 6:56 AM
5537 ↗(On Diff #128968)

This is the only place, where __kmp_free_implicit_task is called. So semantically, there is no difference for moving the assignment into the function.

990 ↗(On Diff #128968)

Moving above assignment into the function:

void __kmp_free_implicit_task(kmp_info_t *thread) {
​  kmp_taskdata_t *task = thread->th.th_current_task;
​  if (task && task->td_dephash) {
​    __kmp_dephash_free(thread, task->td_dephash);
​    task->td_dephash = NULL;
​  }
  thread->th.th_current_task = NULL;

From the comment in above code I understood, that various threads might call this function with the same argument?
-> Then there would be a race between assigning NULL and the if.
Reading the comment again is the issue, that the same kmp_taskdata is reference by multiple threads, that used the task before?
-> No race, but I would still prefer to move the assignment into the function, for the case that the function will be used somewhere else.

tlwilmar added inline comments.Jan 9 2018, 12:15 PM
5537 ↗(On Diff #128968)

There is another use of this function in __kmp_reap_thread that does not nullify th_current_task after it.

tlwilmar added inline comments.Jan 9 2018, 12:51 PM
990 ↗(On Diff #128968)

In the context of __kmp_free_thread the assignment to NULL is safe. The context of __kmp_free_thread is either being called by a master thread allocating or changing team size under a lock, or freeing non-hot teams, so there should be no race.

protze.joachim accepted this revision.Jan 10 2018, 4:37 AM

Ok, I think, I now understood the change.

Closed by commit rL322203: Minor code cleanup (authored by jlpeyton, committed by ). · Explain WhyJan 10 2018, 10:25 AM
This revision was automatically updated to reflect the committed changes.