Re: [PATCH v2 05/12] powerpc: add pte_free_defer() for pgtables sharing page
On Tue, 27 Jun 2023, Jason Gunthorpe wrote: > On Wed, Jun 21, 2023 at 07:36:11PM -0700, Hugh Dickins wrote: > > [PATCH v3 05/12] powerpc: add pte_free_defer() for pgtables sharing page ... > Yes, this makes sense to me, very simple.. > > I always for get these details but atomic_dec_and_test() is a release? > So the SetPageActive is guarenteed to be visible in another thread > that reaches 0? Yes, that's my understanding - so the TestClearPageActive adds more to the guarantee than is actually needed. "release": you speak the modern language, whereas I haven't advanced from olden-days barriers: atomic_dec_and_test() meets atomic_t.txt's - RMW operations that have a return value are fully ordered; which a quick skim of present-day memory-barriers.txt suggests would imply both ACQUIRE and RELEASE. Please correct me if I'm mistaken! Hugh
Re: [PATCH v2 05/12] powerpc: add pte_free_defer() for pgtables sharing page
On Wed, Jun 21, 2023 at 07:36:11PM -0700, Hugh Dickins wrote:
> [PATCH v3 05/12] powerpc: add pte_free_defer() for pgtables sharing page
>
> Add powerpc-specific pte_free_defer(), to free table page via call_rcu().
> pte_free_defer() will be called inside khugepaged's retract_page_tables()
> loop, where allocating extra memory cannot be relied upon. This precedes
> the generic version to avoid build breakage from incompatible pgtable_t.
>
> This is awkward because the struct page contains only one rcu_head, but
> that page may be shared between PTE_FRAG_NR pagetables, each wanting to
> use the rcu_head at the same time. But powerpc never reuses a fragment
> once it has been freed: so mark the page Active in pte_free_defer(),
> before calling pte_fragment_free() directly; and there call_rcu() to
> pte_free_now() when last fragment is freed and the page is PageActive.
>
> Suggested-by: Jason Gunthorpe
> Signed-off-by: Hugh Dickins
> ---
> arch/powerpc/include/asm/pgalloc.h | 4
> arch/powerpc/mm/pgtable-frag.c | 29 ++---
> 2 files changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/pgalloc.h
> b/arch/powerpc/include/asm/pgalloc.h
> index 3360cad78ace..3a971e2a8c73 100644
> --- a/arch/powerpc/include/asm/pgalloc.h
> +++ b/arch/powerpc/include/asm/pgalloc.h
> @@ -45,6 +45,10 @@ static inline void pte_free(struct mm_struct *mm,
> pgtable_t ptepage)
> pte_fragment_free((unsigned long *)ptepage, 0);
> }
>
> +/* arch use pte_free_defer() implementation in
> arch/powerpc/mm/pgtable-frag.c */
> +#define pte_free_defer pte_free_defer
> +void pte_free_defer(struct mm_struct *mm, pgtable_t pgtable);
> +
> /*
> * Functions that deal with pagetables that could be at any level of
> * the table need to be passed an "index_size" so they know how to
> diff --git a/arch/powerpc/mm/pgtable-frag.c b/arch/powerpc/mm/pgtable-frag.c
> index 20652daa1d7e..0c6b68130025 100644
> --- a/arch/powerpc/mm/pgtable-frag.c
> +++ b/arch/powerpc/mm/pgtable-frag.c
> @@ -106,6 +106,15 @@ pte_t *pte_fragment_alloc(struct mm_struct *mm, int
> kernel)
> return __alloc_for_ptecache(mm, kernel);
> }
>
> +static void pte_free_now(struct rcu_head *head)
> +{
> + struct page *page;
> +
> + page = container_of(head, struct page, rcu_head);
> + pgtable_pte_page_dtor(page);
> + __free_page(page);
> +}
> +
> void pte_fragment_free(unsigned long *table, int kernel)
> {
> struct page *page = virt_to_page(table);
> @@ -115,8 +124,22 @@ void pte_fragment_free(unsigned long *table, int kernel)
>
> BUG_ON(atomic_read(&page->pt_frag_refcount) <= 0);
> if (atomic_dec_and_test(&page->pt_frag_refcount)) {
> - if (!kernel)
> - pgtable_pte_page_dtor(page);
> - __free_page(page);
> + if (kernel)
> + __free_page(page);
> + else if (TestClearPageActive(page))
> + call_rcu(&page->rcu_head, pte_free_now);
> + else
> + pte_free_now(&page->rcu_head);
> }
> }
> +
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +void pte_free_defer(struct mm_struct *mm, pgtable_t pgtable)
> +{
> + struct page *page;
> +
> + page = virt_to_page(pgtable);
> + SetPageActive(page);
> + pte_fragment_free((unsigned long *)pgtable, 0);
> +}
> +#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
Yes, this makes sense to me, very simple..
I always for get these details but atomic_dec_and_test() is a release?
So the SetPageActive is guarenteed to be visible in another thread
that reaches 0?
Thanks,
Jason
Re: [PATCH v2 05/12] powerpc: add pte_free_defer() for pgtables sharing page
On Tue, 20 Jun 2023, Jason Gunthorpe wrote:
> On Tue, Jun 20, 2023 at 12:54:25PM -0700, Hugh Dickins wrote:
> > On Tue, 20 Jun 2023, Jason Gunthorpe wrote:
> > > On Tue, Jun 20, 2023 at 12:47:54AM -0700, Hugh Dickins wrote:
> > > > Add powerpc-specific pte_free_defer(), to call pte_free() via
> > > > call_rcu().
> > > > pte_free_defer() will be called inside khugepaged's
> > > > retract_page_tables()
> > > > loop, where allocating extra memory cannot be relied upon. This
> > > > precedes
> > > > the generic version to avoid build breakage from incompatible pgtable_t.
> > > >
> > > > This is awkward because the struct page contains only one rcu_head, but
> > > > that page may be shared between PTE_FRAG_NR pagetables, each wanting to
> > > > use the rcu_head at the same time: account concurrent deferrals with a
> > > > heightened refcount, only the first making use of the rcu_head, but
> > > > re-deferring if more deferrals arrived during its grace period.
> > >
> > > You didn't answer my question why we can't just move the rcu to the
> > > actual free page?
> >
> > I thought that I had answered it, perhaps not to your satisfaction:
> >
> > https://lore.kernel.org/linux-mm/9130acb-193-6fdd-f8df-75766e663...@google.com/
> >
> > My conclusion then was:
> > Not very good reasons: good enough, or can you supply a better patch?
>
> Oh, I guess I didn't read that email as answering the question..
>
> I was saying to make pte_fragment_free() unconditionally do the
> RCU. It is the only thing that uses the page->rcu_head, and it means
> PPC would double RCU the final free on the TLB path, but that is
> probably OK for now. This means pte_free_defer() won't do anything
> special on PPC as PPC will always RCU free these things, this address
> the defer concern too, I think. Overall it is easier to reason about.
>
> I looked at fixing the TLB stuff to avoid the double rcu but quickly
> got scared that ppc was using a kmem_cache to allocate other page
> table sizes so there is not a reliable struct page to get a rcu_head
> from. This looks like the main challenge for ppc... We'd have to teach
> the tlb code to not do its own RCU stuff for table levels that the
> arch is already RCU freeing - and that won't get us to full RCU
> freeing on PPC.
Sorry for being so dense all along: yes, your way is unquestionably
much better than mine. I guess I must have been obsessive about
keeping pte_free_defer()+pte_free_now() "on the outside", as they
were on x86, and never perceived how much easier it is with a small
tweak inside pte_fragment_free(); and never reconsidered it since.
But I'm not so keen on the double-RCU, extending this call_rcu() to
all the normal cases, while still leaving the TLB batching in place:
here is the replacement patch I'd prefer us to go forward with now.
Many thanks!
[PATCH v3 05/12] powerpc: add pte_free_defer() for pgtables sharing page
Add powerpc-specific pte_free_defer(), to free table page via call_rcu().
pte_free_defer() will be called inside khugepaged's retract_page_tables()
loop, where allocating extra memory cannot be relied upon. This precedes
the generic version to avoid build breakage from incompatible pgtable_t.
This is awkward because the struct page contains only one rcu_head, but
that page may be shared between PTE_FRAG_NR pagetables, each wanting to
use the rcu_head at the same time. But powerpc never reuses a fragment
once it has been freed: so mark the page Active in pte_free_defer(),
before calling pte_fragment_free() directly; and there call_rcu() to
pte_free_now() when last fragment is freed and the page is PageActive.
Suggested-by: Jason Gunthorpe
Signed-off-by: Hugh Dickins
---
arch/powerpc/include/asm/pgalloc.h | 4
arch/powerpc/mm/pgtable-frag.c | 29 ++---
2 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/pgalloc.h
b/arch/powerpc/include/asm/pgalloc.h
index 3360cad78ace..3a971e2a8c73 100644
--- a/arch/powerpc/include/asm/pgalloc.h
+++ b/arch/powerpc/include/asm/pgalloc.h
@@ -45,6 +45,10 @@ static inline void pte_free(struct mm_struct *mm, pgtable_t
ptepage)
pte_fragment_free((unsigned long *)ptepage, 0);
}
+/* arch use pte_free_defer() implementation in arch/powerpc/mm/pgtable-frag.c
*/
+#define pte_free_defer pte_free_defer
+void pte_free_defer(struct mm_struct *mm, pgtable_t pgtable);
+
/*
* Functions that deal with pagetables that could be at any level of
* the table need to be passed an "index_size" so they know how to
diff --git a/arch/powerpc/mm/pgtable-frag.c b/arch/powerpc/mm/pgtable-frag.c
index 20652daa1d7e..0c6b68130025 100644
--- a/arch/powerpc/mm/pgtable-frag.c
+++ b/arch/powerpc/mm/pgtable-frag.c
@@ -106,6 +106,15 @@ pte_t *pte_fragment_alloc(struct mm_struct *mm, int kernel)
return __alloc_for_ptecache(mm, kernel);
}
+static void pte_free_now(struct rcu_head *head)
+{
+ struct page *page;
+
+ page = container_of(head, struc
Re: [PATCH v2 05/12] powerpc: add pte_free_defer() for pgtables sharing page
On Tue, Jun 20, 2023 at 12:54:25PM -0700, Hugh Dickins wrote:
> On Tue, 20 Jun 2023, Jason Gunthorpe wrote:
> > On Tue, Jun 20, 2023 at 12:47:54AM -0700, Hugh Dickins wrote:
> > > Add powerpc-specific pte_free_defer(), to call pte_free() via call_rcu().
> > > pte_free_defer() will be called inside khugepaged's retract_page_tables()
> > > loop, where allocating extra memory cannot be relied upon. This precedes
> > > the generic version to avoid build breakage from incompatible pgtable_t.
> > >
> > > This is awkward because the struct page contains only one rcu_head, but
> > > that page may be shared between PTE_FRAG_NR pagetables, each wanting to
> > > use the rcu_head at the same time: account concurrent deferrals with a
> > > heightened refcount, only the first making use of the rcu_head, but
> > > re-deferring if more deferrals arrived during its grace period.
> >
> > You didn't answer my question why we can't just move the rcu to the
> > actual free page?
>
> I thought that I had answered it, perhaps not to your satisfaction:
>
> https://lore.kernel.org/linux-mm/9130acb-193-6fdd-f8df-75766e663...@google.com/
>
> My conclusion then was:
> Not very good reasons: good enough, or can you supply a better patch?
Oh, I guess I didn't read that email as answering the question..
I was saying to make pte_fragment_free() unconditionally do the
RCU. It is the only thing that uses the page->rcu_head, and it means
PPC would double RCU the final free on the TLB path, but that is
probably OK for now. This means pte_free_defer() won't do anything
special on PPC as PPC will always RCU free these things, this address
the defer concern too, I think. Overall it is easier to reason about.
I looked at fixing the TLB stuff to avoid the double rcu but quickly
got scared that ppc was using a kmem_cache to allocate other page
table sizes so there is not a reliable struct page to get a rcu_head
from. This looks like the main challenge for ppc... We'd have to teach
the tlb code to not do its own RCU stuff for table levels that the
arch is already RCU freeing - and that won't get us to full RCU
freeing on PPC.
Anyhow, this is a full version of what I was thinking:
diff --git a/arch/powerpc/mm/pgtable-frag.c b/arch/powerpc/mm/pgtable-frag.c
index 20652daa1d7e3a..b5dcd0f27fc115 100644
--- a/arch/powerpc/mm/pgtable-frag.c
+++ b/arch/powerpc/mm/pgtable-frag.c
@@ -106,6 +106,21 @@ pte_t *pte_fragment_alloc(struct mm_struct *mm, int kernel)
return __alloc_for_ptecache(mm, kernel);
}
+static void pgtable_free_cb(struct rcu_head *head)
+{
+ struct page *page = container_of(head, struct page, rcu_head);
+
+ pgtable_pte_page_dtor(page);
+ __free_page(page);
+}
+
+static void pgtable_free_cb_kernel(struct rcu_head *head)
+{
+ struct page *page = container_of(head, struct page, rcu_head);
+
+ __free_page(page);
+}
+
void pte_fragment_free(unsigned long *table, int kernel)
{
struct page *page = virt_to_page(table);
@@ -115,8 +130,13 @@ void pte_fragment_free(unsigned long *table, int kernel)
BUG_ON(atomic_read(&page->pt_frag_refcount) <= 0);
if (atomic_dec_and_test(&page->pt_frag_refcount)) {
+ /*
+* Always RCU free pagetable memory. rcu_head overlaps with lru
+* which is no longer in use by the time the table is freed.
+*/
if (!kernel)
- pgtable_pte_page_dtor(page);
- __free_page(page);
+ call_rcu(&page->rcu_head, pgtable_free_cb);
+ else
+ call_rcu(&page->rcu_head, pgtable_free_cb_kernel);
}
}
Re: [PATCH v2 05/12] powerpc: add pte_free_defer() for pgtables sharing page
On Tue, 20 Jun 2023, Jason Gunthorpe wrote: > On Tue, Jun 20, 2023 at 12:47:54AM -0700, Hugh Dickins wrote: > > Add powerpc-specific pte_free_defer(), to call pte_free() via call_rcu(). > > pte_free_defer() will be called inside khugepaged's retract_page_tables() > > loop, where allocating extra memory cannot be relied upon. This precedes > > the generic version to avoid build breakage from incompatible pgtable_t. > > > > This is awkward because the struct page contains only one rcu_head, but > > that page may be shared between PTE_FRAG_NR pagetables, each wanting to > > use the rcu_head at the same time: account concurrent deferrals with a > > heightened refcount, only the first making use of the rcu_head, but > > re-deferring if more deferrals arrived during its grace period. > > You didn't answer my question why we can't just move the rcu to the > actual free page? I thought that I had answered it, perhaps not to your satisfaction: https://lore.kernel.org/linux-mm/9130acb-193-6fdd-f8df-75766e663...@google.com/ My conclusion then was: Not very good reasons: good enough, or can you supply a better patch? Hugh > > Since PPC doesn't recycle the frags, we don't need to carefully RCU > free each frag, we just need to RCU free the entire page when it > becomes eventually free? > > Jason
Re: [PATCH v2 05/12] powerpc: add pte_free_defer() for pgtables sharing page
On Tue, Jun 20, 2023 at 12:47:54AM -0700, Hugh Dickins wrote: > Add powerpc-specific pte_free_defer(), to call pte_free() via call_rcu(). > pte_free_defer() will be called inside khugepaged's retract_page_tables() > loop, where allocating extra memory cannot be relied upon. This precedes > the generic version to avoid build breakage from incompatible pgtable_t. > > This is awkward because the struct page contains only one rcu_head, but > that page may be shared between PTE_FRAG_NR pagetables, each wanting to > use the rcu_head at the same time: account concurrent deferrals with a > heightened refcount, only the first making use of the rcu_head, but > re-deferring if more deferrals arrived during its grace period. You didn't answer my question why we can't just move the rcu to the actual free page? Since PPC doesn't recycle the frags, we don't need to carefully RCU free each frag, we just need to RCU free the entire page when it becomes eventually free? Jason
[PATCH v2 05/12] powerpc: add pte_free_defer() for pgtables sharing page
Add powerpc-specific pte_free_defer(), to call pte_free() via call_rcu().
pte_free_defer() will be called inside khugepaged's retract_page_tables()
loop, where allocating extra memory cannot be relied upon. This precedes
the generic version to avoid build breakage from incompatible pgtable_t.
This is awkward because the struct page contains only one rcu_head, but
that page may be shared between PTE_FRAG_NR pagetables, each wanting to
use the rcu_head at the same time: account concurrent deferrals with a
heightened refcount, only the first making use of the rcu_head, but
re-deferring if more deferrals arrived during its grace period.
Signed-off-by: Hugh Dickins
---
arch/powerpc/include/asm/pgalloc.h | 4 +++
arch/powerpc/mm/pgtable-frag.c | 51 ++
2 files changed, 55 insertions(+)
diff --git a/arch/powerpc/include/asm/pgalloc.h
b/arch/powerpc/include/asm/pgalloc.h
index 3360cad78ace..3a971e2a8c73 100644
--- a/arch/powerpc/include/asm/pgalloc.h
+++ b/arch/powerpc/include/asm/pgalloc.h
@@ -45,6 +45,10 @@ static inline void pte_free(struct mm_struct *mm, pgtable_t
ptepage)
pte_fragment_free((unsigned long *)ptepage, 0);
}
+/* arch use pte_free_defer() implementation in arch/powerpc/mm/pgtable-frag.c
*/
+#define pte_free_defer pte_free_defer
+void pte_free_defer(struct mm_struct *mm, pgtable_t pgtable);
+
/*
* Functions that deal with pagetables that could be at any level of
* the table need to be passed an "index_size" so they know how to
diff --git a/arch/powerpc/mm/pgtable-frag.c b/arch/powerpc/mm/pgtable-frag.c
index 20652daa1d7e..e4f58c5fc2ac 100644
--- a/arch/powerpc/mm/pgtable-frag.c
+++ b/arch/powerpc/mm/pgtable-frag.c
@@ -120,3 +120,54 @@ void pte_fragment_free(unsigned long *table, int kernel)
__free_page(page);
}
}
+
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+#define PTE_FREE_DEFERRED 0x1 /* beyond any PTE_FRAG_NR */
+
+static void pte_free_now(struct rcu_head *head)
+{
+ struct page *page;
+ int refcount;
+
+ page = container_of(head, struct page, rcu_head);
+ refcount = atomic_sub_return(PTE_FREE_DEFERRED - 1,
+&page->pt_frag_refcount);
+ if (refcount < PTE_FREE_DEFERRED) {
+ pte_fragment_free((unsigned long *)page_address(page), 0);
+ return;
+ }
+ /*
+* One page may be shared between PTE_FRAG_NR pagetables.
+* At least one more call to pte_free_defer() came in while we
+* were already deferring, so the free must be deferred again;
+* but just for one grace period, however many calls came in.
+*/
+ while (refcount >= PTE_FREE_DEFERRED + PTE_FREE_DEFERRED) {
+ refcount = atomic_sub_return(PTE_FREE_DEFERRED,
+&page->pt_frag_refcount);
+ }
+ /* Remove that refcount of 1 left for fragment freeing above */
+ atomic_dec(&page->pt_frag_refcount);
+ call_rcu(&page->rcu_head, pte_free_now);
+}
+
+void pte_free_defer(struct mm_struct *mm, pgtable_t pgtable)
+{
+ struct page *page;
+
+ page = virt_to_page(pgtable);
+ /*
+* One page may be shared between PTE_FRAG_NR pagetables: only queue
+* it once for freeing, but note whenever the free must be deferred.
+*
+* (This would be much simpler if the struct page had an rcu_head for
+* each fragment, or if we could allocate a separate array for that.)
+*
+* Convert our refcount of 1 to a refcount of PTE_FREE_DEFERRED, and
+* proceed to call_rcu() only when the rcu_head is not already in use.
+*/
+ if (atomic_add_return(PTE_FREE_DEFERRED - 1, &page->pt_frag_refcount) <
+ PTE_FREE_DEFERRED + PTE_FREE_DEFERRED)
+ call_rcu(&page->rcu_head, pte_free_now);
+}
+#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
--
2.35.3
