Re: [PATCH] Grab mm lock before grabbing pt lock

[Dave Hansen <dave.hansen@xxxxxxxxx>]

On 12/4/24 02:35, Maksym Planeta wrote:
> Function xen_pin_page calls xen_pte_lock, which in turn grab page
> table lock (ptlock). When locking, xen_pte_lock expect mm->page_table_lock
> to be held before grabbing ptlock, but this does not happen when pinning
> is caused by xen_mm_pin_all.

In changelogs, please indicate functions with parenthesis like this:
func().  It makes it easier to tell what is a function versus a variable
or other text.

The use of init_mm here in the preexisting code is a _bit_ fishy because
these pgds *HAVE* an mm and yet xen_mm_pin_all() passes in '&init_mm'
for them. That's relevant here because locking 'init_mm' obviously
doesn't do any good for other mm's.

I have the _feeling_ it's just a big hack and this code throws caution
tot the wind because of:

>  * Expected to be called in stop_machine() ("equivalent to taking
>  * every spinlock in the system"), so the locking doesn't really
>  * matter all that much.

So the patch here kinda doubles down on the hack and continues the theme
because "locking doesn't really matter all that much."

If so, it's not super satisfying, but it is consistent with the existing
code.

> This commit addresses lockdep warning below, which shows up when
> suspending a Xen VM.

If the comment I quote above is right, this is a _harmless_ warning
because nothing else can race here and the locking that gets added is
useless anyway, right? You seem to agree because there's no cc:stable@
or Fixes: tags.

Either way, I'm hesitant to add incorrect-in-any-other-context and
uncommented locking just to shut up lockdep. Is there no better way?

If not, can we comment it at least, please, so nobody else tries to
duplicate the locking?