Re: [PATCH 01/83] mm: Add kfd_process pointer to mm_struct

From: Jerome Glisse <j.glisse@gmail.com>
To: Oded Gabbay <oded.gabbay@gmail.com>
Cc: "Jérôme Glisse" <jglisse@redhat.com>,
	"Rik van Riel" <riel@redhat.com>,
	"Andrew Lewycky" <Andrew.Lewycky@amd.com>,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linux-mm <linux-mm@kvack.org>,
	"Peter Zijlstra" <peterz@infradead.org>,
	"Alex Deucher" <alexander.deucher@amd.com>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"Michel Lespinasse" <walken@google.com>,
	"Joonsoo Kim" <iamjoonsoo.kim@lge.com>,
	"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
	"Mel Gorman" <mgorman@suse.de>
Subject: Re: [PATCH 01/83] mm: Add kfd_process pointer to mm_struct
Date: Fri, 11 Jul 2014 11:42:32 -0400	[thread overview]
Message-ID: <20140711154231.GB1870@gmail.com> (raw)
In-Reply-To: <1405028848-5660-1-git-send-email-oded.gabbay@amd.com>

On Fri, Jul 11, 2014 at 12:47:26AM +0300, Oded Gabbay wrote:
> This patch enables the KFD to retrieve the kfd_process
> object from the process's mm_struct. This is needed because kfd_process
> lifespan is bound to the process's mm_struct lifespan.
> 
> When KFD is notified about an mm_struct tear-down, it checks if the
> kfd_process pointer is valid. If so, it releases the kfd_process object
> and all relevant resources.
> 
> Signed-off-by: Oded Gabbay <oded.gabbay@amd.com>
> ---
>  include/linux/mm_types.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 678097c..6179107 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -20,6 +20,10 @@
>  struct hmm;
>  #endif
>  
> +#ifdef CONFIG_HSA_RADEON
> +struct kfd_process;
> +#endif
> +
>  #ifndef AT_VECTOR_SIZE_ARCH
>  #define AT_VECTOR_SIZE_ARCH 0
>  #endif
> @@ -439,6 +443,16 @@ struct mm_struct {
>  	 */
>  	struct hmm *hmm;
>  #endif
> +#if defined(CONFIG_HSA_RADEON) || defined(CONFIG_HSA_RADEON_MODULE)
> +	/*
> +	 * kfd always register an mmu_notifier we rely on mmu notifier to keep
> +	 * refcount on mm struct as well as forbiding registering kfd on a
> +	 * dying mm
> +	 *
> +	 * This field is set with mmap_sem old in write mode.
> +	 */
> +	struct kfd_process *kfd_process;
> +#endif

I understand the need to bind kfd to mm life time but this is wrong
on several level. First we do not want per driver define flag here.
Second this should be a IOMMU/PASID pointer of some sort, i am sure
that Intel will want to add itself too to mm_struct so instead of
having each IOMMU add a pointer here, i would rather see a generic
pointer to a generic IOMMU struct and have this use generic IOMMU
code that can then call specific user dispatch function.

I know this add a layer but this is not a critical code path and
should never be.

I am adding Jesse as he might have thought on that.

So this one is NAK

Cheers,
Jérôme

>  #if defined(CONFIG_TRANSPARENT_HUGEPAGE) && !USE_SPLIT_PMD_PTLOCKS
>  	pgtable_t pmd_huge_pte; /* protected by page_table_lock */
>  #endif
> -- 
> 1.9.1
> 