Quantcast

JEP 270 concerns

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

JEP 270 concerns

Andrew Haley
I've been porting "JEP 270: Reserved Stack Areas for Critical
Sections" to AArch64.  I have no particular concerns about the port,
but I found some serious flaws in testing.

The main problem is that it doesn't work when inlining is enabled.  To
demonstrate this fact, remove "-Inline" and add "-Xss512k" to the
runtime arguments in ReservedStackTest.  (I tried x86_64 and AArch64.)

The first problem is that the stack walking code in
SharedRuntime::look_for_reserved_stack_annotated_method only looks at
stack frames, not at the methods which have been inlined into compiled
methods.  So, if a method marked with a ReservedStackAccess annotation
is inlined, the runtime code will not see the annotation, and the
reserved zone will not be used.

I thought that this was a simple omission, so I changed
look_for_reserved_stack_annotated_method to walk through Scopes
instead of frames, and this indeed detects that a ReservedStackAccess
method has been inlined.  However, there is a deeper flaw: once
control is returned to the method which inlines ReservedStackAccess,
the reserved zone remains disabled, so the next time that a method is
called the protection will not be in place.

I think that the only reasonable way to fix this is to force methods
annotated with ReservedStackAccess not to be inlined.  It would be
possible to fix this in a better way by changing the logic so that a
runtime call to re-enable reserved zone is inserted at the return of
every ReservedStackAccess-annotated method, but this would be more
complex.

There is another problem: if a callee of a ReservedStackAccess method
makes a runtime call, the yellow zone is disabled; when that runtime
call returns, the yellow zone and the reserved zone are re-eanbled, so
the reserved zone protection is re-enabled while that method is
running.  We then return to the ReservedStackAccess method and trigger
an assertion because the reserved zone protection is enabled, which is
unexpected.

Of course I may be very much mistaken about all of this, but I think
not.

Andrew.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JEP 270 concerns

frederic parain
Hi Andrew,

Thank you for the deep investigation and the detailed report.

On 01/11/2017 10:49 AM, Andrew Haley wrote:

> I've been porting "JEP 270: Reserved Stack Areas for Critical
> Sections" to AArch64.  I have no particular concerns about the port,
> but I found some serious flaws in testing.
>
> The main problem is that it doesn't work when inlining is enabled.  To
> demonstrate this fact, remove "-Inline" and add "-Xss512k" to the
> runtime arguments in ReservedStackTest.  (I tried x86_64 and AArch64.)
>
> The first problem is that the stack walking code in
> SharedRuntime::look_for_reserved_stack_annotated_method only looks at
> stack frames, not at the methods which have been inlined into compiled
> methods.  So, if a method marked with a ReservedStackAccess annotation
> is inlined, the runtime code will not see the annotation, and the
> reserved zone will not be used.

JEP 270 tried to resolve a very specific problem with stack overflows
occurring in critical sections. When an annotated method m1 is inlined,
in a method m2, m2 includes the stack requirements of m1 in the
computation of its stack requirements. So no stack overflow is
possible when m1 is "called (not really a call because of inlining).
However, it only works if m1 is a leaf. If m1 calls another method
and this method is not inlined, then I agree there is a case that
has been missed here.

> I thought that this was a simple omission, so I changed
> look_for_reserved_stack_annotated_method to walk through Scopes
> instead of frames, and this indeed detects that a ReservedStackAccess
> method has been inlined.  However, there is a deeper flaw: once
> control is returned to the method which inlines ReservedStackAccess,
> the reserved zone remains disabled, so the next time that a method is
> called the protection will not be in place.

Regarding the very particular usage of the annotation, it could be
acceptable that a method which inlines an annotated method keeps
access to the ReservedStackArea. This is not ideal, but it won't
be worse than the situation before, and it would help anyway in
some cases.

Please, could you sent me your code using Scopes? I'd like to
experiment with it.

> I think that the only reasonable way to fix this is to force methods
> annotated with ReservedStackAccess not to be inlined.

JEP 270's goal is definitively not to prevent in-lining of the
critical methods using the annotation.

> It would be
> possible to fix this in a better way by changing the logic so that a
> runtime call to re-enable reserved zone is inserted at the return of
> every ReservedStackAccess-annotated method, but this would be more
> complex.

This looks too expensive to me regarding the problem the
ReservedStackArea tries to mitigate. It is possible to test with
assembly code if the reserved area has been disabled or not (with
the JavaThread::_reserved_stack_activation).

It seems that there's a trade-off here between guaranteeing the
exact granularity of the annotation and preserving the performances
of in-lining. I think the later is the more important.

Could widening the scope of the of the annotation in case of in-lining,
to include methods in which annotated methods are in-lined, be an
acceptable solution?

> There is another problem: if a callee of a ReservedStackAccess method
> makes a runtime call, the yellow zone is disabled; when that runtime
> call returns, the yellow zone and the reserved zone are re-eanbled, so
> the reserved zone protection is re-enabled while that method is
> running.  We then return to the ReservedStackAccess method and trigger
> an assertion because the reserved zone protection is enabled, which is
> unexpected.

If the yellow zone has been disabled, it means that a StackOverflowError
has been thrown. If it happens, it means that the ReservedStackArea
was sizing too small. But even if a StackOverflowError is thrown in
an annotated section, it should not trigger an assertion failure,
so this is clearly a bug.

> Of course I may be very much mistaken about all of this, but I think
> not.

Your insights were very helpful on the in-lining issue.

Regards,

Fred
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JEP 270 concerns

Andrew Haley
Hi,

On 11/01/17 23:47, Frederic Parain wrote:
>
> JEP 270 tried to resolve a very specific problem with stack overflows
> occurring in critical sections. When an annotated method m1 is inlined,
> in a method m2, m2 includes the stack requirements of m1 in the
> computation of its stack requirements. So no stack overflow is
> possible when m1 is "called (not really a call because of inlining).
> However, it only works if m1 is a leaf. If m1 calls another method
> and this method is not inlined, then I agree there is a case that
> has been missed here.

Right, but even in the jtreg case, m1 is not a leaf: it is
nonfairTryAcquire.  This calls getExclusiveOwnerThread, which is the
method that overflows the stack.  And
look_for_reserved_stack_annotated_method does not see the method which is
at the top anyway, so I think that a ReservedStackAccess method cannot
itself use the reserved area: it has to be one of its callees.

>> I thought that this was a simple omission, so I changed
>> look_for_reserved_stack_annotated_method to walk through Scopes
>> instead of frames, and this indeed detects that a ReservedStackAccess
>> method has been inlined.  However, there is a deeper flaw: once
>> control is returned to the method which inlines ReservedStackAccess,
>> the reserved zone remains disabled, so the next time that a method is
>> called the protection will not be in place.
>
> Regarding the very particular usage of the annotation, it could be
> acceptable that a method which inlines an annotated method keeps
> access to the ReservedStackArea. This is not ideal, but it won't
> be worse than the situation before, and it would help anyway in
> some cases.

I guess so, but IMO the logic which handles a stack overflow when the
reserved area is unprotected would have to be fixed so that it doesn't
blindly re-protect the reserved area when it finishes.

> Please, could you sent me your code using Scopes? I'd like to
> experiment with it.

I will.

>> I think that the only reasonable way to fix this is to force methods
>> annotated with ReservedStackAccess not to be inlined.
>
> JEP 270's goal is definitively not to prevent in-lining of the
> critical methods using the annotation.

OK.  It's the easy way to make this stuff all come out correctly,
but I understand why not.

>> It would be possible to fix this in a better way by changing the
>> logic so that a runtime call to re-enable reserved zone is inserted
>> at the return of every ReservedStackAccess-annotated method, but
>> this would be more complex.
>
> This looks too expensive to me regarding the problem the
> ReservedStackArea tries to mitigate. It is possible to test with
> assembly code if the reserved area has been disabled or not (with
> the JavaThread::_reserved_stack_activation).

It wouldn't need to be any more expensive: a JIT is quite capable of
inlining the exact same assembly code.  It's just more work.

> It seems that there's a trade-off here between guaranteeing the
> exact granularity of the annotation and preserving the performances
> of in-lining. I think the later is the more important.
>
> Could widening the scope of the of the annotation in case of in-lining,
> to include methods in which annotated methods are in-lined, be an
> acceptable solution?

That has a bad code smell, don't you think?  To do that you'll have to
change the test so that it works when inlining is enabled: the test
case works around the bug.  That feels wrong.  I don't think it's in
any sense impossible to make JEP 270 work as designed, even when stuff
is inlined.

Andrew.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JEP 270 concerns

frederic parain
Hi Andrew,

I've re-read the code and my notes and here's
a more accurate description of the way JEP 270
intends to deal with in-lining:

When a method method m1 with the ReservedStackAccess
annotation is in-lined in a method m2, nothing special
is made in the way the method is in-lined and a check
on the reserved stack area is added to m2's return code.
The fact that m1 is a leaf or not doesn't matter (in
fact JEP 270 is important if m1 is not a leaf).

Does it mean that the reserved stack are is not re-enabled
immediately after the execution of an in-lined m1? Yes.

Does it mean m2's code can use the reserved stack area?
No, the stack banging performed at the beginning of m2
ensure that it has enough stack space available without
touching guard pages.

Does it mean that a method called by m2, after an execution
of m1 that trigger access to the reserved stack area, could
use the reserved stack area? Yes, this is unfortunate but
this is part of a trade-off explained below.

However, as you have spotted it, the method
look_for_reserved_stack_annotated_method() fails to detect
in-lined annotated methods. This is clearly a bug which makes
the annotation ineffective for in-lined methods.

The rational of the implementation.

The problem JEP 270 tries to address cannot be solved reliably
in all cases, simply because in Java when a method is invoked,
there's no way to anticipate what the execution will trigger
(class loading for instance).
The consequences of the original bug were considered serious enough
that we would like to mitigate the bug. But the bug occurrences were
also rare, so we didn't want to dedicate too much resources or to
impact performances for a rare bug.

It would be possible to modify the in-lining code to add checks
around an annotated in-lined method. The checks themselves might not
be that expensive (comparing values of
JavaThread::_reserved_stack_activation
before and after the in-lined section is sufficient to detect that
the reserved stack area has been disabled). However, this would
put more constraints on the way the code is in-lined, requiring
to preserve well identified sequences of in-lined code. It was
considered that these constraints would reduce the number of
optimizations the JIT could performed when in-lining methods.

So, instead of modifying the in-lining code, the decision was
made to silently widen the scope of the annotation in case
of in-lining.

Now, if it appears that these hypothesis were wrong, and it
is possible to add the checks in in-lined code without impacting
performances and with a reasonable complexity in the JIT code,
we could revisit these choices. Having the semantic of the
annotation enforced strictly at the granularity of the annotated
method would a nice thing, but how much are we ready to pay for
the protection against this rare bug?

I hope it clarifies the implementation choices that have been
made.

Regards,

Fred

On 01/12/2017 07:19 AM, Andrew Haley wrote:

> Hi,
>
> On 11/01/17 23:47, Frederic Parain wrote:
>>
>> JEP 270 tried to resolve a very specific problem with stack overflows
>> occurring in critical sections. When an annotated method m1 is inlined,
>> in a method m2, m2 includes the stack requirements of m1 in the
>> computation of its stack requirements. So no stack overflow is
>> possible when m1 is "called (not really a call because of inlining).
>> However, it only works if m1 is a leaf. If m1 calls another method
>> and this method is not inlined, then I agree there is a case that
>> has been missed here.
>
> Right, but even in the jtreg case, m1 is not a leaf: it is
> nonfairTryAcquire.  This calls getExclusiveOwnerThread, which is the
> method that overflows the stack.  And
> look_for_reserved_stack_annotated_method does not see the method which is
> at the top anyway, so I think that a ReservedStackAccess method cannot
> itself use the reserved area: it has to be one of its callees.
>
>>> I thought that this was a simple omission, so I changed
>>> look_for_reserved_stack_annotated_method to walk through Scopes
>>> instead of frames, and this indeed detects that a ReservedStackAccess
>>> method has been inlined.  However, there is a deeper flaw: once
>>> control is returned to the method which inlines ReservedStackAccess,
>>> the reserved zone remains disabled, so the next time that a method is
>>> called the protection will not be in place.
>>
>> Regarding the very particular usage of the annotation, it could be
>> acceptable that a method which inlines an annotated method keeps
>> access to the ReservedStackArea. This is not ideal, but it won't
>> be worse than the situation before, and it would help anyway in
>> some cases.
>
> I guess so, but IMO the logic which handles a stack overflow when the
> reserved area is unprotected would have to be fixed so that it doesn't
> blindly re-protect the reserved area when it finishes.
>
>> Please, could you sent me your code using Scopes? I'd like to
>> experiment with it.
>
> I will.
>
>>> I think that the only reasonable way to fix this is to force methods
>>> annotated with ReservedStackAccess not to be inlined.
>>
>> JEP 270's goal is definitively not to prevent in-lining of the
>> critical methods using the annotation.
>
> OK.  It's the easy way to make this stuff all come out correctly,
> but I understand why not.
>
>>> It would be possible to fix this in a better way by changing the
>>> logic so that a runtime call to re-enable reserved zone is inserted
>>> at the return of every ReservedStackAccess-annotated method, but
>>> this would be more complex.
>>
>> This looks too expensive to me regarding the problem the
>> ReservedStackArea tries to mitigate. It is possible to test with
>> assembly code if the reserved area has been disabled or not (with
>> the JavaThread::_reserved_stack_activation).
>
> It wouldn't need to be any more expensive: a JIT is quite capable of
> inlining the exact same assembly code.  It's just more work.
>
>> It seems that there's a trade-off here between guaranteeing the
>> exact granularity of the annotation and preserving the performances
>> of in-lining. I think the later is the more important.
>>
>> Could widening the scope of the of the annotation in case of in-lining,
>> to include methods in which annotated methods are in-lined, be an
>> acceptable solution?
>
> That has a bad code smell, don't you think?  To do that you'll have to
> change the test so that it works when inlining is enabled: the test
> case works around the bug.  That feels wrong.  I don't think it's in
> any sense impossible to make JEP 270 work as designed, even when stuff
> is inlined.
>
> Andrew.
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JEP 270 concerns

Andrew Haley
On 12/01/17 15:28, Frederic Parain wrote:

> Now, if it appears that these hypothesis were wrong, and it
> is possible to add the checks in in-lined code without impacting
> performances and with a reasonable complexity in the JIT code,
> we could revisit these choices. Having the semantic of the
> annotation enforced strictly at the granularity of the annotated
> method would a nice thing, but how much are we ready to pay for
> the protection against this rare bug?
>
> I hope it clarifies the implementation choices that have been
> made.

I take your point.  I think two things must be done:

1.  Correct the detection of inlined methods.  Not difficult, as we
discussed offthread.

2.  Fix assertion failures when we get a stack overflow in another
method with protection disabled.

Does that make sense?

Andrew.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JEP 270 concerns

frederic parain
Hi Andrew,

It makes perfect sense.

I've created bug JDK-8172791 to track these issues.

Thank you for your in-depth analysis of the problems.

Fred

On 01/13/2017 03:50 AM, Andrew Haley wrote:

> On 12/01/17 15:28, Frederic Parain wrote:
>> Now, if it appears that these hypothesis were wrong, and it
>> is possible to add the checks in in-lined code without impacting
>> performances and with a reasonable complexity in the JIT code,
>> we could revisit these choices. Having the semantic of the
>> annotation enforced strictly at the granularity of the annotated
>> method would a nice thing, but how much are we ready to pay for
>> the protection against this rare bug?
>>
>> I hope it clarifies the implementation choices that have been
>> made.
>
> I take your point.  I think two things must be done:
>
> 1.  Correct the detection of inlined methods.  Not difficult, as we
> discussed offthread.
>
> 2.  Fix assertion failures when we get a stack overflow in another
> method with protection disabled.
>
> Does that make sense?
>
> Andrew.
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JEP 270 concerns

Andrew Haley
On 13/01/17 14:11, Frederic Parain wrote:

> It makes perfect sense.
>
> I've created bug JDK-8172791 to track these issues.

Hi,

It looks like this didn't get done.  That's a shame, given that it
wasn't difficult to fix the worst problems and I would have been
quite happy to do it.  I'd already written the code to walk up the
stack finding scopes.

Andrew.


Loading...