On Mon, Aug 24, 2020 at 10:41 AM Utkarsh Rai <utkarsh.ra...@gmail.com> wrote:
>
>
>
> On Mon, Aug 24, 2020 at 9:34 PM Gedare Bloom <ged...@rtems.org> wrote:
>>
>> On Sat, Aug 22, 2020 at 11:27 PM Utkarsh Rai <utkarsh.ra...@gmail.com> wrote:
>> >
>> >
>> >
>> > On Sat, Aug 22, 2020 at 11:32 PM Gedare Bloom <ged...@rtems.org> wrote:
>> >>
>> >> I have some comments below. I'm not that happy with the lack of design
>> >> discussion during the iteration of this code. While it is a little
>> >> easier to critique the code, it is also a bit wasteful because I have
>> >> to also comment on stylistic problems, and when some decisions you
>> >> make while coding end up not being correct or aren't easily understood
>> >> during code review then you might have to spend more time recoding. It
>> >> can be hard to find the balance between design and code reviews,
>> >>
>> >> On Sat, Aug 22, 2020 at 9:19 AM Utkarsh Rai <utkarsh.ra...@gmail.com>
>> >> wrote:
>> >> > +
>> >> > +#if defined ( RTEMS_THREAD_STACK_PROTECTION )
>> >> > +
>> >> > +/**
>> >> > + * The following defines the control block of a shared stack. Each
>> >> > stack can have
>> >> > + * different sharing attributes.
>> >> > + */
>> >> > +typedef struct
>> >> > +{
>> >> > + /** Access flags of the shared stack */
>> >> > + uint32_t access_flags;
>> >> > + /** This is the stack address of the sharing thread*/
>> >> > + void* shared_stack_area;
>> >> > + /** Stack size of the sharing thread*/
>> >> > + size_t shared_stack_size;
>> >> > + /** This is the stack address of the target stack. Maybe this area
>> >> > is not
>> >> > + * needed but this helps in selecting the target thread during stack
>> >> > sharing.
>> >> > + */
>> >> > + void* target_stack_area;
>> >> I'm confused about the difference between shared_stack_area and
>> >> target_stack_area
>> >>
>> >> > + /** Error checking for valid target stack address. This is also used
>> >> > to
>> >> > + * distinguish between a normal mmap operation and a stack sharing
>> >> > operation.
>> >> > + */
>> >> I don't really know what this means.
>> >
>> >
>> > This is related to the mmap() call. When we share stack using mmap, two
>> > of the important parameters that reference to the target thread-stack
>> > and sharing thread-stack are -'void* addr' which is the address of the
>> > target thread stack and 'int fd' that is the file descriptor to the shared
>> > memory
>> > object corresponding to the thread stack being shared. Now, stack sharing
>> > is different from a normal mmap operation in the sense that we need to add
>> > the sharing stack attributes to the stack control structure of the thread
>> > that is specified by 'addr' as compared to adding it to the chain of
>> > mappings. To select the
>> > target thread we need to iterate over all the threads, in the case we do
>> > not find any thread, we can proceed with our normal mmap operation
>> > otherwise we add the
>> > sharing thread stack attribute to the stack control structure of the
>> > target thread. We have the _Thread_Iterate() function but it only has two
>> > parameters the pointer to the
>> > function that performs operation during each iteration and a parameter
>> > related to this operation. My decision to specify the 'target_stack_area',
>> > the 'shared_stack_area' and 'stack_shared' in the structure
>> > was so that I could squeeze in as much information to select the thread,
>> > set the sharing stack parameters in the thread, and validate(through
>> > stack_shared attribute) in the mmap() call that this is
>> > in fact a stack-sharing operation, in a single iteration.
>> >
>>
>> Maybe I should ask this on the mmap patch itself, but I'm more
>> interested in the design than the code right now. I think I'll just
>> ask a few questions:
>>
>> The 'sharing' stack is the stack that is getting r/w permission added
>> to another thread, and the 'target' thread is the thread that is
>> getting the r/w permission to access the shared stack. We should not
>> really care about the 'sharing thread' or the 'target stack'?
>
>
> If the mmap() call for sharing thread stack assumes that it is being called
> from the context of the 'target thread', then no, we should not.
>
>>
>>
>> If mmap() assumes the calling context then the executing thread is the
>> "target thread" and you shouldn't have to search for it? In typical
>> POSIX implementations, mmap() assumes the executing process context
>> called it, but can we make the same assumption?
>>
>> For mmap() the 'addr' should be the 'sharing' stack's (base) address
>> not the 'target' stack, because you want to be accessing the 'sharing'
>> stack memory? We don't have any virtual memory mapping, so the address
>> of the sharing stack needs to be used. Or maybe it can be NULL, or
>> even ignored, since the mmap() call should return the base address of
>> the 'sharing' stack? The fd should be able to find all the information
>> needed about the 'sharing' stack.
>
>
> My confusion was, as you asked above, whether we can assume that the mmap()
> was called by the executing process
> context. I was making the mmap() call from the 'POSIX_Init' thread and
> passing the address of the target thread and then
> selecting the thread inside mmap(). Can we do something like this in the
> application to ensure that the mmap() is called
> from the desired thread?
>
I think it is a reasonable assumption, but it may not be entirely
within the standard. We may need to get a "ruling" on it from Joel,
but I think you should proceed in this direction.
Also, I'm wondering if the stack mmap should also go in the mmap_mappings list?
>>
>> -Gedare
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
