Re: [BUG] random kernel crashes after THP rework on s390 (maybe also on PowerPC and ARM)

[Sebastian Ott]

On Fri, 12 Feb 2016, Will Deacon wrote:
> On Thu, Feb 11, 2016 at 08:57:02PM +0100, Gerald Schaefer wrote:
> > On Thu, 11 Feb 2016 21:09:42 +0200
> > "Kirill A. Shutemov"  wrote:
> > > On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
> > > > Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
> > > > he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
> > > > review of the THP rework patches, which cannot be bisected, revealed
> > > > commit fecffad "s390, thp: remove infrastructure for handling splitting 
> > > > PMDs"
> > > > (and also similar commits for other archs).
> > > > 
> > > > This commit removes the THP splitting bit and also the architecture
> > > > implementation of pmdp_splitting_flush(), which took care of the IPI for
> > > > fast_gup serialization. The commit message says
> > > > 
> > > > pmdp_splitting_flush() is not needed too: on splitting PMD we will 
> > > > do
> > > > pmdp_clear_flush() + set_pte_at().  pmdp_clear_flush() will do IPI 
> > > > as
> > > > needed for fast_gup
> > > > 
> > > > The assumption that a TLB flush will also produce an IPI is wrong on 
> > > > s390,
> > > > and maybe also on other architectures, and I thought that this was 
> > > > actually
> > > > the main reason for having an arch-specific pmdp_splitting_flush().
> > > > 
> > > > At least PowerPC and ARM also had an individual implementation of
> > > > pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
> > > > flush to send the IPI, and those were also removed. Putting the arch
> > > > maintainers and mailing lists on cc to verify.
> > > > 
> > > > On s390 this will break the IPI serialization against fast_gup, which
> > > > would certainly explain the random kernel crashes, please revert or fix
> > > > the pmdp_splitting_flush() removal.
> > > 
> > > Sorry for that.
> > > 
> > > I believe, the problem was already addressed for PowerPC:
> > > 
> > > http://lkml.kernel.org/g/454980831-16631-1-git-send-email-aneesh.ku...@linux.vnet.ibm.com
> > > 
> > > I think kick_all_cpus_sync() in arch-specific pmdp_invalidate() would do
> > > the trick, right?
> > 
> > Hmm, not sure about that. After pmdp_invalidate(), a pmd_none() check in
> > fast_gup will still return false, because the pmd is not empty (at least
> > on s390). So I don't see spontaneously how it will help fast_gup to break
> > out to the slow path in case of THP splitting.
> > 
> > > 
> > > If yes, I'll prepare patch tomorrow (some sleep required).
> > > 
> > 
> > We'll check if adding kick_all_cpus_sync() to pmdp_invalidate() helps.
> > It would also be good if Martin has a look at this, he'll return on
> > Monday.
> 
> Do you have a reliable way to trigger the "random kernel crashes"? We've not
> seen anything reported on arm64, but I don't see why we wouldn't be affected
> by the same bug and it would be good to confirm and validate a fix.

My testcase was compiling the kernel. Most of the time my test system
didn't survive a single compile run. During bisect I did at least 20
compile runs to flag a commit as good.

Sebastian

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] ibmvfc: byteswap scsi_id, wwpn, and node_name prior to logging

[Johannes Thumshirn]

On Thu, Feb 11, 2016 at 04:24:35PM -0600, Tyrel Datwyler wrote:
> When logging async events the scsi_id, wwpn, and node_name values
> are used directly from the CRQ struct which are of type __be64. This
> can be confusing to someone looking through the log on a LE system.
> Instead byteswap these values to host endian prior to logging.
> 
> Signed-off-by: Tyrel Datwyler 

Reviewed-by: Johannes Thumshirn 

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v5] powerpc32: provide VIRT_CPU_ACCOUNTING

[Denis Kirjanov]

On 2/11/16, Christophe Leroy  wrote:
> This patch provides VIRT_CPU_ACCOUTING to PPC32 architecture.
> PPC32 doesn't have the PACA structure, so we use the task_info
> structure to store the accounting data.
>
> In order to reuse on PPC32 the PPC64 functions, all u64 data has
> been replaced by 'unsigned long' so that it is u32 on PPC32 and
> u64 on PPC64
>
> Signed-off-by: Christophe Leroy 

I'll try it out on my ppc32 machines.

Thanks!

> ---
> Changes in v3: unlike previous version of the patch that was inspired
> from IA64 architecture, this new version tries to reuse as much as
> possible the PPC64 implementation.
>
> PPC32 doesn't have PACA and past discusion on v2 version has shown
> that it is not worth implementing a PACA in PPC32 architecture
> (see below benh opinion)
>
> benh: PACA is actually a data structure and you really really don't want it
> on ppc32 :-) Having a register point to current works, having a register
> point to per-cpu data instead works too (ie, change what we do today),
> but don't introduce a PACA *please* :-)
>
> Changes in v4: ACCOUNT_CPU_USER_ENTRY/EXIT() needed updates in other
> places than entry_32.S and entry_64.S (reported by kbuild-robot)
> Related defines in asm-offset.c need to be conditional to
> CONFIG_VIRT_CPU_ACCOUNTING_NATIVE (reported by kbuild-robot)
>
> Changes in v5: Using PPC_LL et PPC_STL instead of defining new macros
> AC_LD and AC_STD
>
>  arch/powerpc/Kconfig |  1 +
>  arch/powerpc/include/asm/cputime.h   |  4 
>  arch/powerpc/include/asm/exception-64s.h |  2 +-
>  arch/powerpc/include/asm/ppc_asm.h   | 24 ++--
>  arch/powerpc/include/asm/reg.h   |  1 +
>  arch/powerpc/include/asm/thread_info.h   | 11 +
>  arch/powerpc/kernel/asm-offsets.c|  7 ++
>  arch/powerpc/kernel/entry_32.S   | 17 ++
>  arch/powerpc/kernel/entry_64.S   |  6 ++---
>  arch/powerpc/kernel/exceptions-64e.S |  4 ++--
>  arch/powerpc/kernel/time.c   | 38
> ++--
>  arch/powerpc/platforms/Kconfig.cputype   |  1 -
>  12 files changed, 90 insertions(+), 26 deletions(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 3a557be..57ce4ff 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -159,6 +159,7 @@ config PPC
>   select ARCH_HAS_DEVMEM_IS_ALLOWED
>   select HAVE_ARCH_SECCOMP_FILTER
>   select ARCH_HAS_UBSAN_SANITIZE_ALL
> + select HAVE_VIRT_CPU_ACCOUNTING
>
>  config GENERIC_CSUM
>   def_bool CPU_LITTLE_ENDIAN
> diff --git a/arch/powerpc/include/asm/cputime.h
> b/arch/powerpc/include/asm/cputime.h
> index e245255..c4c33be 100644
> --- a/arch/powerpc/include/asm/cputime.h
> +++ b/arch/powerpc/include/asm/cputime.h
> @@ -230,7 +230,11 @@ static inline cputime_t clock_t_to_cputime(const
> unsigned long clk)
>
>  #define cputime64_to_clock_t(ct) cputime_to_clock_t((cputime_t)(ct))
>
> +#ifdef CONFIG_PPC64
>  static inline void arch_vtime_task_switch(struct task_struct *tsk) { }
> +#else
> +void arch_vtime_task_switch(struct task_struct *tsk);
> +#endif
>
>  #endif /* __KERNEL__ */
>  #endif /* CONFIG_VIRT_CPU_ACCOUNTING_NATIVE */
> diff --git a/arch/powerpc/include/asm/exception-64s.h
> b/arch/powerpc/include/asm/exception-64s.h
> index 93ae809..8bc38d1 100644
> --- a/arch/powerpc/include/asm/exception-64s.h
> +++ b/arch/powerpc/include/asm/exception-64s.h
> @@ -287,7 +287,7 @@ do_kvm_##n:   
> \
>   std r0,GPR0(r1);/* save r0 in stackframe*/ \
>   std r10,GPR1(r1);   /* save r1 in stackframe*/ \
>   beq 4f; /* if from kernel mode  */ \
> - ACCOUNT_CPU_USER_ENTRY(r9, r10);   \
> + ACCOUNT_CPU_USER_ENTRY(r13, r9, r10);  \
>   SAVE_PPR(area, r9, r10);   \
>  4:   EXCEPTION_PROLOG_COMMON_2(area)\
>   EXCEPTION_PROLOG_COMMON_3(n)   \
> diff --git a/arch/powerpc/include/asm/ppc_asm.h
> b/arch/powerpc/include/asm/ppc_asm.h
> index 499d9f8..07d1bfc 100644
> --- a/arch/powerpc/include/asm/ppc_asm.h
> +++ b/arch/powerpc/include/asm/ppc_asm.h
> @@ -24,27 +24,27 @@
>   */
>
>  #ifndef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
> -#define ACCOUNT_CPU_USER_ENTRY(ra, rb)
> -#define ACCOUNT_CPU_USER_EXIT(ra, rb)
> +#define ACCOUNT_CPU_USER_ENTRY(ptr, ra, rb)
> +#define ACCOUNT_CPU_USER_EXIT(ptr, ra, rb)
>  #define ACCOUNT_STOLEN_TIME
>  #else
> -#define ACCOUNT_CPU_USER_ENTRY(ra, rb)   
> \
> +#define ACCOUNT_CPU_USER_ENTRY(ptr, ra, rb)  \
>   MFTB(ra);   /* get timebase */  \
> - ld  rb,PACA_STARTTIME_USER(r13); 

Re: [BUG] random kernel crashes after THP rework on s390 (maybe also on PowerPC and ARM)

[Will Deacon]

On Thu, Feb 11, 2016 at 08:57:02PM +0100, Gerald Schaefer wrote:
> On Thu, 11 Feb 2016 21:09:42 +0200
> "Kirill A. Shutemov"  wrote:
> > On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
> > > Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
> > > he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
> > > review of the THP rework patches, which cannot be bisected, revealed
> > > commit fecffad "s390, thp: remove infrastructure for handling splitting 
> > > PMDs"
> > > (and also similar commits for other archs).
> > > 
> > > This commit removes the THP splitting bit and also the architecture
> > > implementation of pmdp_splitting_flush(), which took care of the IPI for
> > > fast_gup serialization. The commit message says
> > > 
> > > pmdp_splitting_flush() is not needed too: on splitting PMD we will do
> > > pmdp_clear_flush() + set_pte_at().  pmdp_clear_flush() will do IPI as
> > > needed for fast_gup
> > > 
> > > The assumption that a TLB flush will also produce an IPI is wrong on s390,
> > > and maybe also on other architectures, and I thought that this was 
> > > actually
> > > the main reason for having an arch-specific pmdp_splitting_flush().
> > > 
> > > At least PowerPC and ARM also had an individual implementation of
> > > pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
> > > flush to send the IPI, and those were also removed. Putting the arch
> > > maintainers and mailing lists on cc to verify.
> > > 
> > > On s390 this will break the IPI serialization against fast_gup, which
> > > would certainly explain the random kernel crashes, please revert or fix
> > > the pmdp_splitting_flush() removal.
> > 
> > Sorry for that.
> > 
> > I believe, the problem was already addressed for PowerPC:
> > 
> > http://lkml.kernel.org/g/454980831-16631-1-git-send-email-aneesh.ku...@linux.vnet.ibm.com
> > 
> > I think kick_all_cpus_sync() in arch-specific pmdp_invalidate() would do
> > the trick, right?
> 
> Hmm, not sure about that. After pmdp_invalidate(), a pmd_none() check in
> fast_gup will still return false, because the pmd is not empty (at least
> on s390). So I don't see spontaneously how it will help fast_gup to break
> out to the slow path in case of THP splitting.
> 
> > 
> > If yes, I'll prepare patch tomorrow (some sleep required).
> > 
> 
> We'll check if adding kick_all_cpus_sync() to pmdp_invalidate() helps.
> It would also be good if Martin has a look at this, he'll return on
> Monday.

Do you have a reliable way to trigger the "random kernel crashes"? We've not
seen anything reported on arm64, but I don't see why we wouldn't be affected
by the same bug and it would be good to confirm and validate a fix.

Cheers,

Will
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [BUG] random kernel crashes after THP rework on s390 (maybe also on PowerPC and ARM)

[Sebastian Ott]

On Thu, 11 Feb 2016, Kirill A. Shutemov wrote:
> On Thu, Feb 11, 2016 at 09:09:42PM +0200, Kirill A. Shutemov wrote:
> > On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
> > > Hi,
> > > 
> > > Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
> > > he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
> > > review of the THP rework patches, which cannot be bisected, revealed
> > > commit fecffad "s390, thp: remove infrastructure for handling splitting 
> > > PMDs"
> > > (and also similar commits for other archs).
> > > 
> > > This commit removes the THP splitting bit and also the architecture
> > > implementation of pmdp_splitting_flush(), which took care of the IPI for
> > > fast_gup serialization. The commit message says
> > > 
> > > pmdp_splitting_flush() is not needed too: on splitting PMD we will do
> > > pmdp_clear_flush() + set_pte_at().  pmdp_clear_flush() will do IPI as
> > > needed for fast_gup
> > > 
> > > The assumption that a TLB flush will also produce an IPI is wrong on s390,
> > > and maybe also on other architectures, and I thought that this was 
> > > actually
> > > the main reason for having an arch-specific pmdp_splitting_flush().
> > > 
> > > At least PowerPC and ARM also had an individual implementation of
> > > pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
> > > flush to send the IPI, and those were also removed. Putting the arch
> > > maintainers and mailing lists on cc to verify.
> > > 
> > > On s390 this will break the IPI serialization against fast_gup, which
> > > would certainly explain the random kernel crashes, please revert or fix
> > > the pmdp_splitting_flush() removal.
> > 
> > Sorry for that.
> > 
> > I believe, the problem was already addressed for PowerPC:
> > 
> > http://lkml.kernel.org/g/454980831-16631-1-git-send-email-aneesh.ku...@linux.vnet.ibm.com
> 
> Correct link is
> 
> http://lkml.kernel.org/g/1454980831-16631-1-git-send-email-aneesh.ku...@linux.vnet.ibm.com
> 

Based on your suggestion Gerald provided the following patch but sadly it
didn't fix the problem.

Sebastian


---
 arch/s390/include/asm/pgtable.h |2 ++
 1 file changed, 2 insertions(+)

--- a/arch/s390/include/asm/pgtable.h
+++ b/arch/s390/include/asm/pgtable.h
@@ -1587,6 +1587,8 @@ static inline void pmdp_invalidate(struc
   unsigned long address, pmd_t *pmdp)
 {
pmdp_flush_direct(vma->vm_mm, address, pmdp);
+   /* Serialize against fast_gup with IPI */
+   kick_all_cpus_sync();
 }

 #define __HAVE_ARCH_PMDP_SET_WRPROTECT

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [BUG] random kernel crashes after THP rework on s390 (maybe also on PowerPC and ARM)

[Gerald Schaefer]

On Fri, 12 Feb 2016 09:34:33 +0530
"Aneesh Kumar K.V"  wrote:

> Gerald Schaefer  writes:
> 
> > On Thu, 11 Feb 2016 21:09:42 +0200
> > "Kirill A. Shutemov"  wrote:
> >
> >> On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
> >> > Hi,
> >> > 
> >> > Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
> >> > he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
> >> > review of the THP rework patches, which cannot be bisected, revealed
> >> > commit fecffad "s390, thp: remove infrastructure for handling splitting 
> >> > PMDs"
> >> > (and also similar commits for other archs).
> >> > 
> >> > This commit removes the THP splitting bit and also the architecture
> >> > implementation of pmdp_splitting_flush(), which took care of the IPI for
> >> > fast_gup serialization. The commit message says
> >> > 
> >> > pmdp_splitting_flush() is not needed too: on splitting PMD we will do
> >> > pmdp_clear_flush() + set_pte_at().  pmdp_clear_flush() will do IPI as
> >> > needed for fast_gup
> >> > 
> >> > The assumption that a TLB flush will also produce an IPI is wrong on 
> >> > s390,
> >> > and maybe also on other architectures, and I thought that this was 
> >> > actually
> >> > the main reason for having an arch-specific pmdp_splitting_flush().
> >> > 
> >> > At least PowerPC and ARM also had an individual implementation of
> >> > pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
> >> > flush to send the IPI, and those were also removed. Putting the arch
> >> > maintainers and mailing lists on cc to verify.
> >> > 
> >> > On s390 this will break the IPI serialization against fast_gup, which
> >> > would certainly explain the random kernel crashes, please revert or fix
> >> > the pmdp_splitting_flush() removal.
> >> 
> >> Sorry for that.
> >> 
> >> I believe, the problem was already addressed for PowerPC:
> >> 
> >> http://lkml.kernel.org/g/454980831-16631-1-git-send-email-aneesh.ku...@linux.vnet.ibm.com
> >> 
> >> I think kick_all_cpus_sync() in arch-specific pmdp_invalidate() would do
> >> the trick, right?
> >
> > Hmm, not sure about that. After pmdp_invalidate(), a pmd_none() check in
> > fast_gup will still return false, because the pmd is not empty (at least
> > on s390).
> 
> Why can't we do this ? I did this for ppc64.
> 
>  void pmdp_invalidate(struct vm_area_struct *vma, unsigned long address,
>pmd_t *pmdp)
>  {
> - pmd_hugepage_update(vma->vm_mm, address, pmdp, _PAGE_PRESENT, 0);
> + pmd_hugepage_update(vma->vm_mm, address, pmdp, ~0UL, 0);
> 

Wouldn't that semantically change what pmdp_invalidate() was supposed to
do? The comment before the call says "the pmd_trans_huge and
pmd_trans_splitting must remain set at all times on the pmd". So, after
removing pmd_trans_splitting, it seems to be necessary to at least keep
pmd_trans_huge set.

In your case, the pmd would be completely cleared, which may help to find
it in fast_gup with pmd_none(), but I'm not sure if this would open up
other problems, e.g. with concurrent page faults. But I must also admit that
my THP overview got a little rusty.

> >So I don't see spontaneously how it will help fast_gup to break
> > out to the slow path in case of THP splitting.
> >
> >> 
> >> If yes, I'll prepare patch tomorrow (some sleep required).
> >> 
> >
> > We'll check if adding kick_all_cpus_sync() to pmdp_invalidate() helps.
> > It would also be good if Martin has a look at this, he'll return on
> > Monday.
> 
> -aneesh
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-s390" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 1/7] ibmvscsi: Correct values for several viosrp_crq_format enums

[Manoj Kumar]

Reviewed-by: Manoj Kumar 

---
Manoj Kumar

On 2/10/2016 7:32 PM, Tyrel Datwyler wrote:

The enum values for VIOSRP_LINUX_FORMAT and VIOSRP_INLINE_FORMAT are
off by one. They are currently defined as 0x06 and 0x07 respetively.
These values are defined in PAPR correctly as 0x05 and 0x06. This
inconsistency has gone unnoticed as neither enum is currently used.
The possible future support of PING messages between the VIOS and
client adapter relies on VIOSRP_INLINE_FORMAT crq messages.
Corrected these enum values to match PAPR definitions.

Signed-off-by: Tyrel Datwyler 
Reviewed-by: Johannes Thumshirn 
---
  drivers/scsi/ibmvscsi/viosrp.h | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ibmvscsi/viosrp.h b/drivers/scsi/ibmvscsi/viosrp.h
index 1162430..d1044e9 100644
--- a/drivers/scsi/ibmvscsi/viosrp.h
+++ b/drivers/scsi/ibmvscsi/viosrp.h
@@ -56,8 +56,8 @@ enum viosrp_crq_formats {
VIOSRP_MAD_FORMAT = 0x02,
VIOSRP_OS400_FORMAT = 0x03,
VIOSRP_AIX_FORMAT = 0x04,
-   VIOSRP_LINUX_FORMAT = 0x06,
-   VIOSRP_INLINE_FORMAT = 0x07
+   VIOSRP_LINUX_FORMAT = 0x05,
+   VIOSRP_INLINE_FORMAT = 0x06
  };

  enum viosrp_crq_status {



___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 2/7] ibmvscsi: Add and use enums for valid CRQ header values

[Manoj Kumar]

Tyrel:

Thanks for incorporating the suggestions.

Reviewed-by: Manoj Kumar 

---
Manoj Kumar

On 2/10/2016 7:32 PM, Tyrel Datwyler wrote:

The PAPR defines four valid header values for the first byte of a
CRQ message. Namely, an unused/empty message (0x00), a valid
command/response entry (0x80), a valid initialization entry (0xC0),
and a valid transport event (0xFF). Further, initialization responses
have two formats namely initialize (0x01) and initialize complete
(0x02). Define these values as enums and use them in the code in
place of their magic number equivalents.

Signed-off-by: Tyrel Datwyler 
---
  drivers/scsi/ibmvscsi/ibmvscsi.c | 18 +-
  drivers/scsi/ibmvscsi/viosrp.h   | 12 
  2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index adfef9d..c888ea1 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -182,7 +182,7 @@ static struct viosrp_crq *crq_queue_next_crq(struct 
crq_queue *queue)

spin_lock_irqsave(>lock, flags);
crq = >msgs[queue->cur];
-   if (crq->valid & 0x80) {
+   if (crq->valid != VIOSRP_CRQ_FREE) {
if (++queue->cur == queue->size)
queue->cur = 0;

@@ -231,7 +231,7 @@ static void ibmvscsi_task(void *data)
/* Pull all the valid messages off the CRQ */
while ((crq = crq_queue_next_crq(>queue)) != NULL) {
ibmvscsi_handle_crq(crq, hostdata);
-   crq->valid = 0x00;
+   crq->valid = VIOSRP_CRQ_FREE;
}

vio_enable_interrupts(vdev);
@@ -239,7 +239,7 @@ static void ibmvscsi_task(void *data)
if (crq != NULL) {
vio_disable_interrupts(vdev);
ibmvscsi_handle_crq(crq, hostdata);
-   crq->valid = 0x00;
+   crq->valid = VIOSRP_CRQ_FREE;
} else {
done = 1;
}
@@ -474,7 +474,7 @@ static int initialize_event_pool(struct event_pool *pool,
struct srp_event_struct *evt = >events[i];
memset(>crq, 0x00, sizeof(evt->crq));
atomic_set(>free, 1);
-   evt->crq.valid = 0x80;
+   evt->crq.valid = VIOSRP_CRQ_CMD_RSP;
evt->crq.IU_length = cpu_to_be16(sizeof(*evt->xfer_iu));
evt->crq.IU_data_ptr = cpu_to_be64(pool->iu_token +
sizeof(*evt->xfer_iu) * i);
@@ -1767,9 +1767,9 @@ static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
struct srp_event_struct *evt_struct =
(__force struct srp_event_struct *)crq->IU_data_ptr;
switch (crq->valid) {
-   case 0xC0:  /* initialization */
+   case VIOSRP_CRQ_INIT_RSP:   /* initialization */
switch (crq->format) {
-   case 0x01:  /* Initialization message */
+   case VIOSRP_CRQ_INIT:   /* Initialization message */
dev_info(hostdata->dev, "partner initialized\n");
/* Send back a response */
rc = ibmvscsi_send_crq(hostdata, 0xC002LL, 
0);
@@ -1781,7 +1781,7 @@ static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
}

break;
-   case 0x02:  /* Initialization response */
+   case VIOSRP_CRQ_INIT_COMPLETE:  /* Initialization response */
dev_info(hostdata->dev, "partner initialization 
complete\n");

/* Now login */
@@ -1791,7 +1791,7 @@ static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
dev_err(hostdata->dev, "unknown crq message type: %d\n", 
crq->format);
}
return;
-   case 0xFF:  /* Hypervisor telling us the connection is closed */
+   case VIOSRP_CRQ_XPORT_EVENT:/* Hypervisor telling us the connection 
is closed */
scsi_block_requests(hostdata->host);
atomic_set(>request_limit, 0);
if (crq->format == 0x06) {
@@ -1807,7 +1807,7 @@ static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
ibmvscsi_reset_host(hostdata);
}
return;
-   case 0x80:  /* real payload */
+   case VIOSRP_CRQ_CMD_RSP:/* real payload */
break;
default:
dev_err(hostdata->dev, "got an invalid message type 0x%02x\n",
diff --git a/drivers/scsi/ibmvscsi/viosrp.h b/drivers/scsi/ibmvscsi/viosrp.h
index d1044e9..3d20851 100644
--- a/drivers/scsi/ibmvscsi/viosrp.h
+++ b/drivers/scsi/ibmvscsi/viosrp.h
@@ -51,6 +51,18 @@ union srp_iu {
u8 reserved[SRP_MAX_IU_LEN];
  };

Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build

[Petr Mladek]

On Sat 2016-02-13 03:13:29, Balbir Singh wrote:
> On Thu, 2016-01-28 at 16:32 +0100, Torsten Duwe wrote:
> > From: Petr Mladek 
> > 
> > Livepatch works on x86_64 and s390 only when the ftrace call
> > is at the very beginning of the function. But PPC is different.
> > We need to handle TOC and save LR there before calling the
> > global ftrace handler.
> > 
> > Now, the problem is that the extra operations have different
> > length on PPC depending on the used gcc version. It is
> > 4 instructions (16 bytes) before gcc-6 and only 3 instructions
> > (12 bytes) with gcc-6.
> > 
> > This patch tries to detect the offset a generic way during
> > build. It assumes that the offset of the ftrace location
> > is the same for all functions. It modifies the existing
> > recordmcount tool that is able to find read mcount locations
> > directly from the object files. It adds an option -p
> > to print the first found offset.
> > 
> > The recordmcount tool is then used in the kernel/livepatch
> > subdirectory to generate a header file. It defines
> > a constant that is used to compute the ftrace location
> > from the function address.
> > 
> > Finally, we have to enable the C implementation of the
> > recordmcount tool to be used on PPC and S390. It seems
> > to work fine there. It should be more reliable because
> > it reads the standardized elf structures. The old perl
> > implementation uses rather complex regular expressions
> > to parse objdump output and is therefore much more tricky.
> 
> I'm still missing something, I'm getting offset as 8
>
> When I run, I get
> 
> scripts/recordmcount -p kernel/livepatch/core.o 
> #define KLP_FTRACE_LOCATION 8
> 
> scripts/recordmcount -p kernel/livepatch/ftrace-test.o 
> #define KLP_FTRACE_LOCATION 8
> 
> My sample fails as well, since the expected offset is 16.
> I guess the script is being run against a not so good
> test.

I guess that you used a broken gcc and cheated the check
to pass the compilation. Did you, please?

The test used to detect the offset is using a minimalistic
function is is afftected by the gcc bug.

The patch below might be used to cheat the offset check as well.

Torsten, please mention this somewhere if you, just by chance,
send a new version of the patchset.

From f6a438a3f2f60cc1acc859b41d0cc9259c9a331e Mon Sep 17 00:00:00 2001
From: root 
Date: Tue, 2 Feb 2016 15:35:06 +0100
Subject: [PATCH 2/2] livepatch: Make sure the TOC is handled when detecting
 ftrace location

There seems to be a bug in gcc on PPC. It does not handle TOC
if the function does not access global variables or functions
by default. But it should when profiling is enabled.

This patch works around this problem by adding a call
to a global function.

This patch is for testing only!
---
 kernel/livepatch/ftrace-test.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/livepatch/ftrace-test.c b/kernel/livepatch/ftrace-test.c
index 22f0c54bf7b3..a3b7aabb67e5 100644
--- a/kernel/livepatch/ftrace-test.c
+++ b/kernel/livepatch/ftrace-test.c
@@ -1,6 +1,9 @@
 /* Sample code to figure out mcount location offset */
+#include 
+
 
 int test(int a)
 {
+   printk("%d\n", a);
return ++a;
 }
-- 
1.8.5.6

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 3/7] ibmvscsi: Replace magic values in set_adpater_info() with defines

[James Bottomley]

On Wed, 2016-02-10 at 19:32 -0600, Tyrel Datwyler wrote:
> Add defines for mad version and mad os_type, and replace the magic
> numbers in set_adapter_info() accordingly.
> 
> Signed-off-by: Tyrel Datwyler 
> ---

Is there some reason you didn't carry the review tag over from this:

http://mid.gmane.org/20160204084459.gw27...@c203.arch.suse.de

?

James

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 3/7] ibmvscsi: Replace magic values in set_adpater_info() with defines

[James Bottomley]

On Fri, 2016-02-12 at 08:51 -0800, Tyrel Datwyler wrote:
> On 02/12/2016 08:43 AM, James Bottomley wrote:
> > On Wed, 2016-02-10 at 19:32 -0600, Tyrel Datwyler wrote:
> > > Add defines for mad version and mad os_type, and replace the
> > > magic
> > > numbers in set_adapter_info() accordingly.
> > > 
> > > Signed-off-by: Tyrel Datwyler 
> > > ---
> > 
> > Is there some reason you didn't carry the review tag over from
> > this:
> > 
> > http://mid.gmane.org/20160204084459.gw27...@c203.arch.suse.de
> > 
> > ?
> > 
> > James
> 
> The patch is slightly changed from v1. A define for AIX os type was
> added as mentioned in the cover letter v2 changes, and I moved the
> defines to the mad_adapter_info_data structure around the fields they 
> apply.

OK we need a re-review then ...

James


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [BUG] random kernel crashes after THP rework on s390 (maybe also on PowerPC and ARM)

[Gerald Schaefer]

On Fri, 12 Feb 2016 16:57:27 +0100
Christian Borntraeger  wrote:

> On 02/12/2016 04:41 PM, Kirill A. Shutemov wrote:
> > On Thu, Feb 11, 2016 at 08:57:02PM +0100, Gerald Schaefer wrote:
> >> On Thu, 11 Feb 2016 21:09:42 +0200
> >> "Kirill A. Shutemov"  wrote:
> >>
> >>> On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
>  Hi,
> 
>  Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
>  he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
>  review of the THP rework patches, which cannot be bisected, revealed
>  commit fecffad "s390, thp: remove infrastructure for handling splitting 
>  PMDs"
>  (and also similar commits for other archs).
> 
>  This commit removes the THP splitting bit and also the architecture
>  implementation of pmdp_splitting_flush(), which took care of the IPI for
>  fast_gup serialization. The commit message says
> 
>  pmdp_splitting_flush() is not needed too: on splitting PMD we will do
>  pmdp_clear_flush() + set_pte_at().  pmdp_clear_flush() will do IPI as
>  needed for fast_gup
> 
>  The assumption that a TLB flush will also produce an IPI is wrong on 
>  s390,
>  and maybe also on other architectures, and I thought that this was 
>  actually
>  the main reason for having an arch-specific pmdp_splitting_flush().
> 
>  At least PowerPC and ARM also had an individual implementation of
>  pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
>  flush to send the IPI, and those were also removed. Putting the arch
>  maintainers and mailing lists on cc to verify.
> 
>  On s390 this will break the IPI serialization against fast_gup, which
>  would certainly explain the random kernel crashes, please revert or fix
>  the pmdp_splitting_flush() removal.
> >>>
> >>> Sorry for that.
> >>>
> >>> I believe, the problem was already addressed for PowerPC:
> >>>
> >>> http://lkml.kernel.org/g/454980831-16631-1-git-send-email-aneesh.ku...@linux.vnet.ibm.com
> >>>
> >>> I think kick_all_cpus_sync() in arch-specific pmdp_invalidate() would do
> >>> the trick, right?
> >>
> >> Hmm, not sure about that. After pmdp_invalidate(), a pmd_none() check in
> >> fast_gup will still return false, because the pmd is not empty (at least
> >> on s390). So I don't see spontaneously how it will help fast_gup to break
> >> out to the slow path in case of THP splitting.
> > 
> > What pmdp_flush_direct() does in pmdp_invalidate()? It's hard to unwrap for 
> > me :-/
> > Does it make the pmd !pmd_present()?
> 
> It uses the idte instruction, which in an atomic fashion flushes the 
> associated
> TLB entry and changes the value of the pmd entry to invalid. This comes from 
> the
> HW requirement to not  change a PTE/PMD that might be still in use, other 
> than 
> with special instructions that does the tlb handling and the invalidation 
> together.

Correct, and it does _not_ make the pmd !pmd_present(), that would only be the
case after a _clear_flush(). It only marks the pmd as invalid and flushes,
so that it cannot generate a new TLB entry before the following pmd_populate(),
but it keeps its other content. This is to fulfill the requirements outlined in
the comment in mm/huge_memory.c before the call to pmdp_invalidate(). And
independent from that comment, we would need such an _invalidate() or
_clear_flush() on s390 before the pmd_populate() because of the HW details
that Christian described.

Reading the comment again, I do now notice that it also says "mark the current
pmd notpresent", which we cannot do w/o losing the huge and (formerly) splitting
bits, but it also shouldn't be needed to provide the "single TLB guarantee" that
is required from the comment. So, a pmd_present() check on s390 in this state
would still return true. Not sure yet if this is a problem, need more thinking,
this behavior was already present before the THP rework but maybe it was OK
before and is not OK now.

At least for fast_gup this should not be a problem though.

> (It also does some some other magic to the attach_count, which might hold off
> finish_arch_post_lock_switch while some flushing is happening, but this should
> be unrelated here)
> 
> 
> > I'm also confused by pmd_none() is equal to !pmd_present() on s390. Hm?
> 
> Don't know, Gerald or Martin?

The implementation frequently changes depending on how many new bits Martin
needs to squeeze out :-)
We don't have a _PAGE_PRESENT bit for pmds, so pmd_present() just checks if the
entry is not empty. pmd_none() of course does the opposite, it checks if it is
empty.

> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-s390" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

___

Re: [PATCH v2 3/7] ibmvscsi: Replace magic values in set_adpater_info() with defines

[Tyrel Datwyler]

On 02/12/2016 08:43 AM, James Bottomley wrote:
> On Wed, 2016-02-10 at 19:32 -0600, Tyrel Datwyler wrote:
>> Add defines for mad version and mad os_type, and replace the magic
>> numbers in set_adapter_info() accordingly.
>>
>> Signed-off-by: Tyrel Datwyler 
>> ---
> 
> Is there some reason you didn't carry the review tag over from this:
> 
> http://mid.gmane.org/20160204084459.gw27...@c203.arch.suse.de
> 
> ?
> 
> James

The patch is slightly changed from v1. A define for AIX os type was
added as mentioned in the cover letter v2 changes, and I moved the
defines to the mad_adapter_info_data structure around the fields they apply.

-Tyrel

> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 3/7] ibmvscsi: Replace magic values in set_adpater_info() with defines

[Martin K. Petersen]

> "James" == James Bottomley  writes:

James> OK we need a re-review then ...

I'll hold off pushing for now.

-- 
Martin K. Petersen  Oracle Linux Engineering
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 0/7] ibmvscsi code cleanup

[Martin K. Petersen]

> "Tyrel" == Tyrel Datwyler  writes:

Tyrel> Fixed up a couple spots that were out of line with the PAPR in
Tyrel> regards to its defined VSCSI protocol. Did away with some magic
Tyrel> numbers directly in the code. Fixed a minor endian issue.

Applied to 4.6/scsi-queue.

(And added the missing review from Johannes for patch 3 as pointed out
by James).

-- 
Martin K. Petersen  Oracle Linux Engineering
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [BUG] random kernel crashes after THP rework on s390 (maybe also on PowerPC and ARM)

[Kirill A. Shutemov]

On Thu, Feb 11, 2016 at 08:57:02PM +0100, Gerald Schaefer wrote:
> On Thu, 11 Feb 2016 21:09:42 +0200
> "Kirill A. Shutemov"  wrote:
> 
> > On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
> > > Hi,
> > > 
> > > Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
> > > he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
> > > review of the THP rework patches, which cannot be bisected, revealed
> > > commit fecffad "s390, thp: remove infrastructure for handling splitting 
> > > PMDs"
> > > (and also similar commits for other archs).
> > > 
> > > This commit removes the THP splitting bit and also the architecture
> > > implementation of pmdp_splitting_flush(), which took care of the IPI for
> > > fast_gup serialization. The commit message says
> > > 
> > > pmdp_splitting_flush() is not needed too: on splitting PMD we will do
> > > pmdp_clear_flush() + set_pte_at().  pmdp_clear_flush() will do IPI as
> > > needed for fast_gup
> > > 
> > > The assumption that a TLB flush will also produce an IPI is wrong on s390,
> > > and maybe also on other architectures, and I thought that this was 
> > > actually
> > > the main reason for having an arch-specific pmdp_splitting_flush().
> > > 
> > > At least PowerPC and ARM also had an individual implementation of
> > > pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
> > > flush to send the IPI, and those were also removed. Putting the arch
> > > maintainers and mailing lists on cc to verify.
> > > 
> > > On s390 this will break the IPI serialization against fast_gup, which
> > > would certainly explain the random kernel crashes, please revert or fix
> > > the pmdp_splitting_flush() removal.
> > 
> > Sorry for that.
> > 
> > I believe, the problem was already addressed for PowerPC:
> > 
> > http://lkml.kernel.org/g/454980831-16631-1-git-send-email-aneesh.ku...@linux.vnet.ibm.com
> > 
> > I think kick_all_cpus_sync() in arch-specific pmdp_invalidate() would do
> > the trick, right?
> 
> Hmm, not sure about that. After pmdp_invalidate(), a pmd_none() check in
> fast_gup will still return false, because the pmd is not empty (at least
> on s390). So I don't see spontaneously how it will help fast_gup to break
> out to the slow path in case of THP splitting.

What pmdp_flush_direct() does in pmdp_invalidate()? It's hard to unwrap for me 
:-/
Does it make the pmd !pmd_present()?

I'm also confused by pmd_none() is equal to !pmd_present() on s390. Hm?

-- 
 Kirill A. Shutemov
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [BUG] random kernel crashes after THP rework on s390 (maybe also on PowerPC and ARM)

[Christian Borntraeger]

On 02/12/2016 04:41 PM, Kirill A. Shutemov wrote:
> On Thu, Feb 11, 2016 at 08:57:02PM +0100, Gerald Schaefer wrote:
>> On Thu, 11 Feb 2016 21:09:42 +0200
>> "Kirill A. Shutemov"  wrote:
>>
>>> On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
 Hi,

 Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
 he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
 review of the THP rework patches, which cannot be bisected, revealed
 commit fecffad "s390, thp: remove infrastructure for handling splitting 
 PMDs"
 (and also similar commits for other archs).

 This commit removes the THP splitting bit and also the architecture
 implementation of pmdp_splitting_flush(), which took care of the IPI for
 fast_gup serialization. The commit message says

 pmdp_splitting_flush() is not needed too: on splitting PMD we will do
 pmdp_clear_flush() + set_pte_at().  pmdp_clear_flush() will do IPI as
 needed for fast_gup

 The assumption that a TLB flush will also produce an IPI is wrong on s390,
 and maybe also on other architectures, and I thought that this was actually
 the main reason for having an arch-specific pmdp_splitting_flush().

 At least PowerPC and ARM also had an individual implementation of
 pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
 flush to send the IPI, and those were also removed. Putting the arch
 maintainers and mailing lists on cc to verify.

 On s390 this will break the IPI serialization against fast_gup, which
 would certainly explain the random kernel crashes, please revert or fix
 the pmdp_splitting_flush() removal.
>>>
>>> Sorry for that.
>>>
>>> I believe, the problem was already addressed for PowerPC:
>>>
>>> http://lkml.kernel.org/g/454980831-16631-1-git-send-email-aneesh.ku...@linux.vnet.ibm.com
>>>
>>> I think kick_all_cpus_sync() in arch-specific pmdp_invalidate() would do
>>> the trick, right?
>>
>> Hmm, not sure about that. After pmdp_invalidate(), a pmd_none() check in
>> fast_gup will still return false, because the pmd is not empty (at least
>> on s390). So I don't see spontaneously how it will help fast_gup to break
>> out to the slow path in case of THP splitting.
> 
> What pmdp_flush_direct() does in pmdp_invalidate()? It's hard to unwrap for 
> me :-/
> Does it make the pmd !pmd_present()?

It uses the idte instruction, which in an atomic fashion flushes the associated
TLB entry and changes the value of the pmd entry to invalid. This comes from the
HW requirement to not  change a PTE/PMD that might be still in use, other than 
with special instructions that does the tlb handling and the invalidation 
together.

(It also does some some other magic to the attach_count, which might hold off
finish_arch_post_lock_switch while some flushing is happening, but this should
be unrelated here)


> I'm also confused by pmd_none() is equal to !pmd_present() on s390. Hm?

Don't know, Gerald or Martin?

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [BUG] random kernel crashes after THP rework on s390 (maybe also on PowerPC and ARM)

[Aneesh Kumar K.V]

Gerald Schaefer  writes:

> On Fri, 12 Feb 2016 09:34:33 +0530
> "Aneesh Kumar K.V"  wrote:
>
>> Gerald Schaefer  writes:
>> 
>> > On Thu, 11 Feb 2016 21:09:42 +0200
>> > "Kirill A. Shutemov"  wrote:
>> >
>> >> On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
>> >> > Hi,
>> >> > 
>> >> > Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
>> >> > he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
>> >> > review of the THP rework patches, which cannot be bisected, revealed
>> >> > commit fecffad "s390, thp: remove infrastructure for handling splitting 
>> >> > PMDs"
>> >> > (and also similar commits for other archs).
>> >> > 
>> >> > This commit removes the THP splitting bit and also the architecture
>> >> > implementation of pmdp_splitting_flush(), which took care of the IPI for
>> >> > fast_gup serialization. The commit message says
>> >> > 
>> >> > pmdp_splitting_flush() is not needed too: on splitting PMD we will 
>> >> > do
>> >> > pmdp_clear_flush() + set_pte_at().  pmdp_clear_flush() will do IPI 
>> >> > as
>> >> > needed for fast_gup
>> >> > 
>> >> > The assumption that a TLB flush will also produce an IPI is wrong on 
>> >> > s390,
>> >> > and maybe also on other architectures, and I thought that this was 
>> >> > actually
>> >> > the main reason for having an arch-specific pmdp_splitting_flush().
>> >> > 
>> >> > At least PowerPC and ARM also had an individual implementation of
>> >> > pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
>> >> > flush to send the IPI, and those were also removed. Putting the arch
>> >> > maintainers and mailing lists on cc to verify.
>> >> > 
>> >> > On s390 this will break the IPI serialization against fast_gup, which
>> >> > would certainly explain the random kernel crashes, please revert or fix
>> >> > the pmdp_splitting_flush() removal.
>> >> 
>> >> Sorry for that.
>> >> 
>> >> I believe, the problem was already addressed for PowerPC:
>> >> 
>> >> http://lkml.kernel.org/g/454980831-16631-1-git-send-email-aneesh.ku...@linux.vnet.ibm.com
>> >> 
>> >> I think kick_all_cpus_sync() in arch-specific pmdp_invalidate() would do
>> >> the trick, right?
>> >
>> > Hmm, not sure about that. After pmdp_invalidate(), a pmd_none() check in
>> > fast_gup will still return false, because the pmd is not empty (at least
>> > on s390).
>> 
>> Why can't we do this ? I did this for ppc64.
>> 
>>  void pmdp_invalidate(struct vm_area_struct *vma, unsigned long address,
>>   pmd_t *pmdp)
>>  {
>> -pmd_hugepage_update(vma->vm_mm, address, pmdp, _PAGE_PRESENT, 0);
>> +pmd_hugepage_update(vma->vm_mm, address, pmdp, ~0UL, 0);
>> 
>
> Wouldn't that semantically change what pmdp_invalidate() was supposed to
> do? The comment before the call says "the pmd_trans_huge and
> pmd_trans_splitting must remain set at all times on the pmd". So, after
> removing pmd_trans_splitting, it seems to be necessary to at least keep
> pmd_trans_huge set.
>
> In your case, the pmd would be completely cleared, which may help to find
> it in fast_gup with pmd_none(), but I'm not sure if this would open up
> other problems, e.g. with concurrent page faults. But I must also admit that
> my THP overview got a little rusty.

Thinking about this more, I guess, I should not be doing this. Because
this bring in the exit_mmap race that I outlined in the patch even
though the window now is small. 

I guess we should fix this in the gup path by checking for what ever
trick we are using to mark the pmd splitting. For ppc64 we clear the
_PAGE_USER. We are ok as long as autonuma is enabled because
pmd_protnone() check will check against _PAGE_USER. But that may not be
sufficient. 

-aneesh

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [BUG] random kernel crashes after THP rework on s390 (maybe also on PowerPC and ARM)

[Will Deacon]

On Fri, Feb 12, 2016 at 11:12:34AM +0100, Sebastian Ott wrote:
> On Fri, 12 Feb 2016, Will Deacon wrote:
> > On Thu, Feb 11, 2016 at 08:57:02PM +0100, Gerald Schaefer wrote:
> > > On Thu, 11 Feb 2016 21:09:42 +0200
> > > "Kirill A. Shutemov"  wrote:
> > > > On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
> > > > > Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 
> > > > > and
> > > > > he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
> > > > > review of the THP rework patches, which cannot be bisected, revealed
> > > > > commit fecffad "s390, thp: remove infrastructure for handling 
> > > > > splitting PMDs"
> > > > > (and also similar commits for other archs).

[...]

> > Do you have a reliable way to trigger the "random kernel crashes"? We've not
> > seen anything reported on arm64, but I don't see why we wouldn't be affected
> > by the same bug and it would be good to confirm and validate a fix.
> 
> My testcase was compiling the kernel. Most of the time my test system
> didn't survive a single compile run. During bisect I did at least 20
> compile runs to flag a commit as good.

I've been building kernels all day with -rc3 on my arm64 box and haven't
seen any problems yet.. :/.

I'll leave it going over the weekend.

Will
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build

[Balbir Singh]

On Thu, 2016-01-28 at 16:32 +0100, Torsten Duwe wrote:
> From: Petr Mladek 
> 
> Livepatch works on x86_64 and s390 only when the ftrace call
> is at the very beginning of the function. But PPC is different.
> We need to handle TOC and save LR there before calling the
> global ftrace handler.
> 
> Now, the problem is that the extra operations have different
> length on PPC depending on the used gcc version. It is
> 4 instructions (16 bytes) before gcc-6 and only 3 instructions
> (12 bytes) with gcc-6.
> 
> This patch tries to detect the offset a generic way during
> build. It assumes that the offset of the ftrace location
> is the same for all functions. It modifies the existing
> recordmcount tool that is able to find read mcount locations
> directly from the object files. It adds an option -p
> to print the first found offset.
> 
> The recordmcount tool is then used in the kernel/livepatch
> subdirectory to generate a header file. It defines
> a constant that is used to compute the ftrace location
> from the function address.
> 
> Finally, we have to enable the C implementation of the
> recordmcount tool to be used on PPC and S390. It seems
> to work fine there. It should be more reliable because
> it reads the standardized elf structures. The old perl
> implementation uses rather complex regular expressions
> to parse objdump output and is therefore much more tricky.

I'm still missing something, I'm getting offset as 8

When I run, I get

scripts/recordmcount -p kernel/livepatch/core.o 
#define KLP_FTRACE_LOCATION 8

scripts/recordmcount -p kernel/livepatch/ftrace-test.o 
#define KLP_FTRACE_LOCATION 8

My sample fails as well, since the expected offset is 16.
I guess the script is being run against a not so good
test.

A quick hack (no signoff below, its just an experiment),
seems to do the trick for the provided sample-livepatch.
It is hacky because it uses the sample object and due to
lack of a better description of srctree, it uses 
srctree/../..

I suspect the usage of recordmcount needs to be revisited

diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
index 65a44b68..10b5f38 100644
--- a/kernel/livepatch/Makefile
+++ b/kernel/livepatch/Makefile
@@ -2,7 +2,7 @@ obj-$(CONFIG_LIVEPATCH) += livepatch.o
 
 livepatch-objs := core.o
 
-always := $(hostprogs-y) ftrace-test.o
+always := $(hostprogs-y) 
$(srctree)/../../samples/livepatch/livepatch-sample.o
 
 # dependencies on generated files need to be listed explicitly
 $(obj)/core.o: $(obj)/livepatch-ftrace.h
@@ -10,7 +10,7 @@ $(obj)/core.o: $(obj)/livepatch-ftrace.h
 quiet_cmd_livepatch-rmcount = RMCOUNT $@
   cmd_livepatch-rmcount = $(objtree)/scripts/recordmcount -p $< > $@
 
-$(obj)/livepatch-ftrace.h: $(obj)/ftrace-test.o $(objtree)/scripts/recordmcount
+$(obj)/livepatch-ftrace.h: $(obj)/../../samples/livepatch/livepatch-sample.o 
$(objtree)/scripts/recordmcount
    $(call if_changed,livepatch-rmcount)
 
 targets += livepatch-ftrace.h

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] mtd/ifc: Add support for IFC controller version 2.0

[Brian Norris]

On Thu, Feb 04, 2016 at 05:07:16PM -0600, Li Yang wrote:
> On Wed, Feb 3, 2016 at 12:36 AM, Raghav Dogra  wrote:
> > The new IFC controller version 2.0 has a different memory map page.
> > Upto IFC 1.4 PAGE size is 4 KB and from IFC2.0 PAGE size is 64KB.
> > This patch segregates the IFC global and runtime registers to appropriate
> > PAGE sizes.
> 
> If the global registers and the runtime registers are so independent
> that they have to be on different page boundaries, it would make more
> sense for them to be defined as separate reg regions in the device
> tree at the very beginning.  Then we would only need to change the
> device tree now and it would be future proof for any page size.

To be clear: Scott, you were NACK'ing the DT binding change request,
right? I though we had an Ack on the previous revision (that Raghav
failed to carry).

> >
> > Signed-off-by: Jaiprakash Singh 
> > Signed-off-by: Raghav Dogra 
> 
> The patch cannot apply on latest 4.5-rc cleanly either.  Otherwise,

Yeah... neither this patch nor its (allegedly) dependent patch [1] apply
cleanly.

If you expect me to take this patch via MTD, please rebase to
l2-mtd.git as stated here:

http://linux-mtd.infradead.org/source.html

> Acked-by: Li Yang 

Brian

[1] https://patchwork.ozlabs.org/patch/564785/
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V2 00/29] Book3s abstraction in preparation for new MMU model

[Aneesh Kumar K.V]

Paul Mackerras  writes:

> On Mon, Feb 08, 2016 at 02:50:12PM +0530, Aneesh Kumar K.V wrote:
>> Hello,
>> 
>> This is a large series, mostly consisting of code movement. No new features
>> are done in this series. The changes are done to accomodate the upcoming new 
>> memory
>> model in future powerpc chips. The details of the new MMU model can be found 
>> at
>> 
>>  http://ibm.biz/power-isa3 (Needs registration). I am including a summary of 
>> the changes below.
>
> This series doesn't seem to apply against either v4.4 or Linus'
> current master.  What is this patch against?
>

The patchset have dependencies against other patcheset posted to the
list. The best option is to pull the branch mentioned instead of trying to
apply them individually.

-aneesh

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V2 02/29] powerpc/mm: Split pgtable types to separate header

[Aneesh Kumar K.V]

Paul Mackerras  writes:

> On Mon, Feb 08, 2016 at 02:50:14PM +0530, Aneesh Kumar K.V wrote:
>> We remove real_pte_t out of STRICT_MM_TYPESCHECK. We will later add
>> a radix variant that is big endian
>> 
>> Signed-off-by: Aneesh Kumar K.V 
>
> It looks like most of what this patch does is move a bunch of
> definitions from page.h to a new pgtable-types.h.  What is the
> motivation for this?  Is the code identical (pure code movement) or do
> you make changes along the way, and if so, what and why?

The motivation is to assist in addition of a big endian page table format 
later. 

>
> What exactly are you doing with real_pte_t and why?

Want to avoint STRICT_MM_TYPESCHECK related #ifdef around real_pte_t. I
can split that into a separate patch and keep this patch strictly code
movement.

-aneesh

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 3/7] ibmvscsi: Replace magic values in set_adpater_info() with defines

[Manoj Kumar]

Reviewed-by: Manoj Kumar 

---
Manoj Kumar

On 2/10/2016 7:32 PM, Tyrel Datwyler wrote:

Add defines for mad version and mad os_type, and replace the magic
numbers in set_adapter_info() accordingly.

Signed-off-by: Tyrel Datwyler 
---
  drivers/scsi/ibmvscsi/ibmvscsi.c | 8 
  drivers/scsi/ibmvscsi/viosrp.h   | 3 +++
  2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index c888ea1..4b09a9b 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -283,8 +283,8 @@ static void set_adapter_info(struct ibmvscsi_host_data 
*hostdata)
hostdata->madapter_info.partition_number =
cpu_to_be32(partition_number);

-   hostdata->madapter_info.mad_version = cpu_to_be32(1);
-   hostdata->madapter_info.os_type = cpu_to_be32(2);
+   hostdata->madapter_info.mad_version = cpu_to_be32(SRP_MAD_VERSION_1);
+   hostdata->madapter_info.os_type = cpu_to_be32(SRP_MAD_OS_LINUX);
  }

  /**
@@ -1398,7 +1398,7 @@ static void adapter_info_rsp(struct srp_event_struct 
*evt_struct)
hostdata->host->max_sectors =
be32_to_cpu(hostdata->madapter_info.port_max_txu[0]) 
>> 9;

-   if (be32_to_cpu(hostdata->madapter_info.os_type) == 3 &&
+   if (be32_to_cpu(hostdata->madapter_info.os_type) == SRP_MAD_OS_AIX 
&&
strcmp(hostdata->madapter_info.srp_version, "1.6a") <= 0) {
dev_err(hostdata->dev, "host (Ver. %s) doesn't support large 
transfers\n",
hostdata->madapter_info.srp_version);
@@ -1407,7 +1407,7 @@ static void adapter_info_rsp(struct srp_event_struct 
*evt_struct)
hostdata->host->sg_tablesize = MAX_INDIRECT_BUFS;
}

-   if (be32_to_cpu(hostdata->madapter_info.os_type) == 3) {
+   if (be32_to_cpu(hostdata->madapter_info.os_type) == 
SRP_MAD_OS_AIX) {
enable_fast_fail(hostdata);
return;
}
diff --git a/drivers/scsi/ibmvscsi/viosrp.h b/drivers/scsi/ibmvscsi/viosrp.h
index 3d20851..d0f689b 100644
--- a/drivers/scsi/ibmvscsi/viosrp.h
+++ b/drivers/scsi/ibmvscsi/viosrp.h
@@ -221,7 +221,10 @@ struct mad_adapter_info_data {
char srp_version[8];
char partition_name[96];
__be32 partition_number;
+#define SRP_MAD_VERSION_1 1
__be32 mad_version;
+#define SRP_MAD_OS_LINUX 2
+#define SRP_MAD_OS_AIX 3
__be32 os_type;
__be32 port_max_txu[8]; /* per-port maximum transfer */
  };



___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] mtd/ifc: Add support for IFC controller version 2.0

[Scott Wood]

On Fri, 2016-02-12 at 11:44 -0800, Brian Norris wrote:
> On Thu, Feb 04, 2016 at 05:07:16PM -0600, Li Yang wrote:
> > On Wed, Feb 3, 2016 at 12:36 AM, Raghav Dogra 
> > wrote:
> > > The new IFC controller version 2.0 has a different memory map page.
> > > Upto IFC 1.4 PAGE size is 4 KB and from IFC2.0 PAGE size is 64KB.
> > > This patch segregates the IFC global and runtime registers to
> > > appropriate
> > > PAGE sizes.
> > 
> > If the global registers and the runtime registers are so independent
> > that they have to be on different page boundaries, it would make more
> > sense for them to be defined as separate reg regions in the device
> > tree at the very beginning.  Then we would only need to change the
> > device tree now and it would be future proof for any page size.
> 
> To be clear: Scott, you were NACK'ing the DT binding change request,
> right? I though we had an Ack on the previous revision (that Raghav
> failed to carry).

Yes.  The patch is OK, the DT binding change (which is not in the patch) is
not.

-Scott

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [BUG] random kernel crashes after THP rework on s390 (maybe also on PowerPC and ARM)

[Kirill A. Shutemov]

On Fri, Feb 12, 2016 at 06:16:40PM +0100, Gerald Schaefer wrote:
> On Fri, 12 Feb 2016 16:57:27 +0100
> Christian Borntraeger  wrote:
> 
> > On 02/12/2016 04:41 PM, Kirill A. Shutemov wrote:
> > > On Thu, Feb 11, 2016 at 08:57:02PM +0100, Gerald Schaefer wrote:
> > >> On Thu, 11 Feb 2016 21:09:42 +0200
> > >> "Kirill A. Shutemov"  wrote:
> > >>
> > >>> On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
> >  Hi,
> > 
> >  Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 
> >  and
> >  he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
> >  review of the THP rework patches, which cannot be bisected, revealed
> >  commit fecffad "s390, thp: remove infrastructure for handling 
> >  splitting PMDs"
> >  (and also similar commits for other archs).
> > 
> >  This commit removes the THP splitting bit and also the architecture
> >  implementation of pmdp_splitting_flush(), which took care of the IPI 
> >  for
> >  fast_gup serialization. The commit message says
> > 
> >  pmdp_splitting_flush() is not needed too: on splitting PMD we will 
> >  do
> >  pmdp_clear_flush() + set_pte_at().  pmdp_clear_flush() will do IPI 
> >  as
> >  needed for fast_gup
> > 
> >  The assumption that a TLB flush will also produce an IPI is wrong on 
> >  s390,
> >  and maybe also on other architectures, and I thought that this was 
> >  actually
> >  the main reason for having an arch-specific pmdp_splitting_flush().
> > 
> >  At least PowerPC and ARM also had an individual implementation of
> >  pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
> >  flush to send the IPI, and those were also removed. Putting the arch
> >  maintainers and mailing lists on cc to verify.
> > 
> >  On s390 this will break the IPI serialization against fast_gup, which
> >  would certainly explain the random kernel crashes, please revert or fix
> >  the pmdp_splitting_flush() removal.
> > >>>
> > >>> Sorry for that.
> > >>>
> > >>> I believe, the problem was already addressed for PowerPC:
> > >>>
> > >>> http://lkml.kernel.org/g/454980831-16631-1-git-send-email-aneesh.ku...@linux.vnet.ibm.com
> > >>>
> > >>> I think kick_all_cpus_sync() in arch-specific pmdp_invalidate() would do
> > >>> the trick, right?
> > >>
> > >> Hmm, not sure about that. After pmdp_invalidate(), a pmd_none() check in
> > >> fast_gup will still return false, because the pmd is not empty (at least
> > >> on s390). So I don't see spontaneously how it will help fast_gup to break
> > >> out to the slow path in case of THP splitting.
> > > 
> > > What pmdp_flush_direct() does in pmdp_invalidate()? It's hard to unwrap 
> > > for me :-/
> > > Does it make the pmd !pmd_present()?
> > 
> > It uses the idte instruction, which in an atomic fashion flushes the 
> > associated
> > TLB entry and changes the value of the pmd entry to invalid. This comes 
> > from the
> > HW requirement to not  change a PTE/PMD that might be still in use, other 
> > than 
> > with special instructions that does the tlb handling and the invalidation 
> > together.
> 
> Correct, and it does _not_ make the pmd !pmd_present(), that would only be the
> case after a _clear_flush(). It only marks the pmd as invalid and flushes,
> so that it cannot generate a new TLB entry before the following 
> pmd_populate(),
> but it keeps its other content. This is to fulfill the requirements outlined 
> in
> the comment in mm/huge_memory.c before the call to pmdp_invalidate(). And
> independent from that comment, we would need such an _invalidate() or
> _clear_flush() on s390 before the pmd_populate() because of the HW details
> that Christian described.
> 
> Reading the comment again, I do now notice that it also says "mark the current
> pmd notpresent", which we cannot do w/o losing the huge and (formerly) 
> splitting
> bits, but it also shouldn't be needed to provide the "single TLB guarantee" 
> that
> is required from the comment. So, a pmd_present() check on s390 in this state
> would still return true. Not sure yet if this is a problem, need more 
> thinking,
> this behavior was already present before the THP rework but maybe it was OK
> before and is not OK now.
> 
> At least for fast_gup this should not be a problem though.

I'm trying to wrap my head around the issue and I don't think missing
serialization with gup_fast is the cause -- we just don't need it
anymore.

Previously, __split_huge_page_splitting() required serialization against
gup_fast to make sure nobody can obtain new reference to the page after
__split_huge_page_splitting() returns. This was a way to stabilize page
references before starting to distribute them from head page to tail
pages.

With new refcounting, we don't care about this. Splitting PMD is now
decoupled from splitting underlying 

Re: [PATCH v8 8/8] livepatch: Detect offset for the ftrace location during build

[Balbir Singh]

On Fri, 2016-02-12 at 17:45 +0100, Petr Mladek wrote:
> On Sat 2016-02-13 03:13:29, Balbir Singh wrote:
> > On Thu, 2016-01-28 at 16:32 +0100, Torsten Duwe wrote:
> > > From: Petr Mladek 
> > > 
> > > Livepatch works on x86_64 and s390 only when the ftrace call
> > > is at the very beginning of the function. But PPC is different.
> > > We need to handle TOC and save LR there before calling the
> > > global ftrace handler.
> > > 
> > > Now, the problem is that the extra operations have different
> > > length on PPC depending on the used gcc version. It is
> > > 4 instructions (16 bytes) before gcc-6 and only 3 instructions
> > > (12 bytes) with gcc-6.
> > > 
> > > This patch tries to detect the offset a generic way during
> > > build. It assumes that the offset of the ftrace location
> > > is the same for all functions. It modifies the existing
> > > recordmcount tool that is able to find read mcount locations
> > > directly from the object files. It adds an option -p
> > > to print the first found offset.
> > > 
> > > The recordmcount tool is then used in the kernel/livepatch
> > > subdirectory to generate a header file. It defines
> > > a constant that is used to compute the ftrace location
> > > from the function address.
> > > 
> > > Finally, we have to enable the C implementation of the
> > > recordmcount tool to be used on PPC and S390. It seems
> > > to work fine there. It should be more reliable because
> > > it reads the standardized elf structures. The old perl
> > > implementation uses rather complex regular expressions
> > > to parse objdump output and is therefore much more tricky.
> > 
> > I'm still missing something, I'm getting offset as 8
> > 
> > When I run, I get
> > 
> > scripts/recordmcount -p kernel/livepatch/core.o 
> > #define KLP_FTRACE_LOCATION 8
> > 
> > scripts/recordmcount -p kernel/livepatch/ftrace-test.o 
> > #define KLP_FTRACE_LOCATION 8
> > 
> > My sample fails as well, since the expected offset is 16.
> > I guess the script is being run against a not so good
> > test.
> 
> I guess that you used a broken gcc and cheated the check
> to pass the compilation. Did you, please?
> 
> The test used to detect the offset is using a minimalistic
> function is is afftected by the gcc bug.
> 
> The patch below might be used to cheat the offset check as well.
> 
> Torsten, please mention this somewhere if you, just by chance,
> send a new version of the patchset.
> 
> From f6a438a3f2f60cc1acc859b41d0cc9259c9a331e Mon Sep 17 00:00:00 2001
> From: root 
> Date: Tue, 2 Feb 2016 15:35:06 +0100
> Subject: [PATCH 2/2] livepatch: Make sure the TOC is handled when detecting
>  ftrace location
> 
> There seems to be a bug in gcc on PPC. It does not handle TOC
> if the function does not access global variables or functions
> by default. But it should when profiling is enabled.
> 

Yep.. Please see see http://marc.info/?l=linux-kernel=145518015816435=2
and my question at http://marc.info/?l=linuxppc-embedded=145518330317496=2

> This patch works around this problem by adding a call
> to a global function.
> 
> This patch is for testing only!
> ---
>  kernel/livepatch/ftrace-test.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/livepatch/ftrace-test.c b/kernel/livepatch/ftrace-test.c
> index 22f0c54bf7b3..a3b7aabb67e5 100644
> --- a/kernel/livepatch/ftrace-test.c
> +++ b/kernel/livepatch/ftrace-test.c
> @@ -1,6 +1,9 @@
>  /* Sample code to figure out mcount location offset */
> +#include 
> +
>  
>  int test(int a)
>  {
> + printk("%d\n", a);
>   return ++a;
>  }

This is much better, I see the offset of 16.

Balbir Singh
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev