Patch for pthread_attr_getguardsize

[unitedroad]

Hi,

this patch implements the pthread_attr_getguardsize function on the lines of how pthread_attr_getstacksize has been implmented in Haiku.

I needed this functionality in the port of Apache Harmony I am doing, but I think it will be useful to others as well since I believe it is a part of the POSIX thread API.

Regards, Dhruwat

[unitedroad]

patch for pthread_attr_getguardsize

[unitedroad]

Could the pthread owners please look into this patch? I have identified one function that I have created and I haven't called (thread_attr_getguardsize). I wanted to call this method in pthread_attr_init to initialize the attr->guard_size field.

I want to consolidate all the recommendations in the updated patch if the owners think this can be accepted with changes.

• Dhruwat

[bonefish]

Replying to unitedroad:

I have identified one function that I have created and I haven't called (thread_attr_getguardsize). I wanted to call this method in pthread_attr_init to initialize the attr->guard_size field.

Just assign the value in pthread_attr_init() as it is done for the other fields and remove thread_attr_getguardsize() again (global private functions must be in the "__" namespace, BTW). Also watch the coding style. The function introduced several style issues.

I want to consolidate all the recommendations in the updated patch if the owners think this can be accepted with changes.

The patch looks OK so far. Obviously it would be nice, if the feature was fully implemented. Adding pthread_attr_setguardsize() is trivial. The more interesting part is making use of the thread_creation_attributes::guard_size attribute you introduced in the kernel. That's just about making sure the field is properly initialized everywhere and the value passed to the places where it is needed.

PS: Patches are preferred in "git format-patch" format, so that they include the author and commit message.

[unitedroad]

Updated patch for pthread_attr_getguardsize

[unitedroad]

Hi,

sorry I am mailing after two weeks. I have removed the thread_attr_getguardsize function.

I didn't feel confident about the changes for pthread_attr_setguardsize, therefore I didn't try doing them them.

I have attached the updated patch in git format-patch format.

• Dhruwat

[hamish]

I can implement the actual functionality, though I have a question:

Should get_thread_info return a stack_base and stack_end that include the guard pages or not? Currently, if I'm not mistaken, the range it returns includes the guard pages (though it excludes the TLS space). To me it seems more consistent (and useful) for it to return only the usable range of the stack, though I'm not sure what the behaviour was on BeOS, and this might break compatibility.

[bonefish]

Replying to hamish:

I can implement the actual functionality, though I have a question:

Great!

Should get_thread_info return a stack_base and stack_end that include the guard pages or not? Currently, if I'm not mistaken, the range it returns includes the guard pages (though it excludes the TLS space). To me it seems more consistent (and useful) for it to return only the usable range of the stack, though I'm not sure what the behaviour was on BeOS, and this might break compatibility.

I don't think this is much of a compatibility concern. The stack range returned by get_thread_info() is mostly informative only. There isn't much one can do with it anyway -- maybe check whether a certain object has been allocated on the stack or paranoia debug checks (like whether a certain address is a valid stack address). So I think either including or excluding the guard pages is fine. Feel free to change that. I think all that has to be done is set Thread::stack_base/stack_size in create_thread_user_stack() accordingly, but the usage of the fields in the kernel should be checked to be sure.

[hamish]

Currently the B_STACK_AREA flag is passed to create_area_etc(), which makes it pass the USER_STACK_GUARD_PAGES constant to VMCacheFactory::CreateAnonymousCache().

Would it be desirable to add an extra parameter to create_area_etc() for guard size? Or is it possible to simply guard the pages manually with _user_set_memory_protection() in create_thread_user_stack()?

[bonefish]

Replying to hamish:

Currently the B_STACK_AREA flag is passed to create_area_etc(), which makes it pass the USER_STACK_GUARD_PAGES constant to VMCacheFactory::CreateAnonymousCache().

Would it be desirable to add an extra parameter to create_area_etc() for guard size? Or is it possible to simply guard the pages manually with _user_set_memory_protection() in create_thread_user_stack()?

The guard size should be passed to create_area_etc() directly. This way the explicit support for guard pages in the anonymous caches will be used. Setting the memory protection would have the same effect from the perspective of the userland (well, at least as long as the program doesn't meddle with the memory protections itself), but it has more overhead in the kernel (the page protection array needs to be allocated).

[korli]

This needs review from Ingo. Assigning...

[bonefish]

I finally got around to review the patch:

• You didn't rebase after hrev43737, so the patch doesn't apply cleanly.
• I'm not really fond of create_area_etc() and vm_create_anonymous_area() taking the number of guard pages instead of the guard size. None of the other parameters is/contains a page count, only sizes and addresses. (I guess it doesn't make much sense in the first place that the macros for the default guard sizes (USER/KERNEL_STACK_GUARD_PAGES) are page counts.)
• Mostly for sake of completeness: The ThreadCreationAttributes constructor should init the newly introduced guard_size attribute.
• Wrappers for pthread_attr_getguardsize() and pthread_attr_setguardsize() need to be added in src/libs/posix_error_mapper/.

[hamish]

I'll make those changes.

Also it seems I overlooked line 820 in vm.cpp: ​http://cgit.haiku-os.org/haiku/tree/src/system/kernel/vm/vm.cpp#n820. The stack guard page logic there is not actually used because the stack is mapped REGION_NO_PRIVATE_MAP, but for the sake of consistency should the private cache be created with the same guard size as the underlying cache?

[bonefish]

Replying to hamish:

Also it seems I overlooked line 820 in vm.cpp: ​http://cgit.haiku-os.org/haiku/tree/src/system/kernel/vm/vm.cpp#n820. The stack guard page logic there is not actually used because the stack is mapped REGION_NO_PRIVATE_MAP, but for the sake of consistency should the private cache be created with the same guard size as the underlying cache?

Yes it should. And REGION_PRIVATE_MAP is actually used also for stacks when they are CoW'ed -- otherwise after fork()ing one could write in the guard range. A new VMCache method will be needed to get the guard size.

[hamish]

Replying to bonefish:

otherwise after fork()ing one could write in the guard range

Hmm. As it stands now, after a fork() the guard pages of the parent thread's stack become writeable. I'm guessing this is because vm_copy_on_write_area() doesn't take into account the guard size (​http://cgit.haiku-os.org/haiku/tree/src/system/kernel/vm/vm.cpp#n2304) of the existing cache.

However even when vm_copy_on_write_area() does take the guard size into account, the guard region logic in VMAnonymousCache::Fault() is only considered if overcommiting is on (which it isn't for the copy cache). Is there any good reason why this is so?

[bonefish]

Replying to hamish:

Replying to bonefish:

otherwise after fork()ing one could write in the guard range

Hmm. As it stands now, after a fork() the guard pages of the parent thread's stack become writeable. I'm guessing this is because vm_copy_on_write_area() doesn't take into account the guard size (​http://cgit.haiku-os.org/haiku/tree/src/system/kernel/vm/vm.cpp#n2304) of the existing cache.

Yes, neither vm_copy_on_write_area() nor vm_copy_area() take that into account yet.

However even when vm_copy_on_write_area() does take the guard size into account, the guard region logic in VMAnonymousCache::Fault() is only considered if overcommiting is on (which it isn't for the copy cache). Is there any good reason why this is so?

No, I think the only reason for this is that both overcommitting and guard page support were introduced for the stacks and thus the features got lumped together (also in VMAnonymousNoSwapCache BTW).

I'm afraid the guard size is also not handled correctly when resizing and cutting areas/caches. Though that's getting farther and farther away from the pthread guard size feature. Feel free to add TODO's for the time being.

[hamish]

That should address everything. The guard pages remain in both stacks after a fork() now.

Replying to bonefish:

I'm afraid the guard size is also not handled correctly when resizing and cutting areas/caches. Though that's getting farther and farther away from the pthread guard size feature. Feel free to add TODO's for the time being.

I'm planning to fix the TODOs in cut_area() relating to cache resizing next. I'll fix the guard size behaviour as part of that.

[korli]

hamish,

could you see if the patch is OK to be applied? I see you didn't add TODO's in this patch. thanks!

[hamish]

Hi korli. Here's the patch rebased against the latest revision. Like bonefish said, the guard size isn't considered when cutting and resizing areas (but that problem was there already). I'll fix that as part of my fix for #8466.

Otherwise, if no one has any objections, this should be OK to apply. Shall I commit it myself?

[korli]

Replying to hamish:

Otherwise, if no one has any objections, this should be OK to apply. Shall I commit it myself?

Please do, thanks!

[hamish]

Applied in hrev45098.