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.

Sign up for GitHub

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

Jump to bottom

lwip: fix lwip thread_create() wrapper #9588

Merged
merged 1 commit into from
Jul 26, 2018
Merged

lwip: fix lwip thread_create() wrapper #9588

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion pkg/lwip/contrib/sys_arch.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,19 +178,46 @@ u32_t sys_arch_mbox_tryfetch(sys_mbox_t *mbox, void **msg)
}
}

/**
* @brief parameters to _lwip_thread_wrapper
*/
typedef struct {
mutex_t sync;
lwip_thread_fn thread;
void *arg;
} _lwip_thread_params_t;

static void *_lwip_thread_wrapper(void *params_ptr)
{
_lwip_thread_params_t *params = params_ptr;
lwip_thread_fn thread = params->thread;
void *arg = params->arg;
mutex_unlock(&params->sync);
thread(arg);
/* TODO: free stack? */
Copy link
Member Author

Choose a reason for hiding this comment

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

@kaspar030 any idea how we might do that? It's allocated here

Copy link
Contributor

Choose a reason for hiding this comment

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

If we were sure that the thread is not going to be forcefully cancelled, it could be done in the wrapper, before returning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Depending on the architecture, the stack might still be used to write the return value, so this sounds like a bad idea ;-).

Copy link
Contributor

Choose a reason for hiding this comment

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

@miri64 You are right. I just realized how stupid my comment was!! How can one going to release the stack if the function is still executing!

What we need is either:

  • A termination/cancellation function
  • A thread creation mechanism that handles stack allocations automatically.

Multithread programming sucks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, at the moment non of the threads started by lwIP terminate, so this is a more theoretical issue in case lwIP ever decides to have something that actually finishes ;-).

return NULL;
}

sys_thread_t sys_thread_new(const char *name, lwip_thread_fn thread, void *arg,
int stacksize, int prio)
{
kernel_pid_t res;
char *stack = mem_malloc((size_t)stacksize);
_lwip_thread_params_t params = {
.sync = MUTEX_INIT_LOCKED,
.thread = thread,
.arg = arg,
};

if (stack == NULL) {
return ERR_MEM;
}
if ((res = thread_create(stack, stacksize, prio, THREAD_CREATE_STACKTEST,
(thread_task_func_t)thread, arg, name)) <= KERNEL_PID_UNDEF) {
_lwip_thread_wrapper, &params,
name)) <= KERNEL_PID_UNDEF) {
abort();
}
mutex_lock(&params.sync);
Copy link
Contributor

Choose a reason for hiding this comment

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

This usage of mutexes - unlocking in a different thread than the one it is locked from- is kind of non-standard. I happens to work with RIOT mutexes, but I don't think it's a behaviour that should be relied upon or guaranteed.
The "correct" primitive for this usage is either a semaphore or a condition variable (I think we don't have those in RIOT).

@kaspar030 What do you think about this use of semaphores?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jcarrano since mutexes are the only reliable core synchronization mechanism RIOT has (at the moment) I rather go for them. If it works in RIOT, why not use it in RIOT ;-) (as it is in several other places, e.g. asymcute).

PS: there is a bug in sema that actually causes problems in lwip already ;-) #7035

Copy link
Contributor

Choose a reason for hiding this comment

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

I opened a question-bug: #9594 .

sched_switch((char)prio);
return res;
}
Expand Down