On 2/22/23 17:32, Matthew Wilcox wrote:
On Wed, Feb 22, 2023 at 05:11:34PM +0100, Danilo Krummrich wrote:
On 2/21/23 19:31, Matthew Wilcox wrote:
on tue, feb 21, 2023 at 03:37:49pm +0100, danilo krummrich wrote:
It feels a bit weird that I, as a user of the API, would need to lock certain
(or all?) mas_*() functions with the internal spinlock in order to protect
(future) internal features of the tree, such as the slab cache defragmentation
you mentioned. Because from my perspective, as the generic component that tells
it's users (the drivers) to take care of locking VA space operations (and hence
tree operations) I don't have an own purpose of this internal spinlock, right?
You don't ... but we can't know that.
Thanks for the clarification. I think I should now know what to for the
GPUVA manager in terms of locking the maple tree in general.
Though I still have very limited insights on the maple tree I want to share
some further thoughts.
From what I got so far it really seems to me that it would be better to just
take the internal spinlock for both APIs (normal and advanced) whenever you
need to internally.
No. Really, no. The point of the advanced API is that it's a toolbox
for doing the operation you want, but isn't a generic enough operation
to be part of the normal API.
Again the disclaimer, I'm just sharing my thoughts from the perspective
of a user from a generic tree API.
For me it feels like - and this purely is an assumption, hence please
correct me if I'm wrong on that - you consider the advanced API to be
more of a collection of internal functions not *really* being meant to
be used by arbitrary users and maybe even being slightly tied to mm
since it originated there?
However, from my external perspective I see it the following way.
Even if an operation is not part of the 'normal API', but an API called
'advanced API', it still is a generic API operation being exposed to
arbitrary users. However, my point is not (at least not exclusively)
that I do not consider this to be safe enough or something.
Its just that I think that when the API *enforces* the user to take an
internal lock at certain places it can also just take the lock itself no
matter what the API is being called. Especially when one can't rely on
this lock at all for other (external) purposes anyways because the
implementation behind the API is free to drop the lock whenever it needs to.
To take an example from the radix
tree days, in the page cache, we need to walk a range of the tree,
looking for any entries that are marked as DIRTY, clear the DIRTY
mark and set the TOWRITE mark. There was a horrendous function called
radix_tree_range_tag_if_tagged() which did exactly this. Now look at
the implementation of tag_pages_for_writeback(); it's a simple loop over
a range with an occasional pause to check whether we need to reschedule.
But that means you need to know how to use the toolbox. Some of the
tools are dangerous and you can cut yourself on them.
Another plus would probably be maintainability. Once you got quite a few
maple tree users using external locks (either in the sense of calling
I don't want maple tree users using external locks. That exists
because it was the only reasonable way of converting the VMA tree
from the rbtree to the maple tree. I intend to get rid of
mt_set_external_lock(). The VMAs are eventually going to be protected
by the internal spinlock.
But the argument also holds for the case of using the advanced API and
using the internal spinlock. If your requirements on locking change in
the future every user implementation must be re-validated.
