Re: Bug in the error handling in TTMs pool implementation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 27.11.23 um 13:24 schrieb Karolina Stolarek:

On 24.11.2023 16:53, Christian König wrote:
@Karolina do you of hand know a way how we could exercise this in a TTM unit test? Basically we would need to redirect the alloc_pages_node() symbol to an unit test internal function and let it return an error (or something like that).

Do I understand correctly that you want to mock NULL from alloc_pages_node() in ttm_pool_alloc_page()? I had a quick look at it, and found a(n ugly) way to do it.

KUnit provides an API to redirect calls using KUNIT_STATIC_STUB_REDIRECT that can be registered in the original function and then overridden in the test. You can read more about the mechanism in [1].

The problem is that we'd need to introduce a wrapper call for alloc_pages_node() in TTM and expose it, so the function can be substituted during the test. We can't directly modify alloc_pages_node() due to cyclic dependencies caused by kunit/static_stub.h.

For example, in ttm_pool.c, we'd have:

+struct page *ttm_alloc_pages_node(int nid, gfp_t gfp_mask,
+                                 unsigned int order)
+{
+       KUNIT_STATIC_STUB_REDIRECT(ttm_alloc_pages_node, nid, gfp_mask,
order);
+       return alloc_pages_node(nid, gfp_mask, order);
+}
+EXPORT_SYMBOL(ttm_alloc_pages_node);
+
 /* Allocate pages of size 1 << order with the given gfp_flags */
 static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t
gfp_flags,
                                        unsigned int order)
(...)
        if (!pool->use_dma_alloc) {
-               p = alloc_pages_node(pool->nid, gfp_flags, order);
+               p = ttm_alloc_pages_node(pool->nid, gfp_flags, order);
                if (p)
                        p->private = order;
                return p;

And in the test we would say:

+static struct page *fake_ttm_alloc_pages_node(int nid, gfp_t gfp_mask,
+                                             unsigned int order)
+{
+       return NULL;
+}
+
+static void ttm_pool_alloc_failed(struct kunit *test)
+{
(...)
+       kunit_activate_static_stub(test, ttm_alloc_pages_node,
+                                  fake_ttm_alloc_pages_node);
+       err = ttm_pool_alloc(pool, tt, &simple_ctx);
+       kunit_deactivate_static_stub(test, ttm_alloc_pages_node);

Christian, what do you think?

Yes, that was exactly what I was looking for.

And you are right that is indeed quite a bit ugly, I was hoping more like adding debugging hooks or something like that.

Need to look into that in more detail, thanks for the pointer.

Regards,
Christian.


All the best,
Karolina

---------------------------------------------------------
[1] - https://lore.kernel.org/all/20230128074918.1180523-1-davidgow@xxxxxxxxxx/






[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux