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
<https://gist.github.com/ur10/a1829b7b8648521d90703be014e3feef> in the
application to ensure that the mmap() is called
from the desired thread?
> -Gedare
>
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
