The struct page->mapping, index fields are deprecated and soon to be only
available as part of a folio.

It is likely the intel_th code which sets page->mapping, index is was
implemented out of concern that some aspect of the page fault logic may
encounter unexpected problems should they not.

However, the appropriate interface for inserting kernel-allocated memory is
vm_insert_page() in a VM_MIXEDMAP. By using the helper function
vmf_insert_mixed() we can do this with minimal churn in the existing fault
handler.

By doing so, we bypass the remainder of the faulting logic. The pages are
still pinned so there is no possibility of anything unexpected being done
with the pages once established.

It would also be reasonable to pre-map everything on fault, however to
minimise churn we retain the fault handler.

We also eliminate all code which clears page->mapping on teardown as this
has now become unnecessary.

The MSU code relies on faulting to function correctly, so is by definition
dependent on CONFIG_MMU. We avoid spurious reports about compilation
failure for unsupported platforms by making this requirement explicit in
Kconfig as part of this change too.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---

v2:
* Make the MSU driver dependent on CONFIG_MMU to avoid spurious compilation
  failure reports.

v1:
https://lore.kernel.org/all/20241202122127.51313-1-lorenzo.stoakes@oracle.com/

 drivers/hwtracing/intel_th/Kconfig |  1 +
 drivers/hwtracing/intel_th/msu.c   | 31 +++++++-----------------------
 2 files changed, 8 insertions(+), 24 deletions(-)

diff --git a/drivers/hwtracing/intel_th/Kconfig b/drivers/hwtracing/intel_th/Kconfig
index 4b6359326ede..4f7d2b6d79e2 100644
--- a/drivers/hwtracing/intel_th/Kconfig
+++ b/drivers/hwtracing/intel_th/Kconfig
@@ -60,6 +60,7 @@ config INTEL_TH_STH

 config INTEL_TH_MSU
 	tristate "Intel(R) Trace Hub Memory Storage Unit"
+	depends on MMU
 	help
 	  Memory Storage Unit (MSU) trace output device enables
 	  storing STP traces to system memory. It supports single
diff --git a/drivers/hwtracing/intel_th/msu.c b/drivers/hwtracing/intel_th/msu.c
index 66123d684ac9..93b65a9731d7 100644
--- a/drivers/hwtracing/intel_th/msu.c
+++ b/drivers/hwtracing/intel_th/msu.c
@@ -19,6 +19,7 @@
 #include <linux/io.h>
 #include <linux/workqueue.h>
 #include <linux/dma-mapping.h>
+#include <linux/pfn_t.h>

 #ifdef CONFIG_X86
 #include <asm/set_memory.h>
@@ -967,7 +968,6 @@ static void msc_buffer_contig_free(struct msc *msc)
 	for (off = 0; off < msc->nr_pages << PAGE_SHIFT; off += PAGE_SIZE) {
 		struct page *page = virt_to_page(msc->base + off);

-		page->mapping = NULL;
 		__free_page(page);
 	}

@@ -1149,9 +1149,6 @@ static void __msc_buffer_win_free(struct msc *msc, struct msc_window *win)
 	int i;

 	for_each_sg(win->sgt->sgl, sg, win->nr_segs, i) {
-		struct page *page = msc_sg_page(sg);
-
-		page->mapping = NULL;
 		dma_free_coherent(msc_dev(win->msc)->parent->parent, PAGE_SIZE,
 				  sg_virt(sg), sg_dma_address(sg));
 	}
@@ -1592,22 +1589,10 @@ static void msc_mmap_close(struct vm_area_struct *vma)
 {
 	struct msc_iter *iter = vma->vm_file->private_data;
 	struct msc *msc = iter->msc;
-	unsigned long pg;

 	if (!atomic_dec_and_mutex_lock(&msc->mmap_count, &msc->buf_mutex))
 		return;

-	/* drop page _refcounts */
-	for (pg = 0; pg < msc->nr_pages; pg++) {
-		struct page *page = msc_buffer_get_page(msc, pg);
-
-		if (WARN_ON_ONCE(!page))
-			continue;
-
-		if (page->mapping)
-			page->mapping = NULL;
-	}
-
 	/* last mapping -- drop user_count */
 	atomic_dec(&msc->user_count);
 	mutex_unlock(&msc->buf_mutex);
@@ -1617,16 +1602,14 @@ static vm_fault_t msc_mmap_fault(struct vm_fault *vmf)
 {
 	struct msc_iter *iter = vmf->vma->vm_file->private_data;
 	struct msc *msc = iter->msc;
+	struct page *page;

-	vmf->page = msc_buffer_get_page(msc, vmf->pgoff);
-	if (!vmf->page)
+	page = msc_buffer_get_page(msc, vmf->pgoff);
+	if (!page)
 		return VM_FAULT_SIGBUS;

-	get_page(vmf->page);
-	vmf->page->mapping = vmf->vma->vm_file->f_mapping;
-	vmf->page->index = vmf->pgoff;
-
-	return 0;
+	get_page(page);
+	return vmf_insert_mixed(vmf->vma, vmf->address, page_to_pfn_t(page));
 }

 static const struct vm_operations_struct msc_mmap_ops = {
@@ -1667,7 +1650,7 @@ static int intel_th_msc_mmap(struct file *file, struct vm_area_struct *vma)
 		atomic_dec(&msc->user_count);

 	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
-	vm_flags_set(vma, VM_DONTEXPAND | VM_DONTCOPY);
+	vm_flags_set(vma, VM_DONTEXPAND | VM_DONTCOPY | VM_MIXEDMAP);
 	vma->vm_ops = &msc_mmap_ops;
 	return ret;
 }
--
2.47.1

Hi Alexander,

Happy New Year! I realise it's been the holidays recently, but Wondering if
you'd had a chance to look at this?

Thanks, Lorenzo

On Tue, Dec 03, 2024 at 08:00:01AM +0000, Lorenzo Stoakes wrote:
> The struct page->mapping, index fields are deprecated and soon to be only
> available as part of a folio.
>
> It is likely the intel_th code which sets page->mapping, index is was
> implemented out of concern that some aspect of the page fault logic may
> encounter unexpected problems should they not.
>
> However, the appropriate interface for inserting kernel-allocated memory is
> vm_insert_page() in a VM_MIXEDMAP. By using the helper function
> vmf_insert_mixed() we can do this with minimal churn in the existing fault
> handler.
>
> By doing so, we bypass the remainder of the faulting logic. The pages are
> still pinned so there is no possibility of anything unexpected being done
> with the pages once established.
>
> It would also be reasonable to pre-map everything on fault, however to
> minimise churn we retain the fault handler.
>
> We also eliminate all code which clears page->mapping on teardown as this
> has now become unnecessary.
>
> The MSU code relies on faulting to function correctly, so is by definition
> dependent on CONFIG_MMU. We avoid spurious reports about compilation
> failure for unsupported platforms by making this requirement explicit in
> Kconfig as part of this change too.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
>
> v2:
> * Make the MSU driver dependent on CONFIG_MMU to avoid spurious compilation
>   failure reports.
>
> v1:
> https://lore.kernel.org/all/20241202122127.51313-1-lorenzo.stoakes@oracle.com/
>
>  drivers/hwtracing/intel_th/Kconfig |  1 +
>  drivers/hwtracing/intel_th/msu.c   | 31 +++++++-----------------------
>  2 files changed, 8 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/hwtracing/intel_th/Kconfig b/drivers/hwtracing/intel_th/Kconfig
> index 4b6359326ede..4f7d2b6d79e2 100644
> --- a/drivers/hwtracing/intel_th/Kconfig
> +++ b/drivers/hwtracing/intel_th/Kconfig
> @@ -60,6 +60,7 @@ config INTEL_TH_STH
>
>  config INTEL_TH_MSU
>  	tristate "Intel(R) Trace Hub Memory Storage Unit"
> +	depends on MMU
>  	help
>  	  Memory Storage Unit (MSU) trace output device enables
>  	  storing STP traces to system memory. It supports single
> diff --git a/drivers/hwtracing/intel_th/msu.c b/drivers/hwtracing/intel_th/msu.c
> index 66123d684ac9..93b65a9731d7 100644
> --- a/drivers/hwtracing/intel_th/msu.c
> +++ b/drivers/hwtracing/intel_th/msu.c
> @@ -19,6 +19,7 @@
>  #include <linux/io.h>
>  #include <linux/workqueue.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/pfn_t.h>
>
>  #ifdef CONFIG_X86
>  #include <asm/set_memory.h>
> @@ -967,7 +968,6 @@ static void msc_buffer_contig_free(struct msc *msc)
>  	for (off = 0; off < msc->nr_pages << PAGE_SHIFT; off += PAGE_SIZE) {
>  		struct page *page = virt_to_page(msc->base + off);
>
> -		page->mapping = NULL;
>  		__free_page(page);
>  	}
>
> @@ -1149,9 +1149,6 @@ static void __msc_buffer_win_free(struct msc *msc, struct msc_window *win)
>  	int i;
>
>  	for_each_sg(win->sgt->sgl, sg, win->nr_segs, i) {
> -		struct page *page = msc_sg_page(sg);
> -
> -		page->mapping = NULL;
>  		dma_free_coherent(msc_dev(win->msc)->parent->parent, PAGE_SIZE,
>  				  sg_virt(sg), sg_dma_address(sg));
>  	}
> @@ -1592,22 +1589,10 @@ static void msc_mmap_close(struct vm_area_struct *vma)
>  {
>  	struct msc_iter *iter = vma->vm_file->private_data;
>  	struct msc *msc = iter->msc;
> -	unsigned long pg;
>
>  	if (!atomic_dec_and_mutex_lock(&msc->mmap_count, &msc->buf_mutex))
>  		return;
>
> -	/* drop page _refcounts */
> -	for (pg = 0; pg < msc->nr_pages; pg++) {
> -		struct page *page = msc_buffer_get_page(msc, pg);
> -
> -		if (WARN_ON_ONCE(!page))
> -			continue;
> -
> -		if (page->mapping)
> -			page->mapping = NULL;
> -	}
> -
>  	/* last mapping -- drop user_count */
>  	atomic_dec(&msc->user_count);
>  	mutex_unlock(&msc->buf_mutex);
> @@ -1617,16 +1602,14 @@ static vm_fault_t msc_mmap_fault(struct vm_fault *vmf)
>  {
>  	struct msc_iter *iter = vmf->vma->vm_file->private_data;
>  	struct msc *msc = iter->msc;
> +	struct page *page;
>
> -	vmf->page = msc_buffer_get_page(msc, vmf->pgoff);
> -	if (!vmf->page)
> +	page = msc_buffer_get_page(msc, vmf->pgoff);
> +	if (!page)
>  		return VM_FAULT_SIGBUS;
>
> -	get_page(vmf->page);
> -	vmf->page->mapping = vmf->vma->vm_file->f_mapping;
> -	vmf->page->index = vmf->pgoff;
> -
> -	return 0;
> +	get_page(page);
> +	return vmf_insert_mixed(vmf->vma, vmf->address, page_to_pfn_t(page));
>  }
>
>  static const struct vm_operations_struct msc_mmap_ops = {
> @@ -1667,7 +1650,7 @@ static int intel_th_msc_mmap(struct file *file, struct vm_area_struct *vma)
>  		atomic_dec(&msc->user_count);
>
>  	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> -	vm_flags_set(vma, VM_DONTEXPAND | VM_DONTCOPY);
> +	vm_flags_set(vma, VM_DONTEXPAND | VM_DONTCOPY | VM_MIXEDMAP);
>  	vma->vm_ops = &msc_mmap_ops;
>  	return ret;
>  }
> --
> 2.47.1

Lorenzo Stoakes <lorenzo.stoakes@oracle.com> writes:

> Hi Alexander,

Hi Lorenzo,

> Happy New Year! I realise it's been the holidays recently, but Wondering if
> you'd had a chance to look at this?

Please forgive the tardiness. I'm not ignoring your patch.
At a cursory glance, this patch is fine. I'll pick it up and send with
the rest of intel_th patches after the merge window closes, I hope
that's ok. Or, would you prefer my ack and send it in a larger series
yourself?

Thanks,
--
Alex

On Wed, Jan 29, 2025 at 09:57:28AM +0200, Alexander Shishkin wrote:
> Lorenzo Stoakes <lorenzo.stoakes@oracle.com> writes:
>
> > Hi Alexander,
>
> Hi Lorenzo,
>
> > Happy New Year! I realise it's been the holidays recently, but Wondering if
> > you'd had a chance to look at this?
>
> Please forgive the tardiness. I'm not ignoring your patch.
> At a cursory glance, this patch is fine. I'll pick it up and send with
> the rest of intel_th patches after the merge window closes, I hope
> that's ok. Or, would you prefer my ack and send it in a larger series
> yourself?

Hi Alex,

Thanks very much! Yeah just keen to get this in as we are moving towards
removing these fields very soon.

Could you take this in your tree? I think that'd work best.

Thanks, Lorenzo

>
> Thanks,
> --
> Alex

Lorenzo Stoakes <lorenzo.stoakes@oracle.com> writes:

> Thanks very much! Yeah just keen to get this in as we are moving towards
> removing these fields very soon.

My understanding is that this is a part of larger effort to reduce
struct page to 8 bytes and an optionally dynamically allocated slightly
larger structure, is that correct? Just curious.

> Could you take this in your tree? I think that'd work best.

Sure, will do.

Thanks,
--
Alex

On Wed, Jan 29, 2025 at 01:39:06PM +0200, Alexander Shishkin wrote:
> Lorenzo Stoakes <lorenzo.stoakes@oracle.com> writes:
> 
> > Thanks very much! Yeah just keen to get this in as we are moving towards
> > removing these fields very soon.
> 
> My understanding is that this is a part of larger effort to reduce
> struct page to 8 bytes and an optionally dynamically allocated slightly
> larger structure, is that correct? Just curious.
> 
> > Could you take this in your tree? I think that'd work best.
> 
> Sure, will do.

Hi, this doesn't appear to be in linux-next yet.  Could you confirm it's
scheduled to hit the next merge window?

Matthew Wilcox <willy@infradead.org> writes:

> On Wed, Jan 29, 2025 at 01:39:06PM +0200, Alexander Shishkin wrote:
>> Lorenzo Stoakes <lorenzo.stoakes@oracle.com> writes:
>> 
>> > Thanks very much! Yeah just keen to get this in as we are moving towards
>> > removing these fields very soon.
>> 
>> My understanding is that this is a part of larger effort to reduce
>> struct page to 8 bytes and an optionally dynamically allocated slightly
>> larger structure, is that correct? Just curious.
>> 
>> > Could you take this in your tree? I think that'd work best.
>> 
>> Sure, will do.
>
> Hi, this doesn't appear to be in linux-next yet.  Could you confirm it's
> scheduled to hit the next merge window?

Yes, I'll send it to Greg once -rc1 is tagged.

Regards,
--
Alex

On Mon, Mar 31, 2025 at 09:36:40AM +0300, Alexander Shishkin wrote:
> Matthew Wilcox <willy@infradead.org> writes:
> 
> > On Wed, Jan 29, 2025 at 01:39:06PM +0200, Alexander Shishkin wrote:
> >> Lorenzo Stoakes <lorenzo.stoakes@oracle.com> writes:
> >> 
> >> > Thanks very much! Yeah just keen to get this in as we are moving towards
> >> > removing these fields very soon.
> >> 
> >> My understanding is that this is a part of larger effort to reduce
> >> struct page to 8 bytes and an optionally dynamically allocated slightly
> >> larger structure, is that correct? Just curious.
> >> 
> >> > Could you take this in your tree? I think that'd work best.
> >> 
> >> Sure, will do.
> >
> > Hi, this doesn't appear to be in linux-next yet.  Could you confirm it's
> > scheduled to hit the next merge window?
> 
> Yes, I'll send it to Greg once -rc1 is tagged.

What?!  This should be in *BEFORE RC1*!

+cc Greg

On Mon, Mar 31, 2025 at 09:36:40AM +0300, Alexander Shishkin wrote:
> Matthew Wilcox <willy@infradead.org> writes:
>
> > On Wed, Jan 29, 2025 at 01:39:06PM +0200, Alexander Shishkin wrote:
> >> Lorenzo Stoakes <lorenzo.stoakes@oracle.com> writes:
> >>
> >> > Thanks very much! Yeah just keen to get this in as we are moving towards
> >> > removing these fields very soon.
> >>
> >> My understanding is that this is a part of larger effort to reduce
> >> struct page to 8 bytes and an optionally dynamically allocated slightly
> >> larger structure, is that correct? Just curious.
> >>
> >> > Could you take this in your tree? I think that'd work best.
> >>
> >> Sure, will do.
> >
> > Hi, this doesn't appear to be in linux-next yet.  Could you confirm it's
> > scheduled to hit the next merge window?
>
> Yes, I'll send it to Greg once -rc1 is tagged.

Right, is this ultimately handled in Greg's PR? Did you not send to him
ready for the merge window?

Or did you? I'm confused.

We do really need this merged for 6.15, can we make sure this actually
lands? You did confirm it'd go in 2 months ago, and the patch was sent 4
months ago, and we have been chasing this repeatedly.

For reference, patch is [0].

[0]: https://lore.kernel.org/all/20241203080001.12341-1-lorenzo.stoakes@oracle.com/

>
> Regards,
> --
> Alex

Thanks, Lorenzo

On Mon, Mar 31, 2025 at 12:59:36PM +0100, Lorenzo Stoakes wrote:
> +cc Greg
> 
> On Mon, Mar 31, 2025 at 09:36:40AM +0300, Alexander Shishkin wrote:
> > Matthew Wilcox <willy@infradead.org> writes:
> >
> > > On Wed, Jan 29, 2025 at 01:39:06PM +0200, Alexander Shishkin wrote:
> > >> Lorenzo Stoakes <lorenzo.stoakes@oracle.com> writes:
> > >>
> > >> > Thanks very much! Yeah just keen to get this in as we are moving towards
> > >> > removing these fields very soon.
> > >>
> > >> My understanding is that this is a part of larger effort to reduce
> > >> struct page to 8 bytes and an optionally dynamically allocated slightly
> > >> larger structure, is that correct? Just curious.
> > >>
> > >> > Could you take this in your tree? I think that'd work best.
> > >>
> > >> Sure, will do.
> > >
> > > Hi, this doesn't appear to be in linux-next yet.  Could you confirm it's
> > > scheduled to hit the next merge window?
> >
> > Yes, I'll send it to Greg once -rc1 is tagged.
> 
> Right, is this ultimately handled in Greg's PR? Did you not send to him
> ready for the merge window?
> 
> Or did you? I'm confused.

I don't see it in my tree right now, is it in linux-next?  Nope, don't
see it there either :(

> We do really need this merged for 6.15, can we make sure this actually
> lands? You did confirm it'd go in 2 months ago, and the patch was sent 4
> months ago, and we have been chasing this repeatedly.
> 
> For reference, patch is [0].
> 
> [0]: https://lore.kernel.org/all/20241203080001.12341-1-lorenzo.stoakes@oracle.com/

What changes in 6.15 to require this then?  If it's a bugfix for older
kernels too, why isn't it tagged for stable inclusion as well?

thanks,

greg k-h

On Mon, Mar 31, 2025 at 02:01:38PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Mar 31, 2025 at 12:59:36PM +0100, Lorenzo Stoakes wrote:
> > +cc Greg
> >
> > On Mon, Mar 31, 2025 at 09:36:40AM +0300, Alexander Shishkin wrote:
> > > Matthew Wilcox <willy@infradead.org> writes:
> > >
> > > > On Wed, Jan 29, 2025 at 01:39:06PM +0200, Alexander Shishkin wrote:
> > > >> Lorenzo Stoakes <lorenzo.stoakes@oracle.com> writes:
> > > >>
> > > >> > Thanks very much! Yeah just keen to get this in as we are moving towards
> > > >> > removing these fields very soon.
> > > >>
> > > >> My understanding is that this is a part of larger effort to reduce
> > > >> struct page to 8 bytes and an optionally dynamically allocated slightly
> > > >> larger structure, is that correct? Just curious.
> > > >>
> > > >> > Could you take this in your tree? I think that'd work best.
> > > >>
> > > >> Sure, will do.
> > > >
> > > > Hi, this doesn't appear to be in linux-next yet.  Could you confirm it's
> > > > scheduled to hit the next merge window?
> > >
> > > Yes, I'll send it to Greg once -rc1 is tagged.
> >
> > Right, is this ultimately handled in Greg's PR? Did you not send to him
> > ready for the merge window?
> >
> > Or did you? I'm confused.
>
> I don't see it in my tree right now, is it in linux-next?  Nope, don't
> see it there either :(

Yup... we did ask about this, quite a few times :)

Can you take direct, if you typically handle these from Alex?

>
> > We do really need this merged for 6.15, can we make sure this actually
> > lands? You did confirm it'd go in 2 months ago, and the patch was sent 4
> > months ago, and we have been chasing this repeatedly.
> >
> > For reference, patch is [0].
> >
> > [0]: https://lore.kernel.org/all/20241203080001.12341-1-lorenzo.stoakes@oracle.com/
>
> What changes in 6.15 to require this then?  If it's a bugfix for older
> kernels too, why isn't it tagged for stable inclusion as well?

We're removing page->mapping, index, not required for stable inclusion. This is
a hold-up to a critical mm change, once of many such changes.

We have confirmation from Alex this is fine and he agreed to send in _this_
merge window well in advance of it [1].

So I don't think there's any reason not to send, it's just for whatever reason
not happened...

[1]: https://lore.kernel.org/all/87v7tyyrvb.fsf@ubik.fi.intel.com/

>
> thanks,
>
> greg k-h

Thanks, Lorenzo

On Mon, Mar 31, 2025 at 01:08:43PM +0100, Lorenzo Stoakes wrote:
> On Mon, Mar 31, 2025 at 02:01:38PM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Mar 31, 2025 at 12:59:36PM +0100, Lorenzo Stoakes wrote:
> > > +cc Greg
> > >
> > > On Mon, Mar 31, 2025 at 09:36:40AM +0300, Alexander Shishkin wrote:
> > > > Matthew Wilcox <willy@infradead.org> writes:
> > > >
> > > > > On Wed, Jan 29, 2025 at 01:39:06PM +0200, Alexander Shishkin wrote:
> > > > >> Lorenzo Stoakes <lorenzo.stoakes@oracle.com> writes:
> > > > >>
> > > > >> > Thanks very much! Yeah just keen to get this in as we are moving towards
> > > > >> > removing these fields very soon.
> > > > >>
> > > > >> My understanding is that this is a part of larger effort to reduce
> > > > >> struct page to 8 bytes and an optionally dynamically allocated slightly
> > > > >> larger structure, is that correct? Just curious.
> > > > >>
> > > > >> > Could you take this in your tree? I think that'd work best.
> > > > >>
> > > > >> Sure, will do.
> > > > >
> > > > > Hi, this doesn't appear to be in linux-next yet.  Could you confirm it's
> > > > > scheduled to hit the next merge window?
> > > >
> > > > Yes, I'll send it to Greg once -rc1 is tagged.
> > >
> > > Right, is this ultimately handled in Greg's PR? Did you not send to him
> > > ready for the merge window?
> > >
> > > Or did you? I'm confused.
> >
> > I don't see it in my tree right now, is it in linux-next?  Nope, don't
> > see it there either :(
> 
> Yup... we did ask about this, quite a few times :)
> 
> Can you take direct, if you typically handle these from Alex?

I do normally take pull requests from him, yes.

Please resend it, with his signed-off-by or ack or whatever he wants to
give, and I'll queue it up after -rc1 is out.

thanks,

greg k-h

On Mon, Mar 31, 2025 at 02:39:14PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Mar 31, 2025 at 01:08:43PM +0100, Lorenzo Stoakes wrote:
> > On Mon, Mar 31, 2025 at 02:01:38PM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Mar 31, 2025 at 12:59:36PM +0100, Lorenzo Stoakes wrote:
> > > > +cc Greg
> > > >
> > > > On Mon, Mar 31, 2025 at 09:36:40AM +0300, Alexander Shishkin wrote:
> > > > > Matthew Wilcox <willy@infradead.org> writes:
> > > > >
> > > > > > On Wed, Jan 29, 2025 at 01:39:06PM +0200, Alexander Shishkin wrote:
> > > > > >> Lorenzo Stoakes <lorenzo.stoakes@oracle.com> writes:
> > > > > >>
> > > > > >> > Thanks very much! Yeah just keen to get this in as we are moving towards
> > > > > >> > removing these fields very soon.
> > > > > >>
> > > > > >> My understanding is that this is a part of larger effort to reduce
> > > > > >> struct page to 8 bytes and an optionally dynamically allocated slightly
> > > > > >> larger structure, is that correct? Just curious.
> > > > > >>
> > > > > >> > Could you take this in your tree? I think that'd work best.
> > > > > >>
> > > > > >> Sure, will do.
> > > > > >
> > > > > > Hi, this doesn't appear to be in linux-next yet.  Could you confirm it's
> > > > > > scheduled to hit the next merge window?
> > > > >
> > > > > Yes, I'll send it to Greg once -rc1 is tagged.
> > > >
> > > > Right, is this ultimately handled in Greg's PR? Did you not send to him
> > > > ready for the merge window?
> > > >
> > > > Or did you? I'm confused.
> > >
> > > I don't see it in my tree right now, is it in linux-next?  Nope, don't
> > > see it there either :(
> >
> > Yup... we did ask about this, quite a few times :)
> >
> > Can you take direct, if you typically handle these from Alex?
>
> I do normally take pull requests from him, yes.
>
> Please resend it, with his signed-off-by or ack or whatever he wants to
> give, and I'll queue it up after -rc1 is out.

Thanks, much appreciated! Sent out.

>
> thanks,
>
> greg k-h

Cheers, Lorenzo

On Wed, Jan 29, 2025 at 01:39:06PM +0200, Alexander Shishkin wrote:
> Lorenzo Stoakes <lorenzo.stoakes@oracle.com> writes:
>
> > Thanks very much! Yeah just keen to get this in as we are moving towards
> > removing these fields very soon.
>
> My understanding is that this is a part of larger effort to reduce
> struct page to 8 bytes and an optionally dynamically allocated slightly
> larger structure, is that correct? Just curious.
>
> > Could you take this in your tree? I think that'd work best.
>
> Sure, will do.

Hi, we don't see it in linux-next yet, did you take this patch? Or just not
sync with -next?

As we'd like to assess the current state of pending 6.15 for page->index,
mapping usage to ensure we 'got' everything.

Thanks, Lorenzo

>
> Thanks,
> --
> Alex

On Wed, Jan 29, 2025 at 01:39:06PM +0200, Alexander Shishkin wrote:
> Lorenzo Stoakes <lorenzo.stoakes@oracle.com> writes:
>
> > Thanks very much! Yeah just keen to get this in as we are moving towards
> > removing these fields very soon.
>
> My understanding is that this is a part of larger effort to reduce
> struct page to 8 bytes and an optionally dynamically allocated slightly
> larger structure, is that correct? Just curious.

Broadly yes indeed, it's a big lot of unnecessary memory usage and a key long
term mm project (of Matthew's, I am merely a humble helper in this area :)

If you're curious, you can read Matthew's latest update on the project at [0].

[0]:https://lore.kernel.org/linux-mm/Z37pxbkHPbLYnDKn@casper.infradead.org/

>
> > Could you take this in your tree? I think that'd work best.
>
> Sure, will do.

Thanks!

>
> Thanks,
> --
> Alex

+cc some commit signers due to total lack of response here.

Hi Alexander/anybody at intel/elsewhere who can look at this.

These fields are deprecated and will soon be non-optionally removed, it
would be good to confirm this works on actual hardware before this change
goes in.

So this patch is probably more urgent than you think.

If there is a problem with maintainership in this area, MAINTAINERS should
be updated accordingly so I know who to contact.

Thanks.

On Mon, Jan 06, 2025 at 11:06:56AM +0000, Lorenzo Stoakes wrote:
> Hi Alexander,
>
> Happy New Year! I realise it's been the holidays recently, but Wondering if
> you'd had a chance to look at this?
>
> Thanks, Lorenzo
>
> On Tue, Dec 03, 2024 at 08:00:01AM +0000, Lorenzo Stoakes wrote:
> > The struct page->mapping, index fields are deprecated and soon to be only
> > available as part of a folio.
> >
> > It is likely the intel_th code which sets page->mapping, index is was
> > implemented out of concern that some aspect of the page fault logic may
> > encounter unexpected problems should they not.
> >
> > However, the appropriate interface for inserting kernel-allocated memory is
> > vm_insert_page() in a VM_MIXEDMAP. By using the helper function
> > vmf_insert_mixed() we can do this with minimal churn in the existing fault
> > handler.
> >
> > By doing so, we bypass the remainder of the faulting logic. The pages are
> > still pinned so there is no possibility of anything unexpected being done
> > with the pages once established.
> >
> > It would also be reasonable to pre-map everything on fault, however to
> > minimise churn we retain the fault handler.
> >
> > We also eliminate all code which clears page->mapping on teardown as this
> > has now become unnecessary.
> >
> > The MSU code relies on faulting to function correctly, so is by definition
> > dependent on CONFIG_MMU. We avoid spurious reports about compilation
> > failure for unsupported platforms by making this requirement explicit in
> > Kconfig as part of this change too.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> >
> > v2:
> > * Make the MSU driver dependent on CONFIG_MMU to avoid spurious compilation
> >   failure reports.
> >
> > v1:
> > https://lore.kernel.org/all/20241202122127.51313-1-lorenzo.stoakes@oracle.com/
> >
> >  drivers/hwtracing/intel_th/Kconfig |  1 +
> >  drivers/hwtracing/intel_th/msu.c   | 31 +++++++-----------------------
> >  2 files changed, 8 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/hwtracing/intel_th/Kconfig b/drivers/hwtracing/intel_th/Kconfig
> > index 4b6359326ede..4f7d2b6d79e2 100644
> > --- a/drivers/hwtracing/intel_th/Kconfig
> > +++ b/drivers/hwtracing/intel_th/Kconfig
> > @@ -60,6 +60,7 @@ config INTEL_TH_STH
> >
> >  config INTEL_TH_MSU
> >  	tristate "Intel(R) Trace Hub Memory Storage Unit"
> > +	depends on MMU
> >  	help
> >  	  Memory Storage Unit (MSU) trace output device enables
> >  	  storing STP traces to system memory. It supports single
> > diff --git a/drivers/hwtracing/intel_th/msu.c b/drivers/hwtracing/intel_th/msu.c
> > index 66123d684ac9..93b65a9731d7 100644
> > --- a/drivers/hwtracing/intel_th/msu.c
> > +++ b/drivers/hwtracing/intel_th/msu.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/io.h>
> >  #include <linux/workqueue.h>
> >  #include <linux/dma-mapping.h>
> > +#include <linux/pfn_t.h>
> >
> >  #ifdef CONFIG_X86
> >  #include <asm/set_memory.h>
> > @@ -967,7 +968,6 @@ static void msc_buffer_contig_free(struct msc *msc)
> >  	for (off = 0; off < msc->nr_pages << PAGE_SHIFT; off += PAGE_SIZE) {
> >  		struct page *page = virt_to_page(msc->base + off);
> >
> > -		page->mapping = NULL;
> >  		__free_page(page);
> >  	}
> >
> > @@ -1149,9 +1149,6 @@ static void __msc_buffer_win_free(struct msc *msc, struct msc_window *win)
> >  	int i;
> >
> >  	for_each_sg(win->sgt->sgl, sg, win->nr_segs, i) {
> > -		struct page *page = msc_sg_page(sg);
> > -
> > -		page->mapping = NULL;
> >  		dma_free_coherent(msc_dev(win->msc)->parent->parent, PAGE_SIZE,
> >  				  sg_virt(sg), sg_dma_address(sg));
> >  	}
> > @@ -1592,22 +1589,10 @@ static void msc_mmap_close(struct vm_area_struct *vma)
> >  {
> >  	struct msc_iter *iter = vma->vm_file->private_data;
> >  	struct msc *msc = iter->msc;
> > -	unsigned long pg;
> >
> >  	if (!atomic_dec_and_mutex_lock(&msc->mmap_count, &msc->buf_mutex))
> >  		return;
> >
> > -	/* drop page _refcounts */
> > -	for (pg = 0; pg < msc->nr_pages; pg++) {
> > -		struct page *page = msc_buffer_get_page(msc, pg);
> > -
> > -		if (WARN_ON_ONCE(!page))
> > -			continue;
> > -
> > -		if (page->mapping)
> > -			page->mapping = NULL;
> > -	}
> > -
> >  	/* last mapping -- drop user_count */
> >  	atomic_dec(&msc->user_count);
> >  	mutex_unlock(&msc->buf_mutex);
> > @@ -1617,16 +1602,14 @@ static vm_fault_t msc_mmap_fault(struct vm_fault *vmf)
> >  {
> >  	struct msc_iter *iter = vmf->vma->vm_file->private_data;
> >  	struct msc *msc = iter->msc;
> > +	struct page *page;
> >
> > -	vmf->page = msc_buffer_get_page(msc, vmf->pgoff);
> > -	if (!vmf->page)
> > +	page = msc_buffer_get_page(msc, vmf->pgoff);
> > +	if (!page)
> >  		return VM_FAULT_SIGBUS;
> >
> > -	get_page(vmf->page);
> > -	vmf->page->mapping = vmf->vma->vm_file->f_mapping;
> > -	vmf->page->index = vmf->pgoff;
> > -
> > -	return 0;
> > +	get_page(page);
> > +	return vmf_insert_mixed(vmf->vma, vmf->address, page_to_pfn_t(page));
> >  }
> >
> >  static const struct vm_operations_struct msc_mmap_ops = {
> > @@ -1667,7 +1650,7 @@ static int intel_th_msc_mmap(struct file *file, struct vm_area_struct *vma)
> >  		atomic_dec(&msc->user_count);
> >
> >  	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> > -	vm_flags_set(vma, VM_DONTEXPAND | VM_DONTCOPY);
> > +	vm_flags_set(vma, VM_DONTEXPAND | VM_DONTCOPY | VM_MIXEDMAP);
> >  	vma->vm_ops = &msc_mmap_ops;
> >  	return ret;
> >  }
> > --
> > 2.47.1

I pinged Alexander, but It will mostly have the same effect as your email.
So, no guarantees from me (it is not my area in the kernel, I can't help
more).

On Mon, Jan 27, 2025 at 06:53:23PM +0000, Lorenzo Stoakes wrote:
> +cc some commit signers due to total lack of response here.
> 
> Hi Alexander/anybody at intel/elsewhere who can look at this.
> 
> These fields are deprecated and will soon be non-optionally removed, it
> would be good to confirm this works on actual hardware before this change
> goes in.
> 
> So this patch is probably more urgent than you think.
> 
> If there is a problem with maintainership in this area, MAINTAINERS should
> be updated accordingly so I know who to contact.

-- 
With Best Regards,
Andy Shevchenko

Hi Alexander,

I pinged earlier as this wasn't in -next, but got no response again, and
not seeing it in Linus's tree yet, can you please confirm that this change
is going to land in 6.15 given we have a week of merge window left?

We are going to remove these struct page fields so this is an urgent change
that needs to go in.

Andy - could you ping Alexander internally again, as that seemed to help
last time?

Thanks, Lorenzo

On Tue, Jan 28, 2025 at 07:01:52PM +0200, Andy Shevchenko wrote:
> I pinged Alexander, but It will mostly have the same effect as your email.
> So, no guarantees from me (it is not my area in the kernel, I can't help
> more).
>
> On Mon, Jan 27, 2025 at 06:53:23PM +0000, Lorenzo Stoakes wrote:
> > +cc some commit signers due to total lack of response here.
> >
> > Hi Alexander/anybody at intel/elsewhere who can look at this.
> >
> > These fields are deprecated and will soon be non-optionally removed, it
> > would be good to confirm this works on actual hardware before this change
> > goes in.
> >
> > So this patch is probably more urgent than you think.
> >
> > If there is a problem with maintainership in this area, MAINTAINERS should
> > be updated accordingly so I know who to contact.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

On Tue, Jan 28, 2025 at 07:01:52PM +0200, Andy Shevchenko wrote:
> I pinged Alexander, but It will mostly have the same effect as your email.
> So, no guarantees from me (it is not my area in the kernel, I can't help
> more).

Thanks :) regardless, appreciate you pinging!

These kind of change that touches other areas of the kernel are always tricky...

>
> On Mon, Jan 27, 2025 at 06:53:23PM +0000, Lorenzo Stoakes wrote:
> > +cc some commit signers due to total lack of response here.
> >
> > Hi Alexander/anybody at intel/elsewhere who can look at this.
> >
> > These fields are deprecated and will soon be non-optionally removed, it
> > would be good to confirm this works on actual hardware before this change
> > goes in.
> >
> > So this patch is probably more urgent than you think.
> >
> > If there is a problem with maintainership in this area, MAINTAINERS should
> > be updated accordingly so I know who to contact.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>