On 12/6/22 13:51, Peter Xu wrote:
On Tue, Dec 06, 2022 at 01:03:45PM -0800, John Hubbard wrote:
On 12/6/22 08:45, Peter Xu wrote:
I've got a fixup attached. John, since this got your attention please also
have a look too in case there's further issues.
Well, one question: Normally, the pattern of "release_lock(A); call f();
acquire_lock(A);" is tricky, because one must revalidate that the state
protected by A has not changed while the lock was released. However, in
this case, it's letting page fault handling proceed, which already
assumes that pages might be gone, so generally that seems OK.
Yes it's tricky, but not as tricky in this case.
I hope my documentation supplemented that (in the fixup patch):
+ * @hugetlb_entry: if set, called for each hugetlb entry. Note that
+ * currently the hook function is protected by hugetlb
+ * vma lock to make sure pte_t* and the spinlock is valid
+ * to access. If the hook function needs to yield the
So far so good...
+ * thread or retake the vma lock for some reason, it
+ * needs to properly release the vma lock manually,
+ * and retake it before the function returns.
...but you can actually delete this second sentence. It does not add
any real information--clearly, if you must drop the lock, then you must
"manually" drop the lock.
And it still ignores my original question, which I don't think I've
fully communicated. Basically, what can happen to the protected data
during the time when the lock is not held?
The vma lock here makes sure the pte_t and the pgtable spinlock being
stable. Without the lock, they're prone to be freed in parallel.
Yes, but think about this: if the vma lock protects against the pte
going away, then:
lock()
get a pte
unlock()
...let hmm_vma_fault() cond_resched() run...
lock()
...whoops, something else release the pte that I'd previously
retrieved.
However, I'm lagging behind on understanding what the vma lock actually
protects. It seems to be a hugetlb-specific protection for concurrent
freeing of the page tables?
Not exactly freeing, but unsharing. Mike probably has more to say. The
series is here:
https://lore.kernel.org/all/20220914221810.95771-1-mike.kravetz@xxxxxxxxxx/#t
If so, then running a page fault handler seems safe. If there's something
else it protects, then we might need to revalidate that after
re-acquiring the vma lock.
Nothing to validate here. The only reason to take the vma lock is to match
with the caller who assumes the lock taken, so either it'll be released
very soon or it prepares for the next hugetlb pgtable walk (huge_pte_offset).
ummm, see above. :)
Also, scattering hugetlb-specific locks throughout mm seems like an
unfortuate thing, I wonder if there is a longer term plan to Not Do
That?
So far HMM is really the only one - normally hugetlb_entry() hook is pretty
light, so not really throughout the whole mm yet. It's even not urgently
needed for the other two places calling cond_sched(), I added it mostly
just for completeness, and with the slight hope that maybe we can yield
earlier for some pmd unsharers.
But yes it's unfortunate, I just didn't come up with a good solution.
Suggestion is always welcomed.
I guess it's on me to think of something cleaner, so if I do I'll pipe
up. :)
thanks,
--
John Hubbard
NVIDIA
