| Message ID | 5718F901.6010104@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 21/04/2016 18:00, Laurent Dufour wrote: > On 13/04/2016 07:14, Michael Ellerman wrote: >> On Mon, 2016-04-11 at 09:40 +0200, Laurent Dufour wrote: >>> On 07/04/2016 23:49, Michael Ellerman wrote: >>>> On 7 April 2016 7:23:46 pm AEST, Laurent Dufour <ldufour@linux.vnet.ibm.com> wrote: >>>>> This series is required to handle TM state in CRIU. >>>>> Is there a chance to get it upstream soon ? >>>> >>>> We were waiting on the gdb support to make sure it had some testing. If it's working for CRIU that would be a good data point, have you actually tested it with CRIU? >>> >>> I just started integrating it in CRIU, my basic tests didn't report any >>> issue with the new ptrace API, but I can't state that it is bug free ;) >> >> Sure. But if it's working for CRIU that's at least postive :) > > I did additional tests and the Anshuman's series is working fine for > CRIU's support with the attached patch applied. > > Michael, could you please applied the attached patch among the > Anshuman's series ? Hi Michael, Is there any chance we get this series pushed in 4.7 ? Thanks, Laurent.
On Fri, 2016-05-27 at 10:07 +0200, Laurent Dufour wrote: > On 21/04/2016 18:00, Laurent Dufour wrote: > > On 13/04/2016 07:14, Michael Ellerman wrote: > > > On Mon, 2016-04-11 at 09:40 +0200, Laurent Dufour wrote: > > > > On 07/04/2016 23:49, Michael Ellerman wrote: > > > > > On 7 April 2016 7:23:46 pm AEST, Laurent Dufour <ldufour@linux.vnet.ibm.com> wrote: > > > > > > This series is required to handle TM state in CRIU. > > > > > > Is there a chance to get it upstream soon ? > > > > > > > > > > We were waiting on the gdb support to make sure it had some testing. If it's working for CRIU that would be a good data point, have you actually tested it with CRIU? > > > > > > > > I just started integrating it in CRIU, my basic tests didn't report any > > > > issue with the new ptrace API, but I can't state that it is bug free ;) > > > > > > Sure. But if it's working for CRIU that's at least postive :) > > > > I did additional tests and the Anshuman's series is working fine for > > CRIU's support with the attached patch applied. > > > > Michael, could you please applied the attached patch among the > > Anshuman's series ? > > Hi Michael, > > Is there any chance we get this series pushed in 4.7 ? Hi Laurent, Sorry no. My next branch closed for 4.7 about 3 weeks ago. This series has been blocked for a long time on the gdb support, but that is now working. However it still doesn't pass its own selftests, and I had some disagreements with the implementation - it duplicates a lot of code rather than refactoring things. I'm waiting on a patch from Cyril which will rework how the TM FP state is handled, and that should make this series easier to implement. The plan is that both should go into 4.8. cheers
On 05/31/2016 04:42 AM, Michael Ellerman wrote: > Hi Laurent, > > Sorry no. My next branch closed for 4.7 about 3 weeks ago. > > This series has been blocked for a long time on the gdb support, but that is > now working. However it still doesn't pass its own selftests, and I had some This series was clearing all of the selftests at the time it was posted. But yes, it has some assumptions from timing and sync perspective which gets broken some times as the kernel changes. Its been bit difficult to perfect the sync requirements as we can do only some much inside the transaction once it gets started. There are scopes here to improve these selftests but not clearing them today does not really mean the patches are now functionally broken. > disagreements with the implementation - it duplicates a lot of code rather > than refactoring things. hmm, sorry, I dont remember the context here. Can you please point to the discussion in this regard ? > > I'm waiting on a patch from Cyril which will rework how the TM FP state is > handled, and that should make this series easier to implement. Can you please elaborate on this ? Has this patch been posted in the mailing list ? How does this make it easier for us to implement these ELF notes ? > > The plan is that both should go into 4.8.
On 1 June 2016 at 18:26, Anshuman Khandual <khandual@linux.vnet.ibm.com> wrote: > On 05/31/2016 04:42 AM, Michael Ellerman wrote: > > Hi Laurent, > > > > Sorry no. My next branch closed for 4.7 about 3 weeks ago. > > > > This series has been blocked for a long time on the gdb support, but > that is > > now working. However it still doesn't pass its own selftests, and I had > some > > This series was clearing all of the selftests at the time it was posted. > But yes, it has some assumptions from timing and sync perspective which > gets broken some times as the kernel changes. Its been bit difficult to > perfect the sync requirements as we can do only some much inside the > transaction once it gets started. There are scopes here to improve these > selftests but not clearing them today does not really mean the patches are > now functionally broken. > > > disagreements with the implementation - it duplicates a lot of code > rather > > than refactoring things. > > hmm, sorry, I dont remember the context here. Can you please point to the > discussion in this regard ? > > > > > I'm waiting on a patch from Cyril which will rework how the TM FP state > is > > handled, and that should make this series easier to implement. > > Can you please elaborate on this ? Has this patch been posted in the > mailing > list ? How does this make it easier for us to implement these ELF notes ? Hi Anshuman, I'm doing a bit of a rewrite of the TM handling of the FP/VMX/VSX state. At the moment is is rather confusing since pt_regs is the always the 'live' state and theres a ckpt_regs that is the pt_regs for the checkpointed state. FPU/VMX/VSX is done differently which is really only creating confusion so I'm changing it to do the same at for pt_regs/ckpt_regs. Ultimately this is part of more work from me but Michael has told me that at least this bit is useful now so I'm splitting it off from the bigger picture and sending asap. At the very least it will make it easier to know what and where the transactional state it and where the checkpointed state is. It isn't on the list but I hope I'll get it out today. Cyril > > > > The plan is that both should go into 4.8. > > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev >
On 06/03/2016 03:56 AM, Cyril Bur wrote: > On 1 June 2016 at 18:26, Anshuman Khandual <khandual@linux.vnet.ibm.com> > wrote: > >> On 05/31/2016 04:42 AM, Michael Ellerman wrote: >>> Hi Laurent, >>> >>> Sorry no. My next branch closed for 4.7 about 3 weeks ago. >>> >>> This series has been blocked for a long time on the gdb support, but >> that is >>> now working. However it still doesn't pass its own selftests, and I had >> some >> >> This series was clearing all of the selftests at the time it was posted. >> But yes, it has some assumptions from timing and sync perspective which >> gets broken some times as the kernel changes. Its been bit difficult to >> perfect the sync requirements as we can do only some much inside the >> transaction once it gets started. There are scopes here to improve these >> selftests but not clearing them today does not really mean the patches are >> now functionally broken. >> >>> disagreements with the implementation - it duplicates a lot of code >> rather >>> than refactoring things. >> >> hmm, sorry, I dont remember the context here. Can you please point to the >> discussion in this regard ? >> >>> >>> I'm waiting on a patch from Cyril which will rework how the TM FP state >> is >>> handled, and that should make this series easier to implement. >> >> Can you please elaborate on this ? Has this patch been posted in the >> mailing >> list ? How does this make it easier for us to implement these ELF notes ? > > > Hi Anshuman, > > I'm doing a bit of a rewrite of the TM handling of the FP/VMX/VSX state. > > At the moment is is rather confusing since pt_regs is the always the 'live' > state > and theres a ckpt_regs that is the pt_regs for the checkpointed state. > FPU/VMX/VSX > is done differently which is really only creating confusion so I'm changing > it to do the > same at for pt_regs/ckpt_regs. Ultimately this is part of more work from me But that changes the basic semantics on which this ptrace series is written. With this change, a significant part of the ptrace series has to be changed. Its just an improvement on how we store running and check pointed values for FP/VSX/VMX registers inside the kernel. How does it improve ptrace interface from the user point of view ? If not, then why this change is necessary for the acceptance of this patch series ? This change should be implemented as an independent work and then necessary ptrace change can be incorporated there after. > but > Michael has told me that at least this bit is useful now so I'm splitting > it off from > the bigger picture and sending asap. At the very least it will make it > easier to know > what and where the transactional state it and where the checkpointed state > is. > > It isn't on the list but I hope I'll get it out today.
On Mon, 2016-06-06 at 14:27 +0530, Anshuman Khandual wrote: > On 06/03/2016 03:56 AM, Cyril Bur wrote: > > > > At the moment is is rather confusing since pt_regs is the always the 'live' > > state and theres a ckpt_regs that is the pt_regs for the checkpointed state. > > FPU/VMX/VSX is done differently which is really only creating confusion so I'm changing > > it to do the same at for pt_regs/ckpt_regs. Ultimately this is part of more work from me > > But that changes the basic semantics on which this ptrace series is written. > With this change, a significant part of the ptrace series has to be changed. Yes, that's the whole point. In fact half of the code should vanish, because the only difference between copying the live or checkpointed state out to userspace should be which regs struct you pass to the function. > Its just an improvement on how we store running and check pointed values for > FP/VSX/VMX registers inside the kernel. How does it improve ptrace interface > from the user point of view ? If not, then why this change is necessary for > the acceptance of this patch series ? Because the clean-ups never happen once a series is merged, and I'm left to deal with it. cheers
From 7a4f07c54afdbe7bef84d1f700ab9262f449513a Mon Sep 17 00:00:00 2001 From: Laurent Dufour <ldufour@linux.vnet.ibm.com> Date: Mon, 11 Apr 2016 18:59:16 +0200 Subject: [PATCH] ppc64: allow ptrace to set TM bits This patch allows the MSR bits relative to the Transactional memory state to be manipulated through the ptrace API. However, in the case the TM available bit is not set in the manipulated MSR, the changes are ignored. When dealing with the checkpointed MSR, we must be sure that the TM state bits will not be set since the checkpointed state can't be a transactional one. Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com> --- arch/powerpc/kernel/ptrace.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c index b063fc499c1d..5c792f0bf1ca 100644 --- a/arch/powerpc/kernel/ptrace.c +++ b/arch/powerpc/kernel/ptrace.c @@ -161,8 +161,12 @@ const char *regs_query_register_name(unsigned int offset) #ifdef CONFIG_PPC_ADV_DEBUG_REGS #define MSR_DEBUGCHANGE 0 #else +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM +#define MSR_DEBUGCHANGE (MSR_TS_MASK | MSR_SE | MSR_BE) +#else #define MSR_DEBUGCHANGE (MSR_SE | MSR_BE) #endif +#endif /* * Max register writeable via put_reg @@ -180,6 +184,12 @@ static unsigned long get_user_msr(struct task_struct *task) static int set_user_msr(struct task_struct *task, unsigned long msr) { +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM + if (!(task->thread.regs->msr & MSR_TM)) { + /* If TM is not available, discard TM bits changes */ + msr &= ~(MSR_TM | MSR_TS_MASK); + } +#endif task->thread.regs->msr &= ~MSR_DEBUGCHANGE; task->thread.regs->msr |= msr & MSR_DEBUGCHANGE; return 0; @@ -193,6 +203,7 @@ static unsigned long get_user_ckpt_msr(struct task_struct *task) static int set_user_ckpt_msr(struct task_struct *task, unsigned long msr) { + msr &= ~MSR_TS_MASK; /* Checkpoint state can't be in transaction */ task->thread.ckpt_regs.msr &= ~MSR_DEBUGCHANGE; task->thread.ckpt_regs.msr |= msr & MSR_DEBUGCHANGE; return 0; -- 1.9.1