On Wed, Jun 28, 2023 at 09:16:24PM +0200, Gerald Schaefer wrote:
> On Tue, 20 Jun 2023 00:51:19 -0700 (PDT)
> Hugh Dickins <hughd@xxxxxxxxxx> wrote:
Hi Gerald, Hugh!
...
> @@ -407,6 +445,88 @@ void __tlb_remove_table(void *_table)
> __free_page(page);
> }
>
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +static void pte_free_now0(struct rcu_head *head);
> +static void pte_free_now1(struct rcu_head *head);
What about pte_free_lower() / pte_free_upper()?
...
> +void pte_free_defer(struct mm_struct *mm, pgtable_t pgtable)
> +{
> + unsigned int bit, mask;
> + struct page *page;
> +
> + page = virt_to_page(pgtable);
> + if (mm_alloc_pgste(mm)) {
> + /*
> + * TODO: Do we need gmap_unlink(mm, pgtable, addr), like in
> + * page_table_free_rcu()?
> + * If yes -> need addr parameter here, like in pte_free_tlb().
> + */
> + call_rcu(&page->rcu_head, pte_free_pgste);
> + return;
> +}
> + bit = ((unsigned long)pgtable & ~PAGE_MASK) / (PTRS_PER_PTE * sizeof(pte_t));
> +
> + spin_lock_bh(&mm->context.lock);
> + mask = atomic_xor_bits(&page->_refcount, 0x15U << (bit + 24));
This makes the bit logic increasingly complicated to me.
What if instead we set the rule "one bit at a time only"?
That means an atomic group bit flip is only allowed between
pairs of bits, namely:
bit flip initiated from
----------- ----------------------------------------
P <- A page_table_free(), page_table_free_rcu()
H <- A pte_free_defer()
P <- H pte_free_half()
In the current model P bit could be on together with H
bit simultaneously. That actually brings in equation
nothing.
Besides, this check in page_table_alloc() (while still
correct) makes one (well, me) wonder "what about HH bits?":
mask = (mask | (mask >> 4)) & 0x03U;
if (mask != 0x03U) {
...
}
By contrast, with "one bit at a time only" policy every
of three bits effectevely indicates which state a page
half is currently in.
Thanks!
