Am 06.07.21 um 23:19 schrieb John Stultz:
On Tue, Jul 6, 2021 at 2:15 PM Daniel Vetter <daniel@xxxxxxxx> wrote:
On Tue, Jul 6, 2021 at 11:04 PM John Stultz <john.stultz@xxxxxxxxxx> wrote:
On Wed, Jun 30, 2021 at 11:52 PM Christian König
<christian.koenig@xxxxxxx> wrote:
Am 01.07.21 um 00:24 schrieb John Stultz:
On Wed, Jun 30, 2021 at 2:10 AM Christian König
<christian.koenig@xxxxxxx> wrote:
Am 30.06.21 um 03:34 schrieb John Stultz:
+static unsigned long page_pool_size; /* max size of the pool */
+
+MODULE_PARM_DESC(page_pool_size, "Number of pages in the drm page pool");
+module_param(page_pool_size, ulong, 0644);
+
+static atomic_long_t nr_managed_pages;
+
+static struct mutex shrinker_lock;
+static struct list_head shrinker_list;
+static struct shrinker mm_shrinker;
+
+/**
+ * drm_page_pool_set_max - Sets maximum size of all pools
+ *
+ * Sets the maximum number of pages allows in all pools.
+ * This can only be set once, and the first caller wins.
+ */
+void drm_page_pool_set_max(unsigned long max)
+{
+ if (!page_pool_size)
+ page_pool_size = max;
+}
+
+/**
+ * drm_page_pool_get_max - Maximum size of all pools
+ *
+ * Return the maximum number of pages allows in all pools
+ */
+unsigned long drm_page_pool_get_max(void)
+{
+ return page_pool_size;
+}
Well in general I don't think it is a good idea to have getters/setters
for one line functionality, similar applies to locking/unlocking the
mutex below.
Then in this specific case what those functions do is to aid
initializing the general pool manager and that in turn should absolutely
not be exposed.
The TTM pool manager exposes this as function because initializing the
pool manager is done in one part of the module and calculating the
default value for the pages in another one. But that is not something I
would like to see here.
So, I guess I'm not quite clear on what you'd like to see...
Part of what I'm balancing here is the TTM subsystem normally sets a
global max size, whereas the old ION pool didn't have caps (instead
just relying on the shrinker when needed).
So I'm trying to come up with a solution that can serve both uses. So
I've got this drm_page_pool_set_max() function to optionally set the
maximum value, which is called in the TTM initialization path or set
the boot argument. But for systems that use the dmabuf system heap,
but don't use TTM, no global limit is enforced.
Yeah, exactly that's what I'm trying to prevent.
See if we have the same functionality used by different use cases we
should not have different behavior depending on what drivers are loaded.
Is it a problem if we restrict the ION pool to 50% of system memory as
well? If yes than I would rather drop the limit from TTM and only rely
on the shrinker there as well.
Would having the default value as a config option (still overridable
via boot argument) be an acceptable solution?
We're also trying to get ttm over to the shrinker model, and a first
cut of that even landed, but didn't really work out yet. So maybe just
aiming for the shrinker? I do agree this should be consistent across
the board, otherwise we're just sharing code but not actually sharing
functionality, which is a recipe for disaster because one side will
end up breaking the other side's use-case.
Fair enough, maybe it would be best to remove the default limit, but
leave the logic so it can still be set via the boot argument?
Yeah, that would work for me and the shrinker implementation should
already be good enough.
Regards,
Christian.
thanks
-john
