RFR(L): 8193257: PPC64, s390 implementation for Thread-local handshakes

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(L): 8193257: PPC64, s390 implementation for Thread-local handshakes

Doerr, Martin
Hi,

I've implemented Thread-local handshakes on PPC64 and s390.

We may improve it by using SIGTRAP-based polling on these 2 platforms in the future (which requires changes in shared code).
For now, I just implemented it like on the other platforms without further improvements.

Webrev is here:
http://cr.openjdk.java.net/~mdoerr/8193257_PPC64_s390_Thread-local_hs/webrev.00/

Please review.

Best regards,
Martin

Reply | Threaded
Open this post in threaded view
|

RE: RFR(L): 8193257: PPC64, s390 implementation for Thread-local handshakes

Lindenmaier, Goetz
Hi Martin,

I had a look at your change.

I see you enabled it per default?

macroAssembler_ppc/s390.cpp:
MacroAssembler::safepoint_poll()
Could you add some comment that says what this is doing?
As it's not doing a safepoint_poll ... its just preparing it, right?
Maybe also "slow_target" should be "do_safepoint".
 
templateInterpreterGenerator_ppc.cpp:
Why do you change memory ordering from release() to lwsync()?

sharedRuntime_ppc/s390.cpp
If I understand right you skip the safepoint instruction.
Why that? Could you please document that this differs
from the other platforms here?

Best regards,
  Goetz.

> -----Original Message-----
> From: Doerr, Martin
> Sent: Freitag, 8. Dezember 2017 17:17
> To: hotspot-dev developers ([hidden email]) <hotspot-
> [hidden email]>; Lindenmaier, Goetz
> <[hidden email]>; Schmidt, Lutz <[hidden email]>
> Subject: RFR(L): 8193257: PPC64, s390 implementation for Thread-local
> handshakes
>
> Hi,
>
>
>
> I've implemented Thread-local handshakes on PPC64 and s390.
>
>
>
> We may improve it by using SIGTRAP-based polling on these 2 platforms in
> the future (which requires changes in shared code).
>
> For now, I just implemented it like on the other platforms without further
> improvements.
>
>
>
> Webrev is here:
>
> http://cr.openjdk.java.net/~mdoerr/8193257_PPC64_s390_Thread-
> local_hs/webrev.00/
>
>
>
> Please review.
>
>
>
> Best regards,
>
> Martin
>
>

Reply | Threaded
Open this post in threaded view
|

RE: RFR(L): 8193257: PPC64, s390 implementation for Thread-local handshakes

Doerr, Martin
Hi Götz,

thanks reviewing. Please see my answers inline.

Best regards,
Martin


> I see you enabled it per default?
Yes. I have tried to keep the implementation as close to the other platforms as possible. x86, SPARC and aarch64 have it enabled by default, too.

> macroAssembler_ppc/s390.cpp:
> MacroAssembler::safepoint_poll()
> Could you add some comment that says what this is doing?
> As it's not doing a safepoint_poll ... its just preparing it, right?
> Maybe also "slow_target" should be "do_safepoint".
I agree with that the label name "do_safepoint" would be more comprehensible, but I'd prefer to use the same name as x86, SPARC and aarch64.
What the function safepoint_poll does is it simply performs the poll by checking the poll bit + conditional branch to the safepoint code. Would you like to see a comment like "Perform poll and branch if safepoint requested"?

> templateInterpreterGenerator_ppc.cpp:
> Why do you change memory ordering from release() to lwsync()?
This is not a functional change. release() emits an lwsync() instruction. I have changed it to emphasize that we're using it for acquire and release in one instruction. release() hides that we use it for acquire, too.

> sharedRuntime_ppc/s390.cpp
> If I understand right you skip the safepoint instruction.
> Why that? Could you please document that this differs
> from the other platforms here?
The s390 code does exactly the same thing as on the other platforms. It adjusts to PC to point to the instruction following the poll instruction. This is part of the new concept which needs to be implemented on all platforms supporting local handshakes. The difference to PPC for example is that s390 has different instruction sizes (2, 4 and 6 bytes) and I implemented it in a way that it works with any poll instruction regardless of its size.
Would you like to see a comment like "Support poll instruction with any size"?

Reply | Threaded
Open this post in threaded view
|

RE: RFR(L): 8193257: PPC64, s390 implementation for Thread-local handshakes

Lindenmaier, Goetz
Hi,

I would prefer
"Check whether safepoint is requested and if so branch to code suspending the thread."
The word 'poll' isn't very precise to describe what is happening here.

Other stuff all fine. Reviewed. No new webrev needed.

Best regards,
  Goetz.

> -----Original Message-----
> From: Doerr, Martin
> Sent: Dienstag, 12. Dezember 2017 11:08
> To: Lindenmaier, Goetz <[hidden email]>; hotspot-dev
> developers ([hidden email]) <hotspot-
> [hidden email]>; Schmidt, Lutz <[hidden email]>
> Subject: RE: RFR(L): 8193257: PPC64, s390 implementation for Thread-local
> handshakes
>
> Hi Götz,
>
> thanks reviewing. Please see my answers inline.
>
> Best regards,
> Martin
>
>
> > I see you enabled it per default?
> Yes. I have tried to keep the implementation as close to the other platforms
> as possible. x86, SPARC and aarch64 have it enabled by default, too.
>
> > macroAssembler_ppc/s390.cpp:
> > MacroAssembler::safepoint_poll()
> > Could you add some comment that says what this is doing?
> > As it's not doing a safepoint_poll ... its just preparing it, right?
> > Maybe also "slow_target" should be "do_safepoint".
> I agree with that the label name "do_safepoint" would be more
> comprehensible, but I'd prefer to use the same name as x86, SPARC and
> aarch64.
> What the function safepoint_poll does is it simply performs the poll by
> checking the poll bit + conditional branch to the safepoint code. Would you
> like to see a comment like "Perform poll and branch if safepoint requested"?
>
> > templateInterpreterGenerator_ppc.cpp:
> > Why do you change memory ordering from release() to lwsync()?
> This is not a functional change. release() emits an lwsync() instruction. I have
> changed it to emphasize that we're using it for acquire and release in one
> instruction. release() hides that we use it for acquire, too.
>
> > sharedRuntime_ppc/s390.cpp
> > If I understand right you skip the safepoint instruction.
> > Why that? Could you please document that this differs
> > from the other platforms here?
> The s390 code does exactly the same thing as on the other platforms. It
> adjusts to PC to point to the instruction following the poll instruction. This is
> part of the new concept which needs to be implemented on all platforms
> supporting local handshakes. The difference to PPC for example is that s390
> has different instruction sizes (2, 4 and 6 bytes) and I implemented it in a way
> that it works with any poll instruction regardless of its size.
> Would you like to see a comment like "Support poll instruction with any
> size"?

Reply | Threaded
Open this post in threaded view
|

Re: RFR(L): 8193257: PPC64, s390 implementation for Thread-local handshakes

Schmidt, Lutz
Hi Martin,

Your change looks good. I’d like to point out just two little details:
 - sharedRuntime_ppc.cpp, around line 236:
   Your comment talks about saving r31, but in fact you are saving r31 and r30.
 - macroAssembler_s390.hpp, around line 440:
   Your comment suggests instr_size(size, pc) actually updates the pc, but it does not.

If you agree, please just update the comments before you push. No new webrev required.
 
Thanks for implementing this feature.

Lutz

 

On 12.12.2017, 11:20, "Lindenmaier, Goetz" <[hidden email]> wrote:

    Hi,
   
    I would prefer
    "Check whether safepoint is requested and if so branch to code suspending the thread."
    The word 'poll' isn't very precise to describe what is happening here.
   
    Other stuff all fine. Reviewed. No new webrev needed.
   
    Best regards,
      Goetz.
   
    > -----Original Message-----
    > From: Doerr, Martin
    > Sent: Dienstag, 12. Dezember 2017 11:08
    > To: Lindenmaier, Goetz <[hidden email]>; hotspot-dev
    > developers ([hidden email]) <hotspot-
    > [hidden email]>; Schmidt, Lutz <[hidden email]>
    > Subject: RE: RFR(L): 8193257: PPC64, s390 implementation for Thread-local
    > handshakes
    >
    > Hi Götz,
    >
    > thanks reviewing. Please see my answers inline.
    >
    > Best regards,
    > Martin
    >
    >
    > > I see you enabled it per default?
    > Yes. I have tried to keep the implementation as close to the other platforms
    > as possible. x86, SPARC and aarch64 have it enabled by default, too.
    >
    > > macroAssembler_ppc/s390.cpp:
    > > MacroAssembler::safepoint_poll()
    > > Could you add some comment that says what this is doing?
    > > As it's not doing a safepoint_poll ... its just preparing it, right?
    > > Maybe also "slow_target" should be "do_safepoint".
    > I agree with that the label name "do_safepoint" would be more
    > comprehensible, but I'd prefer to use the same name as x86, SPARC and
    > aarch64.
    > What the function safepoint_poll does is it simply performs the poll by
    > checking the poll bit + conditional branch to the safepoint code. Would you
    > like to see a comment like "Perform poll and branch if safepoint requested"?
    >
    > > templateInterpreterGenerator_ppc.cpp:
    > > Why do you change memory ordering from release() to lwsync()?
    > This is not a functional change. release() emits an lwsync() instruction. I have
    > changed it to emphasize that we're using it for acquire and release in one
    > instruction. release() hides that we use it for acquire, too.
    >
    > > sharedRuntime_ppc/s390.cpp
    > > If I understand right you skip the safepoint instruction.
    > > Why that? Could you please document that this differs
    > > from the other platforms here?
    > The s390 code does exactly the same thing as on the other platforms. It
    > adjusts to PC to point to the instruction following the poll instruction. This is
    > part of the new concept which needs to be implemented on all platforms
    > supporting local handshakes. The difference to PPC for example is that s390
    > has different instruction sizes (2, 4 and 6 bytes) and I implemented it in a way
    > that it works with any poll instruction regardless of its size.
    > Would you like to see a comment like "Support poll instruction with any
    > size"?
   
   

Reply | Threaded
Open this post in threaded view
|

RE: RFR(L): 8193257: PPC64, s390 implementation for Thread-local handshakes

Doerr, Martin
Hi Lutz and Götz,

thanks for reviewing. I have pushed the change with the proposed comment changes to jdk/jdk.

Best regards,
Martin


-----Original Message-----
From: Schmidt, Lutz
Sent: Donnerstag, 14. Dezember 2017 12:49
To: Lindenmaier, Goetz <[hidden email]>; Doerr, Martin <[hidden email]>; hotspot-dev developers ([hidden email]) <[hidden email]>
Subject: Re: RFR(L): 8193257: PPC64, s390 implementation for Thread-local handshakes

Hi Martin,

Your change looks good. I’d like to point out just two little details:
 - sharedRuntime_ppc.cpp, around line 236:
   Your comment talks about saving r31, but in fact you are saving r31 and r30.
 - macroAssembler_s390.hpp, around line 440:
   Your comment suggests instr_size(size, pc) actually updates the pc, but it does not.

If you agree, please just update the comments before you push. No new webrev required.
 
Thanks for implementing this feature.

Lutz

 

On 12.12.2017, 11:20, "Lindenmaier, Goetz" <[hidden email]> wrote:

    Hi,
   
    I would prefer
    "Check whether safepoint is requested and if so branch to code suspending the thread."
    The word 'poll' isn't very precise to describe what is happening here.
   
    Other stuff all fine. Reviewed. No new webrev needed.
   
    Best regards,
      Goetz.
   
    > -----Original Message-----
    > From: Doerr, Martin
    > Sent: Dienstag, 12. Dezember 2017 11:08
    > To: Lindenmaier, Goetz <[hidden email]>; hotspot-dev
    > developers ([hidden email]) <hotspot-
    > [hidden email]>; Schmidt, Lutz <[hidden email]>
    > Subject: RE: RFR(L): 8193257: PPC64, s390 implementation for Thread-local
    > handshakes
    >
    > Hi Götz,
    >
    > thanks reviewing. Please see my answers inline.
    >
    > Best regards,
    > Martin
    >
    >
    > > I see you enabled it per default?
    > Yes. I have tried to keep the implementation as close to the other platforms
    > as possible. x86, SPARC and aarch64 have it enabled by default, too.
    >
    > > macroAssembler_ppc/s390.cpp:
    > > MacroAssembler::safepoint_poll()
    > > Could you add some comment that says what this is doing?
    > > As it's not doing a safepoint_poll ... its just preparing it, right?
    > > Maybe also "slow_target" should be "do_safepoint".
    > I agree with that the label name "do_safepoint" would be more
    > comprehensible, but I'd prefer to use the same name as x86, SPARC and
    > aarch64.
    > What the function safepoint_poll does is it simply performs the poll by
    > checking the poll bit + conditional branch to the safepoint code. Would you
    > like to see a comment like "Perform poll and branch if safepoint requested"?
    >
    > > templateInterpreterGenerator_ppc.cpp:
    > > Why do you change memory ordering from release() to lwsync()?
    > This is not a functional change. release() emits an lwsync() instruction. I have
    > changed it to emphasize that we're using it for acquire and release in one
    > instruction. release() hides that we use it for acquire, too.
    >
    > > sharedRuntime_ppc/s390.cpp
    > > If I understand right you skip the safepoint instruction.
    > > Why that? Could you please document that this differs
    > > from the other platforms here?
    > The s390 code does exactly the same thing as on the other platforms. It
    > adjusts to PC to point to the instruction following the poll instruction. This is
    > part of the new concept which needs to be implemented on all platforms
    > supporting local handshakes. The difference to PPC for example is that s390
    > has different instruction sizes (2, 4 and 6 bytes) and I implemented it in a way
    > that it works with any poll instruction regardless of its size.
    > Would you like to see a comment like "Support poll instruction with any
    > size"?