Skip to content

Posix implementation layer #5667

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

Merged
merged 14 commits into from
Feb 22, 2018

Conversation

youvedeep
Copy link
Contributor

@youvedeep youvedeep commented Jan 13, 2018

Currently Zephyr does not support POSIX APIs. This patch patch provides a compatibility layer for supporting POSIX1003.1 PSE52. patch series use Zephyr APIs as foundation and provides a layer to map POSIX APIs to ZEPHYR APIs.

@codecov-io
Copy link

codecov-io commented Jan 13, 2018

Codecov Report

Merging #5667 into master will increase coverage by 0.13%.
The diff coverage is 62%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5667      +/-   ##
==========================================
+ Coverage   53.14%   53.28%   +0.13%     
==========================================
  Files         412      424      +12     
  Lines       40148    40729     +581     
  Branches     7733     7845     +112     
==========================================
+ Hits        21338    21702     +364     
- Misses      15675    15817     +142     
- Partials     3135     3210      +75
Impacted Files Coverage Δ
kernel/posix/pthread_cond.c 82.75% <0%> (ø) ⬆️
include/posix/time.h 100% <100%> (ø)
kernel/posix/pthread_mutex.c 57.14% <100%> (ø) ⬆️
include/posix/pthread.h 100% <100%> (+28.57%) ⬆️
include/posix/posix_sched.h 100% <100%> (ø)
kernel/posix/pthread_sched.c 41.66% <41.66%> (ø)
kernel/posix/timer.c 46.98% <46.98%> (ø)
kernel/posix/pthread.c 61.5% <61.5%> (ø)
tests/kernel/posix/pthread/src/pthread.c 61.85% <61.85%> (ø)
tests/kernel/posix/pthread_join/src/pthread.c 64.7% <64.7%> (ø)
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a73e8fa...9ae6024. Read the comment docs.

@youvedeep youvedeep force-pushed the Posix_Implementation_Layer branch 2 times, most recently from 871177c to 6e82ccd Compare January 14, 2018 09:38
@SebastianBoe SebastianBoe removed their request for review January 15, 2018 14:40

}

int pthread_attr_setschedparam(pthread_attr_t *attr,
Copy link
Member

@aescolar aescolar Jan 15, 2018

Choose a reason for hiding this comment

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

Please add all these functions also to posix_cheats.h
Otherwise you prevent the POSIX arch port from using the host ones.
It wouldn't harm adding also the ones you included as static inlines in the header, in case somebody would change them later to normal/global definitions.

#include <kernel.h>
#include <sched.h>

int sched_get_priority_min(int policy)
Copy link
Member

@aescolar aescolar Jan 15, 2018

Choose a reason for hiding this comment

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

Also these 2, and any other which is in the POSIX API

Copy link
Collaborator

@lpereira lpereira left a comment

Choose a reason for hiding this comment

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

Please review my comments on the other pthread PR; some of the issues there were not addressed in this PR.

(I did not review this fully.)

* Default pthread attributes on attributes initialization.
*/
static pthread_attr_t initpthreadattrs = {
K_LOWEST_APPLICATION_THREAD_PRIO, /* priority */
Copy link
Collaborator

@lpereira lpereira Jan 16, 2018

Choose a reason for hiding this comment

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

Please see comments on #5494 about this. Structs should be initialized like so:

static const struct foo = {
   .bar = 3,
   .baz = 4,
   .quux = SOME_CONSTANT,
};

This way, you can get rid of comments, and if the order of elements change (due to packing reasons, implementing new features, etc), you can be sure that the values that were set before will continue to be set in the future; unset values will be initialized to 0 by the compiler.

kernel/Kconfig Outdated
if PTHREAD_IPC
config MAX_PTHREAD_COUNT
int
prompt "Maximu pthread count in POSIX application"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo: maximu

int pthread_create(pthread_t *newthread, const pthread_attr_t *attr,
void *(*threadroutine)(void *), void *arg)
{
static int thread_count;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be an atomic_val_t.

errno = EINVAL;
return -1;
}
if (thread_count < CONFIG_MAX_PTHREAD_COUNT) {
Copy link
Collaborator

@lpereira lpereira Jan 16, 2018

Choose a reason for hiding this comment

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

Between creating a thread and incrementing the value, preemption might occur. This will cause a race condition that will lead to either invalid memory access, or elements being skipped in the threads array (and two elements being initialized in the same spot).

A way to fix this is to peruse the fact that atomic_inc() will return the previous value, which is the one you want to use as an index to the threads array:

atomic_val_t index = atomic_inc(&thread_count);
if (index < CONFIG_MAX_PTHREAD_COUNT) {
    ...  &threads[index] ...
}

There's no race condition here, because index is guaranteed to be below CONFIG_MAX_PTHREAD_COUNT, even if preemption occurs: no other thread will use the same index. (That's why index here is not static like thread_count is.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @lpereira I will take care of this.

@youvedeep youvedeep force-pushed the Posix_Implementation_Layer branch from 6e82ccd to d56ecad Compare January 17, 2018 10:12
@youvedeep
Copy link
Contributor Author

Addressed comments from @lpereira and @aescolar-ot in new patch version.

@youvedeep youvedeep force-pushed the Posix_Implementation_Layer branch from d56ecad to 02f1655 Compare January 18, 2018 16:19

int sched_get_priority_min(int policy)
{
if (!((policy == SCHED_FIFO) || (policy == SCHED_RR))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can apply De Morgan's Law here and simplify this expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I will change it to - if((policy != SCHED_FIFO) && (policy != SCHED_RR)))

Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for the extra parenthesis.


if (IS_ENABLED(CONFIG_COOP_ENABLED)) {
if (policy == SCHED_FIFO) {
errno = 0; /* Valid negative return */
Copy link
Collaborator

@lpereira lpereira Jan 19, 2018

Choose a reason for hiding this comment

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

This function can't return a negative value on success; that's how the API is defined, and how code using it will expect it to behave.

A solution is not to have priorities mapping 1:1 between Zephyr and POSIX. You can derive the formula for the proper mapping like so:

  • Consider two points in the space: (x0, y0) and (x1, y1). The X axis is the POSIX priority, and the Y axis is the Zephyr priority.
  • Find two points, P1 and P2, that maps the minimum priority in Zephyr (that's a negative value) to a value that's acceptable in POSIX (say, 1). Do the same for P2, for the maximum value.
  • Calculate the slope: m = (y1 - y0) / (x1 - x0)
  • Use the calculated slope in the line equation: y - y0 = m(x - x0), where y0 and x0 are the coordinates from P1
  • Isolate y or x depending on which direction you want to go (Zephyr -> POSIX or POSIX -> Zephyr). If you're calculating y, any x in the line is valid (could be even x0), and vice versa.
  • It's important to round m to the nearest integer, to not use floating point math here. It shouldn't matter if it's ceiled or floored; just make sure that you'll never get a negative x.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per my understanding if a function returns a negative value then it should set errno to 0.
because of which I was putting 0 as ERROR code.

https://www.unix.com/man-page/posix/3posix/sched_get_priority_min/

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's the opposite: if the function returns -1, errno must be set; otherwise, errno isn't touched (see RETURN VALUE in the linked man page).

Per the POSIX specification, only a return value of -1 will signal error. However, a lot of code will verify error conditions by testing the sign bit (i.e. checking if the return value is < 0 rather than == -1), as it often generates slightly tighter code.

Being a compatibility layer, it's best to assume that any function returning -1 per the specification will also return a negative number (whatever it is) on error, and 0 or a positive value on success. Returning any value from 0 to INT_MAX is fine for these functions, as long as they're consistent with each other, and that the functions to set the priorities are also consistent with these priority values.

Copy link
Contributor Author

@youvedeep youvedeep Jan 22, 2018

Choose a reason for hiding this comment

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

Looking at documentation @ https://www.systutorials.com/docs/linux/man/2-sched_get_priority_max/#index, (Looks like man page for IEEE Std 1003.13-2003 is silent on number of priorities level in POSIX https://www.unix.com/man-page/posix/3posix/sched_get_priority_min/)-
As per POSIX :-

  1. Higher the numerical value Priority, Higher the priority. Something like a thread with priority 5 will have higher priority than a thread with priority 1. (something opposite of Zephyr).
  2. There should be at least 32 priority levels, but this can vary based on other standards. I think we can avoid this as for RTOS this may be too high number of priority levels.

I think we can not use the slope technique to map Zephyr priort levels to POSIX prioirty levels as this may create confusion/issues. Let us assume (Numerical lower priority has Higher Priority than numberical high only for example).
Let us take an example - If we have premptive based scheduling enabled with
Zephyr_Max_ priority = 15
Zephyr_Min_ priority = 0
Now mapping this range to POSIX_MAX = 31, POSIX_MIN=0.
Let us say that a POSIX compliant application has 3 threads with priority 2, 3, 4. then it may be expected that 1 has higher priority than 2 and so on.
But internally :-
Thread 1 ==> POSIX_PPRIOITY = 2 mapped to ZEPHYR_PRIORTY = 1
Thread 2 ==> POSIX_PPRIOITY = 3 mapped to ZEPHYR_PRIORTY = 1
Thread 3 ==> POSIX_PPRIOITY = 4 mapped to ZEPHYR_PRIORTY = 2
So this may give undesirable results. So i think there should be 1:1 or n:m (n>= m) mapping between POSIX and ZEPHYR .

I think we can map ZEPHYR_PRIORITY to POSIX_PRIORTY :-

For Preemptive_thread :-
posix_priorty= (CONFIG_NUM_PREEMPT_PRIORITIES - 1) - zephyr_priorty
*-1 to keep maximum priority reserved for highest priority thread.
This will make POSIX_MIN_PRIORITY = 0 and POSIX_MAX_PRIORTY = CONFIG_MAX_APPLICATION_PRIORITY

For COOPERATIVE scheduling policy:-
posix_application_priority = (-1 * zephyr_priority) +1 . (cooperative priorities are -ive).

*
* See IEEE 1003.1
*/
static inline void pthread_exit(void *retval)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have a pthread_join()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As of now we do not have pthread_join(). We need to figure out how to implement it. Before that we will target other POSIX APIs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

One way to do this is the following:

struct pthread {
    struct k_thread thread;
    void *retval;
};

typedef pthread_t struct pthread;

This way, it's possible to use type-punning to convert between a struct k_thread and a struct pthread. And, for pthreads, it's possible to set/obtain the return value. So, this function could be implemented like so:

static void pthread_exit(void *retval)
{
        pthread_t *self = (pthread_t *)k_current_get();

        self->retval = retval;

        k_thread_abort(self);
}

Then, pthread_join() will get the retval from pthread_t. Calling pthread_exit() should also give a mutex that pthread_join() is waiting on, before it aborts itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I got your Idea, but I wanted to enhance it to be compatible with pthread_join after first set of patch merge.

For making pthread_join() we need to have 2 members (mutex and Condition variable to signal the state exit). But before that i am waiting for my first set of patches to be merged. I am planning to take this as enhancement,
struct pthread {
struct k_thread thread;
void retval;
pthread_mutex_t state_lock; /
Locks the state. /
pthread_cond_t state_cond; /
Signalled when the state changes. */
};

@lpereira
Copy link
Collaborator

There are some spelling/typos in the commit messages as well; please fix them.

Copy link
Contributor

@andrewboie andrewboie left a comment

Choose a reason for hiding this comment

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

Why is this PR and #5494 both open for review at the same time, they have the same code in them!
This is wasting developer time, clean up your stuff and send for review in one or more PRs that are completely disjoint.

#define PTHREAD_CANCELABLE (0 << PTHREAD_CANCEL_POS)
/* Essential Thread */
#define PTHREAD_NON_CANCELABLE (1 << PTHREAD_CANCEL_POS)
#define K_LOWEST_STACK_SIZE 256
Copy link
Contributor

Choose a reason for hiding this comment

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

how was this value arrived at?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just went through the Zephyr Code base and looked that 256 is least stack size used by a thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @andrewboie Sorry for confusion. But i have mentioned clearly on PR #5494 that discussion for this patch is moved to #5667.
@nashif requested to merge all POSIX related patches into 1 PR and for that I created this PR later.
To avoid confusion, I have closed other POSIX related PRs.

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need this value at all unless I'm missing something.
It only seems to be used for the defaults which have a NULL stack pointer anyway,
Just use NULL for the default stack pointer and 0 for the size. There is no actual minimum. I have a stack in a double fault handler which is about 12 bytes in size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm got it, So basically your suggestion to to define stack_size as 0, something like stack pointer. Let the user provide stack_size along with stack pointer,

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @andrewboie Sorry for confusion. But i have mentioned clearly on PR #5494 that discussion for this patch is moved to #5667.

Don't have multiple open PRs with the same code. Just don't do it. Close one of them.

@youvedeep youvedeep force-pushed the Posix_Implementation_Layer branch from 02f1655 to efca769 Compare January 28, 2018 17:06
@youvedeep
Copy link
Contributor Author

Guys, Can you please please provide quick review comments on this PR as this is targeted for Release 1.11

/* Pthread cancellation */
#define PTHREAD_CANCEL_POS 0
/* Non Essential Thread */
#define PTHREAD_CANCELABLE (0 << PTHREAD_CANCEL_POS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't seem to find any reference to PTHREAD_CANCELABLE. Is this documented somewhere?

#define PTHREAD_CANCELABLE (0 << PTHREAD_CANCEL_POS)
/* Essential Thread */
#define PTHREAD_NON_CANCELABLE (1 << PTHREAD_CANCEL_POS)
#define THREAD_INIT_FLAGS PTHREAD_CANCELABLE
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this prefixed with THREAD_? If someone includes pthread.h, this will pollute their namespace with things that are not POSIX-specific. There doesn't seem to exist any THREAD_INIT_FLAGS in Linux as well, so I don't know why is this defined in the header file.

struct timespec {
s32_t tv_sec;
s32_t tv_nsec;
static const pthread_attr_t initpthreadattrs = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick, but please use _ to separate words in variable names: init_pthread_attrs.

Also, this shouldn't be in the header file; there will be multiple copies of it if it's included multiple times. Define it inside pthread_attr_init() instead, even if you have to move it to a .c file.

k_thread_abort((k_tid_t) self);
}

static inline u32_t zephyr_to_posix_priority(s32_t z_prio, int *policy)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Functions that are implementation details should be in the .c file.

*/
static inline int clock_settime(clockid_t clock_id, const struct timespec *ts)
{
errno = (clock_id != CLOCK_MONOTONIC) ? EINVAL : ENOSYS;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The error here should always be ENOSYS, since it's just a stub. Returning EINVAL does not help the user figure out what's wrong.

new_prio = posix_to_zephyr_priority(param->priority, policy);
cur_prio = k_thread_priority_get(*thread);

if ((is_posix_prio_valid(new_prio, policy) == false) ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: please remove the excessive parenthesis.


static bool valid_posix_policy(int policy)
{
if ((policy != SCHED_FIFO) && (policy != SCHED_RR)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: please remove the excessive parenthesis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel removing internal braces (if (policy != SCHED_FIFO && policy != SCHED_RR) ) will lose the readability. So keeping the braces is better. What is your thought ?

Copy link
Collaborator

@lpereira lpereira Jan 31, 2018

Choose a reason for hiding this comment

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

I don't have strong opinions on this -- I find it less visually polluted without the parenthesis, and don't see any readability issues without them. It's just a nitpick, we can agree to disagree.


return true;
}
int sched_get_priority_min(int policy)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: Missing newline before function.

prio = posix_to_zephyr_priority(attr->priority, attr->schedpolicy);

thread_index = atomic_inc(&thread_count);
if (thread_index < CONFIG_MAX_PTHREAD_COUNT) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW, I was thinking: can't we just use a memory pool for this? It's possible to return a handle to a pool when the thread ends. Doing it this way, there's no way to recycle a pthread struct: once the system runs out of them, they're gone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @lpereira I will take this as enhancement after M2 milestone.

}

for (i = 0; i < N_THR; i++) {
/* Thread Attribute initialization */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please avoid comments that are not adding any value. If a line says foo_init(&foo), don't say /* Initializes foo */. Comments should document the intent of the code or things that are not obvious just from reading the code itself.

@lpereira
Copy link
Collaborator

The patches are looking better, but I don't think they're ready to be merged yet.

@nashif nashif added this to the v1.11.0 milestone Jan 31, 2018
@youvedeep youvedeep force-pushed the Posix_Implementation_Layer branch from efca769 to f3dca31 Compare February 3, 2018 08:15
pthread_cond_wait(&pthread->state_cond, &pthread->state_lock);
}

if (pthread->state == PTHREAD_EXITED) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use an else if here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PTHREAD_JOINABLE and PTHREAD_EXITED are not implemented through ifelse as status of thread will change from PTHREAD_JOINABLE --> PTHREAD_EXITED on pthread_cond_broadcast (inside pthread_exit).
Example :

  1. Thread T1 calls pthread_join for thread T2.
  2. Thread T1 waits at pthread_cond_wait as T2 has not exited.
    .
    .
  3. Thread T2 calls pthread_exit and it updates self->state and retvalue. and calls pthread_cond_broadcast.
  4. Now thread T1 receives the condition variable.
  5. It enters into pthread->state == PTHREAD_EXITED and gets the return value,

So a thread need to enter into both the conditions, so it has not been put through else if.

pthread_mutex_lock(&pthread->state_lock);

if (pthread->state == PTHREAD_JOINABLE) {
pthread_cond_wait(&pthread->state_cond, &pthread->state_lock);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should return 0 here

Copy link
Contributor Author

@youvedeep youvedeep Feb 15, 2018

Choose a reason for hiding this comment

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

when the state if thread will change from PTHREAD_JOINABLE to PTHREAD_EXITED. it will pass through pthread->state == PTHREAD_EXITED) and will eventually return 0.

detailed explanation in previous comment.

*
* See IEEE 1003.1
*/
int pthread_join(pthread_t thread, void **status)
Copy link
Collaborator

Choose a reason for hiding this comment

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

For this function, it might be better to have a int ret that's set by each block comparing with pthread->state. This will reduce its size slightly, and have only one mutex_unlock/return block.

Copy link
Contributor Author

@youvedeep youvedeep Feb 15, 2018

Choose a reason for hiding this comment

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

Addressed.

@youvedeep youvedeep force-pushed the Posix_Implementation_Layer branch from 9fd2ae7 to abe7859 Compare February 15, 2018 14:29
@youvedeep
Copy link
Contributor Author

Addressed comments received (except @andrew's comment to fix mutex Object issue). Can you please review it further and provide feedbacks.

@youvedeep youvedeep force-pushed the Posix_Implementation_Layer branch 3 times, most recently from e348fbd to b39dc64 Compare February 16, 2018 07:47
@youvedeep
Copy link
Contributor Author

  1. implemented memory allocation for pthread object using mempool.
  2. Added pthread_cancel test.

@youvedeep
Copy link
Contributor Author

Guys Can you please give further comments or provide feedback on this PR?

Copy link
Collaborator

@lpereira lpereira left a comment

Choose a reason for hiding this comment

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

Looking a lot better, thanks!

struct posix_thread *thread;
struct k_mem_block block;

/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comments such as these should be in the comment block that doxygen will find. Users are not expected to look at the implementation to find out why the function is returning EINVAL.

Copy link
Contributor Author

@youvedeep youvedeep Feb 21, 2018

Choose a reason for hiding this comment

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

Addressed
sure I put comment in doxygen.

prio = posix_to_zephyr_priority(attr->priority, attr->schedpolicy);

if (k_mem_pool_alloc(&posix_thread_pool, &block,
sizeof(sizeof(struct posix_thread)), 100) == 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why sizeof(sizeof(T))?

Copy link
Contributor Author

@youvedeep youvedeep Feb 21, 2018

Choose a reason for hiding this comment

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

Addressed.
This is a typo, thanks for catching it. I will fix this.

*/
int pthread_setcancelstate(int state, int *oldstate)
{
struct posix_thread *pthread = (struct posix_thread *) pthread_self();
Copy link
Collaborator

@lpereira lpereira Feb 21, 2018

Choose a reason for hiding this comment

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

Put in doxychain currently.

This is still an issue: if pthread_setcancelstate() is called from a thread that's not a pthread, it'll access memory that it shouldn't access (in the best case scenario; it's undefined behavior, so who knows what's going to happen.)

This is an issue with all functions that call pthread_self().

If we're not going to fix this -- which is a big issue, IMO -- at least comment in the pthread_self() Doxygen block that the results of calling it outside a thread created with pthread_create() are undefined.

};

/* Memory pool for pthread space */
K_MEM_POOL_DEFINE(posix_thread_pool, sizeof(struct posix_thread),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nothing is returned to this memory pool; we'll still run out of pthread handles.

I remember threads having a callback function that got called whenever they were destroyed. Can't we used that to return something to the pool? It might be only for threads that are aborted, which is fine; pthread_exit() can schedule a call to k_mem_pool_free() (it can't call it directly).

Copy link
Contributor Author

@youvedeep youvedeep Feb 21, 2018

Choose a reason for hiding this comment

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

Not Addressed. Need to figure out solution for this.

Currently I am not returning back to mem pool becuase we need to keep pthread object to handle
pthread_join requests. If we will free the space then pthread_join will have issue.

But I agree we need to figure out a way to return back to mem pool on pthread termination. I can explore it further. But even when k_thread is destroyed, we may need to access pthread object such as retval, pthtread state, statue_mutex to address pthread_join.

@youvedeep youvedeep force-pushed the Posix_Implementation_Layer branch from b39dc64 to 1e92b5a Compare February 21, 2018 16:03
@youvedeep
Copy link
Contributor Author

Addressed comments from @lpereira .
Also Added 2 patches for :
POSIX Timer APIs (12/13)
POSIX timer test API (13/13)

As per IEEE 1003.1 POSIX APIs should return ERROR_CODE on error.
But currently these are returning -ERROR_CODE instead of ERROR_CODE.
So fixing the return value.

Signed-off-by: Youvedeep Singh <[email protected]>
As per POSIX standard typedef should be part of sys/types.h file.
So moving typedef from pthread.h to sys/types.h file.

Signed-off-by: Youvedeep Singh <[email protected]>
This patch provides pthread APIs for POSIX 1003.1 PSE52 standard.

Signed-off-by: Youvedeep Singh <[email protected]>
This patch provides scheduler APIs for POSIX 1003.1 PSE52 standard.

Signed-off-by: Youvedeep Singh <[email protected]>
This patch provides POSIX sleep APIs for POSIX 1003.1 PSE52 standard.
sleep(n) is implemented using Zephyr k_sleep API.
uleep(n) is implemented using Zephyr k_sleep/k_busy_Wait API.

Signed-off-by: Youvedeep Singh <[email protected]>
This patch provides POSIX clock APIs for POSIX 1003.1 PSE52 standard.

Signed-off-by: Youvedeep Singh <[email protected]>
This patch removes unused member element from POSIX object attributes
(mutex, condition variable and barrier).

Signed-off-by: Youvedeep Singh <[email protected]>
Implement pthread_join test application.

Signed-off-by: Youvedeep Singh <[email protected]>
Add posix clock API test.

Signed-off-by: Youvedeep Singh <[email protected]>
This test is POSIX based implementation of tests:kernel:pthread test.
It used POSIX APIs instead of Zephyr APIs.

Signed-off-by: Youvedeep Singh <[email protected]>
Added test for POSIX thread cancel APIs.

Signed-off-by: Youvedeep Singh <[email protected]>
This patch provides POSIX timer APIs for POSIX 1003.1 PSE52 standard.

Signed-off-by: Youvedeep Singh <[email protected]>
Added test for POSIX timer APIs.

Signed-off-by: Youvedeep Singh <[email protected]>
Some of APIs of POSIX implmentation layer has same name as
native_posix architecture. posix_cheats.h is used to handle this
duplication in API name. Adding a guard in posix_cheats.h based on
CONFIG_PTHREAD_API.

Signed-off-by: Youvedeep Singh <[email protected]>
@youvedeep youvedeep force-pushed the Posix_Implementation_Layer branch from 1e92b5a to 9ae6024 Compare February 21, 2018 17:46
@nashif nashif merged commit 4d72e48 into zephyrproject-rtos:master Feb 22, 2018
@nashif nashif mentioned this pull request Feb 22, 2018
12 tasks
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.

9 participants