context tracking vs. syscall_trace_leave & do_notify_resume loop

context tracking vs. syscall_trace_leave & do_notify_resume loop

[Rik van Riel]

Andy pointed out to me something I should have seen earlier: both
syscall_trace_leave and do_notify_resume call both user_exit()
and user_enter(), which has the potential to greatly increase the
cost of context tracking.

I believe (though it is hard to know for sure) there are legitimate
reasons why there is a loop around syscall_trace_leave and
do_notify_resume, but I strongly suspect the context tracking code
does not need to be in that loop.

I suspect it would be possible to stick a call to a new function
(return_to_user ?) right after the DISABLE_INTERRUPTS below, which
could be used to do the context tracking user_enter just once, and
later on also to load the user FPU context (patches I have sitting
around).

syscall_return:
        /* The IRETQ could re-enable interrupts: */
        DISABLE_INTERRUPTS(CLBR_ANY)
        TRACE_IRQS_IRETQ

Andy, Denys, do you guys see any issues with that idea?

I realize that would mean a RESTORE_EXTRA_REGS after that call
to return_to_user(), but it looks like that could be achieved
without making the code any worse than it already is :)

-- 
All rights reversed

Re: context tracking vs. syscall_trace_leave & do_notify_resume loop

[Andy Lutomirski]

On Thu, Apr 30, 2015 at 6:30 PM, Rik van Riel <riel@redhat.com> wrote:
> Andy pointed out to me something I should have seen earlier: both
> syscall_trace_leave and do_notify_resume call both user_exit()
> and user_enter(), which has the potential to greatly increase the
> cost of context tracking.
>
> I believe (though it is hard to know for sure) there are legitimate
> reasons why there is a loop around syscall_trace_leave and
> do_notify_resume, but I strongly suspect the context tracking code
> does not need to be in that loop.
>
> I suspect it would be possible to stick a call to a new function
> (return_to_user ?) right after the DISABLE_INTERRUPTS below, which
> could be used to do the context tracking user_enter just once, and
> later on also to load the user FPU context (patches I have sitting
> around).
>
> syscall_return:
>         /* The IRETQ could re-enable interrupts: */
>         DISABLE_INTERRUPTS(CLBR_ANY)
>         TRACE_IRQS_IRETQ
>
> Andy, Denys, do you guys see any issues with that idea?

Ick.  Let's make the mess better before we make it worse.  Now that
Denys disentangled the syscall exit path from the interrupt exit path,
let me see if I can just rewrite the syscall exit path entirely later
this week.
'
--Andy

Re: context tracking vs. syscall_trace_leave & do_notify_resume loop

[Rik van Riel]

On 05/01/2015 11:55 AM, Andy Lutomirski wrote:
> On Thu, Apr 30, 2015 at 6:30 PM, Rik van Riel <riel@redhat.com> wrote:

>> I suspect it would be possible to stick a call to a new function
>> (return_to_user ?) right after the DISABLE_INTERRUPTS below, which
>> could be used to do the context tracking user_enter just once, and
>> later on also to load the user FPU context (patches I have sitting
>> around).
>>
>> syscall_return:
>>         /* The IRETQ could re-enable interrupts: */
>>         DISABLE_INTERRUPTS(CLBR_ANY)
>>         TRACE_IRQS_IRETQ
>>
>> Andy, Denys, do you guys see any issues with that idea?
> 
> Ick.  Let's make the mess better before we make it worse.  Now that
> Denys disentangled the syscall exit path from the interrupt exit path,
> let me see if I can just rewrite the syscall exit path entirely later
> this week.

I suspect we probably only need two possible function
calls at syscall exit time:

1) A function that is called with interrupts still
   enabled, testing flags that could be set again
   if something happens (eg. preemption) between
   when the function is called, and we return to
   user space.

2) A function that is called after the point of
   no return, with interrupts disabled, which
   does (mostly) small things that only happen
   once.

-- 
All rights reversed

Re: context tracking vs. syscall_trace_leave & do_notify_resume loop

[Andy Lutomirski]

On Fri, May 1, 2015 at 9:00 AM, Rik van Riel <riel@redhat.com> wrote:
> On 05/01/2015 11:55 AM, Andy Lutomirski wrote:
>> On Thu, Apr 30, 2015 at 6:30 PM, Rik van Riel <riel@redhat.com> wrote:
>
>>> I suspect it would be possible to stick a call to a new function
>>> (return_to_user ?) right after the DISABLE_INTERRUPTS below, which
>>> could be used to do the context tracking user_enter just once, and
>>> later on also to load the user FPU context (patches I have sitting
>>> around).
>>>
>>> syscall_return:
>>>         /* The IRETQ could re-enable interrupts: */
>>>         DISABLE_INTERRUPTS(CLBR_ANY)
>>>         TRACE_IRQS_IRETQ
>>>
>>> Andy, Denys, do you guys see any issues with that idea?
>>
>> Ick.  Let's make the mess better before we make it worse.  Now that
>> Denys disentangled the syscall exit path from the interrupt exit path,
>> let me see if I can just rewrite the syscall exit path entirely later
>> this week.
>
> I suspect we probably only need two possible function
> calls at syscall exit time:
>
> 1) A function that is called with interrupts still
>    enabled, testing flags that could be set again
>    if something happens (eg. preemption) between
>    when the function is called, and we return to
>    user space.
>
> 2) A function that is called after the point of
>    no return, with interrupts disabled, which
>    does (mostly) small things that only happen
>    once.

I think we only need one function.  It would be (asm pseudocode):

disable irqs;
if (slow) {
  save extra regs;
  call function;
  restore extra regs;
}

return via opportunistic sysret path.

I can't see any legitimate reason for the current mess, except that
it's no complicated and so poorly documented that everyone's afraid of
fixing it.

--Andy

Re: context tracking vs. syscall_trace_leave & do_notify_resume loop

[Rik van Riel]

On 05/01/2015 12:05 PM, Andy Lutomirski wrote:
> On Fri, May 1, 2015 at 9:00 AM, Rik van Riel <riel@redhat.com> wrote:

>> I suspect we probably only need two possible function
>> calls at syscall exit time:
>>
>> 1) A function that is called with interrupts still
>>    enabled, testing flags that could be set again
>>    if something happens (eg. preemption) between
>>    when the function is called, and we return to
>>    user space.
>>
>> 2) A function that is called after the point of
>>    no return, with interrupts disabled, which
>>    does (mostly) small things that only happen
>>    once.
> 
> I think we only need one function.  It would be (asm pseudocode):
> 
> disable irqs;
> if (slow) {
>   save extra regs;
>   call function;
>   restore extra regs;
> }
> 
> return via opportunistic sysret path.
> 
> I can't see any legitimate reason for the current mess, except that
> it's no complicated and so poorly documented that everyone's afraid of
> fixing it.

do_notify_resume() can call do_signal(), which can sleep, after
which all bets are off on what new flags may have been set.

On the other hand, we have stuff that can run just fine with
irqs disabled that we really want to call only once.

For that reason, I suspect we need two functions.

-- 
All rights reversed

Re: context tracking vs. syscall_trace_leave & do_notify_resume loop

[Andy Lutomirski]

On Fri, May 1, 2015 at 9:14 AM, Rik van Riel <riel@redhat.com> wrote:
> On 05/01/2015 12:05 PM, Andy Lutomirski wrote:
>> On Fri, May 1, 2015 at 9:00 AM, Rik van Riel <riel@redhat.com> wrote:
>
>>> I suspect we probably only need two possible function
>>> calls at syscall exit time:
>>>
>>> 1) A function that is called with interrupts still
>>>    enabled, testing flags that could be set again
>>>    if something happens (eg. preemption) between
>>>    when the function is called, and we return to
>>>    user space.
>>>
>>> 2) A function that is called after the point of
>>>    no return, with interrupts disabled, which
>>>    does (mostly) small things that only happen
>>>    once.
>>
>> I think we only need one function.  It would be (asm pseudocode):
>>
>> disable irqs;
>> if (slow) {
>>   save extra regs;
>>   call function;
>>   restore extra regs;
>> }
>>
>> return via opportunistic sysret path.
>>
>> I can't see any legitimate reason for the current mess, except that
>> it's no complicated and so poorly documented that everyone's afraid of
>> fixing it.
>
> do_notify_resume() can call do_signal(), which can sleep, after
> which all bets are off on what new flags may have been set.
>
> On the other hand, we have stuff that can run just fine with
> irqs disabled that we really want to call only once.
>
> For that reason, I suspect we need two functions.

C can have loops just as easily as assembly can :)  I still don't see
why we need magic asm code to schedule and deliver signals.  We
certainly need to have valid pt_regs to deliver signals, but that's
easy and much cheaper than it used to be.

--Andy

Re: context tracking vs. syscall_trace_leave & do_notify_resume loop

[Rik van Riel]

On 05/01/2015 12:16 PM, Andy Lutomirski wrote:
> On Fri, May 1, 2015 at 9:14 AM, Rik van Riel <riel@redhat.com> wrote:
>> On 05/01/2015 12:05 PM, Andy Lutomirski wrote:
>>> On Fri, May 1, 2015 at 9:00 AM, Rik van Riel <riel@redhat.com> wrote:
>>
>>>> I suspect we probably only need two possible function
>>>> calls at syscall exit time:
>>>>
>>>> 1) A function that is called with interrupts still
>>>>    enabled, testing flags that could be set again
>>>>    if something happens (eg. preemption) between
>>>>    when the function is called, and we return to
>>>>    user space.
>>>>
>>>> 2) A function that is called after the point of
>>>>    no return, with interrupts disabled, which
>>>>    does (mostly) small things that only happen
>>>>    once.

> C can have loops just as easily as assembly can :)  I still don't see
> why we need magic asm code to schedule and deliver signals.  We
> certainly need to have valid pt_regs to deliver signals, but that's
> easy and much cheaper than it used to be.

Oh, I never said it would all have to be in assembly :)

I would love to see the stuff in entry.S greatly simplified.

-- 
All rights reversed